Change in vdsm[master]: v2v: Convert VM from external source to Data Domain

shavivi at redhat.com shavivi at redhat.com
Mon Mar 30 08:19:40 UTC 2015


Shahar Havivi has posted comments on this change.

Change subject: v2v: Convert VM from external source to Data Domain
......................................................................


Patch Set 14:

(22 comments)

https://gerrit.ovirt.org/#/c/37509/14/vdsm/v2v.py
File vdsm/v2v.py:

Line 78: class InvalidVMConfiguration(ValueError):
Line 79:     ''' Unexpected error while parsing libvirt domain xml '''
Line 80: 
Line 81: 
Line 82: class JobOutputParsing(ValueError):
> Lets rename to OutputParserError
Done
Line 83:     ''' Error while parsing virt-v2v output '''
Line 84: 
Line 85: 
Line 86: class V2VJobError(ValueError):


Line 88: 
Line 89: 
Line 90: class NoSuchJob(V2VJobError):
Line 91:     ''' Job not exists in _jobs collection '''
Line 92:     err_name = 'invalidV2VJob'
> Lets be consistent - V2VNoSuchJob
Done
Line 93: 
Line 94: 
Line 95: class JobExists(V2VJobError):
Line 96:     ''' Job already exists in _jobs collection '''


Line 93: 
Line 94: 
Line 95: class JobExists(V2VJobError):
Line 96:     ''' Job already exists in _jobs collection '''
Line 97:     err_name = 'v2vJobExists'
> Lets be consistent - V2VJobExists
Done
Line 98: 
Line 99: 
Line 100: class JobNotDone(V2VJobError):
Line 101:     ''' Import process still in progress '''


Line 107:     err_name = 'V2VNoSuchOvf'
Line 108: 
Line 109: 
Line 110: class VolumeError(RuntimeError):
Line 111:     ''' Bad volume specification '''
> A better description would be:
I will change the comment,
The reason that not all exception classes have error code is not all interact with the engine - in case of the ValumeError its run inside of a thread and the feedback to the engine is via status and status_msg and not via the verb return value.
Line 112: 
Line 113: 
Line 114: class V2VProcessError(ValueError):
Line 115:     ''' virt-v2v process had error in execution '''


Line 119:     ''' Error parsing virt-v2v progress output '''
Line 120: 
Line 121: 
Line 122: class InvalidParsingEvent(ValueError):
Line 123:     ''' Unknown parsing event caught '''
> Can we use one error for output parsing? we have:
JobProgressError is not need (jobOutputParsing is replacing it)
InvalidParsingEvent is a little different its for the yield events (not suppose happened)
Line 124: 
Line 125: 
Line 126: class InvalidInput(ValueError):
Line 127:     ''' Invalid input received '''


