Add concurrency option #7

dudemcbacon · 2015-04-11T05:56:45Z

Adding concurrency option to control the number of concurrent hosts that SSH is run on.

mmoll · 2015-04-12T10:50:37Z

lib/hammer_cli_foreman/ssh.rb

@@ -1,6 +1,7 @@
 require 'hammer_cli'
 require 'hammer_cli_foreman/host'
 require 'net/ssh/multi'
+require 'pry'


leftover from testing? :)

Derp. Let me get rid of that...

dudemcbacon · 2015-04-12T22:33:23Z

I also noticed there were some race conditions affecting net-ssh-multi when setting concurrent_connections -- fortunately, they have been fixed in an upstream pull request. I've updated the gemspec to reflect the new dependency.

See this pull request for more information on the race conditions.

mmoll · 2015-04-12T22:36:50Z

Thats, OK, we can handle the version dependency in our packaging, I guess...
@ohadlevy, could you test?

mmoll · 2015-04-13T00:24:47Z

@dudemcbacon BTW, could you squash your commits together into one?

dudemcbacon · 2015-04-13T22:24:36Z

Done!

ohadlevy · 2015-04-14T11:45:47Z

lib/hammer_cli_foreman/ssh.rb

@@ -12,6 +12,7 @@ class Command < HammerCLIForeman::Command

    command_name 'SSH to hosts'
    option %w(-c --command), 'COMMAND', _('Command to execute'), :attribute_name => :command, :required => true
+    option %w(-n --concurrent), 'CONCURRENCY', _('Number of concurrent SSH sessions'), :attribute_name => :concurrent


should we provide a default? i think asking each time could be annoying ?

I'd actually prefer if we didn't. It'd make the code more complex. When the option isn't set and no default is specified concurrent == nil. When concurrent_connections is set to nil (and passed to Net::SSH::Multi) it disables the concurrency limit. If we set a default value for concurrent it will be set to whatever the default is even if the option isn't specified. We'd either need to add an option to disable concurrency limits or use some other method to determine when the option wasn't actually passed by the user.

"it disables the concurrency limit." means 1 or infinity?

infinity -- as in the way it behaved before this pull request.

I guess if it's like before I'm OK with it - and see this PR even more valuable as it's making a limit possible. 🎯

You can add

option ....... do |o| Integer(o) end

for Hammer to ensure valid Integer is given

+1 to not force user to set concurrency limit

dudemcbacon · 2015-04-14T20:22:39Z

I have modified the pull request slightly in the following ways:

move the logic for concurrent to the top of the execute method -- that way if the parameter supplied for concurrent is 0 (or anything that the .to_i method might turn into 0) it will display an error message before the host list and confirmation prompt.
Added a variable 'limit' which is set to either nil or an integer depending on whether the user sets the concurrent option when they run hammer.
Concurrent_connections is set to the limit variable (as either nil or an integer).

This fixes the assumption I made about the .to_i method when used with nil and assures the following behavior:

If the concurrent option is specified with an integer > 0 then the number of concurrent connections Net::SSH::Multi will allow is set to that integer.
If the concurrent option is not specified than the original behavior of not limiting the number of concurrent_connections will be present.

mbacovsky · 2015-04-15T08:57:15Z

lib/hammer_cli_foreman/ssh.rb

@@ -28,6 +29,13 @@ def request_params
    end

    def execute
+      if concurrent && concurrent.to_i == 0
+        warn _("specify 1 or more concurrent hosts")
+        return HammerCLI::EX_OK


I'd prefer to

signal_usage_error(_("specify 1 or more concurrent hosts")) if (!concurrent.nil? && concurrent < 1)

to get proper exit code and error handling

mbacovsky · 2015-04-15T09:16:47Z

Besides the minor things mentioned inline it looks good. Thanks @dudemcbacon!

mbacovsky · 2015-04-15T15:01:33Z

@dudemcbacon, could you please rebase?

dudemcbacon · 2015-04-15T17:43:44Z

I have rebased this pull request and modified the code as suggested by @mbacovsky.

mbacovsky · 2015-04-16T21:20:00Z

lib/hammer_cli_foreman_ssh/ssh.rb

@@ -12,6 +12,10 @@ class Command < HammerCLIForeman::Command

    command_name 'SSH to hosts'
    option %w(-c --command), 'COMMAND', _('Command to execute'), :attribute_name => :command, :required => true
