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