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

fromani at redhat.com fromani at redhat.com
Mon Feb 16 16:34:35 UTC 2015


Francesco Romani has posted comments on this change.

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


Patch Set 4:

(13 comments)

a few more comments about ImportVmThread. Still processing it.

http://gerrit.ovirt.org/#/c/37509/4/vdsm/v2v.py
File vdsm/v2v.py:

Line 36: import caps
Line 37: 
Line 38: 
Line 39: _jobs = {}
Line 40: _lock = threading.Lock()
nit: it is usually preferred to put the lock first, then just after the guarded fields, then a blank space.

Example:

  lock = threading.Lock()
  something = {}
  something_else = []
  silly_field = 1

  more_fields = 3.14

so, better if you move _lock just above _jobs.
But this is minor, you can do if and only you have or want resubmit for other reasons
Line 41: _P_V2V_DIR = os.path.join(P_VDSM_RUN, 'v2v')
Line 42: 
Line 43: 
Line 44: class InvalidVMConfiguration(ValueError):


Line 110:                 kill_zombie_jobs()
Line 111:     return ovf
Line 112: 
Line 113: 
Line 114: def kill_zombie_jobs():
For the sake of easier review, I'm repeating and expanding the same comment of version 3.

I'm ok with the concept here (see commentS in version 3), there are two things that concerns me
- the first, major, is that I'm a bit worried by the pgrep/kill dance this function is doing. I need to think a bit more about that.
- the second, minor, is that kill_zombie_job*S*() suggests that there may be more than one stale job, while during the review I learned this isn't true. If AFAIU, then this may be misleading and can be easily fixed.
Line 115:     ret, out, err = execCmd([EXT_PGREP, 'virt-v2v'], sudo=True)
Line 116:     if len(err) > 0:
Line 117:         logging.error('error while trying to grep virt-v2v processes: %r', err)
Line 118:         return


Line 153:             }
Line 154:     return ret
Line 155: 
Line 156: 
Line 157: class ImportVmThread(threading.Thread):
I need to think about this as well, I haven't reviewed this class properly yet.
Line 158:     """
Line 159:     Importing VM to data domain
Line 160:     Running virt-v2v command
Line 161:     Parsing the stdout


Line 160:     Running virt-v2v command
Line 161:     Parsing the stdout
Line 162:     """
Line 163:     def __init__(self, uri, username, password, vmProperties, jobId, cif):
Line 164:         threading.Thread.__init__(self)
it is usually preferred to
* not inherit from threading.Thread, use composition instead, so
* have a private threading.Thread internally. There are other examples of this usage scatthered into the codebase.
Line 165:         self.setDaemon(True)
Line 166:         self._uri = uri
Line 167:         self._username = username
Line 168:         self._password = password


Line 168:         self._password = password
Line 169:         self._vmProperties = vmProperties
Line 170:         self._job_id = jobId
Line 171:         self._cif = cif
Line 172:         self._status = 'initializing'
it is preferred to group constants like this in a helper enum-like class, e.g. (naming will be ugly, is just for illustration)

  class V2VImport:
      INITIALIZING = 'initializing'
      STARTING = 'starting'
      ...
Line 173:         self._progress = 0
Line 174:         self._abort = False
Line 175: 
Line 176:     @property


Line 184:     @property
Line 185:     def progress(self):
Line 186:         return self._progress
Line 187: 
Line 188:     def run(self):
the utils.traceback decorator may be handy here.
Line 189:         try:
Line 190:             self._import_vm()
Line 191:         except Exception as ex:
Line 192:             logging.info('error in importing external vm: %r', ex)


Line 206:                                 self._vmProperties['vmName'])
Line 207: 
Line 208:         logging.info('import vm, (jobId %s) started, cmd: %r', self._job_id,
Line 209:                      cmd)
Line 210:         proc = subprocess.Popen(cmd, env={'LIBGUESTFS_BACKEND': 'direct'},
we'd probably need some input from infra here to make sure we are talking with an extern process properly and to make sure we aren't missing any best practice.
Line 211:                                 shell=True,
Line 212:                                 stdout=subprocess.PIPE,
Line 213:                                 stderr=subprocess.PIPE)
Line 214: 


