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

Add unified_mode for resources #8668

Merged
merged 2 commits into from Aug 16, 2019

Conversation

@lamont-granquist
Copy link
Contributor

commented Jun 18, 2019

This is inspired by "use_inline_resources".

Setting unified_mode false in a resource would be the existing
behavior with separate compile/converge phases. This remains the
default.

Setting unified_mode true in a resource will eliminate the converge
phase. Reverse notifications and delayed notifications will still
fire. The resource action will behave like all resources are executing
at compile time.

As a aside, notifications have never worked for resources firing at
compile time. This implementation gets that behavior correct so
that notifications will work.

Of course forward notifications to resources not yet declared will not
be possible.

Setting resource_unified_mode_default true in Chef::Config would
turn off unified mode and turn off the split compile/converge mode for every
custom resource.

NOTE: This does not affect recipe mode at all.

As an example old annoying code:

action :doit do
  file "/tmp/foo.xyz" do
    content "something"
    action :nothing
  end.run_action(:create)

  puts "exists" if File.exist?("/tmp/foo.xyz")

  if some_conditional?
    converge_by("do something to /tmp/foo.xyz") do
      [ ... some code here that consumes /tmp/foo.xyz... ]
    end
  end
end
unified_mode true # put this anywhere in the declaration of the resource

action :doit do
  file "/tmp/foo.xyz" do
    content "something"
    # it is no longer necessary to `action :nothing` this
    # there simply IS no converge_phase
  end  # no forcing to compile_time here

  # at this point normal non-resource code will see the file as having been created

  puts "exists" if File.exist?("/tmp/foo.xyz")

  if some_conditional?
    converge_by("do something to /tmp/foo.xyz") do
      [ ... some code here that consumes /tmp/foo.xyz... ]
    end
  end
end

@lamont-granquist lamont-granquist force-pushed the lcg/converge_mode_switch branch from 0242c80 to 4b01fb4 Jun 18, 2019

@lamont-granquist

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Thoughts on names:

converge_mode false
one_pass true
unified_mode true
fused_mode true
combined_mode true
merged_mode true

Fused has some name-conflicts with poise's fused resources though which are kinda like custom resources.

I think it should probably get inverted and be a "true" thing rather than a "false" thing, because most humans seem to like being positive.

Feel like I'm missing a noun or something with most of those (Updated to have "_mode" on all of them).

@lamont-granquist

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

I'm sorta leaning unified_mode true

@lamont-granquist lamont-granquist force-pushed the lcg/converge_mode_switch branch from 875455d to ec662e7 Jun 19, 2019

@lamont-granquist lamont-granquist changed the title Add converge_mode switch for resources Add unified_mode for resources Jun 19, 2019

@lamont-granquist

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

failures seem to be SuSE stuff (looks like SuSE passed on retry) and the ruby-prof appveyor one.

@tas50
tas50 approved these changes Jun 26, 2019
@lamont-granquist

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

So per meeting notes:

  • should log that it is experimental only for now
  • need to investigate how it interacts with subscribes since in my head what happens is that the subscribes will necessarily have to happen too late, and the resource will have already have fired a long time ago so nothing will happen
@lamont-granquist

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

somehow an early resource which subscribes to a later resource works (for subscribes the resolution of the string to the resource name must be lazy?) while having a later resource subscribe to an earlier resource fails because the action isn't tracked (maybe this is a job for the action_collection? hrm...).

@lamont-granquist lamont-granquist force-pushed the lcg/converge_mode_switch branch from ec662e7 to ed69e97 Jul 30, 2019

@lamont-granquist lamont-granquist requested review from chef/chef-infra-approvers as code owners Jul 30, 2019

@lamont-granquist

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

