On Thu, Jul 25, 2013 at 1:41 AM, Dimitris Aragiorgis <[email protected]> wrote:
> Hotpluging is done by functions invoked by ApplyContainerMods(). In
> order hotpluging to take place the --hotplug option must be passed
> otherwise the modifications will take place after reboot.
>
> NIC hotplug supports add, remove and modify. The modify is done by removing
> the existing NIC and adding a new one in the same pci slot.
>
> Disk hotplug support add and remove. Before hotpluging a Disk it
> must be assembled. During LUInstanceSetParams() newly created disks
> are not added to the instance so ExpandCheckDisks() in
> AssembleInstanceDisks() will fail. So we make this check optional
> only for this case.
>
> In order to remove a disk (with blockdev_remove) it must be shutdown. So
> after unpluging the disk ShutdownDiskInstanceDisks() must be invoked.
>
> For both device types we use the generic RPC call_hotplug_device.
>
> Signed-off-by: Dimitris Aragiorgis <[email protected]>
> ---
> lib/cmdlib/instance.py | 52
> ++++++++++++++++++++++++++++++++++++----
> lib/cmdlib/instance_storage.py | 5 ++--
> 2 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py
> index 22812ea..a602512 100644
> --- a/lib/cmdlib/instance.py
> +++ b/lib/cmdlib/instance.py
> @@ -76,6 +76,11 @@ _TApplyContModsCbChanges = \
> ])))
>
>
> +def _DeviceHotplugable(dev):
> +
Guess what I'm going to say? :)
> + return dev.uuid is not None
> +
Can it possibly be None ? Wouldn't that be a ProgrammerError?
> +
> def _CheckHostnameSane(lu, name):
> """Ensures that a given hostname resolves to a 'sane' name.
>
> @@ -3028,7 +3033,8 @@ class LUInstanceSetParams(LogicalUnit):
> # Operate on copies as this is still in prereq
> nics = [nic.Copy() for nic in self.instance.nics]
> _ApplyContainerMods("NIC", nics, self._nic_chgdesc, self.nicmod,
> - self._CreateNewNic, self._ApplyNicMods, None)
> + self._CreateNewNic, self._ApplyNicMods,
> + self._RemoveNic)
> # Verify that NIC names are unique and valid
> utils.ValidateDeviceNames("NIC", nics)
> self._new_nics = nics
> @@ -3203,6 +3209,14 @@ class LUInstanceSetParams(LogicalUnit):
> " continuing anyway: %s", idx,
> self.cfg.GetNodeName(pnode_uuid), msg)
>
> + def _HotplugDevice(self, action, dev_type, device, extra, seq):
> + self.LogInfo("Trying to hotplug device...")
... and not quite telling you whether we succeeded or not? :)
> + result = self.rpc.call_hotplug_device(self.instance.primary_node,
> + self.instance, action, dev_type,
> + device, extra, seq)
> + result.Raise("Could not hotplug device.")
Maybe you should raise an internal exception here...
> + self.LogInfo("Hotplug done.")
> +
> def _CreateNewDisk(self, idx, params, _):
> """Creates a new disk.
>
> @@ -3228,6 +3242,13 @@ class LUInstanceSetParams(LogicalUnit):
> disks=[(idx, disk, 0)],
> cleanup=new_disks)
>
> + if self.op.hotplug:
> + _, device_info = AssembleInstanceDisks(self, self.instance,
> + [disk], check=False)
> + _, _, dev_path = device_info[0]
> + self._HotplugDevice(constants.HOTPLUG_ADD, constants.HOTPLUG_DISK,
> + disk, dev_path, idx)
> +
... and catch it here, transform it into a warning, and move on?
> return (disk, [
> ("disk/%d" % idx, "add:size=%s,mode=%s" % (disk.size, disk.mode)),
> ])
> @@ -3253,6 +3274,11 @@ class LUInstanceSetParams(LogicalUnit):
> """Removes a disk.
>
> """
> + if self.op.hotplug and _DeviceHotplugable(root):
> + self._HotplugDevice(constants.HOTPLUG_REMOVE, constants.HOTPLUG_DISK,
> + root, None, idx)
> + ShutdownInstanceDisks(self, self.instance, [root])
> +
> (anno_disk,) = AnnotateDiskParams(self.instance, [root], self.cfg)
> for node_uuid, disk in anno_disk.ComputeNodeTree(
> self.instance.primary_node):
> @@ -3282,13 +3308,18 @@ class LUInstanceSetParams(LogicalUnit):
> nicparams=nicparams)
> nobj.uuid = self.cfg.GenerateUniqueID(self.proc.GetECId())
>
> - return (nobj, [
> + if self.op.hotplug:
> + self._HotplugDevice(constants.HOTPLUG_ADD, constants.HOTPLUG_NIC,
> + nobj, None, idx)
> +
> + desc = [
> ("nic.%d" % idx,
> "add:mac=%s,ip=%s,mode=%s,link=%s,network=%s" %
> (mac, ip, private.filled[constants.NIC_MODE],
> - private.filled[constants.NIC_LINK],
> - net)),
> - ])
> + private.filled[constants.NIC_LINK], net)),
> + ]
> +
> + return (nobj, desc)
>
> def _ApplyNicMods(self, idx, nic, params, private):
> """Modifies a network interface.
> @@ -3313,8 +3344,19 @@ class LUInstanceSetParams(LogicalUnit):
> for (key, val) in nic.nicparams.items():
> changes.append(("nic.%s/%d" % (key, idx), val))
>
> + if self.op.hotplug and _DeviceHotplugable(nic):
Should we warn here about the possible breakage?
Also perhaps should we be more sneaky here? Only a few of the possible
NIC modification require actual unplug and replug.
MAC does for example. But network could be done completely externally
from the hypervisor without the unplugging and replugging and possibly
breaking. Name needs no unplug/replug at all, also? (the guest doesn't
know the name right? so unplugging it for changing it doesn't help).
> + self._HotplugDevice(constants.HOTPLUG_REMOVE, constants.HOTPLUG_NIC,
> + nic, None, idx)
> + self._HotplugDevice(constants.HOTPLUG_ADD, constants.HOTPLUG_NIC,
> + nic, None, idx)
> +
Maybe there should be a REPLUG mode to avoid the RTT and doing
everything in one op?
Also we can't guarantee we won't fail to remove and then add, and
other such fun situations, like this, if we keep ignoring failures.
> return changes
>
> + def _RemoveNic(self, idx, nic, _):
> + if self.op.hotplug and _DeviceHotplugable(nic):
> + self._HotplugDevice(constants.HOTPLUG_REMOVE, constants.HOTPLUG_NIC,
> + nic, None, idx)
> +
> def Exec(self, feedback_fn):
> """Modifies an instance.
>
> diff --git a/lib/cmdlib/instance_storage.py b/lib/cmdlib/instance_storage.py
> index 793cfa6..1516826 100644
> --- a/lib/cmdlib/instance_storage.py
> +++ b/lib/cmdlib/instance_storage.py
> @@ -1259,7 +1259,7 @@ def _SafeShutdownInstanceDisks(lu, instance,
> disks=None):
>
>
> def AssembleInstanceDisks(lu, instance, disks=None, ignore_secondaries=False,
> - ignore_size=False):
> + ignore_size=False, check=True):
> """Prepare the block devices for an instance.
>
> This sets up the block devices on all nodes.
> @@ -1284,7 +1284,8 @@ def AssembleInstanceDisks(lu, instance, disks=None,
> ignore_secondaries=False,
> """
> device_info = []
> disks_ok = True
> - disks = ExpandCheckDisks(instance, disks)
> + if check:
> + disks = ExpandCheckDisks(instance, disks)
>
Mmm... what if we added it before to the instance and left the check?
Skipping it might be again wrong... We can just add it to the instance
object and *then* try to hotplug.
Thanks,
Guido
