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 <fsimo...@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 <deepa...@linux.vnet.ibm.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Deepak C Shetty <deepa...@linux.vnet.ibm.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Timothy Asir <tjeya...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches