Change in vdsm[master]: BZ#807642 Use Securable as base class

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: BZ#807642 Use Securable as base class
......................................................................

BZ#807642 Use Securable as base class

Using Securable as base class allow us to take the best of metaclasses
(being able to modifying methods at import time) and the best of
subclassing (explicit inherited methods such as __init__, _setSafe and
_setUnsafe).
It's also now possible to instantiate attributes per object (__init__)
instead of per class and therefore fix BZ#807642.

Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
---
M vdsm/storage/securable.py
M vdsm/storage/sp.py
2 files changed, 44 insertions(+), 44 deletions(-)

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: BZ#807642 Use Securable as base class
......................................................................

Patch Set 1:

Proof of concept. Tested locally with some example classes, but not tested with StoragePool (yet).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Ayal Baron has posted comments on this change.

Change subject: BZ#807642 Use Securable as base class
......................................................................

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has posted comments on this change.

Change subject: BZ#807642 Use Securable as base class
......................................................................

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#807642 Use Securable as base class
......................................................................

Patch Set 3: Looks good to me, approved

I've asked preinteg655 to verify this

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: BZ#807642 Use Securable as base class
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/securable.py
Line 60:         if not (self._isSafe() or override):
Dan do you want me to get this race too? The solution is not completely trivial, it might require a rwlock to avoid the serialization of all the calls.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#807642 Use Securable as base class
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/securable.py
Line 60:         if not (self._isSafe() or override):
I think that the problem with Securable runs even deeper than this race. Even if fixed, someone can call self._setUnsafe while f() is running, and nothing in Securable protects f from doing harm.

spmStop makes sure to not only _setUnsafe, but umount the master filesystem so we have some additional protection.

In any case, this issue should not be mixed with the current patch.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: BZ#807642 Use Securable as base class
......................................................................

Patch Set 3: (1 inline comment)

....................................................
File vdsm/storage/securable.py
Line 60:         if not (self._isSafe() or override):
> Even if fixed, someone can call self._setUnsafe while f() is running, and nothing in Securable protects f from doing harm.

The idea here is to be consistent in the positive flow. The self._setUnsafe() shouldn't complete until f() completed. Nothing in securable protects from f(), that is correct, but we shouldn't advertise that we stopped the spm while we still have f() running. That is why we would need to queue stopSpm behind the secured calls.
I agree on postponing this fix.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: BZ#807642 Use Securable as base class
......................................................................

Patch Set 3: Verified

<hateya> its working

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: BZ#807642 Use Securable as base class
......................................................................

BZ#807642 Use Securable as base class

Using Securable as base class allow us to take the best of metaclasses
(being able to modifying methods at import time) and the best of
subclassing (explicit inherited methods such as __init__, _setSafe and
_setUnsafe).
It's also now possible to instantiate attributes per object (__init__)
instead of per class and therefore fix BZ#807642.

Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
---
M vdsm/storage/securable.py
M vdsm/storage/sp.py
2 files changed, 34 insertions(+), 34 deletions(-)

Approvals:
  Ayal Baron: Looks good to me, but someone else must approve
  Saggi Mizrahi: Looks good to me, but someone else must approve
  Dan Kenigsberg: Verified; Looks good to me, approved

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi