Question on attributes "methodology"

[lusis]

(Apologies if this sounds accusatory. It is not and I'm trying to understand the design goals before submitting pull requests)

What exactly is the reasoning/what's trying be accomplished with the attributes being handled the way they are? It causes a lot of unexpected behavior (see changing version or even the datadir) and goes against the entire attribute model of chef.

I understand that attributes were much more "messy" in Chef 10. Is that the reasoning? Was part of the intention here to minimize having to add support for all the possible configuration file options for ES?

I'd like to try and clean the cookbook up so that it works the way a user would expect (as in overriding attributes should work). It feels like maybe it's time for a bit of a reboot of the cookbook?

It feels like maybe some things that are being attempted should be rolled into an LWRP?

[karmi]

Thanks for the question, and no worries, John!

The design goals for the attributes handling in this cookbook were something like "make this utter mess which is Chef work as we need" :) Let me provide a bit of context throughout the answer.

The cookbook has been extracted from a real project, which needed a non-trivial Elasticsearch setup (HTTP proxies with authentication, EBS volumes, Monit supervising, ...) which was part of a larger infrastructure, all driven by Chef Server, implemented by me and a former colleague, @vhyza.

I'd like to try and clean the cookbook up so that it works the way a user would expect (as in overriding attributes should work).

Yes, attribute handling is a shame, and there are issues opened for them, mostly #178 and #100. There has been some fixing going on around attributes in the last months, but it was all superficial.

Note, that the cookbook is a bit "special" (in all the ambiguity of the word :), that it allows you to set node attributes via standard Chef means, but also from dedicated databags.

This is the main reason for the strange and buggy behaviour of attributes, as far as I remember. There's a note at #89 (comment) with a suggestion how to refactor all that stuff. This would solve many more issues, such as #150.

As a side note, I wasn't following the Chef gem development closely for a while, but I do know it has improved significantly, attribute handling being one area of improvement. What's strange, though, is that I always had trouble to consistently replicate reported issues in Vagrant or EC2 -- sometimes I could replicate the bug, sometimes not, sometimes I have discovered another bug.

All in all, I'm strongly in favour of any consistent attribute handling in this cookbook; nothing ruins the user experience more than problems here.

There's issue #179 opened for that, which solves couple of problems, but leaves something to be desired usability-wise: you can't set node.elasticsearch.download_url to a custom URL and let the cookbook pick that up.

I haven't made time to dilligently debug the issues, but my feeling is that the cookbook should get rid of the chef-ark provider, and use regular a remote_file, bash "tar xvf" combo. There are many issues reported at Github, which in one way or another have something to do with chef-ark. I would start any potential refactoring here.

It feels like maybe it's time for a bit of a reboot of the cookbook?

First, I'm always for getting into the jumpsuit and refactoring the hell out of a piece of software :) On the other hand, what does it mean in concrete terms? I don't feel like the cookbook is in some kind of catastrophic state, or that large parts of it are not working. It has a rather significant testing suite (Vagrant-based), and I regularly test larger pull requests or changes on EC2 as well... I would say it's one of the better coded, tested and maintained cookbooks out there, also considering the broad scope of features it has.

In any case, I'm all for discussing any improvements to the cookbook, code-, docs-, test- or feature-wise! The problem on my side is that I was quite busy with other Ruby and Elasticsearch related open source projects lately, so I was short on time. This was amplified by the fact, that I usually verify any significant change manually in Vagrant and on EC2.

It feels like maybe some things that are being attempted should be rolled into an LWRP?

I'm not sure about LWRPs, but would be sincerely interested in concrete examples of such refactorings!

The thing is, my experience with Chef as a framework and as a Ruby codebase here is -- I'm sorry to say that -- horrible, horrible beyond imagination.

That's, for example, why the extensions like print_value or create_ebs are implemented as regular Ruby "monkeypatches" or "mixins" -- it is simply easier to write and test it like that. When I was looking up the examples and implementation of LWRPs, I was always put off by the complexity and . As noted, I haven't been following the latest Chef development, so I bet it has got somewhat better, and I imagine that some features -- eg. create_ebs -- could be implemented as true LWRPs, if the benefits are clear.

That's also why the dependency on the main Nginx cookbook was removed, after long struggle, in e9cfb00. The problems we were running into were simply impossible to live out. I was thinking about doing the same with the dependency on the Java cookbook, but it has settled in the meantime.

Was part of the intention here to minimize having to add support for all the possible configuration file options for ES?

