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

nsoffer at redhat.com nsoffer at redhat.com
Wed Apr 1 17:50:21 UTC 2015


Nir Soffer has posted comments on this change.

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


Patch Set 16:

(18 comments)

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

Line 74:     ''' Base class for virt-v2v errors '''
Line 75: 
Line 76: 
Line 77: class ClientError(Exception):
Line 78:     ''' Base class for job related error '''
I commented in the previous version:

    Base class for job related error -> Base class for client errors
Line 79: 
Line 80: 
Line 81: class InvalidVMConfigError(ValueError):
Line 82:     ''' Unexpected error while parsing libvirt domain xml '''


Line 154:         items = tuple(_jobs.items())
Line 155:     for job_id, job in items:
Line 156:         ret[job_id] = {
Line 157:             'status': job.status,
Line 158:             'status_msg': job.status_msg,
See my comments in the previous version
Line 159:             'disk_progress': job.disk_progress,
Line 160:             'progress': job.progress
Line 161:         }
Line 162:     return ret


Line 155:     for job_id, job in items:
Line 156:         ret[job_id] = {
Line 157:             'status': job.status,
Line 158:             'status_msg': job.status_msg,
Line 159:             'disk_progress': job.disk_progress,
Why do we need this? See my comment in the previous version.
Line 160:             'progress': job.progress
Line 161:         }
Line 162:     return ret
Line 163: 


Line 215:         return self._status_msg
Line 216: 
Line 217:     @property
Line 218:     def disk_progress(self):
Line 219:         return self._disk_progress
Why do we need this? the caller does not have enough context to deal with this raw value (no disk_count and current disk).

