Change in vdsm[master]: Introduce VolumeArtifacts

nsoffer at redhat.com nsoffer at redhat.com
Wed Dec 23 22:55:03 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 14:

(12 comments)

I like this

https://gerrit.ovirt.org/#/c/48097/14/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 83:     def metadata_volatile_path(self):
Line 84:         return self.metadata_stable_path + TEMP_VOL_FILEEXT
Line 85: 
Line 86:     @property
Line 87:     def metadata_stable_path(self):
For consistency with volume_path and lease_path, I think we should call this meta_path, and meta_volatile_path.
Line 88:         vol_path = os.path.join(self.artifacts_dir, self.vol_id)
Line 89:         return self.vol_class.metaVolumePath(vol_path)
Line 90: 
Line 91:     @property


Line 108:         self.sd_manifest.validateCreateVolumeParams(
Line 109:             vol_format, parent_vol_id, preallocate=prealloc)
Line 110: 
Line 111:         # If these artifacts are forming a new image we need to create a new
Line 112:         # temporary image directory.
This comment is not needed now, the code explain itself.
Line 113:         if not self.image_exists:
Line 114:             self._create_image_artifact(self.artifacts_dir)
Line 115: 
Line 116:         self._create_metadata_artifact(size, vol_format, prealloc, disk_type,


Line 110: 
Line 111:         # If these artifacts are forming a new image we need to create a new
Line 112:         # temporary image directory.
Line 113:         if not self.image_exists:
Line 114:             self._create_image_artifact(self.artifacts_dir)
We don't need this parameter now
Line 115: 
Line 116:         self._create_metadata_artifact(size, vol_format, prealloc, disk_type,
Line 117:                                        desc, parent_vol_id)
Line 118:         self._create_lease_file()


Line 119:         self._create_volume_file(vol_format, size)
Line 120: 
Line 121:     def commit(self):
Line 122:         lease_path = self.vol_class.leaseVolumePath(self.volume_path)
Line 123:         for path in self.metadata_volatile_path, self.volume_path, lease_path:
The lease_path is relevant only if the domain has volume leases.
Line 124:             if not os.path.exists(path):
Line 125:                 raise se.VolumeDoesNotExist("Path %s not found" % path)
Line 126: 
Line 127:         self._oop.os.rename(self.metadata_volatile_path,


Line 120: 
Line 121:     def commit(self):
Line 122:         lease_path = self.vol_class.leaseVolumePath(self.volume_path)
Line 123:         for path in self.metadata_volatile_path, self.volume_path, lease_path:
Line 124:             if not os.path.exists(path):
You must use _oop.os.path.exists - stat may block.

We don't need to validated the .meta.volatile path, as it will fail in the rename in the next block. 

Validating volume_path and lease_path can be nice though, but created them while we hold a lock, so I don't see any reason that will cause these files to disappear, and we will fail to use this volume in this case anyway, so I think we should simplify and not validate them now.
Line 125:                 raise se.VolumeDoesNotExist("Path %s not found" % path)
Line 126: 
Line 127:         self._oop.os.rename(self.metadata_volatile_path,
Line 128:                             self.metadata_stable_path)


Line 121:     def commit(self):
Line 122:         lease_path = self.vol_class.leaseVolumePath(self.volume_path)
Line 123:         for path in self.metadata_volatile_path, self.volume_path, lease_path:
Line 124:             if not os.path.exists(path):
Line 125:                 raise se.VolumeDoesNotExist("Path %s not found" % path)
Use %r for such logs
Line 126: 
Line 127:         self._oop.os.rename(self.metadata_volatile_path,
Line 128:                             self.metadata_stable_path)
Line 129: 


Line 124:             if not os.path.exists(path):
Line 125:                 raise se.VolumeDoesNotExist("Path %s not found" % path)
Line 126: 
Line 127:         self._oop.os.rename(self.metadata_volatile_path,
Line 128:                             self.metadata_stable_path)
This will raise OSError, and we do not handle it nicely yet. We need to raise vdsm api error for this, not sure about the error, as this may mean lot of errors (EPERM, ENOENT, etc.).
Line 129: 
Line 130:         # If we created a new image directory, rename it to the correct name
Line 131:         if not self.image_exists:
Line 132:             self._oop.os.rename(self.artifacts_dir, self._image_dir)


Line 140:         leaf_type = volume.type2name(volume.LEAF_VOL)
Line 141:         meta = self.vol_class.makeMetadata(
Line 142:             self.sd_manifest.sdUUID, self.img_id, parent_vol_id, size,
Line 143:             volume.type2name(vol_format), volume.type2name(prealloc),
Line 144:             leaf_type, disk_type, desc, volume.LEGAL_VOL)
Lets format this one parameter per line, it is very hard to read such blob. Then we don't need the temporay leaf_type.
Line 145: 
Line 146:         data = self.vol_class.formatMetadata(meta)
Line 147:         # XXX: Do we need to use ioprocess here?
Line 148:         if os.path.exists(self.metadata_volatile_path):


Line 143:             volume.type2name(vol_format), volume.type2name(prealloc),
Line 144:             leaf_type, disk_type, desc, volume.LEGAL_VOL)
Line 145: 
Line 146:         data = self.vol_class.formatMetadata(meta)
Line 147:         # XXX: Do we need to use ioprocess here?
Yes
Line 148:         if os.path.exists(self.metadata_volatile_path):
Line 149:             raise se.VolumeAlreadyExists("Path: %s already exists" %
Line 150:                                          self.metadata_volatile_path)
Line 151:         with open(self.metadata_volatile_path, "w") as f:


Line 146:         data = self.vol_class.formatMetadata(meta)
Line 147:         # XXX: Do we need to use ioprocess here?
Line 148:         if os.path.exists(self.metadata_volatile_path):
Line 149:             raise se.VolumeAlreadyExists("Path: %s already exists" %
Line 150:                                          self.metadata_volatile_path)
Instead of this check, open the file with O_EXCL | O_CREAT, and let it fail if the file exists. Not sure that ioprocess is supporting these flags.

This should be raised only if open fails with EEXIST, other errors should probably not handled here.
Line 151:         with open(self.metadata_volatile_path, "w") as f:
Line 152:             f.write(data)
Line 153:             f.flush()
Line 154: 


Line 147:         # XXX: Do we need to use ioprocess here?
Line 148:         if os.path.exists(self.metadata_volatile_path):
Line 149:             raise se.VolumeAlreadyExists("Path: %s already exists" %
Line 150:                                          self.metadata_volatile_path)
Line 151:         with open(self.metadata_volatile_path, "w") as f:
This must be done with ioprocess, it may block.
Line 152:             f.write(data)
Line 153:             f.flush()
Line 154: 
Line 155:     def _create_lease_file(self):


Line 149:             raise se.VolumeAlreadyExists("Path: %s already exists" %
Line 150:                                          self.metadata_volatile_path)
Line 151:         with open(self.metadata_volatile_path, "w") as f:
Line 152:             f.write(data)
Line 153:             f.flush()
flush is not needed, when we exit the context, the file is flushed and closed.
Line 154: 
Line 155:     def _create_lease_file(self):
Line 156:         if self.sd_manifest.hasVolumeLeases():
Line 157:             meta_id = (self.volume_path,)


-- 
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: 14
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