Thanks, will make the changes!
On Mon, Feb 24, 2014 at 11:48 AM, Petr Pudlák <[email protected]> wrote: > 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. >>>> >>> >>> >> >
