Nir Soffer has posted comments on this change. Change subject: storage: Add basic BlockVolumeArtifacts ......................................................................
Patch Set 17: Code-Review-1 (7 comments) https://gerrit.ovirt.org/#/c/55987/17/vdsm/storage/sdm/volume_artifacts.py File vdsm/storage/sdm/volume_artifacts.py: Line 305: Line 306: GARBAGE Line 307: Line 308: - States: Line 309: - A logical volume with the TEMP_VOL_LVTAG tag exists We have many sub states here: - setting lv md tag failed - writing metadata failed - creating the lease failed Line 310: - Operations: Line 311: - is_garbage -> true Line 312: - is_image -> true or false Line 313: - clean -> change state to MISSING Line 393: tags = (TEMP_VOL_LVTAG, Line 394: blockVolume.TAG_PREFIX_PARENT + parent_vol_id, Line 395: blockVolume.TAG_PREFIX_IMAGE + self.img_id) Line 396: lvm.createLV(self.sd_manifest.sdUUID, self.vol_id, lv_size, Line 397: activate=True, initialTags=tags) Do we have a test for lvm.createLV failure? Line 398: Line 399: def _acquire_metadata_slot(self): Line 400: with self.sd_manifest.acquireVolumeMetadataSlot( Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot: Line 395: blockVolume.TAG_PREFIX_IMAGE + self.img_id) Line 396: lvm.createLV(self.sd_manifest.sdUUID, self.vol_id, lv_size, Line 397: activate=True, initialTags=tags) Line 398: Line 399: def _acquire_metadata_slot(self): We need a test for failure to acquire the slot, and failure to change the lv. Line 400: with self.sd_manifest.acquireVolumeMetadataSlot( Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot: Line 402: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, Line 403: addTags=[blockVolume.TAG_PREFIX_MD + str(slot)]) Line 399: def _acquire_metadata_slot(self): Line 400: with self.sd_manifest.acquireVolumeMetadataSlot( Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot: Line 402: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, Line 403: addTags=[blockVolume.TAG_PREFIX_MD + str(slot)]) I would like to make the code easier to read(or debug) like this: md_tag = blockVolume.TAG_PREFIX_MD + str(slot) lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, addTags=[md_tag]) Line 404: return self.sd_manifest.sdUUID, slot Line 405: Line 406: def _create_metadata(self, meta_id, size, vol_format, prealloc, disk_type, Line 407: desc, parent): Line 400: with self.sd_manifest.acquireVolumeMetadataSlot( Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot: Line 402: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, Line 403: addTags=[blockVolume.TAG_PREFIX_MD + str(slot)]) Line 404: return self.sd_manifest.sdUUID, slot Returning meta_id is useful for the callers, but it is not related to the purpose of this function. The caller can build the meta_id using self.sd_manifest.sdUUID themself. Line 405: Line 406: def _create_metadata(self, meta_id, size, vol_format, prealloc, disk_type, Line 407: desc, parent): Line 408: # When the volume format is RAW the real volume capacity is the device Line 408: # When the volume format is RAW the real volume capacity is the device Line 409: # size. The device size may have been rounded up if 'size' is not Line 410: # divisible by the domain's extent size. Line 411: if vol_format == volume.RAW_FORMAT: Line 412: size = int(self.sd_manifest.getVSize(self.img_id, self.vol_id)) We have a test for this, right? Line 413: Line 414: # We use the BlockVolumeManifest API here because we create the Line 415: # metadata in the standard way. We cannot do this for file volumes Line 416: # because the metadata needs to be written to a specially named file. Line 430: volume.LEGAL_VOL) Line 431: Line 432: def _create_lease(self, meta_id): Line 433: self.vol_class.newVolumeLease( Line 434: meta_id, self.sd_manifest.sdUUID, self.vol_id) We need a test for failure to create the lease. -- To view, visit https://gerrit.ovirt.org/55987 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d Gerrit-PatchSet: 17 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
