Change in vdsm[master]: Introduce VolumeArtifacts
nsoffer at redhat.com
nsoffer at redhat.com
Thu Dec 17 23:28:09 UTC 2015
Nir Soffer has posted comments on this change.
Change subject: Introduce VolumeArtifacts
......................................................................
Patch Set 12: Code-Review-1
(6 comments)
https://gerrit.ovirt.org/#/c/48097/12/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:
Line 69: """
Line 70: Create metadata file artifact, lease file, and volume file on storage.
Line 71:
Line 72: Caller must hold the domain lock (paxos lease) and the image resource
Line 73: corresponding to self.img_id in exclusive mode.
This is correct for all methods, right? better put this in __init__
Line 74: """
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 84: vol_format, parent_vol_id, preallocate=prealloc)
Line 85:
Line 86: # If these artifacts are forming a new image the artifacts path will be
Line 87: # a temporary directory. Otherwise it's the existing image directory.
Line 88: artifacts_path = self._get_artifacts_path()
We are doing this too many times, we can do it once if image does not exists, and remember this state.
Line 89: if not self._image_exists():
Line 90: self._create_image_artifact(artifacts_path)
Line 91:
Line 92: self._create_metadata_artifact(size, vol_format, prealloc, disk_type,
Line 86: # If these artifacts are forming a new image the artifacts path will be
Line 87: # a temporary directory. Otherwise it's the existing image directory.
Line 88: artifacts_path = self._get_artifacts_path()
Line 89: if not self._image_exists():
Line 90: self._create_image_artifact(artifacts_path)
I like image_exists() - it should be public.
I think this class should work like this:
- detect() - look in storage and understand if we are adding a volume to existing
image or creating a new image, and save this state.
- create() - create the directories and files based on the detected state
- commit() - commit the created artifacts based on the detected state
- dismantle() - convert the image/volume to artifacts based on the detected state
- purge() - remove the artifacts
After you detect(), the caller may like to learn about the instance, so methods such image_exits, or maybe is_existing_image should be public. Such info may be needed for the garbage collector.
detect can be part of init.
Line 91:
Line 92: self._create_metadata_artifact(size, vol_format, prealloc, disk_type,
Line 93: desc, parent_vol_id)
Line 94: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 122: leaf_type, disk_type, desc, volume.LEGAL_VOL)
Line 123:
Line 124: data = self.vol_class.formatMetadata(meta)
Line 125: with open(self._get_meta_artifact_path(), "w") as f:
Line 126: f.write(data)
We should fsync() here, otherwise the file may be empty or contain partial content when after we rename it.
Line 127:
Line 128: def _create_lease_file(self, vol_path):
Line 129: if self.domain_manifest.hasVolumeLeases():
Line 130: meta_id = (vol_path,)
Line 157:
Line 158: def _get_meta_artifact_path(self):
Line 159: return self._get_meta_path() + TEMP_VOL_FILEEXT
Line 160:
Line 161: def _get_meta_path(self):
Lets remove the _get from the names, and make these properties
Line 162: artifacts_path = self._get_artifacts_path()
Line 163: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 164: return self.vol_class._metaVolumePath(vol_path)
Line 165:
Line 160:
Line 161: def _get_meta_path(self):
Line 162: artifacts_path = self._get_artifacts_path()
Line 163: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 164: return self.vol_class._metaVolumePath(vol_path)
_metaVolumePath() should be public
Line 165:
Line 166: def _create_image_artifact(self, artifacts_path):
Line 167: self.log.debug("Creating image artifact directory: %r", artifacts_path)
Line 168: try:
--
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: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Daniel Erez <derez at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadgett at redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot <laravot at redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list