Change in vdsm[master]: Live Merge: Prevent merge when base volume is too small

alitke at redhat.com alitke at redhat.com
Fri Jun 26 18:12:23 UTC 2015


Adam Litke has posted comments on this change.

Change subject: Live Merge: Prevent merge when base volume is too small
......................................................................


Patch Set 1:

(6 comments)

https://gerrit.ovirt.org/#/c/42836/1/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4569:                         startCleanup(storedJob, drive, doPivot)
Line 4570:                 jobsRet[jobID] = entry
Line 4571:         return jobsRet
Line 4572: 
Line 4573:     def _merge_check_base_size(self, drive, base_info, top_info):
> This should be something like can_merge_into(drive, base_info, top_info).
ok
Line 4574:         # If the drive waa resized the top volume could be larger than the
Line 4575:         # base volume.  Libvirt can handle this situation for filw-based
Line 4576:         # volumes and block qcow volumes (where extension happens dynamically).
Line 4577:         # Raw block volumes cannot be extended by libvirt so we require ovirt


Line 4580:             return True
Line 4581: 
Line 4582:         if int(base_info['capacity']) < int(top_info['capacity']):
Line 4583:             self.log.error("merge: The base volume is undersized and cannot "
Line 4584:                            "be extended.  Aborting.")
> This is not a vdsm error, maybe client error, not checking the sizes before
ok
Line 4585:             return False
Line 4586:         return True
Line 4587: 
Line 4588:     def merge(self, driveSpec, baseVolUUID, topVolUUID, bandwidth, jobUUID):


Line 4585:             return False
Line 4586:         return True
Line 4587: 
Line 4588:     def merge(self, driveSpec, baseVolUUID, topVolUUID, bandwidth, jobUUID):
Line 4589:         def get_volume_info(drive, volUUID):
> This should be a regular method, see getVolumeSize().
ok
Line 4590:             res = self.cif.irs.getVolumeInfo(drive.domainID, drive.poolID,
Line 4591:                                              drive.imageID, volUUID)
Line 4592:             if res['status']['code'] != 0:
Line 4593:                 self.log.error("Unable to get volume info for '%s'",  volUUID)


Line 4590:             res = self.cif.irs.getVolumeInfo(drive.domainID, drive.poolID,
Line 4591:                                              drive.imageID, volUUID)
Line 4592:             if res['status']['code'] != 0:
Line 4593:                 self.log.error("Unable to get volume info for '%s'",  volUUID)
Line 4594:                 raise LookupError(volUUID)
> This is not a lookup error (no such volume), we don't know why it failed. S
ok
Line 4595:             return res['info']
Line 4596: 
Line 4597:         if not caps.getLiveMergeSupport():
Line 4598:             self.log.error("Live merge is not supported on this host")


Line 4629: 
Line 4630:         try:
Line 4631:             baseInfo = get_volume_info(drive, baseVolUUID)
Line 4632:             topInfo = get_volume_info(drive, topVolUUID)
Line 4633:         except LookupError:
> Better log the error here instead of the helper, where we have more context
ok
Line 4634:             return errCode['mergeErr']
Line 4635: 
Line 4636:         # If base is a shared volume then we cannot allow a merge.  Otherwise
Line 4637:         # We'd corrupt the shared volume for other users.


Line 4686:         # plus a bit more to accommodate additional writes to 'top' during the
Line 4687:         # live merge operation.
Line 4688:         if drive.chunked:
Line 4689:             capacity, alloc, physical = self._getExtendInfo(drive)
Line 4690:             topAllocSize = int(topInfo['apparentsize'])
> Why using apparentsize is better then the alloc returned by libvirt?
Because we collect topInfo in all cases and the libvirt info is only available for the leaf volume (maybe others in future versions of libvirt).  It's best to reduce the number of special cases.
Line 4691:             self.extendDriveVolume(drive, baseVolUUID, topAllocSize, capacity)
Line 4692: 
Line 4693:         # Trigger the collection of stats before returning so that callers
Line 4694:         # of getVmStats after this returns will see the new job


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf77a7c5108b500c6ec34653ef7570a841def1b4
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadgett at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
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