Replace all MCollective agent calls with the agent lib. #44
Conversation
Added new mcollective class in the plugins directory that will execute mcollective via ruby rather than using system calls. This will replace and consolidate the use_mco_ruby and use_mcollective options into a single option.
The mco helper method has been replaced by the PuppetWebhook::MCollective class and as such has been replaced.
lib/helpers/deployments.rb
Outdated
| # Currently requires puppet_webhook to be run as a non-root | ||
| # user with access to running the MCollective client. | ||
| if settings.use_mcollective | ||
| result = PuppetWebhook::Mcollective.new('r10k', |
There was a problem hiding this comment.
This should probably be turned into a helper method to reduce duplicated code.
lib/helpers/deployments.rb
Outdated
| result = PuppetWebhook::Mcollective.new('r10k', | ||
| 'deploy', | ||
| { | ||
| dtimeoute: settings.discovery_timeout, |
| # If you don't use mcollective then this hook needs to be running as r10k's user i.e. root | ||
| "#{settings.command_prefix} r10k deploy environment #{branch} #{settings.r10k_deploy_arguments}" | ||
| end | ||
| command = "#{settings.command_prefix} r10k deploy environment #{branch} #{settings.r10k_deploy_arguments}" |
There was a problem hiding this comment.
Are all these variables sanitized already? eg, what if branch was set to ; rm -rf /;?
There was a problem hiding this comment.
We are not sanitizing that, no. I'm not sure if that config parameter will still be around in the future. I'm planning on (attempting) to replace r10k open3 calls with the r10k libs (if possible).
I want to remove run_command if at all possible. (Maybe re implement it as another plugin or through something like MCO or bolt)
lib/helpers/deployments.rb
Outdated
| result = PuppetWebhook::Mcollective.new('r10k', | ||
| 'deploy', | ||
| { | ||
| dtimeoute: settings.discovery_timeout, |
There was a problem hiding this comment.
cut&paste, or is it really spelled like that? (if so, maybe a comment that the spelling is intentional)
There was a problem hiding this comment.
Good catch. I just spelled that wrong.
dhollinger
force-pushed
the
mco_lib_calls
branch
from
e281301
to
8ad935b
Compare
lib/helpers/deployments.rb
Outdated
| timeout: settings.client_timeout | ||
| }, | ||
| settings.client_cfg, | ||
| environment: branch).run.first | ||
| raise result.results[:statusmsg] unless result.results[:statuscode].zero? |
There was a problem hiding this comment.
you should also check the RPC stats that the client return and check for nodes that did not respond. You can get to that via the client.stats method.
It'll be an instance of this https://github.com/puppetlabs/marionette-collective/blob/master/lib/mcollective/rpc/stats.rb and there check noresponsefrom
There was a problem hiding this comment.
Good point. I've added some code for that.
@ripienaar @binford2k - could one/both of you give one more pass over before I merge? I added some code to both the plugin and the deployments helper methods based on @ripienaar's comment.
dhollinger
force-pushed
the
mco_lib_calls
branch
from
e081455
to
bf4706d
Compare
dhollinger
force-pushed
the
mco_lib_calls
branch
from
bf4706d
to
8d79caf
Compare
| results.each do |result| | ||
| raise result.results[:statusmsg] unless result.results[:statuscode].zero? | ||
| end | ||
| raise results.stats[:noresponsefrom] unless result.stats[:noresponsefrom].length.zero? |
There was a problem hiding this comment.
seems good, though raising an array is super weird :P
There was a problem hiding this comment.
Each RPC Result object is stored as an array and I couldn't think of a better way (at the time) to validate that each result status code was 0.
This is an initial feature change. The goal was to mirror existing MCollective functionality, but using the Agent library. As the project moves along, additional functionality will be added.
Special thanks to @ripienaar for help.