Line 123:     ''' Unknown parsing event caught '''
Line 124: 
Line 125: 
Line 126: class InvalidInput(ValueError):
Line 127:     ''' Invalid input received '''
> This certainly need to be a public error - it mean that engine passed inval
What do you mean?
Again it is run in a thread context - maybe its best to verify before...?
Line 128: 
Line 129: 
Line 130: def supported():
Line 131:     return not (caps.getos() in (caps.OSName.RHEVH, caps.OSName.RHEL)


Line 177:     try:
Line 178:         job = _get_job(job_id)
Line 179:         _validate_job_done(job)
Line 180:         ovf = _read_ovf(job_id)
Line 181:     except V2VJobError as e:
> This does not handle all the errors inheriting from ValueError. 
Ok,
I will name it V2VError
Line 182:         logging.warning('Converted VM error %s', e)
Line 183:         return errCode[e.err_name]
Line 184:     return {'status': doneCode, 'ovf': ovf}
Line 185: 


Line 325:                 self._status_msg = ex.message
Line 326:                 self._abort()
Line 327:                 raise
Line 328:         finally:
Line 329:             self._delete_passwd_file()
> If this raises OSError that we don't handle, prepared voluems will not be t
Done
Line 330:             self._teardown_volumes()
Line 331: 
Line 332:     def _import(self):
Line 333:         # TODO: use the process handling http://gerrit.ovirt.org/#/c/33909/


Line 329:             self._delete_passwd_file()
Line 330:             self._teardown_volumes()
Line 331: 
Line 332:     def _import(self):
Line 333:         # TODO: use the process handling http://gerrit.ovirt.org/#/c/33909/
> This is a good place to prepare the volumes.
Done
Line 334:         cmd = self._create_command()
Line 335:         logging.info('Import vm, (job_id %s) started, cmd: %s', self._id, cmd)
Line 336: 
Line 337:         self._proc = execCmd(cmd, sync=False, deathSignal=15,


Line 338:                              env={'LIBGUESTFS_BACKEND': 'direct'})
Line 339: 
Line 340:         self._proc.blocking = True
Line 341:         self._watch_process_output()
Line 342: 
> You should wait for the process before checking the return code:
at this time the process is done.
When the last copy is getting 100% it will write the ovf file and exit.
we can wait for 30 seconds and see if we will encounter any problem
Line 343:         if self._proc.returncode != 0:
Line 344:             raise V2VProcessError('Process failed exit-code: %r, stderr: %s' %
Line 345:                                   (self._proc.returncode,
Line 346:                                    self._proc.stderr.read(1024)))


Line 339: 
Line 340:         self._proc.blocking = True
Line 341:         self._watch_process_output()
Line 342: 
Line 343:         if self._proc.returncode != 0:
> returncode may be None if the process did not exit yet. This is unlikely bu
we added a wait,
what are you suggesting to wait more?
we can raise different error/feedback in that case (maybe its stuck on IO/virt-v2v bug)
Line 344:             raise V2VProcessError('Process failed exit-code: %r, stderr: %s' %
Line 345:                                   (self._proc.returncode,
Line 346:                                    self._proc.stderr.read(1024)))
Line 347:         logging.info('Importing VM finish, JobId:', self._id)


Line 351:         parser = OutputParser()
Line 352:         for event in parser.parse(self._proc.stdout):
Line 353:             if isinstance(event, ImportProgress):
Line 354:                 self._status = STATUS.COPYING_DISK
Line 355:                 logging.info('Copying disk, JobId: %s', self._id)
> We like to see the disk number/total
Done
Line 356:                 self._disk_progress = 0
Line 357:                 self._current_disk = event.current_disk
Line 358:                 self._disk_count = event.disk_count
Line 359:                 self._status_msg = event.description


Line 361:                 self._disk_progress = event.progress
Line 362:                 if event.progress % 10 == 0:
Line 363:                     logging.debug('Copying disk progress %n, JobId: %s',
Line 364:                                   event.progress,
Line 365:                                   self._id)
> Lets use same format:
Done
Line 366:             else:
Line 367:                 raise InvalidParsingEvent(event)
Line 368: 
Line 369:     def _create_command(self):


Line 376:                     '--vdsm-ovf-output',
Line 377:                     _V2V_DIR,
Line 378:                     '--machine-readable',
Line 379:                     '-os',
Line 380:                     self._prepare_volumes(),
> You are hiding major step - preparing volumes - in the create command boile
Done
Line 381:                     self._vm_properties['vmName']])
Line 382:         return cmd
Line 383: 
Line 384:     def _create_v2v_dir(self):


Line 460:                                              drive['imageID'],
Line 461:                                              drive['volumeID'])
Line 462:             if res['status']['code']:
Line 463:                 raise VolumeError('Bad volume specification: %s' % drive)
Line 464:             self._prepared_volumes.append([drive])
> What if there are no disks in _vm_properties?
Yes its best to raise an error
Line 465: 
Line 466:         return self._extract_storage_path(self._prepared_volumes[0]['path'])
Line 467: 
Line 468:     def _extract_storage_path(self, path):


