Change in vdsm[master]: Change device ownership for VM's drives.

Eduardo has uploaded a new change for review.

Change subject: Change device ownership for VM's drives.
......................................................................

Change device ownership for VM's drives.

Change-Id: I7859635b8abed67fe006886efd500eb9ac9f3b7c
---
M vdsm/clientIF.py
M vdsm/storage/hsm.py
M vdsm/supervdsmServer.py
M vdsm/vm.py
4 files changed, 31 insertions(+), 2 deletions(-)

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7859635b8abed67fe006886efd500eb9ac9f3b7c
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

(4 inline comments)

minor things

....................................................
File vdsm/clientIF.py
Line 587:                 res = self.irs.appropiateVolume(drive["GUID"], vmId)
appropiate->appropriate

....................................................
File vdsm/storage/hsm.py
Line 2401:     def public_appropiateVolume(self, guid, thiefId):
appropiate->appropriate

Line 2403:         Change owvnership of theguid  device to vdsm:qemu
the guid

....................................................
File vdsm/supervdsmServer.py
Line 158:             #self.log.error("Trigger event for GUID %s failed", guid, exc_info=True)
remove log

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

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

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

Patch Set 2: (3 inline comments)

only minor comments

....................................................
File vdsm/clientIF.py
Line 587:                 res = self.irs.appropriateVolume(drive["GUID"], vmId)
I don't like appropriateVolume name, it's not really fits what this function does.
But I have not a good idea for better one, so ... (maybe ask our names committee)

....................................................
File vdsm/storage/hsm.py
Line 2401:     def public_appropriateVolume(self, guid, thiefId):
please change the thiefId to something else.

....................................................
File vdsm/supervdsmServer.py
Line 154:         cmd =  ['udevadm', 'trigger', '--verbose', '--action', 'change',
why it's not in const ?

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

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

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

(3 inline comments)

where's the rule removal (on vm.destroy)

....................................................
File vdsm/clientIF.py
Line 569:     def _prepareVolumePath(self, drive, vmId = None):
no space in arg=defaultVal

....................................................
File vdsm/storage/hsm.py
Line 2406:         rule = 'SYMLINK=="mapper/%s", OWNER="vdsm", GROUP="qemu"\n' % guid
vdsm/qemu should be taken from constants.

Line 2411:         supervdsm.getProxy().udevTrigger(guid)
how about exposing only supervdsm.addUdevRule()

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

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 2408:         rc, out = misc.writefileSUDO(ruleFile, rule)
writefileSUDO is an evil function that lets a buggy vdsm do anything on the host.

supervdsm should expose an interface which is as secure as possible. in this case: add a rule, remove (a) rule(s), and if you insist - trigger udev.

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

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

Eduardo has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

Patch Set 4: Verified

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

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

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

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

(6 inline comments)

mostly minor comments

....................................................
File vdsm/storage/hsm.py
Line 2401:     def public_appropriateVolume(self, guid, thiefId):
appropriateDevice

please fix docstring format

Line 2409:     def public_rmUdevRules(self, thiefId):
inappropriateDevices? yieldDevices?

add docstring, and hint that this is an internal vdsm API.

....................................................
File vdsm/supervdsmServer.py
Line 42: UDEV_RULE_FILE_DIR = "/etc/udev/rules.d/"
these constants are not interesting to anyone outside. Please name them as such (_)

Line 164:             raise OSError("Could not trigger change for device %s, \
first arg to OSError must be an errno.

Line 168:     def appropriateVolume(self, guid, thiefId):
I think you appropriateDevice, not "volume".

Line 178:                  os.listdir(UDEV_RULE_FILE_DIR) if
I think a plain glob could be safer and clearer

(now someonme could pass empty thiefId, and remove everybody's rules. hmm, glob is worse, as '*' could do the same. how about a complete regexp? )

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

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

Eduardo has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

Patch Set 5: Verified

Tested again, ad-hoc rules are added and removed with the VM.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7859635b8abed67fe006886efd500eb9ac9f3b7c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

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

Eduardo, when you expect a patch to be submitted, you cannot base it on 2 weeks old parent 28b5f4ae5669ba34ab0bcaf60eaafafbb6aadbb1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7859635b8abed67fe006886efd500eb9ac9f3b7c
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

Patch Set 6: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7859635b8abed67fe006886efd500eb9ac9f3b7c
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky

Eduardo has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

Patch Set 6: Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7859635b8abed67fe006886efd500eb9ac9f3b7c
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Change device ownership for VM's drives.
......................................................................

Patch Set 7: Verified; Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7859635b8abed67fe006886efd500eb9ac9f3b7c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Change device ownership for VM's drives.
......................................................................

Change device ownership for VM's drives.

Change-Id: I7859635b8abed67fe006886efd500eb9ac9f3b7c
---
M configure.ac
M vdsm/clientIF.py
M vdsm/constants.py.in
M vdsm/libvirtvm.py
M vdsm/storage/hsm.py
M vdsm/supervdsmServer.py
M vdsm/vm.py
7 files changed, 70 insertions(+), 3 deletions(-)

Approvals:
  Dan Kenigsberg: Verified; Looks good to me, approved

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7859635b8abed67fe006886efd500eb9ac9f3b7c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Igor Lvovsky