Hi Yiannis, Filippos I assume the issue was that VerifyDiskModification wasn't called in case of ADD_REMOVE/ADD_DETACH at _all_ (and it didn't check if hotplug was on). Thanks for the patches, LGTM.
On Thursday, January 26, 2017 at 3:14:30 PM UTC, Yiannis Tsiouris wrote: > > Prior to this commit, removing a disk from a live instance without > using hotplug succeeded incorrectly leaving stale disks behind. This is > due to the fact that the hypervisor holds the device open while Ganeti > tries to shut it down. The operation fails but Ganeti continues anyway, > removing the disk from the config and leaving the disk stale. > > This patch adds an appropriate check before removing a disk from a live > instance. > > Signed-off-by: Yiannis Tsiouris <[email protected]> > Signed-off-by: Filippos Giannakos <[email protected]> > --- > lib/cmdlib/instance_set_params.py | 17 ++++++++++++----- > test/py/cmdlib/instance_unittest.py | 8 ++++++++ > 2 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/lib/cmdlib/instance_set_params.py > b/lib/cmdlib/instance_set_params.py > index 9ffbfdf0a..6dc4c3792 100644 > --- a/lib/cmdlib/instance_set_params.py > +++ b/lib/cmdlib/instance_set_params.py > @@ -162,16 +162,18 @@ class LUInstanceSetParams(LogicalUnit): > utils.ForceDictType(params, (key_types if op != > constants.DDM_ATTACH > else key_types_attach)) > > + if op not in (constants.DDM_ADD, constants.DDM_ATTACH, > + constants.DDM_MODIFY, constants.DDM_REMOVE, > + constants.DDM_DETACH): > + raise errors.ProgrammerError("Unhandled operation '%s'" % op) > + > if op in (constants.DDM_REMOVE, constants.DDM_DETACH): > if params: > raise errors.OpPrereqError("No settings should be passed when" > " removing or detaching a %s" % > kind, > errors.ECODE_INVAL) > - elif op in (constants.DDM_ADD, constants.DDM_ATTACH, > - constants.DDM_MODIFY): > - item_fn(op, params) > - else: > - raise errors.ProgrammerError("Unhandled operation '%s'" % op) > + > + item_fn(op, params) > > def _VerifyDiskModification(self, op, params, excl_stor, > group_access_types): > """Verifies a disk modification. > @@ -263,6 +265,11 @@ class LUInstanceSetParams(LogicalUnit): > if name is not None and name.lower() == constants.VALUE_NONE: > params[constants.IDISK_NAME] = None > > + if op == constants.DDM_REMOVE and not self.op.hotplug: > + CheckInstanceState(self, self.instance, INSTANCE_NOT_RUNNING, > + msg="can't remove volume from a running > instance" > + " without using hotplug") > + > @staticmethod > def _VerifyNicModification(op, params): > """Verifies a network interface modification. > diff --git a/test/py/cmdlib/instance_unittest.py > b/test/py/cmdlib/instance_unittest.py > index 1a4a45b3a..74d48721b 100644 > --- a/test/py/cmdlib/instance_unittest.py > +++ b/test/py/cmdlib/instance_unittest.py > @@ -3041,6 +3041,10 @@ class TestLUInstanceSetParams(CmdlibTestCase): > instance_name=inst.name, > disks=[[constants.DDM_REMOVE, -1, > {}]]) > + self.rpc.call_instance_info.side_effect = [ > + self.RpcResultsBuilder() \ > + .CreateSuccessfulNodeResult(self.master) > + ] > self.rpc.call_file_storage_dir_remove.return_value = \ > self.RpcResultsBuilder() \ > .CreateSuccessfulNodeResult(self.master) > @@ -3055,6 +3059,10 @@ class TestLUInstanceSetParams(CmdlibTestCase): > instance_name=inst.name, > disks=[[constants.DDM_REMOVE, -1, > {}]]) > + self.rpc.call_instance_info.side_effect = [ > + self.RpcResultsBuilder() > + .CreateSuccessfulNodeResult(self.master) > + ] > self.rpc.call_file_storage_dir_remove.return_value = \ > self.RpcResultsBuilder() \ > .CreateSuccessfulNodeResult(self.master) > -- > 2.11.0 > >