This should be kept private.
Line 220: 
Line 221:     @property
Line 222:     def progress(self):
Line 223:         '''


Line 237:             except Exception as ex:
Line 238:                 if not self._aborted:
Line 239:                     self._status = STATUS.FAILED
Line 240:                     self._status_msg = ex.message
Line 241:                     self._abort()
If _abort fails, we will fail to raise the original error, which will make it very hard to debug.

We can handle possible exceptions raised from _abort:

    try:
        self._abort()
    except Exception:
        logging.exception(description...)
    raise

But "raise" will raise the exception we just handled, instead of the original
one (ex).

The way to achieve this is:

    t, v, tb = sys.exc_info()
    try:
        self._abort()
    except Exception:
        logging.exception(description...)
    raise t, v, tb

See http://pastebin.com/eW1eVN37 if you want to try it yourself.
Line 242:                     raise
Line 243:             finally:
Line 244:                 self._teardown_volumes()
Line 245: 


Line 246:     def _import(self):
Line 247:         # TODO: use the process handling http://gerrit.ovirt.org/#/c/33909/
Line 248:         self._prepare_volumes()
Line 249:         cmd = self._create_command()
Line 250:         logging.info('Import vm, (job_id %s) started, cmd: %s', self._id, cmd)
cmd is logged in execCmd - no point to log it twice. I think the log id enough.

Also, lets keep the same log format you use elsewhere:

    "Job %r starting import", self._id
Line 251: 
Line 252:         self._proc = execCmd(cmd, sync=False, deathSignal=15,
Line 253:                              env={'LIBGUESTFS_BACKEND': 'direct'})
Line 254: 


Line 258: 
Line 259:         if self._proc.returncode != 0:
Line 260:             raise V2VProcessError('Process failed exit-code: %r, stderr: %s' %
Line 261:                                   (self._proc.returncode,
Line 262:                                    self._proc.stderr.read(1024)))
We need to add the job id to all errors raised by this class, so it is easy to follow the logs of a single job.

Example: V2VProcessError in line 271.
Line 263:         logging.info('Importing VM finish, JobId:', self._id)
Line 264:         self._status = STATUS.DONE
Line 265: 
Line 266:     def _wait_for_process(self):


Line 259:         if self._proc.returncode != 0:
Line 260:             raise V2VProcessError('Process failed exit-code: %r, stderr: %s' %
Line 261:                                   (self._proc.returncode,
Line 262:                                    self._proc.stderr.read(1024)))
Line 263:         logging.info('Importing VM finish, JobId:', self._id)
Lets keep the same log format you use elsewhere:

    "Job %r finished import successfully", self._id
Line 264:         self._status = STATUS.DONE
Line 265: 
Line 266:     def _wait_for_process(self):
Line 267:         if self._proc.returncode is not None:


Line 287:                 if event.progress % 10 == 0:
Line 288:                     logging.info("Job %r copy disk %d progress %d/100",
Line 289:                                  self._id, self._current_disk, event.progress)
Line 290:             else:
Line 291:                 raise RuntimeError(event)
Please add more context:

    raise RuntimeError("Job %r got unexpected parser event: %s" % (self._id, event))
Line 292: 
Line 293:     def _create_command(self):
Line 294:         cmd = [_VIRT_V2V.path, '-ic', self._uri, '-o', 'vdsm', '-of', 'raw']
Line 295:         cmd.extend(self._generate_disk_parameters())


Line 317:         finally:
Line 318:             try:
Line 319:                 os.remove(filename)
Line 320:             except Exception:
Line 321:                 logging.exception("Error removing passwd file: %s", filename)
This is now a generic utility, and should not be part of ImportVM, making this class smaller and simpler, and allowing using this elsewhere later. Move it to a module function and remove the unneeded self argument.
Line 322: 
Line 323:     def abort(self):
Line 324:         self._status = STATUS.ABORTED
Line 325:         logging.info('aborting job id: %r', self._id)


Line 327: 
Line 328:     def _abort(self):
Line 329:         self._aborted = True
Line 330:         if self._proc.returncode is None:
Line 331:             if self._proc.terminate_process(self.TERM_DELAY):
Nice - but _proc alreay have terminate() and kill(), and this name does not mix well with them.

Also, the termination flow is higher level flow using the basic operations provided by Popen, so this should be an utility function getting a process, instead of a method of _proc object.

To keep this patch simpler and allow quicker merge, lets just use self._proc.kill() here, and in another patch work on a generic utility for terminating processes. When this utility will be ready, we can use it here.
Line 332:                 logging.debug("virt-v2v terminated with exit code: %d",
Line 333:                               self._proc.returncode)
Line 334:             else:
Line 335:                 logging.error("Error killing virt-v2v (pid: %d)",


Line 329:         self._aborted = True
Line 330:         if self._proc.returncode is None:
Line 331:             if self._proc.terminate_process(self.TERM_DELAY):
Line 332:                 logging.debug("virt-v2v terminated with exit code: %d",
Line 333:                               self._proc.returncode)
Add job id to log message
Line 334:             else:
Line 335:                 logging.error("Error killing virt-v2v (pid: %d)",
Line 336:                               self._proc.pid)
Line 337:                 zombiereaper.autoReapPID(self._proc.pid)


Line 332:                 logging.debug("virt-v2v terminated with exit code: %d",
Line 333:                               self._proc.returncode)
Line 334:             else:
Line 335:                 logging.error("Error killing virt-v2v (pid: %d)",
Line 336:                               self._proc.pid)
Add job id to log message
Line 337:                 zombiereaper.autoReapPID(self._proc.pid)
Line 338: 
Line 339:     def _generate_disk_parameters(self):
Line 340:         parameters = []


Line 344:                 parameters.append(disk['imageID'])
Line 345:                 parameters.append('--vdsm-vol-uuid')
Line 346:                 parameters.append(disk['volumeID'])
Line 347:             except KeyError as e:
Line 348:                 raise InvalidInputError('Missing required property: %s' % e)
Add job id to error message
Line 349:         return parameters
Line 350: 
Line 351:     def _prepare_volumes(self):
Line 352:         if len(self._vm_properties['disks'] < 1):


Line 349:         return parameters
Line 350: 
Line 351:     def _prepare_volumes(self):
Line 352:         if len(self._vm_properties['disks'] < 1):
Line 353:                 raise InvalidInputError('Cannot import vm with no disk')
Add job id to error message
Line 354: 
Line 355:         for disk in self._vm_properties['disks']:
Line 356:             drive = {'poolID': self._vm_properties['poolID'],
Line 357:                      'domainID': self._vm_properties['domainID'],


Line 361:                                          drive['poolID'],
Line 362:                                          drive['imageID'],
Line 363:                                          drive['volumeID'])
Line 364:             if res['status']['code']:
Line 365:                 raise VolumeError('Bad volume specification: %s' % drive)
Add job id to error message
Line 366: 
Line 367:             drive['path'] = res['path']
Line 368:             self._prepared_volumes.append([drive])
Line 369: 


Line 373:                 self._irs.teardownImage(drive['domainID'],
Line 374:                                         drive['poolID'],
Line 375:                                         drive['imageID'])
Line 376:             except Exception as e:
Line 377:                 logging.error('Error tearing down drive: %s', e)
Add job id to error message
Line 378: 
Line 379:     def _get_storage_domain_path(self):
Line 380:         '''
Line 381:         since all images are in the same domain we return arbitrary image path.


Line 440:         return size * 1024
Line 441:     elif lunit in ('tib', 't'):
Line 442:         return size * 1024 * 1024
Line 443:     else:
Line 444:         raise InvalidVMConfigError("Invalid currentMemory unit attribute:"
Please separate these renames - thye are not related to this feature.

If you like to add Error suffix, add another patch before this renaming the old errors.
Line 445:                                    " %r" % unit)
Line 446: 
Line 447: 
Line 448: def _add_vm_info(vm, params):


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