Change in vdsm[master]: v2v: Convert VM from external source to Data Domain
shavivi at redhat.com
shavivi at redhat.com
Wed Mar 25 14:54:02 UTC 2015
Shahar Havivi has posted comments on this change.
Change subject: v2v: Convert VM from external source to Data Domain
......................................................................
Patch Set 11:
(14 comments)
https://gerrit.ovirt.org/#/c/37509/11/tests/v2vTests.py
File tests/v2vTests.py:
Line 153: 13
> Please add more progress data, this does not test the progress parsing code
this is not the progress check,
the parse_progress in line 164 is checking the progress.
I will add another disk for the test
Line 155: 256
> What is 256.0 - time from start of the operation?
seconds that pass since the start
Line 163:
Line 164: parser.parse_progress(' (13.16/100%)')
Line 165: self.assertEquals(importVm.progress, 13)
Line 166: parser.parse_progress(' (100/100%)')
Line 167: self.assertEquals(importVm.progress, 100)
> We should convert the parser to generator, accepting lines from v2v process
Very nice code,
Done
https://gerrit.ovirt.org/#/c/37509/11/vdsm/v2v.py
File vdsm/v2v.py:
Line 91:
Line 92:
Line 93: class NoSuchOvf(V2VJobError):
Line 94: ''' Ovf path is not exists in /var/run/vdsm/v2v/ '''
Line 95: err_name = 'invalidV2VOvfPath'
> I think we should fix also the error name.
Done
Line 96:
Line 97:
Line 98: class VolumeError(RuntimeError):
Line 99: def __str__(self):
Line 260: self._preparedVolumes = []
Line 261: self._parser = OutputParser(self)
Line 262: self._pass_file = os.path.join(_V2V_DIR, "%s.tmp" % jobId)
Line 263: self.ABORT_RETRIES = 3
Line 264: self.ABORT_DELAY = 30
> Should be class variable, why create another instance variable for each job
Done
Line 265:
Line 266: def start(self):
Line 267: self._thread = threading.Thread(target=self._run)
Line 268: self._thread.daemon = True
Line 330:
Line 331: if self._proc.returncode != 0:
Line 332: self._status = STATUS.ERROR
Line 333: error = self._get_process_errors()
Line 334: self._status_msg = error
> You don't need to set self._status_msg as it is done in _run. This is the b
Done
Line 335: raise V2VProcessError(error)
Line 336: self._status = STATUS.DONE
Line 337:
Line 338: def disk_copy_stage(self, status_msg, current_disk, disk_count):
Line 331: if self._proc.returncode != 0:
Line 332: self._status = STATUS.ERROR
Line 333: error = self._get_process_errors()
Line 334: self._status_msg = error
Line 335: raise V2VProcessError(error)
> So this can simplified to:
Done
Line 336: self._status = STATUS.DONE
Line 337:
Line 338: def disk_copy_stage(self, status_msg, current_disk, disk_count):
Line 339: self._status = STATUS.DISK_COPY
Line 346: def progress_stage(self, progress):
Line 347: try:
Line 348: self._disk_progress = int(progress)
Line 349: except ValueError:
Line 350: self._abort()
> If this exception is bubbled up to _run - we can let _run do the abort.
Done
Line 351: raise JobProgressError("Error parsing progress: %r" % progress)
Line 352:
Line 353: def _watch_process_output(self):
Line 354: while not self._aborted and self._proc.returncode is None:
Line 371: def _get_process_errors(self):
Line 372: error = self._proc.stderr.readlines()
Line 373: if len(error) > 0:
Line 374: msg = 'error process ended with errors: %s' % error
Line 375: return msg
> This will return None if there are no errors, so we don't get any context i
I don't know the exact number too since the error can be from external source such as qemu-img,
I would go with 5120
Line 376:
Line 377: def _create_v2v_dir(self):
Line 378: try:
Line 379: os.mkdir(_V2V_DIR, 0o700)
Line 421: self._proc.returncode)
Line 422: return
Line 423: raise
Line 424: time.sleep(self.ABORT_DELAY)
Line 425: logging.error("Error killing virt-v2v (pid: %d)", self._proc.pid)
> If we failed, we should pass the pid to zombiereaper, so it would reap the
Done
Line 426:
Line 427: def _abort_msg(self, msg):
Line 428: self._status_msg = msg
Line 429: logging.error(msg)
Line 429: logging
> Why do we need this method now? self._status_msg is set in _run and the mes
Done
Line 462: ''' prepareImage returns /prefix/sdUUID/images/imgUUID/volUUID
Line 463: we need storage domain absolute path so we go up 3 levels '''
Line 464: path = os.path.split(path)
Line 465: path = os.path.split(path)
Line 466: return os.path.split(path)
> This is still broken - see my comment for version 10.
Done
Line 467:
Line 468: def _teardown_volumes(self):
Line 469: for drive in self._preparedVolumes:
Line 470: try:
Line 489: unexpected format in "Copying disk"
> Please include the unexpected line in the error message. Otherwise we will
Done
Line 497: JobOutputParsing
> Please include the unexpected buf in the error message. Otherwise we will h
Done
--
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: 11
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