Change in vdsm[master]: StorageDomainManifest: move extend and extendVolume

nsoffer at redhat.com nsoffer at redhat.com
Thu Jul 16 14:56:16 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: StorageDomainManifest: move extend and extendVolume
......................................................................


Patch Set 14: Code-Review-1

(13 comments)

https://gerrit.ovirt.org/#/c/42266/14/tests/manifest_tests.py
File tests/manifest_tests.py:

Line 22
Line 23
Line 24
Line 25
Line 26
Please use () for long import lists - we don't break lines with \ (it is fragile)


Line 134:             manifest = make_blocksd_manifest(tmpdir)
Line 135:             lvm = FakeLVM(tmpdir)
Line 136:             vg_name = make_vg(lvm, manifest)
Line 137:             with MonkeyPatchScope([(blockSD, 'lvm', lvm)]):
Line 138:                 self.assertEquals(512, manifest.metaSize(vg_name))
Where does this value come from? is this hardcoded value we use in the setup code?

Maybe we need a comment to explain this value.
Line 139:                 lvm.vgmd[vg_name]['free'] = 512 << 20 - 1
Line 140:                 self.assertRaises(se.VolumeGroupSizeError,
Line 141:                                   manifest.metaSize, vg_name)
Line 142: 


Line 135:             lvm = FakeLVM(tmpdir)
Line 136:             vg_name = make_vg(lvm, manifest)
Line 137:             with MonkeyPatchScope([(blockSD, 'lvm', lvm)]):
Line 138:                 self.assertEquals(512, manifest.metaSize(vg_name))
Line 139:                 lvm.vgmd[vg_name]['free'] = 512 << 20 - 1
I find it too hard to work with shifts - please add constant (e.g. MB, GB) and use it instead (e.g, 512 * MB).

Also, what is 512 << 20 ? is this the minimal free size? we need a constant for this to make clear what are you testing:

    lvm.vgmd[vg_name]['free'] = MINIMAL_FREE_SIZE - 1

Don't we have such constant in the storage code somewhere?
Line 140:                 self.assertRaises(se.VolumeGroupSizeError,
Line 141:                                   manifest.metaSize, vg_name)
Line 142: 
Line 143:     def test_getmetadatamapping_bad_metadata_extent(self):


Line 142: 
Line 143:     def test_getmetadatamapping_bad_metadata_extent(self):
Line 144:         """
Line 145:         We require the metadata LV to reside on the first PV in the VG.  Check
Line 146:         that we raise an error if this is not the case.
We force it on first extent of the first pv
Line 147:         """
Line 148:         with namedTemporaryDir() as tmpdir:
Line 149:             manifest = make_blocksd_manifest(tmpdir)
Line 150:             lvm = FakeLVM(tmpdir)


Line 161: 
Line 162:     def test_getmetadatamapping(self):
Line 163:         """
Line 164:         Validate the behavior of getMetadataMapping including:
Line 165:          - pestart is 0 for all PVs in the VG
Please add that we lie and replace the real value (129MB) with 0
Line 166:          - PVs' physical extents (pe) are mapped into the VG in the order that
Line 167:            the PVs are added to the VG.  The mapoffset field shows the pe
Line 168:            number in the VG map to which this PV's first pe maps.
Line 169:         """


https://gerrit.ovirt.org/#/c/42266/14/tests/storagefakelib.py
File tests/storagefakelib.py:

Line 28: class FakeLVM(object):
Line 29:     _PV_SIZE = 10 << 30       # We pretend all PVs contribute 10G of space
Line 30:     _PV_UNUSABLE = 384 << 20  # 2 128M md areas + 1M header aligned to 128M
Line 31:     _PV_PE_SIZE = 128 << 20   # Guessed via inspection of real environment
Line 32:     _PV_PE_START = _PV_UNUSABLE
In real vg, I get somehow 129MB:

 # vgs --units b -o pv_name,pe_start,pv_pe_count,pv_pe_alloc_count,pv_size 2e133cd6-bfea-4655-ae6c-dc51a38bb7b9
  PV                        1st PE     PE  Alloc PSize       
  /dev/mapper/1IET_00070001 135266304B 397    65 53284438016B
  /dev/mapper/1IET_00070002 135266304B 397     0 53284438016B

lets add example output from lvm in each function that assume how lvm behaves, to make it easier to understand what this fake is doing, and also make it easier to verify that it simulate real lvm correctly.
Line 33: 
Line 34:     def __init__(self, root):
Line 35:         self.root = root
Line 36:         os.mkdir(os.path.join(self.root, 'dev'))


