Change in vdsm[master]: v2v: Convert VM from external source to Data Domain
shavivi at redhat.com
shavivi at redhat.com
Wed Apr 1 14:24:48 UTC 2015
Shahar Havivi has posted comments on this change.
Change subject: v2v: Convert VM from external source to Data Domain
......................................................................
Patch Set 15:
(20 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
Done
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 83: class V2VError(Exception):
Line 84: ''' Base class for job related error '''
Line 85:
Line 86:
Line 87: class JobExists(V2VError):
> conventionally, exception names should end with Error.
Done
Line 88: ''' Job already exists in _jobs collection '''
Line 89: err_name = 'V2VJobExists'
Line 90:
Line 91:
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?
yes can be clientError
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?
no this is V2vError
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?
No sure about that since this error can be made in vdsm code only
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?
Yes ClientError
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.
yes this is a job related but we needed it since we want to add to the jobs collection.
The "jobs patch" is misleading its more like abort/delete and get-ovf verbs
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
Done
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
Done
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 ever
Done
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
I stayed with passwd_file all over
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 248: self._delete_passwd_file()
Line 249: except Exception:
Line 250: logging.exception("Error deleting passwd file: %r",
Line 251: self._passwd_file)
Line 252: self._teardown_volumes()
> Using context manager (see bellow), we can make this nicer:
Done
Line 253:
Line 254: def _import(self):
Line 255: # TODO: use the process handling http://gerrit.ovirt.org/#/c/33909/
Line 256: self._prepare_volumes()
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 do
Done
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,
Line 293: if event.progress % 10 == 0:
Line 294: logging.info("Job %r copy disk %d progress %d/100",
Line 295: self._id, self._current_disk, event.progress)
Line 296: else:
Line 297: raise InvalidParsingEvent(event)
> This error is impossible - we don't need to invent a new exception for it.
Done
Line 298:
Line 299: def _create_command(self):
Line 300: cmd = [_VIRT_V2V.path, '-ic', self._uri, '-o', 'vdsm', '-of', 'raw']
Line 301: cmd.extend(self._generate_disk_parameters())
Line 310: self._get_storage_domain_path(),
Line 311: self._vm_properties['vmName']])
Line 312: return cmd
Line 313:
Line 314: def _create_v2v_dir(self):
> This should be done once when starting vdsm and should not be re-created au
Done
Line 315: try:
Line 316: os.mkdir(_V2V_DIR, 0o700)
Line 317: except OSError as e:
Line 318: if e.errno != errno.EEXIST:
Line 319: raise
Line 320: os.chmod(_V2V_DIR, 0o700)
Line 321:
Line 322: def _create_passwd_file(self):
Line 323: self._create_v2v_dir()
> creating the v2v dir should not be a side effect of creating a password, fi
Done
Line 324:
Line 325: fd = os.open(self._pass_file, os.O_WRONLY | os.O_CREAT, 0600)
Line 326: try:
Line 327: os.write(fd, self._password)
Line 332: try:
Line 333: os.remove(self._pass_file)
Line 334: except OSError as e:
Line 335: if e.errno != errno.ENOENT:
Line 336: raise
> We can replace _create_passwd_file and _delete_passwd_file with:
Done
Line 337:
Line 338: def abort(self):
Line 339: self._status = STATUS.ABORTED
Line 340: logging.info('aborting job id: %r', self._id)
Line 350: return
Line 351: logging.debug("Trying to kill virt-v2v (attempt %d/%d)",
Line 352: i + 1, self.ABORT_RETRIES)
Line 353: try:
Line 354: self._proc.kill()
> I agree, we should use the standard procedure here:
Done
Line 355: except OSError as e:
Line 356: if e.errno == errno.ESRCH:
Line 357: logging.debug("virt-v2v terminated with exit code: %d",
Line 358: self._proc.returncode)
Line 358: self._proc.returncode)
Line 359: return
Line 360: raise
Line 361: time.sleep(self.ABORT_DELAY)
Line 362: logging.error("Error killing virt-v2v (pid: %d)", self._proc.pid)
> if we happen to kill it with SIGKILL we need to remove the temporary files
We are doing that...
with try/finally we delete the password file.
we need to add the cleanup on startup as well
Line 363: zombiereaper.autoReapPID(self._proc.pid)
Line 364:
Line 365: def _generate_disk_parameters(self):
Line 366: parameters = []
Line 375: return parameters
Line 376:
Line 377: def _prepare_volumes(self):
Line 378: if len(self._vm_properties['disks'] < 1):
Line 379: raise InvalidInput('Missing required disks property')
> You mean: "Cannot import vm with no disk"
Done
Line 380:
Line 381: for disk in self._vm_properties['disks']:
Line 382: drive = {'poolID': self._vm_properties['poolID'],
Line 383: 'domainID': self._vm_properties['domainID'],
--
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