Line 462:             if res['status']['code']:
Line 463:                 raise VolumeError('Bad volume specification: %s' % drive)
Line 464:             self._prepared_volumes.append([drive])
Line 465: 
Line 466:         return self._extract_storage_path(self._prepared_volumes[0]['path'])
> This is not related to this method. Lets use it in the place that need that
Done
Line 467: 
Line 468:     def _extract_storage_path(self, path):
Line 469:         '''
Line 470:         prepareImage returns /prefix/sdUUID/images/imgUUID/volUUID


Line 464:             self._prepared_volumes.append([drive])
Line 465: 
Line 466:         return self._extract_storage_path(self._prepared_volumes[0]['path'])
Line 467: 
Line 468:     def _extract_storage_path(self, path):
> You can also move it out of the class and make it a module function, accept
Done
Line 469:         '''
Line 470:         prepareImage returns /prefix/sdUUID/images/imgUUID/volUUID
Line 471:         we need storage domain absolute path so we go up 3 levels
Line 472:         '''


Line 464:             self._prepared_volumes.append([drive])
Line 465: 
Line 466:         return self._extract_storage_path(self._prepared_volumes[0]['path'])
Line 467: 
Line 468:     def _extract_storage_path(self, path):
> Rename to _get_storage_domain_path() and access self._prepared_volumes[0] i
Done
Line 469:         '''
Line 470:         prepareImage returns /prefix/sdUUID/images/imgUUID/volUUID
Line 471:         we need storage domain absolute path so we go up 3 levels
Line 472:         '''


Line 471:         we need storage domain absolute path so we go up 3 levels
Line 472:         '''
Line 473:         path = os.path.split(path)
Line 474:         path = os.path.split(path[0])
Line 475:         return os.path.split(path[0])[0]
> For some reason you stick to using os.path.split - we can use it, but you a
it is not,
my initial design was a simple split but Dan insist on using dirname.
this code portion is not yet verify this is why there are runtime bugs - I cannot verify with the engine yet but checking with hardcoded command to virt-v2v which is skip the prepare-volume.
I will go with your approach since its simpler
Line 476: 
Line 477:     def _teardown_volumes(self):
Line 478:         for drive in self._prepared_volumes:
Line 479:             try:


Line 487: class OutputParser(object):
Line 488:     COPY_DISK_RE = re.compile(r'.*(Copying disk (\d+)/(\d+)).*')
Line 489:     DISK_PROGRESS_RE = re.compile(r'\s+\((\d+).*')
Line 490: 
Line 491:     def parse(self, stream):
> it would be simpler to review the code if you introduce this parsing functi
I will separate to few patches:
1. convert
2. parser
3. job related (delete/abort)
Line 492:         for line in stream:
Line 493:             if 'Copying disk' in line:
Line 494:                 description, current_disk, disk_count = self._parse_line(line)
Line 495:                 yield ImportProgress(int(current_disk), int(disk_count),


Line 500:     def _parse_line(self, line):
Line 501:         m = self.COPY_DISK_RE.match(line)
Line 502:         if m is None:
Line 503:             raise JobOutputParsing('unexpected format in "Copying disk"'
Line 504:                                    ', line: %s' % line)
> Use %r - we want to see the exact content of line
Done
Line 505:         return m.group(1), m.group(2), m.group(3)
Line 506: 
Line 507:     def _parse_progress(self, stream):
Line 508:         buf = ''


Line 513:             if c == '\r':
Line 514:                 m = self.DISK_PROGRESS_RE.match(buf)
Line 515:                 if m is None:
Line 516:                     raise JobOutputParsing('error parsing progress, buf: %s'
Line 517:                                            % buf)
> Use %r - we want to see the exact content of buf
Done
Line 518:                 buf = ''
Line 519:                 try:
Line 520:                     progress = int(m.group(1))
Line 521:                 except ValueError:


-- 
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini 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