Please come talk to me or dan at the summit

[lamont-granquist]

The accumulator pattern in this cookbook where it looks like you're modifying the resource collection at converge time creating dynamic attributes in providers is gonna fail in ways that we can't rescue you from. You should really use definitions for this purpose (e.g. the "Many Recipes, One Definition" section of https://docs.getchef.com/essentials_cookbook_definitions.html documentation). I'm not sure how you wound up with the design decision to do that kind of mutating of the resource collection inside providers so I don't quite know what to tell you. If you think you shouldn't be using definitions then that is incorrect, and you have stumbled across the use case for which providers are terrible at and definitions are wonderful for. You can simply disable the foodcritic rule that nag you about them and move on. If there's a problem with using definitions then it'd be nice to know what that problem is, since its probably a chef bug and/or a design element that we need to understand for making them better.

[webframp]

Would love to hear your input, I'm not there sadly but look for @fnichol or @chrisroberts and I'm sure they'll be able to fill me in.

[lamont]

Other lamont chiming in here. I met with some of the heavy water folks at the summit and was sorry you were not able to attend, @webframp. I mentioned the issues I was having with the accumulator pattern (I also commented on issue #183 in this repo as well). I defer to @lamont-granquist as to the pattern changes needed, but the major issue I'm running into is that the carbon.conf file resource does not exist till the last second, and therefore I cannot subscribe to it or notify from it to restart any of the carbon daemons when the configuration changes.

BTW, I used this library cookbook to deploy a 7 node, 1m metric/m cluster, as two major wrapper recipes. About 250 lines, excluding the varnish and collectd stuff I included.

[lamont-granquist]

hehehe, wow, you're getting tag-teamed by lamonts.... usually one of me is bad enough...

there's two cookbooks that i opened up after the conference that i use where i've used definitions to do an accumulator pattern:

https://github.com/lamont-cookbooks/sk_sysctl/blob/master/definitions/sysctl.rb
https://github.com/lamont-cookbooks/sk_ssh_known_hosts/blob/master/definitions/entry.rb

they probably need some clean-up since they're my own internal cookbooks. the advantage of doing the accumulator pattern as a definition is that its compile time, so your resource collection is fully assembled by the time you start converging so that notifications work correctly.

i was thinking after the conference that it might be slick to have the definition return the resource that it is constructing. then you could method chain things like not_if/only_if/notifies/subscribes off of the definition -- and make this the default behavior of the definition. i haven't looked into what the return value of a definition is though. if not, you could provide a library helper which did the look-up-or-create-the-resource-and-return-it function.

[obazoud]

FYI, I use in an internal cookbook the "accumulator cookbook" and it works fine.
https://github.com/kisoku/chef-accumulator /cc @kisoku

Maybe you can use this cookbook instead of an internal mechanism.
Can help with issue #183

[lamont-granquist]

That's the same pattern of editing the resource collection at converge time instead of compile time.

[webframp]

Yea, the pattern from @kisoku was clearly the direct inspiration for the method currently used. Compile time vs converge is a big difference and if it can achieved with a definition at compile time, that's definitely an improvement to make and examples are a big help. @chrisroberts was going to fill me in on discussions with @lamont-granquist from the summit so I'll get more info and see what time I have to work on this.

@lamont nice to hear of some success in using this so far.

[agoddard]

@lamont-granquist @webframp @obradovic @chrisroberts @fnichol @cwjohnston We're going to schedule a hangout next week to have a chat about this, one of us will send a hangout invite once we pick a time, please use this doodle poll to pic a time that works for you http://doodle.com/h3rm5bizdz2xqq9e

[agoddard]

Tuesday at 11AM Pacific / 2PM Eastern it is, we'll have a hangout at https://plus.google.com/hangouts/_/hw-ops.com/graphite

[agoddard]

@lamont @lamont-granquist @webframp @obradovic @chrisroberts @fnichol @cwjohnston just a reminder re: Hangout tomorrow, 11AM PT/2PM ET https://plus.google.com/hangouts/_/hw-ops.com/graphite
If you have any issues joining, hit us up on the IRC backchannel @ #heavywater

[lamont]

trying to join the call now, but I keep getting "the party is over" message. Has it started?

[kisoku]

I am also trying to join, but our corporate firewall is braindead

[agoddard]

@lamont https://plus.google.com/hangouts/_/hw-ops.com/graphite < are you using that link? We're in the HO at the moment

[kisoku]

I am also getting 'this call is already over' from my phone

[agoddard]

@kisoku try again now, we just fixed the HO share settings

[kisoku]

no dice

[agoddard]

Thanks for joining everyone! Would love any feedback you have on whether the HO worked vs further discussion here

[lamont-granquist]

Think that worked pretty well.

[zuazo]

So, if I understand this discussion correctly, we should use definitions instead of the accumulator cookbook to implement the accumulator pattern.

[lamont-granquist]

Yeah, it works much better to edit a single resource on the resource collection via definitions (compile-time macros) rather than providers (which run at converge time). Having the resource collection editing itself at converge time violates the notion that what we're doing with the two steps is to first construct the resource collection and then second to converge it.

But if you step back from the accumulator pattern as its implemented by editing a resource on the resource collection what you're really doing is using a resource as a singleton object to share state between multiple resources. You don't have to solve the problem this way, you could simply use a library and can use the node.run_state for shared state between recipes or you can write your own Singleton in a library or use other similar tactics. The best practice here is probably to use ruby to solve the problem rather than a definition or a provider -- but a definition will move the resource collection editing to compile time and solves the issues with using a provider.

[zuazo]

@lamont-granquist, thanks for your fast and detailed response 😄

Being more specific, is using definitions for this a recommended approach (so we should deprecate FC015)?

I'm not against implementing it using ruby, but using a definition seems to be much easier and more accessible for non-ruby users.

[lamont-granquist]

Yep, TL;DR is to use a definition rather than a provider.

Also when you write the definition, have it return the resource object. Chef 12's definitions will return the return value of the definition correctly. That lets you hang chef-rewind like chaining off of the definition, so then you can do my_definition.notifies :run, 'execute[thing]', :immediately and if the definition (without any argument) just returns the resource it'll work. That will not work in 11 though.

[zuazo]

OK, @lamont-granquist. Thank you for the clarification and the Chef 12 tip 🍺

[webframp]

Some work in response to this discussion is taking place here: https://github.com/hw-cookbooks/graphite/tree/accumulator-definitions

Input appreciated

[jsirex]

Recently, I faced with the same issue of reusing resources when rewriting colllectd plugin.
Use case: from different recipes and/or cookbooks I want to call collectd_plugin which should merge somehow in single file.

Example: collectd has 'processes' plugin which helps to monitor processes. And I have many different cookbooks like: apache, elasticsearch, mongodb, chef-client (in base-cookbook), etc. In each cookbook I want monitoring. So each cookbook has the following piece of code:

# For mongodb
  collectd_plugin 'processes' do
    options :process_name => "mongos",
            :process_match => "/usr/bin/mongos"
  end

Another recipe:

  collectd_plugin 'processes' do
    options :process_name => "chef-client",
            :process_match => "/opt/chef/embedded/bin/ruby /usr/bin/chef-client"
  end

Obviously, if few developers will implement such code and use it all in run list only latest definition wins, and you'll get something:

WARN: Cloning resource attributes for directory[/var/run/chef] from prior resource (CHEF-3694)

I've decided to write HWRP and do merge manually. In different use-cases you'll probably want different merge strategies.

When Chef meets resource definition you can easily load previous resource and do merge as you want.

Here is Chef::Resource which smart enough to collect previous options:
https://github.com/jsirex/cookbook-collectd/blob/master/libraries/collectd_plugin.rb#L27-L40

Of course, you can collect whole previous resources as is :). And this is done on compile time.
Pay attention to:
https://github.com/jsirex/cookbook-collectd/blob/master/libraries/collectd_plugin.rb#L30

At Chef::Provider you can merge all things as you want:
https://github.com/jsirex/cookbook-collectd/blob/master/libraries/collectd_plugin.rb#L75-L77

Also project contains readme with explanations.
This is http://docs.chef.io/definitions.html#many-recipes-one-definition but for HWRP

[lamont-granquist]

Well, that's clever but that's also monkeypatching an internal API, so we may randomly break you in a minor version. I'm also not wild about overloading resource cloning and hacking the prior resource action :nothing, there's a lot of baked in assumptions there. If you want to do something like that it'd be better to open an RFC against chef to propose a maintainable public API to support that:

https://github.com/opscode/chef-rfc

And really to be blunt, that monkeypatch is a hack. It might work for you, but we can't support that, and don't encourage anyone else to use that. You're fiddling with an internal API and we won't adhere to SemVer against that.

The definition accumulator pattern works and is supported, and you should really use that.

[zuazo]

@webframp, reviewing your accumulator-definitions branch, I can see that you use run_state and #lazy in a file resource to implement the accumulator pattern.

This might work, but I think this could be implemented more easily using only definitions and templates instead of files. You can accumulate values merging them in the already existing template variables. Look at the @lamont-granquist examples above.

[jgoldschrafe]

It seems like this discussion has completely stalled all dev on this cookbook for about the last half a year. Is there a clear direction yet? Is anything blocking the transition from LWRPs to definitions? I'd love to know where this left off.

[cwjohnston]

Hi Jeff, thanks for inquiring. I've burned a bunch of hours on a refactor and ran into a few obstacles which have led me to shelf that effort for the time being.

I am beginning to work on other issues outstanding on this project, with the intent of returning to this issue at a later time. In the meantime, if you or anyone else would like to attempt the refactor, I would be happy to assist with that work.

[lamont-granquist]

I'm neck deep in rewriting chef node attributes or I'd try to help out.

[zuazo]

OK. I created a PR to fix this issue: #230

That PR implements the accumulator pattern using definitions and templates as it has been discussed here. Any feedback will be appreciated :-)

