Change in vdsm[master]: Introduce VolumeArtifacts

alitke at redhat.com alitke at redhat.com
Mon Dec 7 19:46:10 UTC 2015


Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
......................................................................


Patch Set 4:

(14 comments)

https://gerrit.ovirt.org/#/c/48097/4/vdsm/storage/sdm/volumeartifacts.py
File vdsm/storage/sdm/volumeartifacts.py:

Line 63:     def _oop(self):
Line 64:         return self.domain_manifest.oop
Line 65: 
Line 66:     def create(self, size, vol_format, disk_type, desc,
Line 67:                parent_vol_id=volume.BLANK_UUID):
> Please document our assumptions - which locks must be held when this is cal
Done
Line 68:         # XXX: Remove these when support is added:
Line 69:         if vol_format != volume.RAW_FORMAT:
Line 70:             raise CannotCreateVolumeArtifacts("Only raw volumes are supported")
Line 71:         if parent_vol_id != volume.BLANK_UUID:


Line 77: 
Line 78:         self._create_metadata_artifact(meta_id, size, vol_format, disk_type,
Line 79:                                        desc, parent_vol_id)
Line 80:         self._create_lease_artifact(meta_id)
Line 81:         self._create_container_artifact(vol_path, vol_format, size)
> All the logic was move out of create, so we don't understand anything from 
Done
Line 82: 
Line 83:     def commit(self):
Line 84:         try:
Line 85:             self._oop.os.rename(self._meta_file_create_path(),


Line 86:                                 self._meta_file_commit_path())
Line 87:         except OSError as e:
Line 88:             if e.errno == errno.ENOENT:
Line 89:                 raise VolumeArtifactsNotFound()
Line 90:             raise
> This step is not needed when creating new image, so better avoid them and t
Not so fast... FileVolumes are identified by searching the domain for *.meta files.  This happens in all directories (even remove_me_* image directories).  I don't want to break that behavior and affect old code.  So for commit we have two methods depending on whether this is a new image or not:

New Image:
Start -> rename metadata -> rename image dir
(So we transition as: Artifacts -> garbage image -> New image)

