Replace Accumulator Resources With Definitions (rebased onto develop)

[zuazo]

#228 rebased onto develop:

Hi,

This is my attempt to try to complete the work started by @cwjohnston in the accumulator-definitions branch. The following PR replaces all the accumulator resources by accumulator definitions as it has been discussed in #197.

Some clarifications:

• Replaced the file resources by templates to avoid using run_state unnecessarily.
• Added more test-kitchen & Serverspec integration tests.
• Moved the whisper installation from the carbon_cache definition to the carbon_conf_accumulator definition: All the carbon daemons depend on it.
• I think the ChefGraphite.ini_file method can be simplified a lot and perhaps even move some of the logic to the carbon.conf.erb template. But I left it as it is to avoid adding too many changes.
• The graphite_example::single_node recipe seems to be broken in both master and in this PR (due to issue "ImportError: No module named fields" when install #227).
• The create_graphite_carbon_conf_accumulator ChefSpec matcher is a bit tricky. It will not work for all cases. But I have not found a simple way to create ChefSpec matchers for definitions.

[zuazo]

The most important changes in this PR are those that are related to the accumulator pattern implementation:

• definitions/graphite_carbon_conf_accumulator.rb
• definitions/graphite_storage_conf_accumulator.rb

[webframp]

@zuazo Thanks for taking a stab at this, we're in the process of testing/reviewing as we have time and will provide feedback

[zuazo]

Thanks for the update, @webframp 😃 Take it easy. By the way, feel free to ask me anything.

[webframp]

This is still the direction we want to go, sorry this has taken so long. @cwjohnston had started on this and so I want to allow him to harmonize any work he did with what we can use from here potentially.

Have you been using this pattern, any feedback as to how it works for you?

[zuazo]

Hi @webframp,

Don't worry, I'm still here 😉

I have been using this accumulator pattern using definitions in this graphite cookbook and also other cookbooks successfully. The only downside I found is that you cannot use it from within LWRPs. The values don't accumulate well from there.

The 100% working alternative to this is to implement it in pure Ruby using a singleton class or a similar pattern. But depending on the requirements this can be considered as a good (more simple) first step.

But I'm no expert. Maybe @lamont-granquist or @martinb3 could give us a more informed opinion about this pattern.

[lamont-granquist]

So accumulators-using-definitions hits a kinda nasty problem that the params hash for definitions is a horrible and buggy hack. That leads into this kinda issue:

lamont-cookbooks/multipackage#6

That is both the params hash and some horrible code in the Timeout module in core ruby colliding in a perfect storm.

I cut this issue to outline what it would take to fix all the issues with the params hash:

chef/chef#4117

What it winds up looking like, though, is pretty much just a resource. The problem, though, is that you want a resource which runs at compile time and then the sub-resource-context from use_inline_resources gets in the way. You can force it to run at compile-time and then either not use_inline_resources or add something to core chef to allow the resource to scribble in its parent run_context.

I'm working on that approach here:

lamont-cookbooks/multipackage#7

And also with some feature adds to core-chef to make this easier here:

chef/chef#4183

Either way you cut it there's always a problem with this approach either as a definition or a resource when run from within LWRPs because that is always at converge-time and you only bounce up one level in your resource_collections. But I tried using the Chef.run_context (the 'root' run_context) instead of the run_context.parent and the real killer issue is that that LWRP you are dropping the code into is running at converge time in the parent run_context, so when you lookup the resource in the run_collection you're typically finding a prior resource which you've already converged.

There's probably a use case here for a resource-based-accumulator which does not force itself to run at compile-time and never does anything until converge time. Then most of the resource_collection is already there, then you do a declare_resource with the new :create_if_missing with the Chef.run_context 'root' collection. Then it gets appended and all the LWRPs will find that one resource and inject their state, and it'll run very close to being one of the last resources without having to screw around with throwing delayed notifications.

Of course the only problem there is if a delayed notification itself is an LWRP with one of these declarations inside of them--but if you just don't do that it works fine (and normally you don't do that since delayed notifications are simple things like bouncing services). And the weakness of this approach is that all the work necessarily gets delayed very, very late in the run. So again, the strength of the definitions-approach and the resource based approach I'm taking is that actions happen early and later resources can depend on the state having actually been setup. If you take the delayed-notifications route to doing accumulators it always happens last, and you may wind up in run-chef-twice hell.

