Federico Simoncelli has posted comments on this change. Change subject: Shrink the new volume When merging block cow volumes ......................................................................
Patch Set 2: (4 inline comments) Since you need to resubmit to make jenkins happy I left few additional comments. .................................................... Commit Message Line 3: AuthorDate: 2012-11-13 17:47:46 +0200 Line 4: Commit: Yeela Kaplan <[email protected]> Line 5: CommitDate: 2012-12-12 11:28:34 +0200 Line 6: Line 7: Shrink the new volume When merging block cow volumes We could try to stick to the subject format suggested by the commit template. I think that the principal module involved in this change (not in quantity of code but in terms of where the feature is added) is the "image", but you can choose something else if you prefer: image: shrink new volume when merging block cow volumes Line 8: Line 9: On merge, size of the new volume is currently the sum of virtual Line 10: sizes of the original volumes that were merged. With qcow Line 11: volumes on block devices we don't use the whole size of the new volume, .................................................... File vdsm/qemuImg.py Line 103: offset = int(__iregexSearch("offset", out[1])) Line 104: except: Line 105: raise QImgError(rc, out, err, "unable to parse qemu-img check output") Line 106: Line 107: return offset The scope of "check" is to check an image and report errors/fixes, it's a little bit odd to return the offset only. I know you probably don't want to implement everything now (reporting all the other information) but you could start returning a dictionary and adding a TODO/FIXME comment for everything else missing. I also think that we should filter the exit codes here (rc) since they have a meaning in terms of what failed during the check. Please add a comment about that too (at line 99). .................................................... File vdsm/storage/blockVolume.py Line 291: """ Line 292: self.log.info("Request to reduce LV %s of image %s in VG %s with " Line 293: "size = %s", self.volUUID, self.imgUUID, self.sdUUID, Line 294: newSize) Line 295: sizemb = (newSize + 2047) / 2048 I can't recall what's 2048... is it MEGAB / BLOCK_SIZE? Can you make that explicit? (in extend too) I feel like we need to align data in many places, we could extract this in an utility eg: utils.alignInteger(newSize, MEGAB / BLOCK_SIZE) Not in this patch though (I have a feeling that python might have something ready). Line 296: lvm.reduceLV(self.sdUUID, self.volUUID, sizemb) Line 297: Line 298: def shrinkToOptimalSize(self): Line 299: """ .................................................... File vdsm/storage/lvm.py Line 1057: _lvminfo._invalidatelvs(vgName, lvNames) Line 1058: raise se.CannotRemoveLogicalVolume(vgName, str(lvNames)) Line 1059: Line 1060: Line 1061: def resizeLV(op, vgName, lvName, size): Let's make this private. Line 1062: """ Line 1063: Size units: MB (1024 ** 2 = 2 ** 20)B. Line 1064: """ Line 1065: # WARNING! From man vgs: -- To view, visit http://gerrit.ovirt.org/9725 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ace4c48d278cb84ce871bc402643131265c3198 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Eduardo <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Shu Ming <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