Line 107:                      active=True)
Line 108: 
Line 109:         vg_dict = self.lvmd.setdefault(vgName, {})
Line 110:         vg_dict[lvName] = lv_md
Line 111:         self.vgmd[vgName]['lv_count'] = int(self.vgmd[vgName]['lv_count']) + 1
You changed lv_count to int above (line 66) - do we still need this conversion?
Line 112: 
Line 113:     def extendLV(self, vgName, lvName, size):
Line 114:         # TODO: Update the allocated physical extents
Line 115:         self.lvmd[vgName][lvName]['size'] = size


Line 134:         lv_attr = real_lvm.LV_ATTR(**lv_md['attr'])
Line 135:         lv_md['attr'] = lv_attr
Line 136:         return real_lvm.LV(**lv_md)
Line 137: 
Line 138:     def getFirstExt(self, vg, lv):
Lets add the lvm command generating this string and the output:

 # lvs -o name,devices
  LV                                   Devices                       
  4c705e28-ad99-46c2-b1eb-43fd8501ea97 /dev/mapper/1IET_00010004(31) 
  ids                                  /dev/mapper/1IET_00010004(21) 
  inbox                                /dev/mapper/1IET_00010004(22) 
  leases                               /dev/mapper/1IET_00010004(5)  
  master                               /dev/mapper/1IET_00010004(23) 
  metadata                             /dev/mapper/1IET_00010004(0)  
  outbox                               /dev/mapper/1IET_00010004(4)
Line 139:         return self.getLV(vg, lv).devices.strip(" )").split("(")
Line 140: 
Line 141:     def listPVNames(self, vgName):
Line 142:         return self.getVG(vgName).pv_name


Line 155:             if pv['pe_start'] > next_pe_start:
Line 156:                 next_pe_start = pv['pe_start'] + pv['pe_count']
Line 157:         return next_pe_start
Line 158: 
Line 159:     def _create_pv(self, pv_name, vg_name, size):
According to this lvm command, pe_start is the same on all pvs:

 # vgs --units b -o pv_name,pe_start,pv_pe_count,pv_pe_alloc_count,pv_size 2e133cd6-bfea-4655-ae6c-dc51a38bb7b9
  PV                        1st PE     PE  Alloc PSize       
  /dev/mapper/1IET_00070001 135266304B 397    65 53284438016B
  /dev/mapper/1IET_00070002 135266304B 397     0 53284438016B

This is example vg with 2 pvs and 10 lvs.

So _find_next_pe_start() is not needed.
Line 160:         pe_start = self._find_next_pe_start(vg_name)
Line 161:         pe_count = (size - self._PV_UNUSABLE) / self._PV_PE_SIZE
Line 162:         pv_md = dict(uuid=fake_lvm_uuid(),
Line 163:                      name=pv_name,


Line 198:             for pv in self.pvmd.values():
Line 199:                 if pv['vg_name'] != vg_name:
Line 200:                     continue
Line 201:                 pe_count += pv['pe_count']
Line 202:             return pe_count
Too much indentation.

Also why not:

    return sum(pv["pe_count"] for pv in self.pvmd.values()
               if pv["vg_name"] == vg_name)
Line 203: 
Line 204: 
Line 205: _fqpvname = real_lvm._fqpvname
Line 206: 


https://gerrit.ovirt.org/#/c/42266/14/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 747
Line 748
Line 749
Line 750
Line 751
You forgot to keep here a wrapper for resizePV


Line 460:         lvm.activateLVs(self.sdUUID, [sd.IDS])
Line 461:         return lvm.lvPath(self.sdUUID, sd.IDS)
Line 462: 
Line 463:     def extendVolume(self, volumeUUID, size, isShuttingDown=None):
Line 464:         self._extendlock.acquire()
For another patch - use with self._extendlock
Line 465:         try:
Line 466:             # FIXME: following line.
Line 467:             lvm.extendLV(self.sdUUID, volumeUUID, size)  # , isShuttingDown)
Line 468:         finally:


https://gerrit.ovirt.org/#/c/42266/14/vdsm/storage/sd.py
File vdsm/storage/sd.py:

Line 329:     def getVersion(self):
Line 330:         return self.getMetaParam(DMDK_VERSION)
Line 331: 
Line 332:     def resizePV(self, guid):
Line 333:         pass
Can you add a todo to remove this method? calling resizePV on a non-block sd is a bug in the caller and should fail loudly.

Same for extendVG and extendVolume.
Line 334: 
Line 335: 
Line 336: class StorageDomain(object):
Line 337:     log = logging.getLogger("Storage.StorageDomain")


-- 
To view, visit https://gerrit.ovirt.org/42266
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0ad1769d7e4db6fc0883b37a67d071c278b96c8b
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list