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

ewarszaw at redhat.com ewarszaw at redhat.com
Sun Feb 23 15:11:33 UTC 2014


Eduardo has posted comments on this change.

Change subject: [WIP] destroy storage pool using command type 1
......................................................................


Patch Set 1: Code-Review-1

(3 comments)

http://gerrit.ovirt.org/#/c/24398/1/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 1366:             self.log.error(
Line 1367:                 "Can't remove pool %s from domain %s pool list %s, "
Line 1368:                 "it does not exist",
Line 1369:                 spUUID, self.sdUUID, str(pools))
Line 1370:             return
Use an else: block please instead this ugly return.
Line 1371:         vgUUID = self.getInfo()['vguuid']
Line 1372:         vg = lvm.getVGbyUUID(vgUUID)
Line 1373:         vgName = vg.name
Line 1374:         toAdd = encodeVgTags({sd.DMDK_POOLS: '',


Line 1369:                 spUUID, self.sdUUID, str(pools))
Line 1370:             return
Line 1371:         vgUUID = self.getInfo()['vguuid']
Line 1372:         vg = lvm.getVGbyUUID(vgUUID)
Line 1373:         vgName = vg.name
This block (3 lines) seems to me to be an echo from code a long time gone.

vgName is self.sdUUID.

Please remove this redundant calls.
Line 1374:         toAdd = encodeVgTags({sd.DMDK_POOLS: '',
Line 1375:                               sd.DMDK_ROLE: sd.REGULAR_DOMAIN})
Line 1376:         toDel = encodeVgTags({sd.DMDK_POOLS: spUUID,
Line 1377:                               sd.DMDK_ROLE: sd.MASTER_DOMAIN})


Line 1374:         toAdd = encodeVgTags({sd.DMDK_POOLS: '',
Line 1375:                               sd.DMDK_ROLE: sd.REGULAR_DOMAIN})
Line 1376:         toDel = encodeVgTags({sd.DMDK_POOLS: spUUID,
Line 1377:                               sd.DMDK_ROLE: sd.MASTER_DOMAIN})
Line 1378:         lvm.changeVGTags(vgName, delTags=toDel, addTags=toAdd, safe=False)
Since detachMaster is called only from hsm.destroyStoragePool() and the domain was already removed from the pool MD, you can avoid here the use of changeVGtags replacing it by addVGTags and remVGTags as was discussed in a previous patch.

The operations will be logically atomic and no one should try ever to access the domain between the removal and the adiditon of the role tag.

You can lock the master domain at hsm.destroyStoragePool() to be sure and this covers the previous forceDetach() call.
Line 1379: 
Line 1380:     def refresh(self):
Line 1381:         self.refreshDirTree()
Line 1382:         lvm.invalidateVG(self.sdUUID)


-- 
To view, visit http://gerrit.ovirt.org/24398
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I67cda9abd0bbc01d7d0642d5d3327f8687d7f728
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan 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