Line 275:             self._status = msg
Line 276:             logging.error(msg)
Line 277: 
Line 278:     def _create_passwd_file(self):
Line 279:         self._pass_file = os.path.join(_P_V2V_DIR, "%s.tmp" % self._job_id)
is this a plain tempfile or there is any special need?
If it is just a tempfile we should use safer tempfile creation from stdlib and/or vdsm library functions.
Line 280:         f = os.fdopen(os.open(self._pass_file,
Line 281:                               os.O_WRONLY | os.O_CREAT, 0600), 'w')
Line 282:         with closing(f):
Line 283:             f.write(self._password)


Line 278:     def _create_passwd_file(self):
Line 279:         self._pass_file = os.path.join(_P_V2V_DIR, "%s.tmp" % self._job_id)
Line 280:         f = os.fdopen(os.open(self._pass_file,
Line 281:                               os.O_WRONLY | os.O_CREAT, 0600), 'w')
Line 282:         with closing(f):
ok, you use this fdopen dance to get permissions right, I believe. If so we should just switch to proper tempfile handling. Just to give an easy example: 

https://docs.python.org/2/library/tempfile.html#tempfile.NamedTemporaryFile
Line 283:             f.write(self._password)
Line 284: 
Line 285:     def _delete_passwd_file(self):
Line 286:         if os.path.exists(self._pass_file):


Line 283:             f.write(self._password)
Line 284: 
Line 285:     def _delete_passwd_file(self):
Line 286:         if os.path.exists(self._pass_file):
Line 287:             os.remove(self._pass_file)
this also should be handled by tempfile above.
Line 288: 
Line 289:     def abort_job(self):
Line 290:         self._abort = True
Line 291:         self._status = 'abort'


Line 303:             if 'imageID' in disk:
Line 304:                 images = '%s --vdsm-image-uuid %s ' % (images, disk['imageID'])
Line 305:             if 'volumeID' in disk:
Line 306:                 images = '%s --vdsm-vol-uuid %s ' % (images, disk['volumeID'])
Line 307:         return images
this works but feels unpythonic. How about

  images = []
  for disk in self._vmProperties['disks']:
    if 'imageID' in disk:
       images.append('--vdsm-image-uuid')
       images.append(disk['imageID'])
    elif 'volumeID' in disk:  # can a disk have both?!
       images.append('--vdsm-vol-uuid')
       images.append(disk['volumeID'])
   return ' '.join(images)
Line 308: 
Line 309:     def _get_domain_path(self):
Line 310:         disk = self._vmProperties['disks'][0]
Line 311:         drive = {'device': 'disk',


Line 306:                 images = '%s --vdsm-vol-uuid %s ' % (images, disk['volumeID'])
Line 307:         return images
Line 308: 
Line 309:     def _get_domain_path(self):
Line 310:         disk = self._vmProperties['disks'][0]
please add a comment to document why the first disk is important/special/good enough for this function.
Line 311:         drive = {'device': 'disk',
Line 312:                  'poolID': self._vmProperties['poolID'],
Line 313:                  'domainID': self._vmProperties['domainID'],
Line 314:                  'imageID': disk['imageID'],


Line 314:                  'imageID': disk['imageID'],
Line 315:                  'volumeID': disk['volumeID']}
Line 316: 
Line 317:         volPath = self._cif.getInstance().prepareVolumePath(drive)
Line 318:         volPath = volPath.split('/images/')[0]
maybe better use os.path.split
Line 319:         return volPath
Line 320: 
Line 321: 
Line 322: def _mem_to_mib(size, unit):


-- 
To view, visit http://gerrit.ovirt.org/37509
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I34bd86d5a87ea8c42113c4a732f87ddd4ceab9ea
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken 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: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list