Skip to content
This repository has been archived by the owner on Jun 13, 2019. It is now read-only.

Update to add an instance_id attribute. #26

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

saheemg
Copy link

@saheemg saheemg commented Nov 21, 2016

zookeeper V3.5.2-alpha dynamically generates a configuration file and updates the configuration file generated by chef to move the ensemble information to the dynamic file. In my chef cookbook, I will generate the dynamic file and stop this cookbook from generating a config file that zookeeper will update. I will do this by not setting the ensemble field. That works but the problem is with my_id.

zookeeper V3.5.2-alpha dynamically generates a configuration file and updates the configuration file generated by chef to move the ensemble information to the dynamic file.   In my chef cookbook, I will generate the dynamic file  and stop this cookbook from generating a config file that zookeeper will update.  I will do this by not setting the ensemble field. That works but the problem is with my_id.
@saheemg
Copy link
Author

saheemg commented Nov 21, 2016

@johnbellone

@@ -34,7 +35,11 @@ class ZookeeperConfig < Chef::Resource
attribute(:properties, option_collector: true, default: {})

def myid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would implement this slightly differently. I would set the default value of the instance_id attribute to be the index of the element in the ensemble array. After that we simply add an alias to the method so that #myid keeps on working.

attribute(:instance_id, kind_of: Integer, default: lazy { ensemble.index(instance_name).next.to_s })
alias_method :myid, :instance_id

@saheemg
Copy link
Author

saheemg commented Apr 10, 2017

@johnbellone Did what you suggested... sorry it took so long.

@saheemg
Copy link
Author

saheemg commented Apr 11, 2017

@johnbellone

notifies :create, 'file[config_base]', :immediately
end

file 'config_base' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you creating a file called config_base?

content new_resource.to_s
path new_resource.path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah missed that, nevermind.

Copy link
Contributor

@johnbellone johnbellone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain more detail why you specifically want the change to how the configuration is written, because I do not understand reasoning,

# this is needed to ensure chef won't constantly update
# zoo.properties and force restarts of zookeeper when
# it dynamically updates zoo.properties.
file new_resource.path+".static" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the file continually updating? This mechanism won't change that it just makes it more obfuscated.

@saheemg
Copy link
Author

saheemg commented Apr 12, 2017

@johnbellone Thanks for taking a look. zookeeper 3.5.2 uses dynamic config. That is, at start up, it takes the defined zoo.properties file, removes the ensemble property, creates a new zoo.properties.dyanmic. file, and then adds a dynamicConfigFile property pointing to this file in the original zoo.properties. So when chef runs, it detects zoo.properties has changed, updates it, and restart zookeeper. zookeeper then once again goes through the process which then chef to reset/restart, and on, and on....

This change will only result in chef updating zoo.properties and restarting zookeeper when the cookbook changes the configuration. This is done by making the zoo.properties.static the file chef compares against. When it changes due to cookbook changes, chef rewrites zoo.properties.static, then zoo.properties, and finally, restarts zookeeper.

zookeeper still uses zoo.properties as its configuration file (not zoo.properties.static)

@johnbellone
Copy link
Contributor

I think that given that new knowledge I would only write the configuration file if the configuration file does not exist. Otherwise, you almost always have the potential of overwriting the changes that the Zookeeper process has made, and this PR only really limits that to when you actually change the attributes inside of the cookbook/policy.

The other alternative, but it is very much more invasive, is re-writing the resource to take into account the configuration on disk and merge in the attributes that are changed by the cookbook/policy. This is a much more advanced topic and we generally do not do this since most of what we control with Chef does not actually modify the configuration files at runtime.

@saheemg
Copy link
Author

saheemg commented Apr 12, 2017 via email

@johnbellone
Copy link
Contributor

How do you ensure that the cookbook updates do not conflict with the updates that the Zookeeper service made? I am also can't seem to find any documentation on the operations section of the Zookeeper documentation about writing configuration out from the service. Do you have a link so I can read up?

@saheemg
Copy link
Author

saheemg commented Apr 12, 2017 via email

@johnbellone
Copy link
Contributor

Yep, I just found that page. This seems to indicate that the dynamic configuration is stored in a different location. Does the regular configuration get rewritten?

@johnbellone
Copy link
Contributor

It looks like you can set this parameter dynamicConfigFile=/zookeeper/conf/zoo_replicated1.cfg.dynamic

@saheemg
Copy link
Author

saheemg commented Apr 12, 2017

Regular configuration does get rewritten once zookeeper starts and creates/updates dynamic config.

I don't see how changing my approach to setting the dynamicConfigFile=/zookeeper/conf/zoo_replicated1.cfg.dynamic is better. I'd have to add support in cookbook to update/create the dynamic file and base config file. The way I am doing it now, cookbook just manages it all in the .static file and lets zookeeper manage the transition from static properties to the static properties+dynamic properties.

@saheemg
Copy link
Author

saheemg commented Apr 13, 2017

@johnbellone ping

@johnbellone
Copy link
Contributor

There is nothing wrong with it per say, but it is the inverse of what the operations manual tells you to do, and we've been following the guidelines there for the configuration resource.

Also, why would you add support to update the dynamic file? My understanding was that this cookbook would manage the static file, and Zookeeper would write dynamic file out to disk to the path that we tell it to? That means we add a new property on the resource, the file gets written out, and this resource only bounces the ZK task when the static file updates.

Am I missing something?

@saheemg
Copy link
Author

saheemg commented Apr 13, 2017

I may have misunderstood your suggestion. I thought you suggested to avoid zookeeper's dynamic config by having the cookbook create
'/zookeeper/conf/zoo.properties' and '/zookeeper/conf/zoo_replicated1.cfg.dynamic', where zoo.properties set 'dynamicConfigFile=/zookeeper/conf/zoo_replicated1.cfg.dynamic'

What I implemented here is intended to do exactly what you describe.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants