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? > > > > # 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 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. > > -- 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? 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
signature.asc
Description: Digital signature
