Just some nitpicking:
On Mon, Feb 24, 2014 at 11:18 AM, Hrvoje Ribicic <[email protected]> wrote: > 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" > I'd suggest to put "or fail" into parentheses. > + " 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" > I'd suggest s/should/can/ > + " opportunistic tries are used") > + else: > + # The default values will be provided later > + pass > > if len(instance_names) == 1: > # Moving one instance only > LGTM, no need to resend. > > > > 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. >>> >> >> >
