Nir Soffer has posted comments on this change. Change subject: Introduce VolumeArtifacts ......................................................................
Patch Set 11: Code-Review-1 (14 comments) https://gerrit.ovirt.org/#/c/48097/11/vdsm/storage/sdm/volume_artifacts.py File vdsm/storage/sdm/volume_artifacts.py: Line 57: Line 58: def __init__(self, domain_manifest, img_id, vol_id): Line 59: super(FileVolumeArtifacts, self).__init__(domain_manifest, img_id, Line 60: vol_id) Line 61: self._image_path = self.domain_manifest.getImagePath(img_id) The old name is too abstract, this is not a path but a directory. We should call this self._image_dir. Line 62: Line 63: @property Line 64: def _oop(self): Line 65: return self.domain_manifest.oop Line 75: # XXX: Remove these when support is added: Line 76: if vol_format != volume.RAW_FORMAT: Line 77: raise CannotCreateVolumeArtifacts("Only raw volumes are supported") Line 78: if parent_vol_id != volume.BLANK_UUID: Line 79: raise CannotCreateVolumeArtifacts("parent_vol_id not supported") validating create parameters should be here. Line 80: Line 81: # If these artifacts are forming a new image the artifacts path will be Line 82: # a temporary directory. Otherwise it's the existing image directory. Line 83: artifacts_path = self._get_artifacts_path() Line 81: # If these artifacts are forming a new image the artifacts path will be Line 82: # a temporary directory. Otherwise it's the existing image directory. Line 83: artifacts_path = self._get_artifacts_path() Line 84: if artifacts_path != self._image_path: Line 85: self._create_artifacts_path(artifacts_path) Add blank line. Line 86: vol_path = os.path.join(artifacts_path, self.vol_id) Line 87: meta_id = (vol_path,) Line 88: Line 89: self._create_metadata_artifact(meta_id, size, vol_format, disk_type, Line 83: artifacts_path = self._get_artifacts_path() Line 84: if artifacts_path != self._image_path: Line 85: self._create_artifacts_path(artifacts_path) Line 86: vol_path = os.path.join(artifacts_path, self.vol_id) Line 87: meta_id = (vol_path,) This does not make any sense in this level. Methods that need to send meta_id can generate this useless tuple. Line 88: Line 89: self._create_metadata_artifact(meta_id, size, vol_format, disk_type, Line 90: desc, parent_vol_id) Line 91: self._create_lease_artifact(meta_id) Line 85: self._create_artifacts_path(artifacts_path) Line 86: vol_path = os.path.join(artifacts_path, self.vol_id) Line 87: meta_id = (vol_path,) Line 88: Line 89: self._create_metadata_artifact(meta_id, size, vol_format, disk_type, send vol_path instead. Line 90: desc, parent_vol_id) Line 91: self._create_lease_artifact(meta_id) Line 92: self._create_container_artifact(vol_path, vol_format, size) Line 93: Line 87: meta_id = (vol_path,) Line 88: Line 89: self._create_metadata_artifact(meta_id, size, vol_format, disk_type, Line 90: desc, parent_vol_id) Line 91: self._create_lease_artifact(meta_id) send vol_path instead Line 92: self._create_container_artifact(vol_path, vol_format, size) Line 93: Line 94: def commit(self): Line 95: artifacts_path = self._get_artifacts_path() Line 94: def commit(self): Line 95: artifacts_path = self._get_artifacts_path() Line 96: vol_path = os.path.join(artifacts_path, self.vol_id) Line 97: commit_path = self.vol_class._metaVolumePath(vol_path) Line 98: create_path = commit_path + constants.ARTIFACT_FILEEXT We should move this to self.meta_artifact_path, and use it when we create the file. Currently we use old code to create the file, and new code to rename it. Either we always get the path from the old code, or always compute it here, but not mix. Line 99: try: Line 100: self._oop.os.rename(create_path, commit_path) Line 101: except OSError as e: Line 102: if e.errno == errno.ENOENT: Line 115: Line 116: # File volumes are always created sparse Line 117: prealloc = volume.SPARSE_VOL Line 118: self.domain_manifest.validateCreateVolumeParams( Line 119: vol_format, parent_vol_id, preallocate=prealloc) This should be first thing we do in create, before touching storage. Line 120: leaf_type = volume.type2name(volume.LEAF_VOL) Line 121: Line 122: meta = self.vol_class.makeMetadata( Line 123: self.domain_manifest.sdUUID, self.img_id, parent_vol_id, size, Line 122: meta = self.vol_class.makeMetadata( Line 123: self.domain_manifest.sdUUID, self.img_id, parent_vol_id, size, Line 124: volume.type2name(vol_format), volume.type2name(prealloc), Line 125: leaf_type, disk_type, desc, volume.LEGAL_VOL) Line 126: self.vol_class.putArtifactMetadata(meta_id, meta) For file volume, this is trivial write, can we avoid this dependency? Also the old code is writing to path + ".new" and renaming. Since we write into temporary file, we don't need this. We don't care if the .meta.tmp file is empty or contains half of the data, since it is considered as garbage. Line 127: Line 128: def _create_lease_artifact(self, meta_id): Line 129: self.vol_class.newVolumeLease(meta_id, self.domain_manifest.sdUUID, Line 130: self.vol_id) Line 124: volume.type2name(vol_format), volume.type2name(prealloc), Line 125: leaf_type, disk_type, desc, volume.LEGAL_VOL) Line 126: self.vol_class.putArtifactMetadata(meta_id, meta) Line 127: Line 128: def _create_lease_artifact(self, meta_id): _create_lease_file? Guard with self._domain_manifest.hasVolumeLeases() Line 129: self.vol_class.newVolumeLease(meta_id, self.domain_manifest.sdUUID, Line 130: self.vol_id) Line 131: Line 132: def _create_container_artifact(self, vol_path, vol_format, size): Line 128: def _create_lease_artifact(self, meta_id): Line 129: self.vol_class.newVolumeLease(meta_id, self.domain_manifest.sdUUID, Line 130: self.vol_id) Line 131: Line 132: def _create_container_artifact(self, vol_path, vol_format, size): _create_volume_file? Line 133: size_bytes = size * volume.BLOCK_SIZE Line 134: trunc_size = size_bytes if vol_format == volume.RAW_FORMAT else 0 Line 135: try: Line 136: self._oop.truncateFile( Line 140: if e.errno == errno.EEXIST: Line 141: raise CannotCreateVolumeArtifacts() Line 142: raise Line 143: Line 144: def _get_artifacts_path(self): I don't like this. Maybe have exists() or has_image() instead? Line 145: # If the artifacts belong to an existing image just return the image Line 146: # directory. Otherwise create a new image directory and return it. Line 147: if self._oop.os.path.exists(self._image_path): Line 148: return self._image_path Line 148: return self._image_path Line 149: else: Line 150: return self.domain_manifest._getDeletedImagePath(self.img_id) Line 151: Line 152: def _create_artifacts_path(self, artifacts_path): _create_image_artifact? Line 153: self.log.debug("Creating path for new image: %s", artifacts_path) Line 154: try: Line 155: self._oop.os.mkdir(artifacts_path) Line 156: except OSError as e: Line 149: else: Line 150: return self.domain_manifest._getDeletedImagePath(self.img_id) Line 151: Line 152: def _create_artifacts_path(self, artifacts_path): Line 153: self.log.debug("Creating path for new image: %s", artifacts_path) Creating image artifact directory: %r Line 154: try: Line 155: self._oop.os.mkdir(artifacts_path) Line 156: except OSError as e: Line 157: if e.errno != errno.EEXIST: -- To view, visit https://gerrit.ovirt.org/48097 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
