Regression in recipe_eval

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: 10.14.4
  • Fix Version/s: 10.18.0
  • Component/s: Chef Client
  • Labels:
    None

Description

I wrote the simplest cookbook I could think of to demonstrate this issue: https://github.com/andreacampi/reval

Running this with Chef 10.12 (e.g. with `kitchen test`) creates two files `/tmp/working` and `/tmp/broken`.
Running it with Chef 10.14.4 only creates `/tmp/working`.

In a nutshell, recipe_eval doesn't actually evaluate the block when called from within a resource, e.g. a `ruby_block`.
The sample cookbook calls recipe_eval from an LWRP; I'm not sure 100% that's strictly relevant but that's the easiest way I know of.

I suspect it's an interaction with why-run, whereby the `converge_by` call in recipe_eval appends the block but it's too late to actually call it.

—

Some background: I noticed because this breaks the `application`, which calls one of its callbacks (`before_deploy`) within a ruby_block.
That's likely unnecessary but I need more time to verify it with the different cookbooks that depend on it.

Issue Links

relates to

Bug - A problem which impairs or prevents the functions of the product. CHEF-3589 Why Run code runs code in {{converge_by}} blocks after the provider action is complete

  • Blocker - Blocks development and/or testing work, production could not run.
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed can be reopened.

Bug - A problem which impairs or prevents the functions of the product. COOK-1673 Deploy_revision in the application cookbook gives an argument error

  • Unknown - Issue untriaged, priority unassigned or questioned
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed can be reopened.

Bug - A problem which impairs or prevents the functions of the product. COOK-1747 Callbacks are not executed during deployment

  • Unknown - Issue untriaged, priority unassigned or questioned
  • Confirmed - This issues existence has been validated

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Bryan McLellan [Opscode] added a comment -

From ML: http://lists.opscode.com/sympa/arc/chef-dev/2012-10/msg00000.html

converge_by adds actions to the @converge_actions instance in the
/provider/ instance, not in a common parent like the run context.

I spent almost all day trying to make sense of this.

1) RevalBroken['broken'] (LWRP) is converged.

It creates ruby_block["broken before_deploy"] which is added to the
resource collection.

It has no converge_by blocks, so it has no ConvergeActions.

2) ruby_block["broken before_deploy"] is converged.

Calling the code block is in converge_by, so it gets added to
ruby_block["broken before_deploy"].converge_actions.

We run that converge_action.

3) recipe_eval is called.

recipe_eval wraps instance_eval (which all happens as a separate chef
run) in a converge_by block, which gets added to:
RevalBroken['broken']

Note that we've already run that providers converge_actions, so the
actual recipe_eval converge_action never happens.

I'm still having a hard time following why this is true.

Show
Bryan McLellan [Opscode] added a comment - From ML: http://lists.opscode.com/sympa/arc/chef-dev/2012-10/msg00000.html converge_by adds actions to the @converge_actions instance in the /provider/ instance, not in a common parent like the run context. I spent almost all day trying to make sense of this. 1) RevalBroken ['broken'] (LWRP) is converged. It creates ruby_block ["broken before_deploy"] which is added to the resource collection. It has no converge_by blocks, so it has no ConvergeActions. 2) ruby_block ["broken before_deploy"] is converged. Calling the code block is in converge_by, so it gets added to ruby_block ["broken before_deploy"] .converge_actions. We run that converge_action. 3) recipe_eval is called. recipe_eval wraps instance_eval (which all happens as a separate chef run) in a converge_by block, which gets added to: RevalBroken ['broken'] Note that we've already run that providers converge_actions, so the actual recipe_eval converge_action never happens. I'm still having a hard time following why this is true.
Hide
Permalink
Andrea Campi added a comment -

COOK-1747 contains some more analysis and a likely fix / workaround.

Show
Andrea Campi added a comment - COOK-1747 contains some more analysis and a likely fix / workaround.
Hide
Permalink
Andrea Campi added a comment -

After some more testing, I think ~aleksei is up to something (in COOK-1747): calling converge does trigger convergence of the recipe_eval'ed block.

I can't say I understand exactly the how and why, but I can't see any harm; and it beats leaving the application cookbook broken.

Still, I'd be interested in learning about a proper fix

Show
Andrea Campi added a comment - After some more testing, I think ~aleksei is up to something (in COOK-1747 ): calling converge does trigger convergence of the recipe_eval'ed block. I can't say I understand exactly the how and why, but I can't see any harm; and it beats leaving the application cookbook broken. Still, I'd be interested in learning about a proper fix
Hide
Permalink
Daniel DeLeo added a comment - - edited

Hey, can you check this out on the latest master? We encountered more ordering issues when why-run-ifying some LWRPs, and have chosen to make the converge_by method call the blocks immediately. That change is tracked here: CHEF-3589

Patch is merged to master, and will be reviewed and merged for 10-stable shortly.

Show
Daniel DeLeo added a comment - - edited Hey, can you check this out on the latest master? We encountered more ordering issues when why-run-ifying some LWRPs, and have chosen to make the converge_by method call the blocks immediately. That change is tracked here: CHEF-3589 Patch is merged to master, and will be reviewed and merged for 10-stable shortly.
Hide
Permalink
Andrea Campi added a comment -

With a fresh checkout this passed by repro case. Great job! (thumbsup).

Show
Andrea Campi added a comment - With a fresh checkout this passed by repro case. Great job! (thumbsup).
Hide
Permalink
Daniel DeLeo added a comment -

Andrea, Thanks for taking the time to test this.

Show
Daniel DeLeo added a comment - Andrea, Thanks for taking the time to test this.

People

  • Assignee:
    Unassigned
    Reporter:
    Andrea Campi
Vote (1)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: