fix adding nil if hostname isn't set up first #18

TD-4242 · 2015-05-20T17:21:20Z

This one small block of code had caused me more headache then anything else so I wanted to send a fix.

In my chef runs the hostname may not be setup prior to the first run so I would always get a compile error if cerner_kafka was in my run_list with:

undefined method `+' for nil:NilClass

Then the entire run would die and it would never setup the host. by moving the brokerId increment after the check and using a return I can now bail out of the cerner_kafka cookbook on my first run and allow the host to be setup and kafka can get setup on a consecutive run. I also added a bit better message to output so I could see what each item was set to and what it was looking for.

A better way may be to do a lazy evaluation of the broker ID prior to writing the template.

bbaugher · 2015-05-20T17:26:08Z

recipes/default.rb

  if brokerId.nil?
-    errors.push "Unable to find node in node[:kafka][:brokers] : #{node["kafka"]["brokers"]}"
+    return "Unable to find #{node["fqdn"]}, #{node["ipaddress"]} or #{node["hostname"]} in node[:kafka][:brokers] : #{node["kafka"]["brokers"]}"


Is return correct here? I would think errors.push would be what we want.

TD-4242 · 2015-05-20T18:09:56Z

I tried with errors.push but then the next line where we increment the brokerId errors with the undefined method +' for nil:NilClass` message halting the chef run. Using the return logs the message but continues the chef run without processing the rest of the recipe. This allows my other recipes to configure the hostname and default setup and kafka will work on the next chef-client run.

Like I mentioned it can probably be done better by moving all the logic to the template generation and use lazy loading for the variables. This would allow the rest of the chef run to complete and still configure kafka in the first path.

I'll dig a little deeper and see what's required to do this.

bbaugher · 2015-05-20T18:32:09Z

Ugh duh should learn to read better. If we don't assign a broker id I'm not sure if kafka will start.

Your work around of not running the recipe seems reasonable but conflicts with someone mis-configuring their node and us not being able to come up with an id. It does seem to log a message about it but that assumes someone is watching and possibly watching closely.

If you really need to run chef twice in this instance would it be worth trying to use an override run list the first time?

TD-4242 · 2015-05-20T19:02:49Z

A single pass client run is of course ideal. I'd really just like for my host setup stuff to run before failing the chef run. As it was it wouldn't even compile the run_list unless the servers hostname was in the brokers list. If I could get the rest of the chef run to run first then check if the brokerId can be set and bail on error at that point I think it would work out better overall. At least if the kafka recipe is run last the host should be in a mostly configured state and ready for a second run after fixing the settings.

I'm trying to set this up to automatically deploy with chef-provisioning in a completely hands off manner, and allow for semi-auto up scaling if needed.

TD-4242 · 2015-05-20T21:22:51Z

re-arranged a few things. I still have it return on any errors so the run will complete, but it pushes the errors to the array like it did before.

I also removed some unneeded checks and cast the brokers to an array prior to checking it. This would allow you to specify a single broker as a string.

bbaugher · 2015-05-21T01:45:29Z

Can we make the behavior to ignore errors and continue an attribute and by default keep the behavior as it was before?

bbaugher · 2015-05-21T15:54:29Z

Should have asked this earlier but if we just make the check run during converge instead of compile will that work for you? We should just be able to wrap that ruby code in a ruby_block.

TD-4242 · 2015-05-21T16:29:36Z

I had the same idea and tried just wrapping it in a ruby_block and it didn't seem to update the node object. I wasn't sure if i was doing it right so I backed that out and made this change. having a continue_on_errorr attribute default to false would be fine as well. Although if it could be moved to converge phase I think that wold be the 'chef' way to do it.

…e convergance rather than compile phase.

TD-4242 · 2015-05-21T17:35:19Z

