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