+    option %w(-n --concurrent), 'CONCURRENCY', _('Number of concurrent SSH sessions'), :attribute_name => :concurrent do |o|
+      Integer(o)
+      o.nil? ? o : o.to_i


this line is not neccessary, as the block is executed only if the parameter was set

Ahh, good catch. I guess I didn't think about it that much. I will remove it.

mbacovsky · 2015-04-16T21:25:38Z

@dudemcbacon, thanks for the update! Looks good now!
I'm curious about the extra line in the option block, was it added to any unexpected behaviour?

…hat SSH is run on; also updating gemspec dependency for net-ssh-multi to address race conditions affecting that gem and concurrency.

dudemcbacon · 2015-04-16T22:36:18Z

I have removed the line you commented on in the option block.

dudemcbacon · 2015-04-16T22:48:24Z

The line you're referring to was to ensure that the concurrent variable was set to either nil or a number > 0. As you said in your inline comment, it's not necessary. If the option block is called it will be a number. If the option block is not called, it will be nil. If the integer is 0, it will be filtered by the signal_usage_error method.

mbacovsky · 2015-04-17T13:32:59Z

👍 @dudemcbacon, that makes sense. Thank you for your patience ;) 🍺 Merging...

Add concurrency option

dudemcbacon · 2015-04-17T18:23:10Z

w00t!

mmoll reviewed Apr 12, 2015
View reviewed changes

dudemcbacon force-pushed the master branch from c601808 to 9418ed5 Compare April 13, 2015 22:24

ohadlevy reviewed Apr 14, 2015
View reviewed changes

dudemcbacon force-pushed the master branch from 9418ed5 to 3909c8e Compare April 14, 2015 20:15

mbacovsky reviewed Apr 15, 2015
View reviewed changes

dudemcbacon force-pushed the master branch from e373efe to 036b98b Compare April 15, 2015 17:34

mbacovsky reviewed Apr 16, 2015
View reviewed changes

Adding concurrency option to control the number of concurrent hosts t…

fb2f345

…hat SSH is run on; also updating gemspec dependency for net-ssh-multi to address race conditions affecting that gem and concurrency.

dudemcbacon force-pushed the master branch from 036b98b to fb2f345 Compare April 16, 2015 22:34

dudemcbacon closed this Apr 16, 2015

dudemcbacon reopened this Apr 16, 2015

mbacovsky added a commit that referenced this pull request Apr 17, 2015

Merge pull request #7 from dudemcbacon/master

4abe922

Add concurrency option

mbacovsky merged commit 4abe922 into theforeman:master Apr 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add concurrency option #7

Add concurrency option #7

dudemcbacon commented Apr 11, 2015

mmoll Apr 12, 2015

dudemcbacon Apr 12, 2015

dudemcbacon commented Apr 12, 2015

mmoll commented Apr 12, 2015

mmoll commented Apr 13, 2015

dudemcbacon commented Apr 13, 2015

ohadlevy Apr 14, 2015

dudemcbacon Apr 14, 2015

mmoll Apr 14, 2015

dudemcbacon Apr 14, 2015

mmoll Apr 14, 2015

mbacovsky Apr 15, 2015

dudemcbacon commented Apr 14, 2015

mbacovsky Apr 15, 2015

mbacovsky commented Apr 15, 2015

mbacovsky commented Apr 15, 2015

dudemcbacon commented Apr 15, 2015

mbacovsky Apr 16, 2015

dudemcbacon Apr 16, 2015

mbacovsky commented Apr 16, 2015

dudemcbacon commented Apr 16, 2015

dudemcbacon commented Apr 16, 2015

mbacovsky commented Apr 17, 2015

dudemcbacon commented Apr 17, 2015

Add concurrency option #7

Add concurrency option #7

Conversation

dudemcbacon commented Apr 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dudemcbacon commented Apr 12, 2015

mmoll commented Apr 12, 2015

mmoll commented Apr 13, 2015

dudemcbacon commented Apr 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dudemcbacon commented Apr 14, 2015

Choose a reason for hiding this comment

mbacovsky commented Apr 15, 2015

mbacovsky commented Apr 15, 2015

dudemcbacon commented Apr 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbacovsky commented Apr 16, 2015

dudemcbacon commented Apr 16, 2015

dudemcbacon commented Apr 16, 2015

mbacovsky commented Apr 17, 2015

dudemcbacon commented Apr 17, 2015