Eduardo has posted comments on this change. Change subject: [WIP] Create storage pool using command type 1 ......................................................................
Patch Set 2: Code-Review-1 (6 comments) http://gerrit.ovirt.org/#/c/23647/2/vdsm/storage/blockSD.py File vdsm/storage/blockSD.py: Line 529: # to belong to the metadata volume. Since the metadata is at Line 530: # least SDMETADATA/METASIZE units, we know we can use the first Line 531: # SDMETADATA bytes of the metadata volume for the SD metadata. Line 532: # pass metadata's dev to ensure it is the first mapping Line 533: #mapping = cls.getMetaDataMapping(vgName) > in final version comment needs to move with code and this needs to disappea We already know that the mapping is redundant and it can be removed. This is a different patch. Line 534: Line 535: # Create the rest of the BlockSD internal volumes Line 536: lvm.createLV(vgName, sd.LEASES, sd.LEASES_SIZE, safe=False) Line 537: lvm.createLV(vgName, sd.IDS, sd.IDS_SIZE, safe=False) Line 593: } Line 594: Line 595: initialMetadata.update(mapping) Line 596: toAdd = encodeVgTags(initialMetadata) Line 597: lvm.changeVGTags(vgName, delTags=(), addTags=toAdd, safe=False) > if it is not safe/possible to call md.update then need to explain this IMHO is better to use the unused addVGTags() here. Since this function is actually unused can be safely converted to lock_type=1 only and assert that no new consumers are added. This is probably True since for any other option than a create will use changeVGTags() since a set of tags already exists. Line 598: Line 599: # Mark VG with Storage Domain Tag Line 600: try: Line 601: lvm.replaceVGTag(vgName, STORAGE_UNREADY_DOMAIN_TAG, Line 1318: for k, v in params.iteritems(): Line 1319: enc = poolMD.getEncoder(k) Line 1320: newParams[k] = enc(v) Line 1321: toAdd = encodeVgTags(newParams) Line 1322: lvm.changeVGTags(vgName, deltags=(), addTags=toAdd, safe=False) Use addVGTags() instead as discussed. Line 1323: Line 1324: def setMasterDomainParams(self, spUUID, leaseParams): Line 1325: vgUUID = self.getInfo()['vguuid'] Line 1326: vg = lvm.getVGbyUUID(vgUUID) Line 1330: sd.DMDK_ROLE: sd.MASTER_DOMAIN}) Line 1331: toDel = encodeVgTags({sd.DMDK_ROLE: sd.REGULAR_DOMAIN, Line 1332: sd.DMDK_POOLS: spUUID, Line 1333: sd.DMDK_POOLS: ''}) Line 1334: lvm.changeVGTags(vgName, delTags=toDel, addTags=toAdd, safe=False) IMHO simpler using the replaceVGTags() change already discussed. Line 1335: Line 1336: def refreshDirTree(self): Line 1337: # create domain images folder Line 1338: imagesPath = os.path.join(self.domaindir, sd.DOMAIN_IMAGES) http://gerrit.ovirt.org/#/c/23647/2/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 941: misc.validateUUID(spUUID, 'spUUID') Line 942: if masterDom not in domList: Line 943: raise se.InvalidParameterException("masterDom", str(masterDom)) Line 944: Line 945: if len(domList) > 1: ==1 semantics will be more strict. Line 946: raise NotImplementedError("Create storage pool " Line 947: "only with master domain") Line 948: Line 949: if len(poolName) > MAX_POOL_DESCRIPTION_SIZE: http://gerrit.ovirt.org/#/c/23647/2/vdsm/storage/sd.py File vdsm/storage/sd.py: Line 768: Line 769: def isMaster(self): Line 770: return self.getMetaParam(DMDK_ROLE).capitalize() == MASTER_DOMAIN Line 771: Line 772: @classmethod initMasterParams() is always called as an instance method. Then this should not be a classmethod. Since this called during pool creation/reconstruction all this redundant calls, including the spbackend mess can be avoided IMHO. Line 773: def initMasterParams(cls, poolMD, params): Line 774: poolMD.update(params) Line 775: Line 776: def setMasterDomainParams(self, spUUID, leaseParams): -- To view, visit http://gerrit.ovirt.org/23647 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia64f6dd2df38d2968f03ce66094f3ba7b4343503 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