[martinb3]

I think @lamont-granquist has given a much better overview of the issues than I could, but I'll add that I've run into the same timing issues between doing accumulation during compilation or doing compilation during convergence that Lamont describes.

Then it gets appended and all the LWRPs will find that one resource and inject their state, and it'll run very close to being one of the last resources without having to screw around with throwing delayed notifications.
Of course the only problem there is if a delayed notification itself is an LWRP with one of these declarations inside of them--but if you just don't do that it works fine (and normally you don't do that since delayed notifications are simple things like bouncing services).

I eventually ended up taking this direction to implementing the accumulator pattern in the firewall cookbook. At first, I had firewall_rule resources that, when they ran an action, they looked up the firewall resource and added their rule data to it (to a hash it owned). But some of the firewall providers restart a service at the end of the run as a delayed notification, and this has also caused some consternation for users. It turns out if there's an error raised in the converge phase, you end up in a case where some firewall_rule resources' actions had run, but not all of them had run, and then the delayed notification still fires after the converge failure, applying half the firewall_rules (only the ones that ran their action and added their rule to the firewall resource's hash before the converge failure).

I ended up re-writing this such that 'firewall' providers searched the run_context for all firewall_rules as a one-shot action, so building a full ruleset was accumulated atomically in a single action. It still has the issues associated with nested run contexts, and an additional issue where I have to manually evaluate the not_if / only_if blocks, but at least it's atomic accumulation instead of partial accumulation. Perhaps Lamont's Chef.run_context suggestion is the answer to the issue of multiple run_contexts all containing firewall_rule(s).

I feel like maybe if there was a way for a delayed notification to know there was a convergence failure (and thus, to know not everything was accumulated), I might be able to avoid apply incomplete rules. Alternately, just avoiding the delayed notification entirely + sticking with the atomic accumulation action that happens in the container/parent resource might be a better approach.

Also, I haven't found a solution I really like for addressing folks' desire to move some actions to run at compile time. Like other uses of compile time actions (chef gems, packages, etc), it feels like it's always an escalating war of moving more and more stuff to run first, until everything runs during the same phase, and you're back to relying on declaration order and/or avoiding dependencies between resources.

[lamont-granquist]

Yeah so if you do a res = declare_resource(:my_resource, "stuff", :create_if_missing: true, run_context: Chef.run_context) from within a resource's provider/action block then it should run at converge time and the resource collection should be mostly fully built. And then the first time you do that you'll throw a resource on the end of the resource collection and all the later calls will just get back that resource that you created. It'll be a standard resource, not a delayed notification so that it will only run if all the resources before it ran successfully. And since you pass the root run_context it should create one singleton resource that manages it all. But that'll only be released in 12.6.0 as it just got merged to master earlier this afternoon. I'll work on getting that into the compat_resource cookbook so that at least earlier 12.x will be able to use this as well via the patches in that cookbook.

[martinb3]

And then the first time you do that you'll throw a resource on the end of the resource collection and all the later calls will just get back that resource that you created.

In my case of a firewall, I'm allowing the user to create both the individual resources that are accumulated (firewall_rule resources) and the overarching resource (firewall), as generally the users have been interested in both. In this case, the firewall resource could show up at an arbitrary point in the run, so I don't think I'd use declare_resource to make it myself, unless the user completely omitted it (which should be unusual).

[damacus]

@zuazo I'm going to close this one down now and look at refactoring it into custom resources

[lock]

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.