Change in vdsm[master]: Hotplug/Hotunplug disks

[Igor Lvovsky]

Igor Lvovsky has uploaded a new change for review.

Change subject: Hotplug/Hotunplug disks
......................................................................

Hotplug/Hotunplug disks

Change-Id: I5feefdba390c4b885aaa5e61f40410419acb3b3c
---
M vdsm/clientIF.py
M vdsm/define.py
M vdsm/libvirtvm.py
M vdsm/vm.py
4 files changed, 147 insertions(+), 20 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/09/409/1
--
To view, visit http://gerrit.ovirt.org/409
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5feefdba390c4b885aaa5e61f40410419acb3b3c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky

Eduardo has posted comments on this change.

Change subject: Hotplug/Hotunplug disks
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(7 inline comments)

....................................................
File vdsm/clientIF.py
Line 611:         self.vmContainerLock.acquire()
Can use with?

Line 613:             for param in ('vmId', 'drives'):
Please split to function the param verification and reuse below.

....................................................
File vdsm/libvirtvm.py
Line 1078:     def _validateDiskParams(self, diskParams):
Unify with the previous validation.
validateMinimalKeySet(dict, keys)

Line 1112:             self._getUnderlyingDriveInfo()
Please rename it.

Line 1141:             # Find proper drive in _preparedDrives dictionary and torn it down.
turn

....................................................
File vdsm/vm.py
Line 404:                 return d
self._drives = [d for d in self._drives if d.path != drive.path]

Line 409:             drive['index'] = idx
Do it outside.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5feefdba390c4b885aaa5e61f40410419acb3b3c
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Eduardo

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Hotplug/Hotunplug disks
......................................................................

Patch Set 2: (7 inline comments)

....................................................
File vdsm/clientIF.py
Line 611:         self.vmContainerLock.acquire()
Actually we don't need it at all

Line 613:             for param in ('vmId', 'drives'):
Done

....................................................
File vdsm/libvirtvm.py
Line 1078:     def _validateDiskParams(self, diskParams):
Done

Line 1112:             self._getUnderlyingDriveInfo()
Done

Line 1141:             # Find proper drive in _preparedDrives dictionary and torn it down.
Done

....................................................
File vdsm/vm.py
Line 404:                 return d
Done

Line 409:             drive['index'] = idx
Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5feefdba390c4b885aaa5e61f40410419acb3b3c
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Hotplug/Hotunplug disks
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File vdsm/utils.py
Line 802: def validateMinimalKeySet(dictionary, keys):
Nice, I'm sure that also the current code can benefit from this. Can you introduce this using a different patch and using it where needed? Then you can place your hotplug patch on top of that.
Raising an exception would be more pythonic, eg: raise ValueError, param

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5feefdba390c4b885aaa5e61f40410419acb3b3c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Hotplug/Hotunplug disks
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File vdsm/utils.py
Line 803:     for param in keys:
return all(x in dictionary for x in keys)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5feefdba390c4b885aaa5e61f40410419acb3b3c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Hotplug/Hotunplug disks
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(1 inline comment)

not reviewed yet, sorry

....................................................
File vdsm/clientIF.py
Line 618:         except KeyError:
I prefer to take the hotplugDisk(params) call outside try--except: if for some reason KeyError is raised in hotplug, we won't be logging it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5feefdba390c4b885aaa5e61f40410419acb3b3c
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Hotplug/Hotunplug disks
......................................................................

Patch Set 4: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5feefdba390c4b885aaa5e61f40410419acb3b3c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky

[Igor Lvovsky]

Igor Lvovsky has abandoned this change.

Change subject: Hotplug/Hotunplug disks
......................................................................

Patch Set 4: Abandoned

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I5feefdba390c4b885aaa5e61f40410419acb3b3c
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Igor Lvovsky