Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

converge_by does not respect sequential order #8616

Closed
Annih opened this issue May 30, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@Annih
Copy link
Contributor

commented May 30, 2019

Description

In a resource (tested with custom resource and LWRP) the use of converge_by (or converge_if_changed) does not respect the sequential order.

Chef Version

12.18.31 to at least 15.0.300

Replication Case

Create a custom resource like below:

provides :test

action :run do
  log '1'
  converge_by 'logging' do
    ::Chef::Log.info '2'
    log '3'
  end
end

Then converge this in a recipe:

  test 'order'

Client Output

  * test[order] action run
INFO: 2
    - logging
    * log[1] action write
INFO: 1
    * log[3] action write
INFO: 3

Extra notes

My original idea of the converge_by, before reading the code, was that the block is wrapped into a kind of anonymous resource which is added to the collection at the right position.
The documentation does not provide much information about this helper, if current behavior is the expected one, maybe we should just improve the doc and close this issue.

I'm don't know either whether it is a good thing to use resources inside the converge_by block. There is no example doing it, but it seems to be working

@lamont-granquist

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

This is just how chef works with compile/converge mode.

The log resources are resources and they are always delayed into converge mode.

The converge_by block is a compile-time declaration. The same with the Log statement.

And no you should in general not be using converge_by. You should almost never be placing converge_by around resources. The purpose of that is NOT to log output if that is what you are using it for (which is the #1 mistake in using converge by). The converge_by statement also always sets the updated_by_last_action flag of the resource it is in, which means that it will cause notifications to fire if it is executed. That means that converge_by statements MUST always be wrapped with conditionals.

The purpose of converge_by is for when you are wrapping pure ruby code. It MUST look like:

   if current_resource.mode != new_resource.mode  # the conditional
    converge_by "fixing the file modes" do # the log output
      File.chmod(new_resource.mode, new_resource.path) # the pure ruby inside of it
    end
  end

If you want to use a chef resource (which you should, you should never roll your own file resource like in that example) then just use it and do not use converge_by.

@Annih

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@lamont-granquist Thanks a lot for the clarification on when to use converge_by vs built-in resources, or that I need to wrap them with conditionals, but I was already aware of that and this is not my issue here, if you told me that because of my example I just wanted to have a clean and simple one.

Maybe I wasn't clear enough; my problem is that the execution order of custom resource with mixed converge_by and built-in resources is not as straight forward as it should be, and/or completely undocumented.

You should almost never be placing converge_by around resources.

This is a good example of non-documentation, how should I know that, but also could you clarify that point? Should I, or not? If I should in which case?
It does seem to "work", but without respecting a sequential order. Honestly, without looking at the code what would you expect?

Here is another example, that I hope will help to clarify the situation:

resource '1

converge_by "a resource in a converge_by" do
  resource '2'
end if should_be_updated?

converge_by "a pure ruby code" do
  ::Chef::Log.warn "X"
end if should_be_updated?

I personally was expecting that "at compile time" of the action evaluation:

  1. resource A is added to the resource collection
  2. resource B is added to the resource collection
  3. 'X' is warned

Then at the "convergence time":

  1. resource A tries to perform its action (or not if up-to-date)
  2. resource B tries to perform its action (or not if up-to-date)

So in the case where my resources were log with :info action, I would have expected the following output:

# kind of compile time
WARN: X
# kind of convergence time
INFO: 1
INFO: 2

But I was wrong, the result is:

# kind of compile time
WARN: X 
# kind of convergence time
INFO: 2
INFO: 1

This is just how chef works with compile/converge mode.

Seems like the "This is not a bug, this is a feature" answer to me. Would you @lamont-granquist really expect the "X 2 1" order?
If your answer is no, maybe we could just add a bit more documentation about the converge_by (or converge_if_changed).
Ideally I would love to have a sequential order, or even a sub run_context, but lets start with documentation :D

@lamont-granquist

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

So this is an issue tracker for the source code. This isn't a source code issue, it is strictly a documentation issue and should get opened against the chef-web-docs:

https://github.com/chef/chef-web-docs

And the answer to this question:

Would you @lamont-granquist really expect the "X 2 1" order?

Is 'Yes'.

Resources are not run when they are declared. The declaration of the resource builds an object which is then put on the resource collection list. Once the ruby code in the recipe or action is fully parsed then the resource collection is converged and the resources are actually run. So all ruby code runs before all resources converge.

https://coderanger.net/two-pass/

The fact that "converge_by" runs at compile time is perhaps confusing, but the name of that method was written 8 years ago and that ship already sailed and isn't coming back. It is mostly intended for advanced users who already understand compile/converge mode.

And you're starting to get into "how do I chef?" questions which are not bug reports. These should be asked on slack, discourse or stackoverflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.