Change in vdsm[master]: vm: add the transient disk support

danken at redhat.com danken at redhat.com
Sun Oct 6 10:41:33 UTC 2013


Dan Kenigsberg has posted comments on this change.

Change subject: vm: add the transient disk support
......................................................................


Patch Set 11: Code-Review-1

(10 comments)

....................................................
Commit Message
Line 4: Commit:     Federico Simoncelli <fsimonce at redhat.com>
Line 5: CommitDate: 2013-10-05 18:04:31 -0400
Line 6: 
Line 7: vm: add the transient disk support
Line 8: 
What are "transient disks"? I am really not sure I understand.
Line 9: This patch adds support for transient disk for 'hotplug' and 'run vm'
Line 10: flows. Libvirt currently doesn't yet support transient disks, hence
Line 11: this patch fills the gap until the time libvirt starts supporting it.
Line 12: 


....................................................
File lib/vdsm/config.py.in
Line 157: 
Line 158:         ('libvirt_env_variable_log_outputs', '',
Line 159:             'Specify the output to track libvirt calls'),
Line 160: 
Line 161:         ('transient_disks_repository', '@VDSMLIBDIR@/transient',
Too bad /var/run/vdsm is too small.
Line 162:             'Local path to the transient disks repository.'),
Line 163:     ]),
Line 164: 
Line 165:     # Section: [ksm]


....................................................
File lib/vdsm/tool/transient.py
Line 27: from vdsm.utils import execCmd, CommandPath
Line 28: 
Line 29: 
Line 30: SELINUX_VIRT_IMAGE_LABEL = "system_u:object_r:virt_image_t:s0"
Line 31: TRANSIENT_DISKS_REPO = config.get('vars', 'transient_disks_repository')
do we really need to have this directory configurable? creating it on install time could have been simpler.
Line 32: 
Line 33: _fuser = CommandPath(
Line 34:     "fuser",
Line 35:     "/sbin/fuser",  # Fedora, EL6


Line 32: 
Line 33: _fuser = CommandPath(
Line 34:     "fuser",
Line 35:     "/sbin/fuser",  # Fedora, EL6
Line 36: )
using vdsm/storage/fuser.py is not easy (it requires moving that module) but would have been better for the project as a whole.
Line 37: 
Line 38: 
Line 39: @expose("setup-transient-repository")
Line 40: def setup_transient_repository(*args):


Line 50:             raise
Line 51: 
Line 52:     os.chown(TRANSIENT_DISKS_REPO, vdsm_uid, vdsm_gid)
Line 53:     os.chmod(TRANSIENT_DISKS_REPO, 0750)
Line 54:     selinux.chcon(TRANSIENT_DISKS_REPO, SELINUX_VIRT_IMAGE_LABEL)
doen't this explode if selinux is disabled?

besides, shouldn't we require a proper selinux-policy change for this? otherwise someone doing "restorecon" would un-do this. change.
Line 55: 
Line 56: 
Line 57: @expose("cleanup-transient-repository")
Line 58: def cleanup_transient_repository(*args):


Line 61:     (NOTE: it is recommended to NOT execute this command when the vdsm
Line 62:     daemon is running)
Line 63:     """
Line 64:     transient_images = set((os.path.join(TRANSIENT_DISKS_REPO, x)
Line 65:                            for x in os.listdir(TRANSIENT_DISKS_REPO)))
nit: doesn't glob() do the right thing cleaner?
Line 66: 
Line 67:     if len(transient_images) == 0:
Line 68:         return  # Nothing to do
Line 69: 


....................................................
File vdsm/vm.py
Line 412: class DRIVE_SHARED_TYPE:
Line 413:     NONE = "none"
Line 414:     EXCLUSIVE = "exclusive"
Line 415:     SHARED = "shared"
Line 416:     TRANSIENT = "transient"
I'm missing something basic. Why is "transient" a DRIVE_SHARED_TYPE? I'd expect transient drive to be all "exclusive".
Line 417: 
Line 418:     @classmethod
Line 419:     def getAllValues(cls):
Line 420:         # TODO: use introspection


Line 3298:                                   params=customProps)
Line 3299:         return {'status': doneCode, 'vmList': self.status()}
Line 3300: 
Line 3301:     def _createTransientDisk(self, drive):
Line 3302:         if drive.get('shared', None) != DRIVE_SHARED_TYPE.TRANSIENT:
style: I prefer to put this condition out of the function, over bailing out silently if the argument is not what we need.
Line 3303:             return
Line 3304: 
Line 3305:         # FIXME: This should be replaced in future the support for transient
Line 3306:         # disk in libvirt (BZ#832194)


Line 3327:         drive['path'] = transientPath
Line 3328:         drive['format'] = 'cow'
Line 3329: 
Line 3330:     def _removeTransientDisk(self, drive):
Line 3331:         if getattr(drive, 'transient', False):
what is setting this attribute? or should you mean 'transientDisk'?
Line 3332:             os.unlink(drive.path)
Line 3333: 
Line 3334:     def hotplugDisk(self, params):
Line 3335:         if self.isMigrating():


Line 3390:         diskParams = params.get('drive', {})
Line 3391:         diskParams['path'] = self.cif.prepareVolumePath(diskParams)
Line 3392: 
Line 3393:         try:
Line 3394:             drive = self._findDriveByUUIDs(diskParams)
a nice cleanup, worthy of its own patch.
Line 3395:         except LookupError:
Line 3396:             self.log.error("Hotunplug disk failed - Disk not found: %s",
Line 3397:                            diskParams)
Line 3398:             return {'status': {


-- 
To view, visit http://gerrit.ovirt.org/18326
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3dfec35e324c47d8c86a965947e3ae4ae48c7524
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Deepak C Shetty <deepakcs at linux.vnet.ibm.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Deepak C Shetty <deepakcs at linux.vnet.ibm.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list