On Tue, Jul 30, 2013 at 9:48 PM, Dimitris Aragiorgis <[email protected]> wrote: > * Guido Trotter <[email protected]> [2013-07-30 13:54:37 +0200]: > >> 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. >> > > True. REPLUG action will guarantee it. Thanks for catching 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. >> > > ACK. Point taken. Be more verbose and return as many log/warnings possible. > But this must be done in Hypervisor level, then rpc, then cmdlib and then > use it in client side. I'll start with feedback from each stage and see > if I can handle exceptions and act accordingly. > >> > 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 ? >> > > In final patch which should be squashed with this one. >
Ack to all, thanks. Guido
