Nir Soffer has posted comments on this change.

Change subject: storage: Add inplace virt-sparsify support
......................................................................


Patch Set 13:

(4 comments)

Great work Shmuel!

The only thing missing is blocking this for volumes that do not support 
sparsifying. If think that this works now only on nfs 4.2 and later (correct me 
if you have more recent info).

I think the best way would be to add supports_sparsify() method to 
sd.StorageDomainManifest and return False.

Then add real implementation to NFStorageDomainManifest returning True if the 
current version > 4.2. We can get the version from the mount options, see 
https://gerrit.ovirt.org/63636, for info on getting the mount options.

There is no NFStorageDomainManifest yet, but it should be easy to add, we need 
to override only one method or even class attribute, and define:

    class NfsStorageDomain(fileSD.FileStorageDomain):
        manifestClass = NFSDomainManifest

Note that gluster storage domain inherits from nfs, so you need to override 
this back to False in this class - unless gluster supports sparsifying. I don't 
think it does since fuse does not support it, but gluster guys know better.

We can then validate the storage domain when creating a job and fail the 
operation with SparifyNotSupported error.

See the comments for some minor improvements.

I think we can merge this patch as is and add the validation in the next 
patch(s).

https://gerrit.ovirt.org/#/c/57347/13/lib/vdsm/virtsparsify.py
File lib/vdsm/virtsparsify.py:

Line 64:     :param vol_path: path to the volume
Line 65:     """
Line 66:     cmd = [_VIRTSPARSIFY.cmd, '--machine-readable', '--in-place', 
vol_path]
Line 67: 
Line 68:     rc, out, err = commands.execCmd(cmd, env={'LIBGUESTFS_BACKEND': 
'direct'})
Please add documentation why this is needed, I know Richard is against using 
this flag.
Line 69: 
Line 70:     if rc != 0:


https://gerrit.ovirt.org/#/c/57347/13/vdsm/storage/sdm/api/sparsify_volume.py
File vdsm/storage/sdm/api/sparsify_volume.py:

Line 22: 
Line 23: from vdsm import virtsparsify
Line 24: from vdsm.storage import guarded
Line 25: 
Line 26: from .copy_data import CopyDataDivEndpoint
Importing from another verb is ugly. Since CopyDataDivEndpoint is reusable, it 
should move out of copy_data.py to a shared module in the sdm package, and it 
should be renamed since it is not related to copying data.

Maybe sdm/endpoints.DIVEndpoint?

Lets discuss this with Adam before we move stuff.
Line 27: 
Line 28: from . import base
Line 29: 
Line 30: 


Line 27: 
Line 28: from . import base
Line 29: 
Line 30: 
Line 31: class Job(base.Job):
Please keep empty line after the class declaration.
Line 32:     def __init__(self, job_id, host_id, vol_info):
Line 33:         super(Job, self).__init__(job_id, 'sparsify_volume', host_id)
Line 34:         self._vol_info = CopyDataDivEndpoint(vol_info, host_id, True)
Line 35: 


Line 30: 
Line 31: class Job(base.Job):
Line 32:     def __init__(self, job_id, host_id, vol_info):
Line 33:         super(Job, self).__init__(job_id, 'sparsify_volume', host_id)
Line 34:         self._vol_info = CopyDataDivEndpoint(vol_info, host_id, True)
True is not clear, better use writable=True - see copy_data.Job.__init__.
Line 35: 
Line 36:     def _run(self):
Line 37:         with guarded.context(self._vol_info.locks):
Line 38:             with self._vol_info.prepare():


-- 
To view, visit https://gerrit.ovirt.org/57347
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ac2bb1fbd2acbe0fc47694d17313c6ccd01a227
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smela...@redhat.com>
Gerrit-Reviewer: Adam Litke <ali...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Francesco Romani <from...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com>
Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Shmuel Leib Melamud <smela...@redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smela...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Reply via email to