Change in vdsm[master]: [WIP] Create storage pool using command type 1

ewarszaw at redhat.com ewarszaw at redhat.com
Sun Feb 23 13:19:08 UTC 2014


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 <ykaplan at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list