Ok, i just retried this and tested it in kitchen. I'll update in a second if you want to take a look. When I unset broker.id in .kitchen.yml I got:

 Recipe: cerner_kafka::default


           ================================================================================
           Error executing action `run` on resource 'ruby_block[assert broker and zookeeper lists are correct]'
           ================================================================================

           RuntimeError
           ------------
           Unable to run kafka::default :
            -node[:kafka][:brokers] or node[:kafka][:server.properties][:broker.id] must be set properly]

           Cookbook Trace:
           ---------------
           /tmp/kitchen/cookbooks/cerner_kafka/recipes/default.rb:36:in `block (2 levels) in from_file'

           Resource Declaration:
           ---------------------
           # In /tmp/kitchen/cookbooks/cerner_kafka/recipes/default.rb

         9: ruby_block 'assert broker and zookeeper lists are correct' do
...

if I set kafka.brokers to [test1,test2] I get:

       Error executing action `run` on resource 'ruby_block[assert broker and zookeeper lists are correct]'
           ================================================================================

           RuntimeError
           ------------
       Unable to run kafka::default :
            -Unable to find kafka, 10.0.2.15 or kafka in node[:kafka][:brokers] : ["test1", "test2"]]

which should be a little more helpful of an error message because you'll see what it's actually looking for if your hostname isn't in the array.

adding kafka (the hostname set in kitchen) to the list it runs as normal with the brokerId based on the array index.

I think this should cover everyone's expectations, am I right?

TD-4242 · 2015-05-21T17:49:30Z

working on the rspec tests now

TD-4242 · 2015-05-21T18:10:55Z

I think chefspec is overriding the code in the ruby_block and not allowing it to execute. Makes sense because if you had something in there to modify the host it would do it to the host running the tests. I'm not sure how to test it other than with an assertion that the ruby_block is there...

bbaugher · 2015-05-21T18:20:15Z

I think I'm ok with us not verifying that behavior anymore in chefspec. If we are really worried about we can move the code to a library and test that.

TD-4242 · 2015-05-21T19:40:13Z

ok, i was able to get all the current tests to run with chefdk 0.5.1 lets see if the work in travis under the older version of chefspec.

TD-4242 · 2015-05-21T19:55:14Z

something's not right. The block is running but I don't think it's updating the node object.

…y ruby_block

TD-4242 · 2015-05-21T20:19:28Z

OK, i found it. because the assignment is happening now at converge time the empty array is being assigned to the template resource for zookeepers and brokerid. I changed those to lazy evaluation so they will be set at run time after the ruby_block

TD-4242 · 2015-05-21T20:27:44Z

Of course now foodcritic is telling us to just make it a library like you suggested earlier.

Ok, not sure I can get to that today. let me see how some of these other deployments go.

bbaugher · 2015-05-21T20:29:35Z

I'm fine with ignoring foodcritic for now about this. You can fix it by changing this line, https://github.com/cerner/cerner_kafka/blob/master/Rakefile#L29

TD-4242 · 2015-05-21T20:37:32Z

I added the comment way to disable foodcritic warnings, that allows it to stay visible and works as a reminder that that section of code isn't as awesome as it could be.

bbaugher · 2015-05-28T20:21:15Z

I can't remember where we left off but this looks good to me if you are fine with it

TD-4242 · 2015-05-28T23:05:53Z

I'm using it with chef provisioning to deploy kafka to aws and it's working well.

bbaugher · 2015-05-29T01:39:16Z

@noslowerdna or @mkwhitacre can you take a look at this?

mkwhitacre · 2015-05-29T15:45:41Z

+1

fix adding nil if hostname isn't set up first

bbaugher · 2015-05-29T15:46:56Z

@TD-4242 Thanks for the pull!

bbaugher · 2015-08-17T21:08:55Z

@TD-4242 do you mind opening a pull request to add yourself as a contributor.

fix adding nil if hostname isn't set up first

54665eb

bbaugher reviewed May 20, 2015
View reviewed changes

this should do more what the original codes intent was

662e81c

moved brokers array assertion to a ruby block so it is executed in th…

87f1770

…e convergance rather than compile phase.

update spec tests

c92f957

changed template evaluation to lazy in order to pickup changes made b…

5514d41

…y ruby_block

disable foodcritic warning on long ruby_block

2e80da4

bbaugher added the enhancement label May 29, 2015

bbaugher added this to the 1.1.0 milestone May 29, 2015

bbaugher added a commit that referenced this pull request May 29, 2015

Merge pull request #18 from TD-4242/master

810283c

fix adding nil if hostname isn't set up first

bbaugher merged commit 810283c into cerner:master May 29, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix adding nil if hostname isn't set up first #18

fix adding nil if hostname isn't set up first #18

TD-4242 commented May 20, 2015

bbaugher May 20, 2015

TD-4242 commented May 20, 2015

bbaugher commented May 20, 2015

TD-4242 commented May 20, 2015

TD-4242 commented May 20, 2015

bbaugher commented May 21, 2015

bbaugher commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

bbaugher commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

bbaugher commented May 21, 2015

TD-4242 commented May 21, 2015

bbaugher commented May 28, 2015

TD-4242 commented May 28, 2015

bbaugher commented May 29, 2015

mkwhitacre commented May 29, 2015

bbaugher commented May 29, 2015

bbaugher commented Aug 17, 2015

fix adding nil if hostname isn't set up first #18

fix adding nil if hostname isn't set up first #18

Conversation

TD-4242 commented May 20, 2015

bbaugher May 20, 2015

Choose a reason for hiding this comment

TD-4242 commented May 20, 2015

bbaugher commented May 20, 2015

TD-4242 commented May 20, 2015

TD-4242 commented May 20, 2015

bbaugher commented May 21, 2015

bbaugher commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

bbaugher commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

TD-4242 commented May 21, 2015

bbaugher commented May 21, 2015

TD-4242 commented May 21, 2015

bbaugher commented May 28, 2015

TD-4242 commented May 28, 2015

bbaugher commented May 29, 2015

mkwhitacre commented May 29, 2015

bbaugher commented May 29, 2015

bbaugher commented Aug 17, 2015