Change in vdsm[master]: v2v: Convert VM from external source to Data Domain
shavivi at redhat.com
shavivi at redhat.com
Mon Mar 16 14:22:46 UTC 2015
Shahar Havivi has posted comments on this change.
Change subject: v2v: Convert VM from external source to Data Domain
......................................................................
Patch Set 10:
(38 comments)
https://gerrit.ovirt.org/#/c/37509/10/vdsm/v2v.py
File vdsm/v2v.py:
Line 41: status is a way to feedback information on the job (init, error etc)
Line 42: status_msg is the descriptoin of the status
Line 43: when the job is done a call to get_converted_vm will return the VMs OVF.
Line 44: to clear a jobs once the OVF retrieved call to delete_job
Line 45: """
> This should be first thing in the module after the copyright block.
Done
Line 46:
Line 47: _lock = threading.Lock()
Line 48: _jobs = {}
Line 49:
Line 74:
Line 75: class NoSuchJob(V2VJobError):
Line 76: ''' Job not exists in _jobs collection '''
Line 77: def __init__(self):
Line 78: self.code = 'invalidV2VJob'
> This is best done as:
I will call it err_name
Line 79:
Line 80:
Line 81: class JobExists(V2VJobError):
Line 82: ''' Job already exists in _jobs collection '''
Line 97:
Line 98:
Line 99: class VolumeError(RuntimeError):
Line 100: def __str__(self):
Line 101: return "Bad volume specification " + RuntimeError.__str__(self)
> Why do you need this?
for the log,
I saw that in vm.volumeError exception
Line 102:
Line 103:
Line 104: def supported():
Line 105: return not (caps.getos() in (caps.OSName.RHEVH, caps.OSName.RHEL)
Line 159:
Line 160: def delete_job(jobId):
Line 161: try:
Line 162: job = _get_job(jobId)
Line 163: _validate_job_done(job)
> This will prevent removal of aborted or failed jobs.
Done
Line 164: _remove_job(jobId)
Line 165: except V2VJobError as e:
Line 166: return errCode[e.code]
Line 167: return {'status': doneCode}
Line 180: def jobs():
Line 181: ret = {}
Line 182: with _lock:
Line 183: jobs = _jobs
Line 184: for jobId, jobValue in jobs.iteritems():
> This is unsafe - iteritems() is good when you have 100000 jobs and you don'
Done
Line 185: ret[jobId] = {
Line 186: 'status': jobValue.status,
Line 187: 'status_msg': jobValue.status_msg,
Line 188: 'disk_progress': jobValue.disk_progress,
Line 182: with _lock:
Line 183: jobs = _jobs
Line 184: for jobId, jobValue in jobs.iteritems():
Line 185: ret[jobId] = {
Line 186: 'status': jobValue.status,
> Whats can be nicer then "job.status" here?
Done
Line 187: 'status_msg': jobValue.status_msg,
Line 188: 'disk_progress': jobValue.disk_progress,
Line 189: 'progress': jobValue.progress
Line 190: }
Line 218:
Line 219:
Line 220: def _read_ovf(jobId):
Line 221: try:
Line 222: file_name = os.path.join(_V2V_DIR, "%s.ovf" % jobId)
> This cannot raise IOError, and should be out of the try except block.
Done
Line 223: with open(file_name, 'r') as f:
Line 224: ovf = f.read()
Line 225: return ovf
Line 226: except IOError as e:
Line 221: try:
Line 222: file_name = os.path.join(_V2V_DIR, "%s.ovf" % jobId)
Line 223: with open(file_name, 'r') as f:
Line 224: ovf = f.read()
Line 225: return ovf
> You can simplify by:
Done
Line 226: except IOError as e:
Line 227: if e.errno == errno.ENOENT:
Line 228: raise InvalidOvfPath()
Line 229: else:
Line 224: ovf = f.read()
Line 225: return ovf
Line 226: except IOError as e:
Line 227: if e.errno == errno.ENOENT:
Line 228: raise InvalidOvfPath()
> This error looks wrong - the error is here is should be something like "NoS
Done
Line 229: else:
Line 230: logging.error('Error reading file "%r" error: %s message %s',
Line 231: file_name, e.errno, e.message)
Line 232: raise
Line 231: file_name, e.errno, e.message)
Line 232: raise
Line 233:
Line 234:
Line 235: class ImportVm():
> This is old style class - use ImportVm(object)
Done
Line 236: def __init__(self, uri, username, password, vmProperties, jobId, cif):
Line 237: self._uri = uri
Line 238: self._username = username
Line 239: self._password = password
Line 273: portion ie if we have 2 disks the first will take
Line 274: 0-50 and the second 50-100
Line 275: '''
Line 276: return (self._disk_progress / self._disk_count +
Line 277: int(100 / self._disk_count) * (self._current_disk - 1))
> This is little too complected, and may never return 100 for some values. Fo
Done
Line 278:
Line 279: @traceback(msg="Error importing vm")
Line 280: def _run(self):
Line 281: self._create_passwd_file()
Line 282: try:
Line 283: self._import()
Line 284: except Exception as ex:
Line 285: self._status = STATUS.ERROR
Line 286: self._status_msg = ex.message
> You must abort here, otherwise you are leaving stale virt-v2v processes.
Done
Line 287: raise
Line 288: finally:
Line 289: self._delete_passwd_file()
Line 290: self._teardownVolumes()
Line 286: self._status_msg = ex.message
Line 287: raise
Line 288: finally:
Line 289: self._delete_passwd_file()
Line 290: self._teardownVolumes()
> Please use pep8_style_function_names - you already use this for most functi
Done
Line 291:
Line 292: def _import(self):
Line 293: self._status = STATUS.STARTING
Line 294: # TODO: use the process handling http://gerrit.ovirt.org/#/c/33909/
Line 314: self._watch_process_output()
Line 315:
Line 316: if self._proc.returncode != 0:
Line 317: self._status = STATUS.ERROR
Line 318: self._handle_process_errors()
> Instead of this, get process error and raise it:
Done
Line 319: else:
Line 320: self._status = STATUS.DONE
Line 321:
Line 322: def disk_copy_stage(self, status_msg, current_disk, disk_count):
Line 315:
Line 316: if self._proc.returncode != 0:
Line 317: self._status = STATUS.ERROR
Line 318: self._handle_process_errors()
Line 319: else:
> If you raise, you don't need the else.
Done
Line 320: self._status = STATUS.DONE
Line 321:
Line 322: def disk_copy_stage(self, status_msg, current_disk, disk_count):
Line 323: self._status = STATUS.DISK_COPY
Line 322: def disk_copy_stage(self, status_msg, current_disk, disk_count):
Line 323: self._status = STATUS.DISK_COPY
Line 324: self._status_msg = status_msg
Line 325: self._current_disk = int(current_disk)
Line 326: self._disk_count = int(disk_count)
> I understand from this that we don't know the number of disks or the size o
Yes,
We may know if we probe again as we did in get_external_vms or we can add that as parameters in convert verb - which the engine will need to pass back.
But we do think that this way is better.
Line 327: self._disk_progress = 0
Line 328: self._watch_copy_prpgress()
Line 329:
Line 330: def progress_stage(self, progress):
Line 333: if self._disk_progress != 100:
Line 334: self._watch_copy_prpgress()
Line 335: except ValueError:
Line 336: self._status = STATUS.ERROR
Line 337: self._abort_msg('error converting disk progress')
> Instead of this, abort the process and raise:
Done
Line 338: raise
Line 339:
Line 340: def _watch_process_output(self):
Line 341: while not self._aborted and self._proc.returncode is None:
Line 340: def _watch_process_output(self):
Line 341: while not self._aborted and self._proc.returncode is None:
Line 342: line = self._proc.stdout.readline()
Line 343: try:
Line 344: self._parser.parse_line(line)
> I this would return a value, it would be much simpler - the logic in disk_c
The call back was Dankens idea (discussed privately).
If we do return the values from parse_line - we need to check if not None because it only return the values if "Copying Disk" is in the line.
I am good with both suggestions
Line 345: except JobOutputParsing:
Line 346: self._status = STATUS.ERROR
Line 347: self._abort_msg("error parsing 'Copying disk' section: %s"
Line 348: % line)
Line 344: self._parser.parse_line(line)
Line 345: except JobOutputParsing:
Line 346: self._status = STATUS.ERROR
Line 347: self._abort_msg("error parsing 'Copying disk' section: %s"
Line 348: % line)
> Do not handle it here, let it bubble up to _run
Done
Line 349: raise
Line 350:
Line 351: def _watch_copy_prpgress(self):
Line 352: while not self._aborted and self._proc.returncode is None:
Line 357: self._parser.parse_progress(buf)
Line 358: except JobOutputParsing:
Line 359: self._status = STATUS.ERROR
Line 360: self._abort_msg('error parsing disk progress')
Line 361: raise
> Don't handle it here - just let it bubble up to _run.
Done
Line 362:
Line 363: def _handle_process_errors(self):
Line 364: error = self._proc.stderr.readlines()
Line 365: if len(error) > 0:
Line 364: error = self._proc.stderr.readlines()
Line 365: if len(error) > 0:
Line 366: msg = 'error process ended with errors: %s' % error
Line 367: self._status_msg = msg
Line 368: logging.error(msg)
> Rename to _get_process_error and return an error message.
Done
Line 369:
Line 370: def _create_passwd_file(self):
Line 371: try:
Line 372: os.mkdir(_V2V_DIR, 0o700)
Line 366: msg = 'error process ended with errors: %s' % error
Line 367: self._status_msg = msg
Line 368: logging.error(msg)
Line 369:
Line 370: def _create_passwd_file(self):
> The first part should be extracted to _create_v2v_dir()
Done
Line 371: try:
Line 372: os.mkdir(_V2V_DIR, 0o700)
Line 373: except OSError as e:
Line 374: if e.errno == os.errno.EEXIST:
Line 370: def _create_passwd_file(self):
Line 371: try:
Line 372: os.mkdir(_V2V_DIR, 0o700)
Line 373: except OSError as e:
Line 374: if e.errno == os.errno.EEXIST:
> os.errno is not a public interface of module "os". Use errno module.
Done
Line 375: os.chmod(_V2V_DIR, 0o700)
Line 376: else:
Line 377: raise
Line 378:
Line 373: except OSError as e:
Line 374: if e.errno == os.errno.EEXIST:
Line 375: os.chmod(_V2V_DIR, 0o700)
Line 376: else:
Line 377: raise
> I would reverse the order:
Done
Line 378:
Line 379: self._pass_file = os.path.join(_V2V_DIR, "%s.tmp" % self.id)
Line 380: f = os.fdopen(os.open(self._pass_file,
Line 381: os.O_WRONLY | os.O_CREAT, 0600), 'w')
Line 375: os.chmod(_V2V_DIR, 0o700)
Line 376: else:
Line 377: raise
Line 378:
Line 379: self._pass_file = os.path.join(_V2V_DIR, "%s.tmp" % self.id)
> You should not add variables outside of __init__. Either initialize this in
Done
Line 380: f = os.fdopen(os.open(self._pass_file,
Line 381: os.O_WRONLY | os.O_CREAT, 0600), 'w')
Line 382: with closing(f):
Line 383: f.write(self._password)
Line 379: self._pass_file = os.path.join(_V2V_DIR, "%s.tmp" % self.id)
Line 380: f = os.fdopen(os.open(self._pass_file,
Line 381: os.O_WRONLY | os.O_CREAT, 0600), 'w')
Line 382: with closing(f):
Line 383: f.write(self._password)
> Why do you need fdopen? You are writing tiny file, and do not need any buff
Done
Line 384:
Line 385: def _delete_passwd_file(self):
Line 386: try:
Line 387: os.remove(self._pass_file)
Line 394: self._status = STATUS.ABORT
Line 395: logging.info('aborting job id: %r', self.id)
Line 396: if self._proc.returncode is None:
Line 397: ret, out, err = execCmd([EXT_KILL, '-9', self._proc.pid],
Line 398: sudo=True)
> There is a another success mode that must be checked - if the process did e
Done
Line 399: if len(err) > 0:
Line 400: self._status_msg = 'Error kill %s: %s' % (self._proc.pid, err)
Line 401: logging.error(self._status_msg)
Line 402:
Line 394: self._status = STATUS.ABORT
Line 395: logging.info('aborting job id: %r', self.id)
Line 396: if self._proc.returncode is None:
Line 397: ret, out, err = execCmd([EXT_KILL, '-9', self._proc.pid],
Line 398: sudo=True)
> virt-v2v runs as root? If we are the parent process, it runs as vdsm, and w
virt-v2v run as vdsm
Line 399: if len(err) > 0:
Line 400: self._status_msg = 'Error kill %s: %s' % (self._proc.pid, err)
Line 401: logging.error(self._status_msg)
Line 402:
Line 402:
Line 403: def _abort_msg(self, msg):
Line 404: self._status_msg = msg
Line 405: logging.error(msg)
Line 406: self._aborted = True
> Not needed if you let errors bubble up to _run and handle them there.
Done
Line 407:
Line 408: def _generate_disk_parameters(self):
Line 409: images = []
Line 410: for disk in self._vmProperties['disks']:
Line 430: if res['status']['code']:
Line 431: self._status = STATUS.ERROR
Line 432: self._status_msg = 'Bad volume specification: %s' % drive
Line 433: raise VolumeError(drive)
Line 434: else:
> Why else? if prepareImage failed, we just raised, and we never reach this p
Done
Line 435: self._preparedImages.extend([drive])
Line 436: return self._extract_storage_domain_path(res['path'])
Line 437:
Line 438: def _extract_storage_domain_path(self, path):
Line 431: self._status = STATUS.ERROR
Line 432: self._status_msg = 'Bad volume specification: %s' % drive
Line 433: raise VolumeError(drive)
Line 434: else:
Line 435: self._preparedImages.extend([drive])
> Use self._preparedImages.append(drive)
Done
Line 436: return self._extract_storage_domain_path(res['path'])
Line 437:
Line 438: def _extract_storage_domain_path(self, path):
Line 439: ''' prepareImage returns /prefix/sdUUID/images/imgUUID/volUUID
Line 432: self._status_msg = 'Bad volume specification: %s' % drive
Line 433: raise VolumeError(drive)
Line 434: else:
Line 435: self._preparedImages.extend([drive])
Line 436: return self._extract_storage_domain_path(res['path'])
> Don't use res, use self._preparedImages[0]['path']
Done
Line 437:
Line 438: def _extract_storage_domain_path(self, path):
Line 439: ''' prepareImage returns /prefix/sdUUID/images/imgUUID/volUUID
Line 440: we need storage domain absolute path so we go up 3 levels '''
Line 437:
Line 438: def _extract_storage_domain_path(self, path):
Line 439: ''' prepareImage returns /prefix/sdUUID/images/imgUUID/volUUID
Line 440: we need storage domain absolute path so we go up 3 levels '''
Line 441: path = os.path.split(path)
> split returns a tuple:
virt-v2v need that for the output base directory.
I need to get the storage domain path so we are going up 3 levels (danken suggestion).
I use to split '/images/' and take the left portion.
Line 442: path = os.path.split(path)
Line 443: return os.path.split(path)
Line 444:
Line 445: def _teardownVolumes(self):
Line 451: except Exception as e:
Line 452: logging.error('Error teardownVolumePath: %s', e)
Line 453:
Line 454:
Line 455: class OutputParser():
> This is old style class - use OutputParser(object)
Done
Line 456: def __init__(self, importVm):
Line 457: self._importVm = importVm
Line 458: self._copy_disk_re = re.compile(r'.*(Copying disk (\d+)/(\d+)).*')
Line 459: self._disk_progress_re = re.compile(r'\s+\((\d+).*')
Line 453:
Line 454:
Line 455: class OutputParser():
Line 456: def __init__(self, importVm):
Line 457: self._importVm = importVm
> Lets remove this - keeping an object and calling it is not needed in this c
leaving that until comment in line 344 will be solved
Line 458: self._copy_disk_re = re.compile(r'.*(Copying disk (\d+)/(\d+)).*')
Line 459: self._disk_progress_re = re.compile(r'\s+\((\d+).*')
Line 460:
Line 461: def parse_line(self, line):
Line 462: if 'Copying disk' in line:
Line 463: m = self._copy_disk_re.match(line)
Line 464: if m is None:
Line 465: raise JobOutputParsing()
Line 466: else:
> else is unneeded
Done
Line 467: self._importVm.disk_copy_stage(m.groups()[0],
Line 468: m.groups()[1],
Line 469: m.groups()[2])
Line 470:
Line 471: def parse_progress(self, buf):
Line 472: m = self._disk_progress_re.match(buf)
Line 473: if m is None:
Line 474: raise JobOutputParsing()
Line 475: else:
> else is unneeded
Done
Line 476: self._importVm.progress_stage(m.groups()[0])
Line 477:
Line 478:
Line 479: def _mem_to_mib(size, unit):
Line 472: m = self._disk_progress_re.match(buf)
Line 473: if m is None:
Line 474: raise JobOutputParsing()
Line 475: else:
Line 476: self._importVm.progress_stage(m.groups()[0])
> Return the progress instead:
Done
Line 477:
Line 478:
Line 479: def _mem_to_mib(size, unit):
Line 480: lunit = unit.lower()
--
To view, visit https://gerrit.ovirt.org/37509
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I34bd86d5a87ea8c42113c4a732f87ddd4ceab9ea
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list