Hello Nir Soffer, Freddy Rolland, I'd like you to do a code review. Please visit
https://gerrit.ovirt.org/62735 to review the following change. Change subject: lvm: Separate lv reduce and extend ...................................................................... lvm: Separate lv reduce and extend Reducing and extending logical volumes were mixed in a horrible and buggy mess in _resizeLV. Now we have a much simpler and clear function for each operation. Change-Id: I0021d380fb26318ed565b3fae0205404d90bea28 Signed-off-by: Nir Soffer <nsof...@redhat.com> Reviewed-on: https://gerrit.ovirt.org/62369 Reviewed-by: Freddy Rolland <froll...@redhat.com> Reviewed-by: Allon Mureinik <amure...@redhat.com> Reviewed-by: Adam Litke <ali...@redhat.com> Continuous-Integration: Jenkins CI --- M vdsm/storage/lvm.py 1 file changed, 41 insertions(+), 42 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/35/62735/1 diff --git a/vdsm/storage/lvm.py b/vdsm/storage/lvm.py index 7717622..7ccf110 100644 --- a/vdsm/storage/lvm.py +++ b/vdsm/storage/lvm.py @@ -1143,19 +1143,10 @@ raise se.CannotRemoveLogicalVolume(vgName, str(lvNames)) -# TODO: kill this, inline into extendLV and reduceLV. -def _resizeLV(op, vgName, lvName, size): - """ - Size units: MB (1024 ** 2 = 2 ** 20)B. - """ - # WARNING! From man vgs: - # All sizes are output in these units: (h)uman-readable, (b)ytes, - # (s)ectors,(k)ilobytes, (m)egabytes, (g)igabytes, (t)erabytes, - # (p)etabytes, (e)xabytes. - # Capitalise to use multiples of 1000 (S.I.) instead of 1024. - cmd = (op,) + LVM_NOBACKUP - cmd += ("--size", "%sm" % (size,), "%s/%s" % (vgName, lvName)) - rc, out, err = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName, ))) +def extendLV(vgName, lvName, size_mb): + cmd = ("lvextend",) + LVM_NOBACKUP + cmd += ("--size", "%sm" % (size_mb,), "%s/%s" % (vgName, lvName)) + rc, out, err = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName,))) if rc != 0: # Since this runs only on the SPM, assume that cached vg and lv # metadata is correct. @@ -1165,46 +1156,54 @@ # Convert sizes to extents to match lvm behavior. extent_size = int(vg.extent_size) lv_extents = int(lv.size) // extent_size - requested_size = size * constants.MEGAB + requested_size = size_mb * constants.MEGAB requested_extents = (requested_size + extent_size - 1) // extent_size - if op == "lvextend": - if lv_extents >= requested_extents: - log.debug("LV %s/%s already extended (extents=%d, " - "requested=%d)", - vgName, lvName, lv_extents, requested_extents) - return + if lv_extents >= requested_extents: + log.debug("LV %s/%s already extended (extents=%d, requested=%d)", + vgName, lvName, lv_extents, requested_extents) + return - needed_extents = requested_extents - lv_extents - free_extents = int(vg.free_count) - if free_extents < needed_extents: - raise se.VolumeGroupSizeError( - "Not enough free extents to extend LV %s/%s (free=%d, " - "needed=%d)" - % (vgName, lvName, needed_extents, free_extents)) - elif op == "lvreduce": - if lv_extents <= requested_extents: - # May happen since callers do not calculate size in extents. - log.debug("LV %s/%s already reduced (extents=%d, " - "requested=%d)", - vgName, lvName, lv_extents, requested_extents) - return - else: - raise RuntimeError("Invalid resize operation %r" % op) + needed_extents = requested_extents - lv_extents + free_extents = int(vg.free_count) + if free_extents < needed_extents: + raise se.VolumeGroupSizeError( + "Not enough free extents for extending LV %s/%s (free=%d, " + "needed=%d)" + % (vgName, lvName, free_extents, needed_extents)) - # TODO: add and raise LogicalVolumeReduceError for lvreduce - raise se.LogicalVolumeExtendError(vgName, lvName, "%sM" % (size, )) + raise se.LogicalVolumeExtendError(vgName, lvName, "%sm" % (size_mb,)) _lvminfo._invalidatevgs(vgName) _lvminfo._invalidatelvs(vgName, lvName) -def extendLV(vgName, lvName, size): - _resizeLV("lvextend", vgName, lvName, size) +def reduceLV(vgName, lvName, size_mb): + cmd = ("lvreduce",) + LVM_NOBACKUP + cmd += ("--size", "%sm" % (size_mb,), "%s/%s" % (vgName, lvName)) + rc, out, err = _lvminfo.cmd(cmd, _lvminfo._getVGDevs((vgName,))) + if rc != 0: + # Since this runs only on the SPM, assume that cached vg and lv + # metadata is correct. + vg = getVG(vgName) + lv = getLV(vgName, lvName) + # Convert sizes to extents + extent_size = int(vg.extent_size) + lv_extents = int(lv.size) // extent_size + requested_size = size_mb * constants.MEGAB + requested_extents = (requested_size + extent_size - 1) // extent_size -def reduceLV(vgName, lvName, size): - _resizeLV("lvreduce", vgName, lvName, size) + if lv_extents <= requested_extents: + log.debug("LV %s/%s already reduced (extents=%d, requested=%d)", + vgName, lvName, lv_extents, requested_extents) + return + + # TODO: add and raise LogicalVolumeReduceError + raise se.LogicalVolumeExtendError(vgName, lvName, "%sm" % (size_mb,)) + + _lvminfo._invalidatevgs(vgName) + _lvminfo._invalidatelvs(vgName, lvName) def activateLVs(vgName, lvNames): -- To view, visit https://gerrit.ovirt.org/62735 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I0021d380fb26318ed565b3fae0205404d90bea28 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: ovirt-3.6 Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org