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

nsoffer at redhat.com nsoffer at redhat.com
Mon Mar 30 16:26:10 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 15:

(12 comments)

https://gerrit.ovirt.org/#/c/37509/15/tests/v2vTests.py
File tests/v2vTests.py:

Line 86:             return [0, 0, 0]
Line 87: 
Line 88: 
Line 89: def hypervisorConnect(uri, username, passwd):
Line 90:     return LibvirtMock()
These tests existed before this patch, and should not be deleted and added back in this patch.
Line 91: 
Line 92: 
Line 93: class v2vTests(TestCaseBase):
Line 94:     @MonkeyPatch(libvirtconnection, 'open_connection', hypervisorConnect)


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

Line 39
Line 40
Line 41
Line 42
Line 43
Isn't this job related error?


Line 89:     err_name = 'V2VJobExists'
Line 90: 
Line 91: 
Line 92: class VolumeError(RuntimeError):
Line 93:     ''' Error preparing volume '''
Isn't this job related error?
Line 94: 
Line 95: 
Line 96: class V2VProcessError(ValueError):
Line 97:     ''' virt-v2v process had error in execution '''


Line 93:     ''' Error preparing volume '''
Line 94: 
Line 95: 
Line 96: class V2VProcessError(ValueError):
Line 97:     ''' virt-v2v process had error in execution '''
Isn't this job related error?
Line 98: 
Line 99: 
Line 100: class InvalidParsingEvent(ValueError):
Line 101:     ''' Unknown parsing event caught '''


Line 97:     ''' virt-v2v process had error in execution '''
Line 98: 
Line 99: 
Line 100: class InvalidParsingEvent(ValueError):
Line 101:     ''' Unknown parsing event caught '''
Isn't this job related error?
Line 102: 
Line 103: 
Line 104: class InvalidInput(ValueError):
Line 105:     ''' Invalid input received '''


Line 101:     ''' Unknown parsing event caught '''
Line 102: 
Line 103: 
Line 104: class InvalidInput(ValueError):
Line 105:     ''' Invalid input received '''
Isn't this job related error?
Line 106: 
Line 107: 
Line 108: def supported():
Line 109:     return not (caps.getos() in (caps.OSName.RHEVH, caps.OSName.RHEL)


Line 167: def _add_job(job_id, job):
Line 168:     with _lock:
Line 169:         if job_id in _jobs:
Line 170:             raise JobExists("job %r exists" % job_id)
Line 171:         _jobs[job_id] = job
This looks like part of the job management patch.

Adding here only the ImportVM class without the entry point to run it, make sure this patch is safe to merge without the next one, you have no way to run the import job :-)
Line 172: 
Line 173: 
Line 174: def _extract_storage_path(path):
Line 175:     '''


Line 183:     ABORT_RETRIES = 3
Line 184:     ABORT_DELAY = 10
Line 185:     PROC_WAIT_TIMEOUT = 30
Line 186: 
Line 187:     def __init__(self, uri, username, password, vm_properties, job_id, cif):
We don't need cif here, we need its irs for interacting with storage. Lets accept only irs, so we don't depend on cif, which has nothing to do with this code.
Line 188:         self._uri = uri
Line 189:         self._username = username
Line 190:         self._password = password
Line 191:         self._vm_properties = vm_properties


Line 195:         self._disk_progress = 0
Line 196:         self._disk_count = 1
Line 197:         self._current_disk = 1
Line 198:         self._aborted = False
Line 199:         self._status_msg = ''
Please move bellow self._status
Line 200:         self._prepared_volumes = []
Line 201:         self._pass_file = os.path.join(_V2V_DIR, "%s.tmp" % job_id)
Line 202: 
Line 203:     def start(self):


Line 197:         self._current_disk = 1
Line 198:         self._aborted = False
Line 199:         self._status_msg = ''
Line 200:         self._prepared_volumes = []
Line 201:         self._pass_file = os.path.join(_V2V_DIR, "%s.tmp" % job_id)
You call this passwd_file (_create_passwd_file) - please use same name everywhere.
Line 202: 
Line 203:     def start(self):
Line 204:         self._thread = threading.Thread(target=self._run)
Line 205:         self._thread.daemon = True


Line 247:             try:
Line 248:                 self._delete_passwd_file()
Line 249:             except Exception:
Line 250:                 logging.exception("Error deleting passwd file: %r",
Line 251:                                   self._passwd_file)
This is called pass_file elsewhere
Line 252:             self._teardown_volumes()
Line 253: 
Line 254:     def _import(self):
Line 255:         # TODO: use the process handling http://gerrit.ovirt.org/#/c/33909/


Line 267:             logging.debug("Job %r waiting for virt-v2v process", self._id)
Line 268:             self._proc.wait(timeout=self.PROC_WAIT_TIMEOUT)
Line 269:         if self._proc.returncode is None:
Line 270:             logging.error("Job %r is not done trying to abort", self._id)
Line 271:             self._abort()
_proc.wait() returns True when process exit, and False on timeout, so we don't need to check returncode again.

If you raise on timeout,  logging an error and _abort is not needed since you log errors and _abort in _run.

Lets move the wait logic to helper to keep the flow clear:

    def _wait_for_process(self):
        if self._proc.returncode is not None:
            return

        logging.debug("Job %r waiting for virt-v2v process", self._id)

        if not self._proc.wait(timeout=self.PROC_WAIT_TIMEOUT):
            raise V2VProcessError("Job %r timeout waiting for process pid=%s",
                self._id, self._proc.pid)
Line 272: 
Line 273:         if self._proc.returncode != 0:
Line 274:             raise V2VProcessError('Process failed exit-code: %r, stderr: %s' %
Line 275:                                   (self._proc.returncode,


-- 
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: 15
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