Change in vdsm[master]: virt: clientIF: extract vmContainer into a module

fromani at redhat.com fromani at redhat.com
Thu Mar 3 11:08:44 UTC 2016


Francesco Romani has posted comments on this change.

Change subject: virt: clientIF: extract vmContainer into a module
......................................................................


Patch Set 6:

(7 comments)

https://gerrit.ovirt.org/#/c/53101/6/lib/vdsm/virt/vmdict.py
File lib/vdsm/virt/vmdict.py:

Line 1: #
Line 2: # Copyright 2011-2016 Red Hat, Inc.
> This is tricky, are we really able to copyright 2011-2015 on a new module?
IANAL. But I moved here code which was (c) 2011-2015, so it is new mode, but no new code, hence the range.
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or


Line 29: 
Line 30: 
Line 31: _log = logging.getLogger('vdsm')
Line 32: _lock = threading.Lock()
Line 33: _vms = {}
> uppercase?
I think not, they are not constants
Line 34: 
Line 35: 
Line 36: class UnknownVM(Exception):
Line 37:     pass


Line 84:     with _lock:
Line 85:         if vm.id in _vms:
Line 86:             del _vms[vm.id]
Line 87:         else:
Line 88:             raise UnknownVM()
> (applies to all of above) this is really just specialized dictionary - is i
You have a point here, but we this 'dict' thingy has to be a singleton, and the most pythonic way to do this is to use a module like this.
But I'll think about what you suggested, let's see what I can do.
Line 89: 
Line 90: 
Line 91: @utils.traceback()
Line 92: def dispatch_eio_cont(libvirt_vms, sdUUID):


Line 103: 
Line 104: @utils.traceback()
Line 105: def dispatch_libvirt_event(conn, dom, *args):
Line 106:     """
Line 107:     conn is unused
> why is it there then?
because the libvirt API requires it (AFAIK, didn't check too deeply. Will do)
Line 108:     """
Line 109:     vmid = dom.UUIDString()
Line 110:     with _lock:
Line 111:         if vmid in _vms:


https://gerrit.ovirt.org/#/c/53101/6/vdsm/virt/periodic.py
File vdsm/virt/periodic.py:

Line 66:                                   scheduler=scheduler)
Line 67:     _executor.start()
Line 68: 
Line 69:     def per_vm_operation(func, period):
Line 70:         disp = VmDispatcher(vmdict.get_all_items,
> (vmdict.py).items
seems nicer, let me try
Line 71:                             _executor, func, _timeout_from(period))
Line 72:         return Operation(disp, period, scheduler)
Line 73: 
Line 74:     _operations = [


Line 93:         # thus, does not need dispatching.
Line 94:         Operation(
Line 95:             sampling.VMBulkSampler(
Line 96:                 libvirtconnection.get(cif),
Line 97:                 vmdict.get_all_items,
> as above
ditto
Line 98:                 sampling.stats_cache),
Line 99:             config.getint('vars', 'vm_sample_interval'),
Line 100:             scheduler),
Line 101: 


https://gerrit.ovirt.org/#/c/53101/6/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 3902:             self.log.exception("Failed to delete VM %s", self.id)
Line 3903:         else:
Line 3904:             self._cleanupRecoveryFile()
Line 3905:             self.log.debug("Total desktops after destroy of %s is %d",
Line 3906:                            self.conf['vmId'], vmdict.num_vms())
> I don't like this (see vmdict.py) - we should be able to use len(${vmdict})
let's see how I can move in this direction
Line 3907: 
Line 3908:     def destroy(self):
Line 3909:         self.log.debug('destroy Called')
Line 3910: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd2ae6c5e9ca6a73c0fed978c78c9ebb001c46d
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list