Ack all comments; additionally, increase the default opportunistic delay
used.
Thanks for the review!
Interdiff:
diff --git a/tools/move-instance b/tools/move-instance
index aa5e83d..5fff353 100755
--- a/tools/move-instance
+++ b/tools/move-instance
@@ -128,13 +128,17 @@ PARALLEL_OPT = \
OPPORTUNISTIC_TRIES_OPT = \
cli.cli_option("--opportunistic-tries", action="store", type="int",
- default=0, dest="opportunistic_tries", metavar="<number>",
+ dest="opportunistic_tries", metavar="<number>",
help="Number of opportunistic instance creation attempts"
- " before a normal creation is performed")
+ " before a normal creation is performed. An
opportunistic"
+ " attempt will use the iallocator with all the nodes"
+ " currently unlocked, failing if not enough nodes
are"
+ " available. Even though it will succeed or fail
more"
+ " quickly, it can result in suboptimal instance"
+ " placement")
OPPORTUNISTIC_DELAY_OPT = \
cli.cli_option("--opportunistic-delay", action="store", type="int",
- default=constants.DEFAULT_OPPORTUNISTIC_RETRY_INTERVAL,
dest="opportunistic_delay", metavar="<number>",
help="The delay between successive opportunistic instance"
" creation attempts, in seconds")
@@ -850,12 +854,21 @@ def CheckOptions(parser, options, args):
parser.error("Opportunistic instance creation can only be used with an"
" iallocator")
- if options.opportunistic_tries < 0:
- parser.error("Number of opportunistic creation attempts must be >= 0")
-
- if options.opportunistic_delay <= 0:
- parser.error("The delay between two successive creation attempts must
be"
- " greater than zero")
+ tries_specified = options.opportunistic_tries is not None
+ delay_specified = options.opportunistic_delay is not None
+ if tries_specified:
+ if options.opportunistic_tries < 0:
+ parser.error("Number of opportunistic creation attempts must be >=
0")
+ if delay_specified:
+ if options.opportunistic_delay <= 0:
+ parser.error("The delay between two successive creation attempts
must"
+ " be greater than zero")
+ elif delay_specified:
+ parser.error("Opportunistic delay should only be specified when"
+ " opportunistic tries are used")
+ else:
+ # The default values will be provided later
+ pass
if len(instance_names) == 1:
# Moving one instance only
On Fri, Feb 21, 2014 at 5:00 PM, Petr Pudlák <[email protected]> wrote:
> I'd suggest to document in more detail the consequences of (not) using
> --opportunistic-tries. In particular
>
> - using it gives the possibility to move instances even if a node is
> locked by some other operation
> - not using it can result in better instance placement, if the nodes
> currently being blocked would be suitable as a target.
>
>
> On Fri, Feb 21, 2014 at 1:30 PM, Petr Pudlák <[email protected]> wrote:
>
>>
>>
>>
>> On Tue, Feb 18, 2014 at 3:39 PM, Hrvoje Ribicic <[email protected]> wrote:
>>
>>> To start off the introduction of oppportunistic locking during instance
>>> creation, this patch adds the options allowing the locking to be
>>> invoked.
>>>
>>> Signed-off-by: Hrvoje Ribicic <[email protected]>
>>> ---
>>> src/Ganeti/Constants.hs | 9 +++++++++
>>> tools/move-instance | 26 ++++++++++++++++++++++++++
>>> 2 files changed, 35 insertions(+)
>>>
>>> diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs
>>> index 7f8cfd1..d44e1fd 100644
>>> --- a/src/Ganeti/Constants.hs
>>> +++ b/src/Ganeti/Constants.hs
>>> @@ -3211,6 +3211,15 @@ iallocatorSearchPath =
>>> AutoConf.iallocatorSearchPath
>>> defaultIallocatorShortcut :: String
>>> defaultIallocatorShortcut = "."
>>>
>>> +-- * Opportunistic allocator usage
>>> +
>>> +-- | Time delay in seconds between repeated opportunistic instance
>>> creations.
>>> +-- Rather than failing with an informative error message if the
>>> opportunistic
>>> +-- creation cannot grab enough nodes, for some uses it is better to
>>> retry the
>>> +-- creation with an interval between attempts. This is a reasonable
>>> default.
>>> +defaultOpportunisticRetryInterval :: Int
>>> +defaultOpportunisticRetryInterval = 10
>>> +
>>> -- * Node evacuation
>>>
>>> nodeEvacPri :: String
>>> diff --git a/tools/move-instance b/tools/move-instance
>>> index 7ff8c4b..aa5e83d 100755
>>> --- a/tools/move-instance
>>> +++ b/tools/move-instance
>>> @@ -126,6 +126,19 @@ PARALLEL_OPT = \
>>> dest="parallel", metavar="<number>",
>>> help="Number of instances to be moved simultaneously")
>>>
>>> +OPPORTUNISTIC_TRIES_OPT = \
>>> + cli.cli_option("--opportunistic-tries", action="store", type="int",
>>> + default=0, dest="opportunistic_tries",
>>> metavar="<number>",
>>> + help="Number of opportunistic instance creation
>>> attempts"
>>> + " before a normal creation is performed")
>>> +
>>> +OPPORTUNISTIC_DELAY_OPT = \
>>> + cli.cli_option("--opportunistic-delay", action="store", type="int",
>>> + default=constants.DEFAULT_OPPORTUNISTIC_RETRY_INTERVAL,
>>> + dest="opportunistic_delay", metavar="<number>",
>>> + help="The delay between successive opportunistic
>>> instance"
>>> + " creation attempts, in seconds")
>>> +
>>>
>>> class Error(Exception):
>>> """Generic error.
>>> @@ -800,6 +813,8 @@ def ParseOptions():
>>> parser.add_option(DEST_DISK_TEMPLATE_OPT)
>>> parser.add_option(COMPRESS_OPT)
>>> parser.add_option(PARALLEL_OPT)
>>> + parser.add_option(OPPORTUNISTIC_TRIES_OPT)
>>> + parser.add_option(OPPORTUNISTIC_DELAY_OPT)
>>>
>>> (options, args) = parser.parse_args()
>>>
>>> @@ -831,6 +846,17 @@ def CheckOptions(parser, options, args):
>>> bool(options.dest_primary_node or options.dest_secondary_node)):
>>> parser.error("Destination node and iallocator options exclude each
>>> other")
>>>
>>> + if (not options.iallocator and (options.opportunistic_tries > 0)):
>>> + parser.error("Opportunistic instance creation can only be used with
>>> an"
>>> + " iallocator")
>>> +
>>> + if options.opportunistic_tries < 0:
>>> + parser.error("Number of opportunistic creation attempts must be >=
>>> 0")
>>> +
>>> + if options.opportunistic_delay <= 0:
>>> + parser.error("The delay between two successive creation attempts
>>> must be"
>>> + " greater than zero")
>>> +
>>> if len(instance_names) == 1:
>>> # Moving one instance only
>>> if options.hvparams:
>>> --
>>> 1.9.0.rc1.175.g0b1dcb5
>>>
>>>
>> I'd add a check that if a user specifies --opportunistic-delay but
>> not --opportunistic-tries, the command fails with an error. Otherwise the
>> user might get confused why the retries aren't happening.
>>
>
>
