Francesco Romani has posted comments on this change. Change subject: v2v: Convert VM from external source to Data Domain ......................................................................
Patch Set 4: (13 comments) a few more comments about ImportVmThread. Still processing it. http://gerrit.ovirt.org/#/c/37509/4/vdsm/v2v.py File vdsm/v2v.py: Line 36: import caps Line 37: Line 38: Line 39: _jobs = {} Line 40: _lock = threading.Lock() nit: it is usually preferred to put the lock first, then just after the guarded fields, then a blank space. Example: lock = threading.Lock() something = {} something_else = [] silly_field = 1 more_fields = 3.14 so, better if you move _lock just above _jobs. But this is minor, you can do if and only you have or want resubmit for other reasons Line 41: _P_V2V_DIR = os.path.join(P_VDSM_RUN, 'v2v') Line 42: Line 43: Line 44: class InvalidVMConfiguration(ValueError): Line 110: kill_zombie_jobs() Line 111: return ovf Line 112: Line 113: Line 114: def kill_zombie_jobs(): For the sake of easier review, I'm repeating and expanding the same comment of version 3. I'm ok with the concept here (see commentS in version 3), there are two things that concerns me - the first, major, is that I'm a bit worried by the pgrep/kill dance this function is doing. I need to think a bit more about that. - the second, minor, is that kill_zombie_job*S*() suggests that there may be more than one stale job, while during the review I learned this isn't true. If AFAIU, then this may be misleading and can be easily fixed. Line 115: ret, out, err = execCmd([EXT_PGREP, 'virt-v2v'], sudo=True) Line 116: if len(err) > 0: Line 117: logging.error('error while trying to grep virt-v2v processes: %r', err) Line 118: return Line 153: } Line 154: return ret Line 155: Line 156: Line 157: class ImportVmThread(threading.Thread): I need to think about this as well, I haven't reviewed this class properly yet. Line 158: """ Line 159: Importing VM to data domain Line 160: Running virt-v2v command Line 161: Parsing the stdout Line 160: Running virt-v2v command Line 161: Parsing the stdout Line 162: """ Line 163: def __init__(self, uri, username, password, vmProperties, jobId, cif): Line 164: threading.Thread.__init__(self) it is usually preferred to * not inherit from threading.Thread, use composition instead, so * have a private threading.Thread internally. There are other examples of this usage scatthered into the codebase. Line 165: self.setDaemon(True) Line 166: self._uri = uri Line 167: self._username = username Line 168: self._password = password Line 168: self._password = password Line 169: self._vmProperties = vmProperties Line 170: self._job_id = jobId Line 171: self._cif = cif Line 172: self._status = 'initializing' it is preferred to group constants like this in a helper enum-like class, e.g. (naming will be ugly, is just for illustration) class V2VImport: INITIALIZING = 'initializing' STARTING = 'starting' ... Line 173: self._progress = 0 Line 174: self._abort = False Line 175: Line 176: @property Line 184: @property Line 185: def progress(self): Line 186: return self._progress Line 187: Line 188: def run(self): the utils.traceback decorator may be handy here. Line 189: try: Line 190: self._import_vm() Line 191: except Exception as ex: Line 192: logging.info('error in importing external vm: %r', ex) Line 206: self._vmProperties['vmName']) Line 207: Line 208: logging.info('import vm, (jobId %s) started, cmd: %r', self._job_id, Line 209: cmd) Line 210: proc = subprocess.Popen(cmd, env={'LIBGUESTFS_BACKEND': 'direct'}, we'd probably need some input from infra here to make sure we are talking with an extern process properly and to make sure we aren't missing any best practice. Line 211: shell=True, Line 212: stdout=subprocess.PIPE, Line 213: stderr=subprocess.PIPE) Line 214: Line 275: self._status = msg Line 276: logging.error(msg) Line 277: Line 278: def _create_passwd_file(self): Line 279: self._pass_file = os.path.join(_P_V2V_DIR, "%s.tmp" % self._job_id) is this a plain tempfile or there is any special need? If it is just a tempfile we should use safer tempfile creation from stdlib and/or vdsm library functions. Line 280: f = os.fdopen(os.open(self._pass_file, Line 281: os.O_WRONLY | os.O_CREAT, 0600), 'w') Line 282: with closing(f): Line 283: f.write(self._password) Line 278: def _create_passwd_file(self): Line 279: self._pass_file = os.path.join(_P_V2V_DIR, "%s.tmp" % self._job_id) Line 280: f = os.fdopen(os.open(self._pass_file, Line 281: os.O_WRONLY | os.O_CREAT, 0600), 'w') Line 282: with closing(f): ok, you use this fdopen dance to get permissions right, I believe. If so we should just switch to proper tempfile handling. Just to give an easy example: https://docs.python.org/2/library/tempfile.html#tempfile.NamedTemporaryFile Line 283: f.write(self._password) Line 284: Line 285: def _delete_passwd_file(self): Line 286: if os.path.exists(self._pass_file): Line 283: f.write(self._password) Line 284: Line 285: def _delete_passwd_file(self): Line 286: if os.path.exists(self._pass_file): Line 287: os.remove(self._pass_file) this also should be handled by tempfile above. Line 288: Line 289: def abort_job(self): Line 290: self._abort = True Line 291: self._status = 'abort' Line 303: if 'imageID' in disk: Line 304: images = '%s --vdsm-image-uuid %s ' % (images, disk['imageID']) Line 305: if 'volumeID' in disk: Line 306: images = '%s --vdsm-vol-uuid %s ' % (images, disk['volumeID']) Line 307: return images this works but feels unpythonic. How about images = [] for disk in self._vmProperties['disks']: if 'imageID' in disk: images.append('--vdsm-image-uuid') images.append(disk['imageID']) elif 'volumeID' in disk: # can a disk have both?! images.append('--vdsm-vol-uuid') images.append(disk['volumeID']) return ' '.join(images) Line 308: Line 309: def _get_domain_path(self): Line 310: disk = self._vmProperties['disks'][0] Line 311: drive = {'device': 'disk', Line 306: images = '%s --vdsm-vol-uuid %s ' % (images, disk['volumeID']) Line 307: return images Line 308: Line 309: def _get_domain_path(self): Line 310: disk = self._vmProperties['disks'][0] please add a comment to document why the first disk is important/special/good enough for this function. Line 311: drive = {'device': 'disk', Line 312: 'poolID': self._vmProperties['poolID'], Line 313: 'domainID': self._vmProperties['domainID'], Line 314: 'imageID': disk['imageID'], Line 314: 'imageID': disk['imageID'], Line 315: 'volumeID': disk['volumeID']} Line 316: Line 317: volPath = self._cif.getInstance().prepareVolumePath(drive) Line 318: volPath = volPath.split('/images/')[0] maybe better use os.path.split Line 319: return volPath Line 320: Line 321: Line 322: def _mem_to_mib(size, unit): -- To view, visit http://gerrit.ovirt.org/37509 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34bd86d5a87ea8c42113c4a732f87ddd4ceab9ea Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