Existing image:
Start -> rename metadata
(So we transition as: Image with garbage -> Image without garbage)
Line 91: 
Line 92:         # If we created a new image directory, rename it to the correct name
Line 93:         if self._get_artifacts_path() == self._new_image_path():
Line 94:             self._oop.os.rename(self._get_artifacts_path(),


Line 89:                 raise VolumeArtifactsNotFound()
Line 90:             raise
Line 91: 
Line 92:         # If we created a new image directory, rename it to the correct name
Line 93:         if self._get_artifacts_path() == self._new_image_path():
> We should not created these paths multiple times. Keep the state, if we are
Done
Line 94:             self._oop.os.rename(self._get_artifacts_path(),
Line 95:                                 self._existing_image_path())
Line 96: 
Line 97:     def _create_metadata_artifact(self, meta_id, size, vol_format, disk_type,


Line 97:     def _create_metadata_artifact(self, meta_id, size, vol_format, disk_type,
Line 98:                                   desc, parent_vol_id):
Line 99:         # Create the metadata artifact.  The metadata file is created with a
Line 100:         # special extension to prevent these artifacts from being recognized as
Line 101:         # a volume until FileVolumeArtifacts.commit() is called.
> I would like to have the same naming conversion for temporary entities (e.g
This contradicts your comment below where you'd like me to reuse _deletedImagePath().  I think the semantics of garbage images is different enough from these artifacts that we shouldn't steamroll the old ImageGarbageCollector code.
Line 102: 
Line 103:         # File volumes are always created sparse
Line 104:         prealloc = volume.SPARSE_VOL
Line 105:         self.domain_manifest.validateCreateVolumeParams(


Line 109:         meta = fileVolume.FileVolumeMetadata.makeMetadata(
Line 110:             self.domain_manifest.sdUUID, self.img_id, parent_vol_id, size,
Line 111:             volume.type2name(vol_format), volume.type2name(prealloc),
Line 112:             leaf_type, disk_type, desc, volume.LEGAL_VOL)
Line 113:         fileVolume.FileVolumeMetadata.putArtifactMetadata(meta_id, meta)
> Please avoid class methods in new code, it makes testing harder. We don't h
I'm conforming to the existing interface here.  At this point we really should not create a VolumeMetadata instance to work with VolumeArtifacts.  It seems safer/better to use a classmethod to prevent us from doing anything else with a VolumeMetadata that we should not be doing.
Line 114: 
Line 115:     def _create_lease_artifact(self, meta_id):
Line 116:         fileVolume.FileVolumeMetadata.newVolumeLease(
Line 117:             meta_id, self.domain_manifest.sdUUID, self.vol_id)


Line 115:     def _create_lease_artifact(self, meta_id):
Line 116:         fileVolume.FileVolumeMetadata.newVolumeLease(
Line 117:             meta_id, self.domain_manifest.sdUUID, self.vol_id)
Line 118: 
Line 119:     def _create_container_artifact(self, vol_path, vol_format, size):
> This is not an artifact.
In my parlance yes it is.

A real "Volume" is composed of three artifacts (lease, metadata, and data) that are properly assembled to be seen by the greater system.

The VolumeArtifacts class manages all three artifacts and their transition to/from a proper Volume.
Line 120:         size_bytes = size * volume.BLOCK_SIZE
Line 121:         trunc_size = size_bytes if vol_format == volume.RAW_FORMAT else 0
Line 122:         try:
Line 123:             self._oop.truncateFile(vol_path, trunc_size,


Line 127:             if e.errno == errno.EEXIST:
Line 128:                 raise se.VolumeAlreadyExists(vol_path)
Line 129:             raise
Line 130: 
Line 131:     def _get_artifacts_path(self):
> Hiding the path of existing image or new image behind this getter does not 
ok, will rework this.
Line 132:         if self._artifacts_path:
Line 133:             return self._artifacts_path
Line 134: 
Line 135:         # If the artifacts belong to an existing image just return the image


Line 132:         if self._artifacts_path:
Line 133:             return self._artifacts_path
Line 134: 
Line 135:         # If the artifacts belong to an existing image just return the image
Line 136:         # directory.  Otherwise create a new image directory and return it.
> You create an image directory as side effect of a getter?!
Yeah, will be reworking this.
Line 137:         img_path = self._existing_image_path()
Line 138:         if not self._oop.os.path.exists(img_path):
Line 139:             img_path = self._new_image_path()
Line 140:         self._artifacts_path = img_path


Line 139:             img_path = self._new_image_path()
Line 140:         self._artifacts_path = img_path
Line 141:         return self._artifacts_path
Line 142: 
Line 143:     def _create_artifacts_path(self):
> This should not do such magic, creating or skipping.
will rework.
Line 144:         path = self._get_artifacts_path()
Line 145:         if path == self._new_image_path():
Line 146:             self.log.debug("Creating path for new image: %s", path)
Line 147:             try:


Line 156:                 raise
Line 157:         else:
Line 158:             self.log.debug("Skipping directory creation for image: %s", path)
Line 159: 
Line 160:     def _new_image_path(self):
> This should be named to temporary_image_path, or some other name that make 
This could only be used if:
1) We agree to keep using REMOVED_IMAGE_PREFIX
2) We rename it from _getDeletedImagePath to getDeletedImagePath
Line 161:         # If forming a new image, create artifacts in a garbage-collectible
Line 162:         # image directory.
Line 163:         return os.path.join(self.domain_manifest.domaindir,
Line 164:                             sd.DOMAIN_IMAGES,


Line 165:                             sd.REMOVED_IMAGE_PREFIX + self.img_id)
Line 166: 
Line 167:     def _existing_image_path(self):
Line 168:         return os.path.join(self.domain_manifest.domaindir,
Line 169:                             sd.DOMAIN_IMAGES, self.img_id)
> This is implemented currently by Image.getImagePath()
yep.
Line 170: 
Line 171:     def _meta_file_commit_path(self):
Line 172:         vol_path = os.path.join(self._get_artifacts_path(), self.vol_id)
Line 173:         return fileVolume.FileVolumeMetadata._metaVolumePath(vol_path)


Line 169:                             sd.DOMAIN_IMAGES, self.img_id)
Line 170: 
Line 171:     def _meta_file_commit_path(self):
Line 172:         vol_path = os.path.join(self._get_artifacts_path(), self.vol_id)
Line 173:         return fileVolume.FileVolumeMetadata._metaVolumePath(vol_path)
> Avoid using class or static methods, this is makes testing much harder. We 
This is preexisting code.
Line 174: 
Line 175:     def _meta_file_create_path(self):
Line 176:         return self._meta_file_commit_path() + fileVolume.ARTIFACT_FILEEXT
Line 177: 


Line 172:         vol_path = os.path.join(self._get_artifacts_path(), self.vol_id)
Line 173:         return fileVolume.FileVolumeMetadata._metaVolumePath(vol_path)
Line 174: 
Line 175:     def _meta_file_create_path(self):
Line 176:         return self._meta_file_commit_path() + fileVolume.ARTIFACT_FILEEXT
> We talked about better names before, .flux, .gc, .tmp, I don't understand t
We'll settle on one eventually.  We now have the VolumeArtifacts class so '.artifact' seemed like a natural choice.
Line 177: 
Line 178: 
Line 179: class BlockVolumeArtifacts(VolumeArtifacts):


-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list