This now works correctly with 6 out of 8 possible notification schemes:

  1. delayed notification to a resource declared either before or after the notifying resource works (2 cases)
  2. delayed subscribing to a resource decalred either before or after the subscribing resource works (2 cases)
  3. immediate notification to a resource that is declared earlier works.
  4. immediate subscribing to a resource that is declared later, and has not yet fired works.
  5. immediate notification to a resource that is declared later fails (that resource has not yet been parsed so i can't "immediately" be notified without predicting the future).
  6. immediate subscribing to a resource that is declared earlier and has fired fails (you cannot rewind time to fire the currently parsed resource when that prior resource had fired earlier).

Since 1+2 are the common case that covers most of the bases, and I think may be mildly surprising that it works, but with the lazy subscribes code that we already have it was fairly easy to lazily keep around the notification reference and only resolve it when the delayed notifications are fired after the resource is fully parsed.

The rule for immediate notifications should really be to declare resources first and then notify them which makes better intuitive sense than the subscribing to resources that are not yet declared.

I could have tried to make the 5+6 cases work, by interpreting :immediate to mean "immediately when the later resource is parsed" but that seem rather weird and magical and the code was gonna be very ugly which is usually a good indication that its a bad idea.

@lamont-granquist

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

So now what I don't like is this pattern is common and now breaks:


actions :install

default_action :install

property :source, String
property :version, String, name_property: true

action :install do
      source_url = new_resource.source

      remote_file "/tmp/redis-#{new_resource.version}.tar.gz" do
        #source "http://download.redis.io/releases/redis-#{new_resource.version}.tar.gz"
        new_resource.source source_url
        notifies :run, "execute[unzip_redis_archive]", :immediately
      end
      
      execute "unzip_redis_archive" do
        command "tar xzf redis-#{new_resource.version}.tar.gz"
        cwd "/tmp"
        action :nothing
        notifies :run, "execute[build_and_install_redis]", :immediately
      end
      
      execute "build_and_install_redis" do
        command 'make && make install'
        cwd "/tmp/redis-#{new_resource.version}"
        action :nothing
        notifies :run, "execute[install_server_redis]", :immediately
      end
      
      execute "install_server_redis" do
        command "echo -n | ./install_server.sh"
        cwd "/tmp/redis-#{new_resource.version}/utils"
        action :nothing
      end
end

We've broken forward immediate notifications, so this doesn't work. Further if you just change the notifies to subscribes and bump then down to the next resource you would get equivalent pre-unified-mode behavior. But that is also broken.

To make it work you have to flip the order resources are declared, which is now the inverse order of how they are declared:

unified_mode true

actions :install

default_action :install

property :source, String
property :version, String, name_property: true

action :install do
      source_url = new_resource.source

      execute "install_server_redis" do
        command "echo -n | ./install_server.sh"
        cwd "/tmp/redis-#{new_resource.version}/utils"
        action :nothing
      end
      
      execute "build_and_install_redis" do
        command 'make && make install'
        cwd "/tmp/redis-#{new_resource.version}"
        action :nothing
        notifies :run, "execute[install_server_redis]", :immediately
      end

      execute "unzip_redis_archive" do
        command "tar xzf redis-#{new_resource.version}.tar.gz"
        cwd "/tmp"
        action :nothing
        notifies :run, "execute[build_and_install_redis]", :immediately
      end

      remote_file "/tmp/redis-#{new_resource.version}.tar.gz" do
        #source "http://download.redis.io/releases/redis-#{new_resource.version}.tar.gz"
        new_resource.source source_url
        notifies :run, "execute[unzip_redis_archive]", :immediately
      end
end

This is a bit easier didactically though "you have to declare things first and you call backwards" is fairly simple to teach, and it will fail hard when the user gets it wrong, so it isn't annoying subtle incorrectness like compile-converge mode. It is incorrectness that will beat you over the head.

Still it feels like we're lacking something here, and that notifications may even be a poor way to express it, because really what you want to express is a simple "if this resource updates, then evaluate all these resources in order". And even the conventional way of expressing this with notifications is awkward.

Add unified_mode switch for resources
This is inspired by "use_inline_resources".

Setting `unified_mode false` in a resource would be the existing
behavior with separate compile/converge phases.

Setting `unified_mode true` in a resource will eliminate the converge
phase.  Reverse notifications and delayed notifications will still
fire.  The resource action will behave like all resources are executing
at compile time.

As a aside, notifications have never worked for resources firing at
compile time.  This implementation gets that behavior correct so
that notifications will work.

Of course forward immediate notifications to resources not yet declared will not
be possible.

Setting `resource_unified_mode_default true` in `Chef::Config` would
turn off the split compile/converge mode for every custom resource.

NOTE: This does not affect recipe mode at all.

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

@lamont-granquist lamont-granquist force-pushed the lcg/converge_mode_switch branch from ed69e97 to 35a63dd Aug 12, 2019

@lamont-granquist

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

So this PR is blowing my mind a bit:

% cat cookbooks/test/resources/myresource.rb
provides :myresource

property :name, String

unified_mode true

action :doit do
  file "/tmp/xyz.out" do
    content "whatever"
    notifies :run, 'ruby_block[boom]', :immediate
  end

  ruby_block "boom" do
    block do
      raise "next resources goes boom"
    end
    action :nothing
  end

rescue
  file "/tmp/xyz.out" do
    action :delete
  end
end

results in:

% chef-client -z -j dna.json -c client.rb
Starting Chef Infra Client, version 15.2.22
resolving cookbooks for run list: ["test::test"]
Synchronizing Cookbooks:
  - test (0.0.1)
Installing Cookbook Gems:
Compiling Cookbooks...
Converging 2 resources
Recipe: test::test
  * log[it] action nothing (skipped due to action :nothing)
  * test_myresource[whatever] action doit
    * file[/tmp/xyz.out] action create
      - create new file /tmp/xyz.out
      - update content in file /tmp/xyz.out from none to 85738f
      --- /tmp/xyz.out	2019-08-13 13:59:11.391832871 -0700
      +++ /tmp/.chef-xyz20190813-60134-vx3by9.out	2019-08-13 13:59:11.391636855 -0700
      @@ -1 +1,2 @@
      +whatever
    * ruby_block[boom] action nothing (skipped due to action :nothing)
    * ruby_block[boom] action run

      ================================================================================
      Error executing action `run` on resource 'ruby_block[boom]'
      ================================================================================

      RuntimeError
      ------------
      next resources goes boom

[.... debugging output ....]

    * file[/tmp/xyz.out] action delete
      - delete file /tmp/xyz.out

[.... rest of the error output ....]

In that kind of scenario where a remote_file creation is acting as a gate which triggers a notify chain which unzips the file and whatnot, you could catch exceptions anywhere in that chain, and then clean up the remote file, so that you don't get 'stuck' in an intermediate state where the file was downloaded but the chained resources never fully completed due to some transient error on the last run.

@lamont-granquist

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

(heh, although because i used two file resources that have the same name in that snippet the notification can't tell the difference between the originating resources, so the file with action :delete causes the notify to run again and the ruby_block to fire again and another exception to get run -- but that is just #7389 and nothing to do with unified_mode specifically but fallout from removing the CHEF-3694 warnings without doing something about warning about when same-named resources cause confusion in notifies).

remove debugging
Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>

@lamont-granquist lamont-granquist merged commit f24218b into master Aug 16, 2019

4 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/chef-chef-master-verify Build #1328 passed (24 minutes, 4 seconds)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
expeditor/config-validation Validated your Expeditor config file
Details

@lamont-granquist lamont-granquist deleted the lcg/converge_mode_switch branch Aug 16, 2019

@lock

This comment has been minimized.

Copy link

commented Aug 30, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 30, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.