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