Change in vdsm[master]: Deactivate libvirtVM.Drives()

ewarszaw at redhat.com ewarszaw at redhat.com
Mon May 20 00:26:51 UTC 2013


Eduardo has posted comments on this change.

Change subject: Deactivate libvirtVM.Drives()
......................................................................


Patch Set 2: (1 inline comment)

....................................................
File vdsm/clientIF.py
Line 343:                                              drive['poolID'], drive['imageID'])
Line 344:             except KeyError:
Line 345:                 #This drive is not a vdsm image (quartet)
Line 346:                 self.log.info("Avoiding tear down drive %s", str(drive))
Line 347:         elif drive.__class__.__name__ == 'Drive':
who passes a drive object (instead of a drivespec) to this function?
 shouldn't THEY do the conversion there?

Probably the dict option should be removed.

THIS want to be the minimal fix but THEY are too much (and counting):

ovirt/master:vdsm/API.py:174:                        
self._cif.teardownVolumePath(paramFilespec)

ovirt/master:vdsm/clientIF.py:338:    def teardownVolumePath(self, drive):

ovirt/master:vdsm/libvirtvm.py:455:                    self._vm.cif.teardownVolumePath(self._dst)

ovirt/master:vdsm/libvirtvm.py:1523:                
self.cif.teardownVolumePath(self.conf['restoreState'])

ovirt/master:vdsm/libvirtvm.py:1814:            self.cif.teardownVolumePath(diskParams)

ovirt/master:vdsm/libvirtvm.py:2080:                    self.cif.teardownVolumePath(drive)

ovirt/master:vdsm/libvirtvm.py:2368:                self.cif.teardownVolumePath(dstDiskCopy)

ovirt/master:vdsm/libvirtvm.py:2442:                self.cif.teardownVolumePath(srcDrive.diskReplicate)

ovirt/master:vdsm/libvirtvm.py:2450:                self.cif.teardownVolumePath(diskToTeardown)

ovirt/master:vdsm/libvirtvm.py:2508:            self.cif.teardownVolumePath(drivespec)

ovirt/master:vdsm/libvirtvm.py:2512:        self.cif.teardownVolumePath(self.conf.get(vmDev))

ovirt/master:vdsm/vm.py:236:                self._vm.cif.teardownVolumePath(self._dstparams)

ovirt/master:vdsm/vm.py:1073:                    self.cif.teardownVolumePath(drive)


 add a Drive member function that does this?

When drive member is instantiated, we have no clue what the teardownVolumePath() should be, and if it exists at all.
Drive objects are not real "storage" objects.

In addition please look at prepareVolumePath() a few (a lot :D) lines up.

Again, the minimal fix that was required, this is _not_ my preferred solution.


"Beauty is in the eye of the beholder"

isinstance(drive, Drive)

I'm not sure that we want to teardownVolumePath() subclasses.


elif drive.__class__ is Drive:

By consistency:

ovirt/master:vdsm/storage/resourceFactories.py:146:        if dom.__class__.__name__ == "BlockStorageDomain":
Line 348:             try:
Line 349:                 res = self.irs.teardownImage(drive.domainID,
Line 350:                                              drive.poolID, drive.imageID)
Line 351:             except AttributeError:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8ba965f0987037fa5dafc6b3ea6eeb6f01a82436
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Daniel Paikov <paikov at gmail.com>
Gerrit-Reviewer: Eduardo <ewarszaw at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list