On Thu, Jul 25, 2013 at 1:41 AM, Dimitris Aragiorgis <[email protected]> wrote: > Add --hotplug option. Only used in OpInstanceSetParams. > If this is omitted, modifications become effective after reboot. > > Ask user confirmation in case NIC modify + hotplug because it will > be done via removing old NIC (and the corresponding tap) and adding > a new one in the same PCI slot.
"in the same PCI slot", would be great, but I didn't see the code that guarantees that. Aren't you just hoping the same one is selected, since you just freed it? But what if you removed a lower one just before? maybe with the REPLUG option we could actually guarantee it. > > Signed-off-by: Dimitris Aragiorgis <[email protected]> > --- > lib/cli.py | 5 +++++ > lib/client/gnt_instance.py | 20 +++++++++++++++----- > lib/opcodes.py | 1 + > 3 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/lib/cli.py b/lib/cli.py > index 7731f43..af7f77d 100644 > --- a/lib/cli.py > +++ b/lib/cli.py > @@ -95,6 +95,7 @@ __all__ = [ > "GLOBAL_FILEDIR_OPT", > "HID_OS_OPT", > "GLOBAL_SHARED_FILEDIR_OPT", > + "HOTPLUG_OPT", > "HVLIST_OPT", > "HVOPTS_OPT", > "HYPERVISOR_OPT", > @@ -1645,6 +1646,10 @@ INCLUDEDEFAULTS_OPT = cli_option("--include-defaults", > dest="include_defaults", > default=False, action="store_true", > help="Include default values") > > +HOTPLUG_OPT = cli_option("--hotplug", dest="hotplug", > + action="store_true", default=False, > + help="Try to hotplug device") > + > #: Options provided by all commands > COMMON_OPTS = [DEBUG_OPT, REASON_OPT] > > diff --git a/lib/client/gnt_instance.py b/lib/client/gnt_instance.py > index 1334212..3d039dd 100644 > --- a/lib/client/gnt_instance.py > +++ b/lib/client/gnt_instance.py > @@ -1320,6 +1320,14 @@ def SetInstanceParams(opts, args): > allowed_values=[constants.VALUE_DEFAULT]) > > nics = _ConvertNicDiskModifications(opts.nics) > + for action, _, __ in nics: > + if action == constants.DDM_MODIFY and opts.hotplug: > + usertext = ("You are about to hot-modify a NIC. This will be done" > + " by removing the exisiting and then adding a new one." > + " Network connection might be lost. Continue?") > + if not AskUser(usertext): > + return 1 > + > disks = _ParseDiskSizes(_ConvertNicDiskModifications(opts.disks)) > > if (opts.disk_template and > @@ -1339,6 +1347,7 @@ def SetInstanceParams(opts, args): > op = opcodes.OpInstanceSetParams(instance_name=args[0], > nics=nics, > disks=disks, > + hotplug=opts.hotplug, > disk_template=opts.disk_template, > remote_node=opts.node, > pnode=opts.new_primary_node, > @@ -1361,10 +1370,11 @@ def SetInstanceParams(opts, args): > ToStdout("Modified instance %s", args[0]) > for param, data in result: > ToStdout(" - %-5s -> %s", param, data) > - ToStdout("Please don't forget that most parameters take effect" > - " only at the next (re)start of the instance initiated by" > - " ganeti; restarting from within the instance will" > - " not be enough.") > + if not opts.hotplug: > + ToStdout("Please don't forget that most parameters take effect" > + " only at the next (re)start of the instance initiated by" > + " ganeti; restarting from within the instance will" > + " not be enough.") Please add a mention that hotplug is experimental and we can't guarantee the modification to be fully successful until the next instance reboot. You know, like live migrations. We can then perhaps remove it after we've had it in production for a while. Also until you ignore many failure mode there's no way for the user to know that they might have to reboot, you know, because some of the preconditions where not met or the operation failed. So here you need to report properly whether we think the hotplugging succeeded or whether we know for sure it didn't and a reboot is needed. Hope is not a strategy. > return 0 > > > @@ -1546,7 +1556,7 @@ commands = { > [DISK_TEMPLATE_OPT, SINGLE_NODE_OPT, OS_OPT, FORCE_VARIANT_OPT, > OSPARAMS_OPT, DRY_RUN_OPT, PRIORITY_OPT, NWSYNC_OPT, OFFLINE_INST_OPT, > ONLINE_INST_OPT, IGNORE_IPOLICY_OPT, RUNTIME_MEM_OPT, > - NOCONFLICTSCHECK_OPT, NEW_PRIMARY_OPT], > + NOCONFLICTSCHECK_OPT, NEW_PRIMARY_OPT, HOTPLUG_OPT], > "<instance>", "Alters the parameters of an instance"), > "shutdown": ( > GenericManyOps("shutdown", _ShutdownInstance), [ArgInstance()], > diff --git a/lib/opcodes.py b/lib/opcodes.py > index d9d04cf..1835f50 100644 > --- a/lib/opcodes.py > +++ b/lib/opcodes.py > @@ -1740,6 +1740,7 @@ class OpInstanceSetParams(OpCode): > "Whether to wait for the disk to synchronize, when changing template"), > ("offline", None, ht.TMaybeBool, "Whether to mark instance as offline"), > ("conflicts_check", True, ht.TBool, "Check for conflicting IPs"), > + ("hotplug", None, ht.TMaybeBool, "Whether to hotplug devices"), > ] > OP_RESULT = _TSetParamsResult > OpCodes.hs ? Thanks, Guido
