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