* Guido Trotter <[email protected]> [2013-07-30 13:48:10 +0200]: > 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? :) >
I am feeling lucky: docstring?
> > + return dev.uuid is not None
> > +
>
> Can it possibly be None ? Wouldn't that be a ProgrammerError?
>
>
No it actually cannot. I asked Christos recently. I'll remove that
helperfunction. With current implementation there is no limitation from
the server side regarding hotplug-able devices.
> > +
> > 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? :)
>
Again here is it OK to use result.payload (that can be the return value of
the hypervisor method) to print messages and warnings?
> > + 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?
>
This is done in client side.
> 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
Not really. Network configuration requires kvm_if up scripts to run.
_ConfigureNIC()
sould run in any case. Putting logic here whether to create a new tap or
obtain it from /var/run/.. will make code ugly and we won't benefit much.
> 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?
Yes this is doable. This will change hypervisor commands as well but will
ensure the "plug it in again in the same slot" (by obtaining the pci slot
from the runtime file, save it locally, remove the entry and re-add it
with the same pci slot)
> 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.
>
Got lost here too. Can you provide more details? What I know is the following:
Before hotplugging a disk it must be created (blockdev_create) and
assembled (blockdev_assemble) which will return its actual dev_path.
ExpandCheckDisk just compares a list of disks (the newly created one)
with the already existing iobj.disks (not including the newly created).
ApplyContainerMods invokes CreateNewDisk and then inserts the newly
created item in the corresponding container (instance.disks). This to
change is a lot of effort and will break things. I think for the time
being this workaround is not that wrong.
In order to avoid this workaround hotplug must be dragged out of CreateNewDisk()
which is not the easiest thing to do..
Thanks,
dimara
> Thanks,
> Guido
signature.asc
Description: Digital signature