[damacus]

@zuazo is this still a thing? Or can we close this one out?

[lamont-granquist]

this is still a total mess.

you probably don't want to try to rescue what @zuazo did with definitions though. if you use the various helpers that i built in chef-12 then you can re-write it all as custom resources:

https://gist.github.com/lamont-granquist/b569d0086d19739ebeb3e8c7ab1d65bc

[damacus]

Yeah that was sort of my terrible, eventual plan. Rewrite with custom resources. I'll close his pr down for now then

[lamont-granquist]

If you want to chat about it ping me offline, I was starting to write up a rage comment about everything wrong with the resources in this cookbook, but decided against it since trashing the crap out of 4+ year old code which desperately needs a rewrite can sometimes be misconstrued as an attack on the authors (who couldn't have known any better at the time).

I suspect that this cookbook may be totally broken in Chef-13 without a fairly substantial rewrite. I still don't quite know how the empty providers are being wired up to actual code in libraries, but I suspect that has to be done through the class names that we removed in 13.0 so nothing will work.

[damacus]

OK I'm going to close this issue down, and run right away :D

Catch you at ChefConf in a few weeks!

[lamont-granquist]

Reminds me that I think I still need to book plane tickets...

[zuazo]

@damacus this is still a problem, yes. But as you correctly guessed, my old PR to solve it probably should be closed. AFAIK using definitions may not be the correct way to implement accumulator pattern nowadays, mainly because with Chef 13 we can use edit_resource to implement it. Something like the following:

action :create do
  with_run_context :root do
    edit_resource(:template, 'carbon.conf') do |new_resource|
      source 'carbon.conf.erb'
      variables[:config] ||= {}
      # TODO: merge variables[:config] with new_resource.config
      action :nothing
      delayed_action :create
    end
  end
end

But I have not tried it yet and, as mentioned in the document linked by @lamont-granquist, it seems to have some problems.

[damacus]

I'll be cribbing from the samba and haproxy accumulators :) Thanks @zuazo

[lamont-granquist]

the cookbook_name used by the template resource has an issue and needs to be forced:

https://github.com/chef-cookbooks/ssh_known_hosts/blob/master/resources/entry.rb#L78-L79

[lamont-granquist]

although i'm not certain that's an issue at all now -- the resource is really running in the cookbook_name that the resource is invoked in, not the one that its written in... fairly certain that's a feature not a bug...

i don't like that workaround though...

[lamont-granquist]

we probably need some kind of variable to stuff the cookbook the resource is defined in into, then the template resource should really search through that cookbook if it doesn't find it in the cookbook_name cookbook or something like that... basically make it so that it uses the template in the cookbook that its defined in, but the wrapper cookbook can override it, which is most likely what everyone wants to have just work right... same for cookbook_file, etc as well...

[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.