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