Can you please clarify this point a bit? (There's the print_value extension method for handling ES configurations.)

[lusis]

@karmi

Thanks for the feedback! I'm still going over it but I wanted to give you a solid example of an LWRP approach that I think might help simplify the cookbook a bit. This is one of my internal recipes ( from using https://github.com/SimpleFinance/chef-elasticsearch ) :

elasticsearch_instance "logstash" do
  destination_dir "/data/elasticsearch"
  user "elasticsearch"
  group "elasticsearch"
  install_options({
    :version => "0.90.10",
    :url => "https://download.elasticsearch.org/elasticsearch/elasticsearch"
  })
  service_options({
    'ES_HEAP_SIZE' => '4096m'
  })
end

elasticsearch_config "logstash_config" do
  instance "logstash"
  config_type "elasticsearch"
  config_options({
    :cluster => { :name => "logstash" },
    :bootstrap => {:mlockall => true },
    :node => {
      :name => node.name,
      :data => true,
      :master => true
    },
    :index => {
      :number_of_shards => 5,
      :number_of_replicas => 1
    }
  })
end

elasticsearch_config "logstash_logging_config" do
  instance "logstash"
  config_type "logging"
  config_options({
    :rootLogger => "INFO, file",
    :logger => {
      :action => "DEBUG",
    },
    :appender => {
      :file => {
        :type => "dailyRollingFile",
        :file => "${path.logs}/${cluster.name}.log",
        :layout => {
          :type => "pattern",
          :conversionPattern => "[%d{ISO8601}][%-5p][%-25c] %m%n"
        }
      }
    }
  })
end

I'm seriously considering adding a similar approach for my logstash cookbook (shout out to @miah) here since the LWRP approach was her idea really.

Note that the benefit is that you don't need to actually ADD a bunch of magic for handling all the possible options. It also supports adding plugins as well with a elasticsearch_plugin resource.

So by leveraging an LWRP approach, you can greatly simplify the cookbook and build a canned server approach in the recipes you DO ship with. It also cleans up the attribute sprawl I think since there are less attributes to manage.

I'm still reading over the rest of your notes.

[lusis]

also one more side note, you can see some of the other examples from that cookbook here. Note that I just discovered it last night so I'm still putting it through the paces:

https://github.com/SimpleFinance/chef-elasticsearch/blob/master/test/cookbooks/es_test/recipes/plugins.rb

[karmi]

John, thanks for your reply!

I wanted to give you a solid example of an LWRP approach that I think might help simplify the cookbook a bit (...)

I'm very well aware of the approach @miah took in those cookbooks, and we have been briefly conversing about it couple of months ago.

I'm afraid I'm still not sold on the approach, mainly because it's not entirely obvious to me how it would affect the end-user experience when using the cookbook, and because I feel it might raise the barrier for contributors. On the other hand, a very lucid example of using LWRPs quite elegantly can be found in the redisio cookbook.

I'm looking for more feedback!

[andrewgross]

+1 for LWRP. (or HWRP)

[miah]

The problem with Attributes can best be summarized by Action at a distance.

[karmi]

@lusis Any more feedback here?

[karmi]

@lusis Hi, closing the ticket, since it seems we've exhausted the topic...

[karmi]

For reference, there's an interesting approach suggested in https://coderanger.net/derived-attributes/, where Noah Kantrowitz proposes interpolating the dynamic parts of attributes with %{}, which sounds promising.

[miah]

Its a ugly pattern that nobody in Ruby uses though. (sorry Noah).

http://batsov.com/articles/2013/06/27/the-elements-of-style-in-ruby-number-2-favor-sprintf-format-over-string-number-percent/

[karmi]

@miah That feels like a bit rigid comment to me -- the article proposes it as a workaround for the problem with attributes in Chef itself. I'm not sure if that has been improved in Chef or not, but that has been the underlying problem (aggravated by the crazy mixing of attributes in this particular cookbook).

[miah]

Could you elaborate on how the attributes are crazy?

Do you mean from setting a attribute to the value of another attribute?

You could do this

foo = 'bar'
bar = "buzz #{ foo }"
node.default[:buz] = foo
node.default[:muz] = bar

This avoids the problem of attributes defined in 'attributes/*.rb' from having potentially nil values inserted due to 'node' not being compiled and being able to infer the value you are referencing. Instead of forcing the value to be evaluated later; we are doing it sooner.

[karmi]

Well, the problem is with the wrapper cookbook, mentioned in the article:

What this means is if we make a wrapper cookbook with an updated version attribute, it won’t have the desired effect:

default['version'] = '2.0'

Remembering that attribute files are processed in dependency order, that means that the evaluated code is effectively:

default['version'] = '1.0'
default['url'] = "http://example.com/#{node['version']}.zip"
default['version'] = '2.0'

[miah]

Wrapper cookbooks shouldn't use the same precedence level. Additionally attributes being overridden in wrapper cookbooks should probably be defined in the recipes themselves.

Remember; we have to deal with this:

But I'm not a proponent of much of the Cookbook Workflow in the community either because it stems from cargo cult-ed misunderstandings of Ruby.

If you develop a Resource & Provider you don't have to include any default attributes which can pollute the 'node' namespace. Instead you define attributes on your resource and set their default values. You don't need to create an attributes/*.rb file at all. Then 'wrappers' only define the values they need without having to override them.

[martinb3]

Switching to an LWRP approach (and we could still provide the recipes from before for those who want them) seems like the only solution that will make both camps happy.

[miah]

My recommendation would be to add Resources and Providers, migrate the recipes to using them, then eventually phase out the recipes. Once you have a clearly defined interface (resources) users can do what they want in their own internal recipes.

Recipes do not offer a 'better' solution than Resources. The comparison here is 'assorted shell scripts' vs 'specialized tool'.

Recipes give you the 'assorted shell scripts' that kind of equals the abstraction you are trying to build. But there is absolutely no coupling between resources in recipes.

Resources give you a single object, that can perform actions through its provider. Resources are the abstraction of what you are trying to build. Providers are the 'action' phase of your resource that enforces the state you want to apply on your node. You create the resource and set it up as you would any object, then 'run_action' and it applies the action/state via the Provider.

[karmi]

Hi, I've merged #242, so this bug should be fixed now. Keeping this issue open for any further discussion.