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

Default attributes set to nil can override attributes set at the same level #23

Closed
alq666 opened this Issue Sep 26, 2012 · 8 comments

Comments

Projects
None yet
4 participants
@alq666
Copy link
Contributor

alq666 commented Sep 26, 2012

In one instance, using our cookbook with chef solo causes the Redis attributes to be set to nil by our cookbook thereby causing the Redis cookbook to run with nil attributes. There are workaround for this, e.g. set the Redis attributes at a higher priority than that of our cookbook but we're not playing nice.

Suggested solution #1: namespace our attributes to read from the Redis attributes with overriding them.

Default['datadog']['redis']['server']=default['redis']['server'] || nil

In the proper ruby way (making sure the above won't bomb)

Suggested solution #2: write a lightweight provider for our stuff (nice and idiomatic) so that you can say

Datadog_monitor(Redis, server, port)

And the rest is taken care of for you (nice in that it will hide our transition to split configuration files)

@alq666

This comment has been minimized.

Copy link
Contributor Author

alq666 commented Sep 26, 2012

@miketheman your feedback is welcome.

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Sep 28, 2012

I like suggestion No.2 - this makes defining a datadog config very conf.d-style and awesome.

However, let me float No.3 (not using hashes, as they link to GH Issues...):

Instead of defining attributes that are "owned" by other namespaces and potentially causing overrides, maybe look at inspecting whether the attribute exists first?

Here's a short repl session in shef:

chef > node.attribute?(:platform)
 => true 
chef > node.attribute?(:redis)
 => false 
chef > node[:redis] = {:server => "localhost"}
 => {:server=>"localhost"} 
chef > node[:redis][:server]
 => "localhost" 
chef > node.attribute?(:redis)
 => true

So as you can see, we might want to inspect the existence of an attribute (or set) in a template before placing code. Code references are here, and I chose to use the alias attribute to be clear that we're looking for a node's attribute.

Something like:

<% if node.attribute?('redis') %>
# redis, from http://community.opscode.com/cookbooks/redis
redis_urls: <%= node['redis']['server']['addr']%>:<%= node['redis']['server']['port'] %>
<% end %>

Or better yet - look for the server subset:

chef > node.attribute?(:redis) && node[:redis].attribute?(:server)
 => true 

It may be exhaustive to test for every attribute, but it's kind of a safe assumption that "if you're using a well-known cookbook to deploy the software, then you'll have these set of attributes", and this allows the DD cookbook to not have to predefine them.

But to be clear - I like No.2 the best - but it's gotta be a little open-ended to allow for any variation of parameters.

@alq666

This comment has been minimized.

Copy link
Contributor Author

alq666 commented Sep 28, 2012

Option 3 makes sense. It'll minimize the disruption.
On Sep 27, 2012 10:56 PM, "Mike Fiedler" notifications@github.com wrote:

I like suggestion No.2 - this makes defining a datadog config very
conf.d-style and awesome.

However, let me float No.3 (not using hashes, as they link to GH
Issues...):

Instead of defining attributes that are "owned" by other namespaces and
potentially causing overrides, maybe look at inspecting whether the
attribute exists first?

Here's a short repl session in shef:

chef > node.attribute?(:platform)
=> true chef > node.attribute?(:redis)
=> false chef > node[:redis] = {:server => "localhost"}
=> {:server=>"localhost"} chef > node[:redis][:server]
=> "localhost" chef > node.attribute?(:redis)
=> true

So as you can see, we might want to inspect the existence of an attribute
(or set) in a template before placing code. Code references are herehttp://rubydoc.info/gems/chef/Chef/Node/Attribute#has_key%3F-instance_method,
and I chose to use the alias attribute to be clear that we're looking for
a node's attribute.

Something like:

<% if node.attribute?('redis') %># redis, from http://community.opscode.com/cookbooks/redisredis_urls: <%= node['redis']['server']['addr']%>:<%= node['redis']['server']['port'] %><% end %>

Or better yet - look for the server subset:

chef > node.attribute?(:redis) && node[:redis].attribute?(:server)
=> true

It may be exhaustive to test for every attribute, but it's kind of a safe
assumption that "if you're using a well-known cookbook to deploy the
software, then you'll have these set of attributes", and this allows the DD
cookbook to not have to predefine them.

But to be clear - I like No.2 the best - but it's gotta be a little
open-ended to allow for any variation of parameters.


Reply to this email directly or view it on GitHubhttps://github.com//issues/23#issuecomment-8964031.

@zimbatm

This comment has been minimized.

Copy link

zimbatm commented Oct 3, 2012

Solution 1 seems the most solid-one.

The biggest issue from re-using other cookbooks's values (apart that the current impl. breaks my redis cookbook) is that you then don't have the choice if you want to monitor or not. Client machines might have the node[:redis] value set but I don't really want to monitor the server from all the client hosts.

Solution 3 is nice too but it's more work. The datadog client needs to understand split config files (eg. /etc/datadog.d/*.conf) so that each LWRP resource can set it's monitor independently (to avoid ordering issues in the cookbook evaluation).

zimbatm pushed a commit to zimbatm/chef-datadog that referenced this issue Oct 3, 2012

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Oct 4, 2012

@zimbatm while adding namespace for attributes would work, it is very much duplication of effort and code. As to splitting the agent configs, this is already in progress (or done?) over in the agent repo.

You raise a good point about clients monitoring server, and that should probably be worked out as well, and can probably be handled with split agent configs.

I envision this working similar to the apache2 cookbook, where you can provide a list of components you want configs for (in a role or such) as well as the default attributes you want to provide to those configs.

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Oct 6, 2012

@zimbatm Here's the references I wrote about:

A conf.d-style approach in the agent: DataDog/dd-agent#194

Apache2 cookbooks module inclusion: https://github.com/opscode-cookbooks/apache2/blob/master/attributes/default.rb#L140-149

@ansel1

This comment has been minimized.

Copy link

ansel1 commented Oct 24, 2012

So others can find this more quickly than I did, the manifestation of this may be something like:

NoMethodError
-------------
undefined method `[]=' for nil:NilClass

Cookbook Trace:
---------------
  /tmp/vagrant-chef-1/chef-solo-1/cookbooks/mysql/attributes/server.rb:81:in `from_file'

datadog is near the beginning of my run list. During compilation phase, it sets default['mysql']['server'] = nil, then mysql cookbooks attributes are compiled, which tries default['mysql']['server']['packages'] = %w{mysql-server}, and fails.

ansel1 added a commit to safe-net/chef-datadog that referenced this issue Oct 24, 2012

Stop clobbering other cookbook's attributes
Fixes DataDog#23

comment out default values in attributes/default.rb.  Instead of setting default values (which can override the real cookbooks' default values), be more paranoid about checking for existence of those nodes in the template.

@ghost ghost assigned alq666 Oct 29, 2012

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Apr 23, 2013

I believe this has been resolved in master branch, to be released soon in the upcoming 1.0.0 release, completed by #46.

Thanks everyone for surfacing your thoughts and helping figure out what the approach should be!

@miketheman miketheman closed this Apr 23, 2013

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.