Hi, thanks for the update!
On Thu, Nov 13, 2014 at 01:52:54PM +0200, Ilias Tsitsimpis wrote: > Hi Aaron, > > Thanks for the review. Comments inline. > > On Mon, Nov 10, 2014 at 03:49PM, 'Aaron Karper' via ganeti-devel wrote: > > Thanks for the patch! > > > > On Thu, Nov 06, 2014 at 12:30:36AM +0200, Alex Pyrgiotis wrote: > > > From: Ilias Tsitsimpis <[email protected]> > > > > > > AllocateDRBDMinor used to allocate some DRBD minors for a given instance > > > using ComputeDRBDMap. ComputeDRBDMap didn't take into account, the DRBD > > > minors for disks that are detached (don't belong to any instance). This > > > patch fixes the above problem by modifying AllocateDRBDMinor and > > > ReleaseDRBDMinors in order to accept the disk for which we want to > > > allocate the minors (and not the instance). In addition, ComputeDRBDMap > > > now examines all the disks (even the detached ones) in order to compute > > > the minors. > > > > > > Signed-off-by: Ilias Tsitsimpis <[email protected]> > > > Signed-off-by: Alex Pyrgiotis <[email protected]> > > > > > > diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py > > > index 2c229c4..f932386 100644 > > > --- a/lib/cmdlib/cluster.py > > > +++ b/lib/cmdlib/cluster.py > > > @@ -2175,6 +2175,7 @@ class LUClusterVerifyGroup(LogicalUnit, > > _VerifyErrors): > > > > > > self.all_node_info = self.cfg.GetAllNodesInfo() > > > self.all_inst_info = self.cfg.GetAllInstancesInfo() > > > + self.all_disks_info = self.cfg.GetAllDisksInfo() > > > > > > self.my_node_uuids = group_node_uuids > > > self.my_node_info = dict((node_uuid, self.all_node_info[node_uuid]) > > > @@ -2913,14 +2914,15 @@ class LUClusterVerifyGroup(LogicalUnit, > > _VerifyErrors): > > > self._ErrorIf(test, constants.CV_ENODEDRBDHELPER, ninfo.name, > > > "wrong drbd usermode helper: %s", payload) > > > > > > - def _VerifyNodeDrbd(self, ninfo, nresult, instanceinfo, drbd_helper, > > > - drbd_map): > > > + def _VerifyNodeDrbd(self, ninfo, nresult, instanceinfo, disks_info, > > > + drbd_helper, drbd_map): > > > """Verifies and the node DRBD status. > > > > > > @type ninfo: L{objects.Node} > > > @param ninfo: the node to check > > > @param nresult: the remote results for the node > > > @param instanceinfo: the dict of instances > > > + @param disks_info: the dict of disks > > > @param drbd_helper: the configured DRBD usermode helper > > > @param drbd_map: the DRBD map as returned by > > > L{ganeti.config.ConfigWriter.ComputeDRBDMap} > > > @@ -2930,18 +2932,22 @@ class LUClusterVerifyGroup(LogicalUnit, > > _VerifyErrors): > > > > > > # compute the DRBD minors > > > node_drbd = {} > > > - for minor, inst_uuid in drbd_map[ninfo.uuid].items(): > > > - test = inst_uuid not in instanceinfo > > > + for minor, disk_uuid in drbd_map[ninfo.uuid].items(): > > > + test = disk_uuid not in disks_info > > > self._ErrorIf(test, constants.CV_ECLUSTERCFG, None, > > > - "ghost instance '%s' in temporary DRBD map", > > inst_uuid) > > > - # ghost instance should not be running, but otherwise we > > > - # don't give double warnings (both ghost instance and > > > + "ghost disk '%s' in temporary DRBD map", disk_uuid) > > > + # ghost disk should not be active, but otherwise we > > > + # don't give double warnings (both ghost disk and > > > # unallocated minor in use) > > > if test: > > > - node_drbd[minor] = (inst_uuid, False) > > > + node_drbd[minor] = (disk_uuid, False) > > > else: > > > - instance = instanceinfo[inst_uuid] > > > - node_drbd[minor] = (inst_uuid, instance.disks_active) > > > + disk_active = False > > > + for (inst_uuid, inst) in instanceinfo.items(): > > > + if disk_uuid in inst.disks: > > > + disk_active = inst.disks_active > > > + break > > > + node_drbd[minor] = (disk_uuid, disk_active) > > > > This seems to be something useful to have in other contexts too. Can you > > extract this? > > I am not sure where else this might be used, as this is the only > place that Ganeti verifies the DRBD status. Is it worth to factor out > just 3 lines? What do you think? I was thinking about the code to find the instance for a given disk, but that wouldn't be applicable here anyway, so never mind! > > > > > > > # and now check them > > > used_minors = nresult.get(constants.NV_DRBDLIST, []) > > > @@ -2952,11 +2958,10 @@ class LUClusterVerifyGroup(LogicalUnit, > > _VerifyErrors): > > > # we cannot check drbd status > > > return > > > > > > - for minor, (inst_uuid, must_exist) in node_drbd.items(): > > > + for minor, (disk_uuid, must_exist) in node_drbd.items(): > > > test = minor not in used_minors and must_exist > > > self._ErrorIf(test, constants.CV_ENODEDRBD, ninfo.name, > > > - "drbd minor %d of instance %s is not active", minor, > > > - self.cfg.GetInstanceName(inst_uuid)) > > > + "drbd minor %d of disk %s is not active", minor, > > disk_uuid) > > > > Can you also provide the instance (if any) in the error message? A user > > wouldn't want to check all disks. > > > Ack. Interdiff: > > diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py > index f932386..c2cfbff 100644 > --- a/lib/cmdlib/cluster.py > +++ b/lib/cmdlib/cluster.py > @@ -2940,14 +2940,16 @@ class LUClusterVerifyGroup(LogicalUnit, > _VerifyErrors): > # don't give double warnings (both ghost disk and > # unallocated minor in use) > if test: > - node_drbd[minor] = (disk_uuid, False) > + node_drbd[minor] = (disk_uuid, None, False) > else: > disk_active = False > + disk_instance = None > for (inst_uuid, inst) in instanceinfo.items(): > if disk_uuid in inst.disks: > disk_active = inst.disks_active > + disk_instance = inst_uuid I think inst.name would make a better user error. > break > - node_drbd[minor] = (disk_uuid, disk_active) > + node_drbd[minor] = (disk_uuid, disk_instance, disk_active) > > # and now check them > used_minors = nresult.get(constants.NV_DRBDLIST, []) > @@ -2958,10 +2960,16 @@ class LUClusterVerifyGroup(LogicalUnit, > _VerifyErrors): > # we cannot check drbd status > return > > - for minor, (disk_uuid, must_exist) in node_drbd.items(): > + for minor, (disk_uuid, inst_uuid, must_exist) in node_drbd.items(): > test = minor not in used_minors and must_exist > + if inst_uuid is not None: > + attached = "(attached in instance '%s')" % \ > + self.cfg.GetInstanceName(inst_uuid) > + else: > + attached = "(detached)" > self._ErrorIf(test, constants.CV_ENODEDRBD, ninfo.name, > - "drbd minor %d of disk %s is not active", minor, > disk_uuid) > + "drbd minor %d of disk %s %s is not active", > + minor, disk_uuid, attached) > for minor in used_minors: > test = minor not in node_drbd > self._ErrorIf(test, constants.CV_ENODEDRBD, ninfo.name, > > > > > for minor in used_minors: > > > test = minor not in node_drbd > > > self._ErrorIf(test, constants.CV_ENODEDRBD, ninfo.name, > > > @@ -3768,8 +3773,8 @@ class LUClusterVerifyGroup(LogicalUnit, > > _VerifyErrors): > > > if nimg.vm_capable: > > > self._UpdateVerifyNodeLVM(node_i, nresult, vg_name, nimg) > > > if constants.DT_DRBD8 in cluster.enabled_disk_templates: > > > - self._VerifyNodeDrbd(node_i, nresult, self.all_inst_info, > > drbd_helper, > > > - all_drbd_map) > > > + self._VerifyNodeDrbd(node_i, nresult, self.all_inst_info, > > > + self.all_disks_info, drbd_helper, > > all_drbd_map) > > > > > > if (constants.DT_PLAIN in cluster.enabled_disk_templates) or \ > > > (constants.DT_DRBD8 in cluster.enabled_disk_templates): > > > diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py > > > index 5d9c6f0..b8e23b5 100644 > > > --- a/lib/cmdlib/instance.py > > > +++ b/lib/cmdlib/instance.py > > > @@ -459,7 +459,8 @@ class LUInstanceMove(LogicalUnit): > > > CreateDisks(self, self.instance, target_node_uuid=target_node.uuid) > > > except errors.OpExecError: > > > self.LogWarning("Device creation failed") > > > - self.cfg.ReleaseDRBDMinors(self.instance.uuid) > > > + for disk_uuid in self.instance.disks: > > > + self.cfg.ReleaseDRBDMinors(disk_uuid) > > > raise > > > > > > errs = [] > > > @@ -492,7 +493,8 @@ class LUInstanceMove(LogicalUnit): > > > try: > > > RemoveDisks(self, self.instance, target_node_uuid=target_node.u > > uid) > > > finally: > > > - self.cfg.ReleaseDRBDMinors(self.instance.uuid) > > > + for disk_uuid in self.instance.disks: > > > + self.cfg.ReleaseDRBDMinors(disk_uuid) > > > raise errors.OpExecError("Errors during disk copy: %s" % > > > (",".join(errs),)) > > > > > > diff --git a/lib/cmdlib/instance_create.py b/lib/cmdlib/instance_create.py > > > index a5201f3..fe23f22 100644 > > > --- a/lib/cmdlib/instance_create.py > > > +++ b/lib/cmdlib/instance_create.py > > > @@ -1452,7 +1452,8 @@ class LUInstanceCreate(LogicalUnit): > > > CreateDisks(self, iobj, disks=disks) > > > except errors.OpExecError: > > > self.LogWarning("Device creation failed") > > > - self.cfg.ReleaseDRBDMinors(instance_uuid) > > > + for disk in disks: > > > + self.cfg.ReleaseDRBDMinors(disk.uuid) > > > raise > > > > > > feedback_fn("adding instance %s to cluster config" % > > self.op.instance_name) > > > diff --git a/lib/cmdlib/instance_set_params.py b/lib/cmdlib/instance_set_ > > params.py > > > index f6fd4bf..37ece5b 100644 > > > --- a/lib/cmdlib/instance_set_params.py > > > +++ b/lib/cmdlib/instance_set_params.py > > > @@ -1223,7 +1223,8 @@ class LUInstanceSetParams(LogicalUnit): > > > disks=new_disks) > > > except errors.OpExecError: > > > self.LogWarning("Device creation failed") > > > - self.cfg.ReleaseDRBDMinors(self.instance.uuid) > > > + for disk in new_disks: > > > + self.cfg.ReleaseDRBDMinors(disk.uuid) > > > raise > > > > > > # Transfer the data from the old to the newly created disks of the > > instance. > > > @@ -1255,7 +1256,8 @@ class LUInstanceSetParams(LogicalUnit): > > > disks=new_disks) > > > self.LogInfo("Newly created disks removed successfully") > > > finally: > > > - self.cfg.ReleaseDRBDMinors(self.instance.uuid) > > > + for disk in new_disks: > > > + self.cfg.ReleaseDRBDMinors(disk.uuid) > > > result.Raise("Error while converting the instance's template") > > > > > > # In case of DRBD disk, return its port to the pool > > > @@ -1700,7 +1702,8 @@ class LUInstanceSetParams(LogicalUnit): > > > else: > > > self._ConvertInstanceTemplate(feedback_fn) > > > except: > > > - self.cfg.ReleaseDRBDMinors(self.instance.uuid) > > > + for disk in inst_disks: > > > + self.cfg.ReleaseDRBDMinors(disk.uuid) > > > raise > > > result.append(("disk_template", self.op.disk_template)) > > > > > > diff --git a/lib/cmdlib/instance_storage.py b/lib/cmdlib/instance_storage. > > py > > > index 1489e0f..dedbba8 100644 > > > --- a/lib/cmdlib/instance_storage.py > > > +++ b/lib/cmdlib/instance_storage.py > > > @@ -538,7 +538,7 @@ def CheckRADOSFreeSpace(): > > > > > > > > > def _GenerateDRBD8Branch(lu, primary_uuid, secondary_uuid, size, > > vgnames, names, > > > - iv_name, p_minor, s_minor): > > > + iv_name): > > > """Generate a drbd8 device complete with its children. > > > > > > """ > > > @@ -557,14 +557,18 @@ def _GenerateDRBD8Branch(lu, primary_uuid, > > secondary_uuid, size, vgnames, names, > > > nodes=[primary_uuid, secondary_uuid], > > > params={}) > > > dev_meta.uuid = lu.cfg.GenerateUniqueID(lu.proc.GetECId()) > > > + > > > + drbd_uuid = lu.cfg.GenerateUniqueID(lu.proc.GetECId()) > > > + minors = lu.cfg.AllocateDRBDMinor([primary_uuid, secondary_uuid], > > drbd_uuid) > > > + assert len(minors) == 2 > > > drbd_dev = objects.Disk(dev_type=constants.DT_DRBD8, size=size, > > > logical_id=(primary_uuid, secondary_uuid, port, > > > - p_minor, s_minor, > > > + minors[0], minors[1], > > > shared_secret), > > > children=[dev_data, dev_meta], > > > nodes=[primary_uuid, secondary_uuid], > > > iv_name=iv_name, params={}) > > > - drbd_dev.uuid = lu.cfg.GenerateUniqueID(lu.proc.GetECId()) > > > + drbd_dev.uuid = drbd_uuid > > > return drbd_dev > > > > > > > > > @@ -587,8 +591,6 @@ def GenerateDiskTemplate( > > > if len(secondary_node_uuids) != 1: > > > raise errors.ProgrammerError("Wrong template configuration") > > > remote_node_uuid = secondary_node_uuids[0] > > > - minors = lu.cfg.AllocateDRBDMinor( > > > - [primary_node_uuid, remote_node_uuid] * len(disk_info), > > instance_uuid) > > > > > > (drbd_params, _, _) = objects.Disk.ComputeLDParams(template_name, > > > full_disk_params) > > > @@ -607,8 +609,7 @@ def GenerateDiskTemplate( > > > disk[constants.IDISK_SIZE], > > > [data_vg, meta_vg], > > > names[idx * 2:idx * 2 + 2], > > > - "disk/%d" % disk_index, > > > - minors[idx * 2], minors[idx * 2 + > > 1]) > > > + "disk/%d" % disk_index) > > > disk_dev.mode = disk[constants.IDISK_MODE] > > > disk_dev.name = disk.get(constants.IDISK_NAME, None) > > > disks.append(disk_dev) > > > @@ -1009,7 +1010,7 @@ class LUInstanceRecreateDisks(LogicalUnit): > > > # have changed > > > (_, _, old_port, _, _, old_secret) = disk.logical_id > > > new_minors = self.cfg.AllocateDRBDMinor(self.op.node_uuids, > > > - self.instance.uuid) > > > + disk.uuid) > > > new_id = (self.op.node_uuids[0], self.op.node_uuids[1], old_port, > > > new_minors[0], new_minors[1], old_secret) > > > assert len(disk.logical_id) == len(new_id) > > > @@ -2776,9 +2777,10 @@ class TLReplaceDisks(Tasklet): > > > # after this, we must manually remove the drbd minors on both the > > > # error and the success paths > > > self.lu.LogStep(4, steps_total, "Changing drbd configuration") > > > - minors = self.cfg.AllocateDRBDMinor([self.new_node_uuid > > > - for _ in inst_disks], > > > - self.instance.uuid) > > > + minors = [] > > > + for disk in inst_disks: > > > + minor = self.cfg.AllocateDRBDMinor([self.new_node_uuid], disk.uuid) > > > + minors.append(minor) > > > logging.debug("Allocated minors %r", minors) > > > > > > iv_names = {} > > > @@ -2817,7 +2819,8 @@ class TLReplaceDisks(Tasklet): > > > GetInstanceInfoText(self.instance), False, > > > excl_stor) > > > except errors.GenericError: > > > - self.cfg.ReleaseDRBDMinors(self.instance.uuid) > > > + for disk in inst_disks: > > > + self.cfg.ReleaseDRBDMinors(disk.uuid) > > > raise > > > > > > # We have new devices, shutdown the drbd on the old secondary > > > @@ -2838,7 +2841,8 @@ class TLReplaceDisks(Tasklet): > > > msg = result.fail_msg > > > if msg: > > > # detaches didn't succeed (unlikely) > > > - self.cfg.ReleaseDRBDMinors(self.instance.uuid) > > > + for disk in inst_disks: > > > + self.cfg.ReleaseDRBDMinors(disk.uuid) > > > raise errors.OpExecError("Can't detach the disks from the network > > on" > > > " old node: %s" % (msg,)) > > > > > > diff --git a/lib/config.py b/lib/config.py > > > index 09ea190..9ad33fe 100644 > > > --- a/lib/config.py > > > +++ b/lib/config.py > > > @@ -450,6 +450,7 @@ class ConfigWriter(object): > > > disk.UpgradeConfig() > > > self._ConfigData().disks[disk.uuid] = disk > > > self._ConfigData().cluster.serial_no += 1 > > > + self._UnlockedReleaseDRBDMinors(disk.uuid) > > > > > > def _UnlockedAttachInstanceDisk(self, inst_uuid, disk_uuid, idx=None): > > > """Attach a disk to an instance. > > > @@ -1379,53 +1380,56 @@ class ConfigWriter(object): > > > return dict(map(lambda (k, v): (k, dict(v)), > > > self._wconfd.ComputeDRBDMap())) > > > > > > - def AllocateDRBDMinor(self, node_uuids, inst_uuid): > > > + def AllocateDRBDMinor(self, node_uuids, disk_uuid): > > > """Allocate a drbd minor. > > > > > > This is just a wrapper over a call to WConfd. > > > > > > The free minor will be automatically computed from the existing > > > - devices. A node can be given multiple times in order to allocate > > > - multiple minors. The result is the list of minors, in the same > > > + devices. A node can not be given multiple times. > > > + The result is the list of minors, in the same > > > order as the passed nodes. > > > > > > - @type inst_uuid: string > > > - @param inst_uuid: the instance for which we allocate minors > > > + @type node_uuids: list of strings > > > + @param node_uuids: the nodes in which we allocate minors > > > + @type disk_uuid: string > > > + @param disk_uuid: the disk for which we allocate minors > > > + @rtype: list of ints > > > + @return: A list of minors in the same order as the passed nodes > > > > > > """ > > > - assert isinstance(inst_uuid, basestring), \ > > > - "Invalid argument '%s' passed to AllocateDRBDMinor" % > > inst_uuid > > > + assert isinstance(disk_uuid, basestring), \ > > > + "Invalid argument '%s' passed to AllocateDRBDMinor" % > > disk_uuid > > > > > > if self._offline: > > > raise errors.ProgrammerError("Can't call AllocateDRBDMinor" > > > " in offline mode") > > > > > > - result = self._wconfd.AllocateDRBDMinor(inst_uuid, node_uuids) > > > + result = self._wconfd.AllocateDRBDMinor(disk_uuid, node_uuids) > > > logging.debug("Request to allocate drbd minors, input: %s, returning > > %s", > > > node_uuids, result) > > > return result > > > > > > - def _UnlockedReleaseDRBDMinors(self, inst_uuid): > > > - """Release temporary drbd minors allocated for a given instance. > > > + def _UnlockedReleaseDRBDMinors(self, disk_uuid): > > > + """Release temporary drbd minors allocated for a given disk. > > > > > > This is just a wrapper over a call to WConfd. > > > > > > - @type inst_uuid: string > > > - @param inst_uuid: the instance for which temporary minors should be > > > - released > > > + @type disk_uuid: string > > > + @param disk_uuid: the disk for which temporary minors should be > > released > > > > > > """ > > > - assert isinstance(inst_uuid, basestring), \ > > > + assert isinstance(disk_uuid, basestring), \ > > > "Invalid argument passed to ReleaseDRBDMinors" > > > # in offline mode we allow the calls to release DRBD minors, > > > # because then nothing can be allocated anyway; > > > # this is useful for testing > > > if not self._offline: > > > - self._wconfd.ReleaseDRBDMinors(inst_uuid) > > > + self._wconfd.ReleaseDRBDMinors(disk_uuid) > > > > > > @_ConfigSync() > > > - def ReleaseDRBDMinors(self, inst_uuid): > > > - """Release temporary drbd minors allocated for a given instance. > > > + def ReleaseDRBDMinors(self, disk_uuid): > > > + """Release temporary drbd minors allocated for a given disk. > > > > > > This should be called on the error paths, on the success paths > > > it's automatically called by the ConfigWriter add and update > > > @@ -1433,12 +1437,11 @@ class ConfigWriter(object): > > > > > > This function is just a wrapper over L{_UnlockedReleaseDRBDMinors}. > > > > > > - @type inst_uuid: string > > > - @param inst_uuid: the instance for which temporary minors should be > > > - released > > > + @type disk_uuid: string > > > + @param disk_uuid: the disk for which temporary minors should be > > released > > > > > > """ > > > - self._UnlockedReleaseDRBDMinors(inst_uuid) > > > + self._UnlockedReleaseDRBDMinors(disk_uuid) > > > > > > @_ConfigSync(shared=1) > > > def GetConfigVersion(self): > > > @@ -1888,7 +1891,6 @@ class ConfigWriter(object): > > > instance.ctime = instance.mtime = time.time() > > > self._ConfigData().instances[instance.uuid] = instance > > > self._ConfigData().cluster.serial_no += 1 > > > - self._UnlockedReleaseDRBDMinors(instance.uuid) > > > # FIXME: After RemoveInstance is moved to WConfd, use its internal > > > # function from TempRes module instead. > > > self._UnlockedCommitTemporaryIps(ec_id) > > > @@ -3309,7 +3311,7 @@ class ConfigWriter(object): > > > self._ConfigData().cluster.serial_no += 1 > > > self._ConfigData().cluster.mtime = now > > > > > > - if isinstance(target, objects.Instance): > > > + if isinstance(target, objects.Disk): > > > self._UnlockedReleaseDRBDMinors(target.uuid) > > > > > > if ec_id is not None: > > > diff --git a/src/Ganeti/Config.hs b/src/Ganeti/Config.hs > > > index d31b23e..5d9c0e0 100644 > > > --- a/src/Ganeti/Config.hs > > > +++ b/src/Ganeti/Config.hs > > > @@ -64,6 +64,7 @@ module Ganeti.Config > > > , getInstAllNodes > > > , getInstDisks > > > , getInstDisksFromObj > > > + , getDrbdMinorsForDisk > > > , getDrbdMinorsForInstance > > > , getFilledInstHvParams > > > , getFilledInstBeParams > > > diff --git a/src/Ganeti/WConfd/Core.hs b/src/Ganeti/WConfd/Core.hs > > > index e3de0e0..492af60 100644 > > > --- a/src/Ganeti/WConfd/Core.hs > > > +++ b/src/Ganeti/WConfd/Core.hs > > > @@ -148,22 +148,22 @@ computeDRBDMap = uncurry T.computeDRBDMap =<< > > readTempResState > > > -- Allocate a drbd minor. > > > -- > > > -- The free minor will be automatically computed from the existing > > devices. > > > --- A node can be given multiple times in order to allocate multiple > > minors. > > > +-- A node can not be given multiple times. > > > -- The result is the list of minors, in the same order as the passed > > nodes. > > > allocateDRBDMinor > > > - :: T.InstanceUUID -> [T.NodeUUID] -> WConfdMonad [T.DRBDMinor] > > > -allocateDRBDMinor inst nodes = > > > - modifyTempResStateErr (\cfg -> T.allocateDRBDMinor cfg inst nodes) > > > + :: T.DiskUUID -> [T.NodeUUID] -> WConfdMonad [T.DRBDMinor] > > > +allocateDRBDMinor disk nodes = > > > + modifyTempResStateErr (\cfg -> T.allocateDRBDMinor cfg disk nodes) > > > > > > --- Release temporary drbd minors allocated for a given instance using > > > +-- Release temporary drbd minors allocated for a given disk using > > > -- 'allocateDRBDMinor'. > > > -- > > > -- This should be called on the error paths, on the success paths > > > -- it's automatically called by the ConfigWriter add and update > > > -- functions. > > > releaseDRBDMinors > > > - :: T.InstanceUUID -> WConfdMonad () > > > -releaseDRBDMinors inst = modifyTempResState (const $ T.releaseDRBDMinors > > inst) > > > + :: T.DiskUUID -> WConfdMonad () > > > +releaseDRBDMinors disk = modifyTempResState (const $ T.releaseDRBDMinors > > disk) > > > > > > -- *** MACs > > > > > > diff --git a/src/Ganeti/WConfd/TempRes.hs b/src/Ganeti/WConfd/TempRes.hs > > > index 020aee8..fdd6594 100644 > > > --- a/src/Ganeti/WConfd/TempRes.hs > > > +++ b/src/Ganeti/WConfd/TempRes.hs > > > @@ -44,6 +44,7 @@ module Ganeti.WConfd.TempRes > > > , emptyTempResState > > > , NodeUUID > > > , InstanceUUID > > > + , DiskUUID > > > , NetworkUUID > > > , DRBDMinor > > > , DRBDMap > > > @@ -111,15 +112,17 @@ type NodeUUID = String > > > > > > type InstanceUUID = String > > > > > > +type DiskUUID = String > > > + > > > type NetworkUUID = String > > > > > > type DRBDMinor = Int > > > > > > -- | A map of the usage of DRBD minors > > > -type DRBDMap = Map NodeUUID (Map DRBDMinor InstanceUUID) > > > +type DRBDMap = Map NodeUUID (Map DRBDMinor DiskUUID) > > > > > > -- | A map of the usage of DRBD minors with possible duplicates > > > -type DRBDMap' = Map NodeUUID (Map DRBDMinor [InstanceUUID]) > > > +type DRBDMap' = Map NodeUUID (Map DRBDMinor [DiskUUID]) > > > > > > -- * The state data structure > > > > > > @@ -218,16 +221,15 @@ computeDRBDMap' :: (MonadError GanetiException m) > > > => ConfigData -> TempResState -> m DRBDMap' > > > computeDRBDMap' cfg trs = > > > flip execStateT (fmap (fmap (: [])) (trsDRBD trs)) > > > - $ F.forM_ (configInstances cfg) addDisks > > > + $ F.forM_ (configDisks cfg) addMinors > > > where > > > -- | Creates a lens for modifying the list of instances > > > - nodeMinor :: NodeUUID -> DRBDMinor -> Lens' DRBDMap' [InstanceUUID] > > > + nodeMinor :: NodeUUID -> DRBDMinor -> Lens' DRBDMap' [DiskUUID] > > > nodeMinor node minor = maybeLens (at node) . maybeLens (at minor) > > > - -- | Adds disks of an instance within the state monad > > > - addDisks inst = do > > > - disks <- toError $ getDrbdMinorsForInstance cfg > > inst > > > - forM_ disks $ \(minor, node) -> nodeMinor node > > minor > > > - %= (uuidOf > > inst :) > > > + -- | Adds minors of a disk within the state monad > > > + addMinors disk = do > > > + let minors = getDrbdMinorsForDisk disk > > > + forM_ minors $ \(minor, node) -> nodeMinor node minor %= (uuidOf > > disk :) > > > > > > -- | Compute the map of used DRBD minor/nodes. > > > -- Report any duplicate entries as an error. > > > @@ -246,25 +248,27 @@ computeDRBDMap cfg trs = do > > > -- Allocate a drbd minor. > > > -- > > > -- The free minor will be automatically computed from the existing > > devices. > > > --- A node can be given multiple times in order to allocate multiple > > minors. > > > +-- A node can not be given multiple times. > > > > Am I understanding this correctly that this was an error in the > > documentation? > > No this wasn't an error in the documentation. Previously > allocateDRBDMinor was used to allocate the DRBD minors for an instance > so a node could be given multiple times (an instance has many disks in > the same node). Now allocateDRBDMinor allocates the DRBD minors for a > disk so a node can not be given more than one times. Ah, true, thanks! > > > > -- The result is the list of minors, in the same order as the passed > > nodes. > > > allocateDRBDMinor :: (MonadError GanetiException m, MonadState > > TempResState m) > > > - => ConfigData -> InstanceUUID -> [NodeUUID] > > > + => ConfigData -> DiskUUID -> [NodeUUID] > > > -> m [DRBDMinor] > > > -allocateDRBDMinor cfg inst nodes = do > > > +allocateDRBDMinor cfg disk nodes = do > > > + unless (nodes == ordNub nodes) . resError > > > + $ "Duplicate nodes detected in list '" ++ show nodes ++ "'" > > > dMap <- computeDRBDMap' cfg =<< get > > > let usedMap = fmap M.keysSet dMap > > > - let alloc :: S.Set DRBDMinor -> Map DRBDMinor InstanceUUID > > > - -> (DRBDMinor, Map DRBDMinor InstanceUUID) > > > + let alloc :: S.Set DRBDMinor -> Map DRBDMinor DiskUUID > > > + -> (DRBDMinor, Map DRBDMinor DiskUUID) > > > alloc used m = let k = findFirst 0 (M.keysSet m `S.union` used) > > > - in (k, M.insert k inst m) > > > + in (k, M.insert k disk m) > > > > Indent! > > I didn't change the indentation here. How do you think this should be > indented? You're right! The previous code doesn't follow the style guide actually. No need to fix it then, I'll make a change for it later. > > > Thanks, > Ilias > > > > forM nodes $ \node -> trsDRBDL . maybeLens (at node) > > > %%= alloc (M.findWithDefault mempty node usedMap) > > > > > > --- Release temporary drbd minors allocated for a given instance using > > > +-- Release temporary drbd minors allocated for a given disk using > > > -- 'allocateDRBDMinor'. > > > -releaseDRBDMinors :: (MonadState TempResState m) => InstanceUUID -> m () > > > -releaseDRBDMinors inst = trsDRBDL %= filterNested (/= inst) > > > +releaseDRBDMinors :: (MonadState TempResState m) => DiskUUID -> m () > > > +releaseDRBDMinors disk = trsDRBDL %= filterNested (/= disk) > > > > > > -- * Other temporary resources > > > > > > diff --git a/test/py/cmdlib/instance_unittest.py > > b/test/py/cmdlib/instance_unittest.py > > > index 7a6e258..bc44dd4 100644 > > > --- a/test/py/cmdlib/instance_unittest.py > > > +++ b/test/py/cmdlib/instance_unittest.py > > > @@ -1133,7 +1133,7 @@ class _FakeConfigForGenDiskTemplate(ConfigMock): > > > def GenerateUniqueID(self, ec_id): > > > return "ec%s-uq%s" % (ec_id, self._unique_id.next()) > > > > > > - def AllocateDRBDMinor(self, nodes, instance): > > > + def AllocateDRBDMinor(self, nodes, disk): > > > return [self._drbd_minor.next() > > > for _ in nodes] > > > > > > diff --git a/test/py/cmdlib/testsupport/config_mock.py > > b/test/py/cmdlib/testsupport/config_mock.py > > > index 85375af..984a14d 100644 > > > --- a/test/py/cmdlib/testsupport/config_mock.py > > > +++ b/test/py/cmdlib/testsupport/config_mock.py > > > @@ -581,10 +581,10 @@ class ConfigMock(config.ConfigWriter): > > > def ComputeDRBDMap(self): > > > return dict((node_uuid, {}) for node_uuid in > > self._ConfigData().nodes) > > > > > > - def AllocateDRBDMinor(self, node_uuids, inst_uuid): > > > + def AllocateDRBDMinor(self, node_uuids, disk_uuid): > > > return map(lambda _: 0, node_uuids) > > > > > > - def _UnlockedReleaseDRBDMinors(self, inst_uuid): > > > + def _UnlockedReleaseDRBDMinors(self, disk_uuid): > > > pass > > > > > > def _CreateConfig(self): > > > -- > > > 1.7.10.4 > > > > > > > Otherwise LGTM -- Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
