Change in vdsm[master]: securable: refactor the scurable implementation

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: securable: refactor the scurable implementation
......................................................................

securable: refactor the scurable implementation

Change-Id: Id5a8be7536748481746795e27701fbecc7c3318c
Signed-off-by: Federico Simoncelli 
---
M tests/main.py
M vdsm/storage/securable.py
M vdsm/storage/sp.py
3 files changed, 28 insertions(+), 68 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/15/22115/1

diff --git a/tests/main.py b/tests/main.py
index dfc8b04..1f87092 100644
--- a/tests/main.py
+++ b/tests/main.py
@@ -23,22 +23,9 @@

from gluster import exception as gluster_exception
 from storage import storage_exception
-from storage import securable
 from vdsm.utils import GeneralException

from testrunner import VdsmTestCase as TestCaseBase
-
-
-class TestSecurable(TestCaseBase):
-    class MySecureClass(securable.Securable):
-        pass
-
-    def test_safety(self):
-        objA = TestSecurable.MySecureClass()
-        objB = TestSecurable.MySecureClass()
-        objA._setSafe()
-        objB._setUnsafe()
-        self.assertTrue(objA._isSafe())

class TestStorageExceptions(TestCaseBase):
diff --git a/vdsm/storage/securable.py b/vdsm/storage/securable.py
index cda74ef..9a31bff 100644
--- a/vdsm/storage/securable.py
+++ b/vdsm/storage/securable.py
@@ -18,35 +18,25 @@
 # Refer to the README and COPYING files for full details of the license
 #

-from threading import Event
-from functools import wraps
+from functools import update_wrapper

OVERRIDE_ARG = "__securityOverride"
 SECURE_FIELD = "__secured__"
+SECURE_METHOD = 'isSafe'

class SecureError(RuntimeError):
     pass

-class MetaSecurable(type):
-    def __new__(cls, name, bases, dct):
-        for fun, val in dct.iteritems():
-            if not callable(val):
-                continue
-
-            if (hasattr(val, SECURE_FIELD) and
-                    not getattr(val, SECURE_FIELD)):
-                continue
-
-            if fun.startswith("__"):
-                # Wrapping builtins might cause weird results
-                continue
-
-            dct[fun] = secured(val)
-
-        dct['__securable__'] = True
-        return type.__new__(cls, name, bases, dct)
+def secured(cls):
+    for fun, val in cls.__dict__.iteritems():
+        if (callable(val) and getattr(val, SECURE_FIELD, True)
+                and not fun.startswith("__")  # skipping builtins
+                and not fun == SECURE_METHOD):
+            setattr(cls, fun, secure_method(val))
+    cls.__securable__ = True
+    return cls

def unsecured(f):
@@ -54,36 +44,16 @@
     return f

-def secured(f):
-    @wraps(f)
+def secure_method(method):
     def wrapper(self, *args, **kwargs):
         if not hasattr(self, "__securable__"):
             raise RuntimeError("Secured object is not a securable")

override = kwargs.pop(OVERRIDE_ARG, False)

-        if not (self._isSafe() or override):
-            raise SecureError()
+        if not (getattr(self, SECURE_METHOD)() or override):
+            raise SecureError("Secured object is not in safe state")

-        return f(self, *args, **kwargs)
+        return method(self, *args, **kwargs)

-    return wrapper
-
-
-class Securable(object):
-    __metaclass__ = MetaSecurable
-
-    def __init__(self):
-        self._safety = Event()
-
-    @unsecured
-    def _isSafe(self):
-        return self._safety.isSet()
-
-    @unsecured
-    def _setSafe(self):
-        self._safety.set()
-
-    @unsecured
-    def _setUnsafe(self):
-        self._safety.clear()
+    return update_wrapper(wrapper, method)
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index 31e0bcd..6e9d1e5 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -43,7 +43,7 @@
 import storage_exception as se
 from persistentDict import DictValidator, unicodeEncoder, unicodeDecoder
 from remoteFileHandler import Timeout
-from securable import Securable, unsecured
+from securable import secured, unsecured
 import image
 from resourceFactories import IMAGE_NAMESPACE
 from storageConstants import STORAGE
@@ -100,7 +100,8 @@
 MAX_DOMAINS /= 48

-class StoragePool(Securable):
+@secured
+class StoragePool(object):
     '''
     StoragePool object should be relatively cheap to construct. It should defer
     any heavy lifting activities until the time it is really needed.
@@ -112,12 +113,11 @@
     lvExtendPolicy = config.get('irs', 'vol_extend_policy')

def __init__(self, spUUID, domainMonitor, taskManager):
-        Securable.__init__(self)
-
+        self._safety = threading.Event()
+        self._safety.clear()
         self._formatConverter = DefaultFormatConverter()
         self._domainsToUpgrade = []
         self.lock = threading.RLock()
-        self._setUnsafe()
         self.spUUID = str(spUUID)
         self.poolPath = os.path.join(self.storage_repository, self.spUUID)
         self.id = SPM_ID_FREE
@@ -131,6 +131,9 @@
         self.domainMonitor = domainMonitor
         self._upgradeCallback = partial(StoragePool._upgradePoolDomain,
                                         proxy(self))
+
+    def isSafe(self):
+        return self._safety.isSet()

@unsecured
     def getSpmLver(self):
@@ -303,11 +306,11 @@
                 self.spmRole = SPM_ACQUIRED

# Once setSafe completes we are running as SPM
-                self._setSafe()
+                self._safety.set()

# Mailbox issues SPM commands, therefore we start it AFTER spm
                 # commands are allowed to run to prevent a race between the
-                # mailbox and the "self._setSafe() call"
+                # mailbox and the "self._safety.set() call"

# Create mailbox if SAN pool (currently not needed on nas)
                 # FIXME: Once pool contains mixed type domains (NFS + Block)
@@ -395,7 +398,7 @@
                 return True

self._shutDownUpgrade()
-            self._setUnsafe()
+            self._safety.clear()

stopFailed = False

@@ -614,7 +617,7 @@
         self._acquireTemporaryClusterLock(msdUUID, leaseParams)

try:
-            self._setSafe()
+            self._safety.set()
             # Mark 'master' domain
             # We should do it before actually attaching this domain to the pool
             # During 'master' marking we create pool metadata and each attached
@@ -644,7 +647,7 @@
                                exc_info=True)
             raise
         finally:
-            self._setUnsafe()
+            self._safety.clear()

self._releaseTemporaryClusterLock(msdUUID)
             self.stopMonitoringDomains()

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: securable: refactor the scurable implementation
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5924/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5128/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6016/ : SUCCESS

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: securable: refactor the scurable implementation
......................................................................

Patch Set 1:

(2 comments)

....................................................
File tests/main.py
Line 28
Line 29
Line 30
Line 31
Line 32
This is useless now. If I'll have time I'll add some more meaningful tests.

....................................................
File vdsm/storage/sp.py
Line 305: 
Line 306:                 self.spmRole = SPM_ACQUIRED
Line 307: 
Line 308:                 # Once setSafe completes we are running as SPM
Line 309:                 self._safety.set()
I don't mind keeping _setSafe/Unsafe, they just seemed redundant to me at this point. (I'd like to think that if we were introducing this now we probably wouldn't add them to begin with).
Line 310: 
Line 311:                 # Mailbox issues SPM commands, therefore we start it AFTER spm
Line 312:                 # commands are allowed to run to prevent a race between the
Line 313:                 # mailbox and the "self._safety.set() call"

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: securable: refactor the scurable implementation
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5955/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5159/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6046/ : SUCCESS

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the scurable implementation
......................................................................

Patch Set 2:

(6 comments)

Can you explain what do we need this strange concept of "safe" or "secured" class/methods?

....................................................
File tests/main.py
Line 25: from storage import storage_exception
Line 26: from vdsm.utils import GeneralException
Line 27: 
Line 28: from testrunner import VdsmTestCase as TestCaseBase
Line 29: 
Where are the new tests?
Line 30: 
Line 31: class TestStorageExceptions(TestCaseBase):
Line 32:     def test_collisions(self):
Line 33:         codes = {}

....................................................
File vdsm/storage/securable.py
Line 21: from functools import update_wrapper
Line 22: 
Line 23: OVERRIDE_ARG = "__securityOverride"
Line 24: SECURE_FIELD = "__secured__"
Line 25: SECURE_METHOD = 'isSafe'
This has to match method implemented by the "safe" class.
Line 26: 
Line 27: 
Line 28: class SecureError(RuntimeError):
Line 29:     pass

Line 55:             raise SecureError("Secured object is not in safe state")
Line 56: 
Line 57:         return method(self, *args, **kwargs)
Line 58: 
Line 59:     return update_wrapper(wrapper, method)
Why update_wrapper is better then @wraps?

....................................................
File vdsm/storage/sp.py
Line 42: from sdc import sdCache
Line 43: import storage_exception as se
Line 44: from persistentDict import DictValidator, unicodeEncoder, unicodeDecoder
Line 45: from remoteFileHandler import Timeout
Line 46: from securable import secured, unsecured
These names are confusing - secured is class decorator, and unsecured is function decorator, but the name suggest that they are the same type.
Line 47: import image
Line 48: from resourceFactories import IMAGE_NAMESPACE
Line 49: from storageConstants import STORAGE
Line 50: import resourceManager as rm

Line 131:         self.domainMonitor = domainMonitor
Line 132:         self._upgradeCallback = partial(StoragePool._upgradePoolDomain,
Line 133:                                         proxy(self))
Line 134: 
Line 135:     def isSafe(self):
How users of the secured class decorator can know that they must implement "isSafe" method?

Also "security" and "safety" are different concepts. Lets choose one of them.
Line 136:         return self._safety.isSet()
Line 137: 
Line 138:     @unsecured
Line 139:     def getSpmLver(self):

Line 319: 
Line 320:                 self.spmRole = SPM_ACQUIRED
Line 321: 
Line 322:                 # Once setSafe completes we are running as SPM
Line 323:                 self._safety.set()
This is worse then the original version. I would add a "_beSafe" and "_beUnsafe" methods.
Line 324:                 self._updateDomainsRole()
Line 325: 
Line 326:                 # Mailbox issues SPM commands, therefore we start it AFTER spm
Line 327:                 # commands are allowed to run to prevent a race between the

Itamar Heim has posted comments on this change.

Change subject: securable: refactor the scurable implementation
......................................................................

Patch Set 2:

(1 comment)

....................................................
Commit Message
Line 3: AuthorDate: 2013-12-05 07:16:59 -0500
Line 4: Commit:     Federico Simoncelli 
Line 5: CommitDate: 2013-12-06 11:54:48 -0500
Line 6: 
Line 7: securable: refactor the scurable implementation
s/scurable/securable/
could have been much worst as typos go for this word ;)
Line 8: 
Line 9: Change-Id: Id5a8be7536748481746795e27701fbecc7c3318c

Ayal Baron has posted comments on this change.

Change subject: securable: refactor the scurable implementation
......................................................................

Patch Set 2:

(3 comments)

....................................................
Commit Message
Line 3: AuthorDate: 2013-12-05 07:16:59 -0500
Line 4: Commit:     Federico Simoncelli 
Line 5: CommitDate: 2013-12-06 11:54:48 -0500
Line 6: 
Line 7: securable: refactor the scurable implementation
refactor why?
Line 8: 
Line 9: Change-Id: Id5a8be7536748481746795e27701fbecc7c3318c

....................................................
File vdsm/storage/securable.py
Line 32: def secured(cls):
Line 33:     for fun, val in cls.__dict__.iteritems():
Line 34:         if (callable(val) and getattr(val, SECURE_FIELD, True)
Line 35:                 and not fun.startswith("__")  # skipping builtins
Line 36:                 and not fun == SECURE_METHOD):
I actually think the previous style is more legible.
Line 37:             setattr(cls, fun, secure_method(val))
Line 38:     cls.__securable__ = True
Line 39:     return cls
Line 40:

Line 43:     setattr(f, SECURE_FIELD, False)
Line 44:     return f
Line 45: 
Line 46: 
Line 47: def secure_method(method):
did we change the function naming convention to using underscores already?
Line 48:     def wrapper(self, *args, **kwargs):
Line 49:         if not hasattr(self, "__securable__"):
Line 50:             raise RuntimeError("Secured object is not a securable")
Line 51:

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the scurable implementation
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm/storage/sp.py
Line 305: 
Line 306:                 self.spmRole = SPM_ACQUIRED
Line 307: 
Line 308:                 # Once setSafe completes we are running as SPM
Line 309:                 self._safety.set()
They just make the code more clear. Compare:

self._safety.set()

and

self._beSafe()
Line 310: 
Line 311:                 # Mailbox issues SPM commands, therefore we start it AFTER spm
Line 312:                 # commands are allowed to run to prevent a race between the
Line 313:                 # mailbox and the "self._safety.set() call"

Saggi Mizrahi has posted comments on this change.

Change subject: securable: refactor the scurable implementation
......................................................................

Patch Set 2:

(2 comments)

Also, why not just remove the whole securable non-sense.
Let's just explicitly validate like normal people.

....................................................
File vdsm/storage/securable.py
Line 32: def secured(cls):
Line 33:     for fun, val in cls.__dict__.iteritems():
Line 34:         if (callable(val) and getattr(val, SECURE_FIELD, True)
Line 35:                 and not fun.startswith("__")  # skipping builtins
Line 36:                 and not fun == SECURE_METHOD):
I wrote the previous one so I obviously think it's better.
But just to explain:

- It's easier to mentally process positive conditionals.
- The rule I'm trying to convey is "skip X if" and not "wrap X when"
- They are logically identical but the former is what you actually think when you modify this method.
Line 37:             setattr(cls, fun, secure_method(val))
Line 38:     cls.__securable__ = True
Line 39:     return cls
Line 40:

Line 43:     setattr(f, SECURE_FIELD, False)
Line 44:     return f
Line 45: 
Line 46: 
Line 47: def secure_method(method):
Were there any talks about it?

Though I prefer the underscore style and use it for everything I pull out of VDSM. Consistency is important.
Line 48:     def wrapper(self, *args, **kwargs):
Line 49:         if not hasattr(self, "__securable__"):
Line 50:             raise RuntimeError("Secured object is not a securable")
Line 51:

Change in vdsm[master]: securable: refactor the securable implementation

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 3:

Can you address comments from the previous patch, and explain whats new in this patch set?

Change in vdsm[master]: securable: refactor the securable implementation

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6643/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5750/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6556/ : SUCCESS

Change in vdsm[master]: securable: refactor the securable implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/22115/2/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 43:     setattr(f, SECURE_FIELD, False)
Line 44:     return f
Line 45: 
Line 46: 
Line 47: def secure_method(method):
> Were there any talks about it?

We always introduced changes gradually with new patches. I prefer underscores and anyway this module is completely unrelated to VDSM (and it doesn't even belong to storage). Moreover the method in question is used only internally and it's never called from outside (do you prefer _secure_method)?

If you still think that this is going to break VDSM consistency I'll change it.
Line 48:     def wrapper(self, *args, **kwargs):
Line 49:         if not hasattr(self, "__securable__"):
Line 50:             raise RuntimeError("Secured object is not a securable")
Line 51:

Change in vdsm[master]: securable: refactor the securable implementation

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 4:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6727/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6640/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5835/ : SUCCESS

Change in vdsm[master]: securable: refactor the securable implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 4: Verified+1

Verified with the unit tests, starting/stopping the SPM few times and using this patch for a while.

Change in vdsm[master]: securable: refactor the securable implementation

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 4: Code-Review+1

I like that.

Change in vdsm[master]: securable: refactor the securable implementation

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 4:

(5 comments)

It's a big improvement relative to the existing code, but I think that it can be kept even simpler.

http://gerrit.ovirt.org/#/c/22115/4//COMMIT_MSG
Commit Message:

Line 11: In fact the different backends (with/without metadata) will rely on the
Line 12: StoragePool SPMness to protect their methods.
Line 13: 
Line 14: In this patch:
Line 15: - transform the Securable base class and MetaSecurable in a decorator
in a decorator -> into a decorator
Line 16: 
Line 17: Change-Id: Id5a8be7536748481746795e27701fbecc7c3318c

http://gerrit.ovirt.org/#/c/22115/4/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 1: #
Line 2: # Copyright 2011 Red Hat, Inc.
happy new year
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or

Line 28: class SecureError(RuntimeError):
Line 29:     pass
Line 30: 
Line 31: 
Line 32: def Secured(check_method_name):
It would be nice if you can document this while it's fresh: the intention here is to wrap all function with a safety check, except of explictly-unsafe ones.

Have you explained why you find it useful to generalize the "check_method_name"? I think that declaring that any @Secured class must implement an _isSafe() method (or __is_safe__()) is good enough, and simplifies the usage and code.
Line 33:     def wrapper(cls):
Line 34:         for fun, val in cls.__dict__.iteritems():
Line 35:             if (not callable(val) or not getattr(val, SECURE_FIELD, True)
Line 36:                     or fun.startswith("__")  # skipping builtins

Line 30: 
Line 31: 
Line 32: def Secured(check_method_name):
Line 33:     def wrapper(cls):
Line 34:         for fun, val in cls.__dict__.iteritems():
it's not your fault, but "fun" is a misleading choice for a name of an attribute (which may well be a non-method)
Line 35:             if (not callable(val) or not getattr(val, SECURE_FIELD, True)
Line 36:                     or fun.startswith("__")  # skipping builtins
Line 37:                     or fun == check_method_name):
Line 38:                 continue

Line 47:     setattr(f, SECURE_FIELD, False)
Line 48:     return f
Line 49: 
Line 50: 
Line 51: def _secure_method(method, check_method_name):
_secure_method() is now private. As such, it cannot be used outside of this module, so there is no worry that it would ever be used on a random object, so the __securable__ verification becomes a needless distraction.
Line 52:     @wraps(method)
Line 53:     def wrapper(self, *args, **kwargs):
Line 54:         if not getattr(self, SECURABLE_FIELD, False):
Line 55:             raise RuntimeError("Secured object is not a securable")

Change in vdsm[master]: securable: refactor the securable implementation

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 4:

(3 comments)

http://gerrit.ovirt.org/#/c/22115/4/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 28: class SecureError(RuntimeError):
Line 29:     pass
Line 30: 
Line 31: 
Line 32: def Secured(check_method_name):
> It would be nice if you can document this while it's fresh: the intention h

Requiring implementation of "_isSafe" does not make sense - maybe "isSafe". But this was what Federico did in the previous version, and I push him to make this configuration visible in the call site, instead of a secret constant in the module.

I think that:

@Secured("isSPM")
    class StoragePool(object):

def isSPM(self):
            ...

Is more clear that:

@Secured
    class StoragePool(object):

def isSafe(self):
            ...

This allows the class to use terms from the relevant domain, instead of generic and confusing "isSafe".

But if both you and Federico like the constant "isSafe" better, go for it.
Line 33:     def wrapper(cls):
Line 34:         for fun, val in cls.__dict__.iteritems():
Line 35:             if (not callable(val) or not getattr(val, SECURE_FIELD, True)
Line 36:                     or fun.startswith("__")  # skipping builtins

Line 31: 
Line 32: def Secured(check_method_name):
Line 33:     def wrapper(cls):
Line 34:         for fun, val in cls.__dict__.iteritems():
Line 35:             if (not callable(val) or not getattr(val, SECURE_FIELD, True)
Actually using callable is even worse, because having class attributes will try to wrap them.

We can use the inspect module to make this more correct (see http://gerrit.ovirt.org/#/c/23130/1/lib/vdsm/utils.py).

But this issue exists in the previous code, so it should not be changed in this patch. The goal of this patch, as I understand after talking with Federico, is to allow multiple objects to refer to single object for keeping "safety" state. The major change to make it work, is removing the state from Securable, and using a method for getting this state.

Any other improvement should be done separately to keep patches small and and clear.
Line 36:                     or fun.startswith("__")  # skipping builtins
Line 37:                     or fun == check_method_name):
Line 38:                 continue
Line 39:             setattr(cls, fun, _secure_method(val, check_method_name))

Line 47:     setattr(f, SECURE_FIELD, False)
Line 48:     return f
Line 49: 
Line 50: 
Line 51: def _secure_method(method, check_method_name):
> _secure_method() is now private. As such, it cannot be used outside of this

I agree, but we can do this extra refactoring in another patch.
Line 52:     @wraps(method)
Line 53:     def wrapper(self, *args, **kwargs):
Line 54:         if not getattr(self, SECURABLE_FIELD, False):
Line 55:             raise RuntimeError("Secured object is not a securable")

Change in vdsm[master]: securable: refactor the securable implementation

Ayal Baron has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 4: Code-Review+1

Change in vdsm[master]: securable: refactor the securable implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 4:

(4 comments)

http://gerrit.ovirt.org/#/c/22115/4/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 28: class SecureError(RuntimeError):
Line 29:     pass
Line 30: 
Line 31: 
Line 32: def Secured(check_method_name):
> Requiring implementation of "_isSafe" does not make sense - maybe "isSafe".

I like the new version better.
Line 33:     def wrapper(cls):
Line 34:         for fun, val in cls.__dict__.iteritems():
Line 35:             if (not callable(val) or not getattr(val, SECURE_FIELD, True)
Line 36:                     or fun.startswith("__")  # skipping builtins

Line 30: 
Line 31: 
Line 32: def Secured(check_method_name):
Line 33:     def wrapper(cls):
Line 34:         for fun, val in cls.__dict__.iteritems():
> it's not your fault, but "fun" is a misleading choice for a name of an attr

Criticism on names is always well accepted, even more so if it includes a better proposal.
Line 35:             if (not callable(val) or not getattr(val, SECURE_FIELD, True)
Line 36:                     or fun.startswith("__")  # skipping builtins
Line 37:                     or fun == check_method_name):
Line 38:                 continue

Line 31: 
Line 32: def Secured(check_method_name):
Line 33:     def wrapper(cls):
Line 34:         for fun, val in cls.__dict__.iteritems():
Line 35:             if (not callable(val) or not getattr(val, SECURE_FIELD, True)
> Actually using callable is even worse, because having class attributes will

Actually callable on a class attribute (even a classmethod) returns False. So if that's what you were referring to, there's nothing to improve here.

(You have to consider that we loop over __dict__ of the class)
Line 36:                     or fun.startswith("__")  # skipping builtins
Line 37:                     or fun == check_method_name):
Line 38:                 continue
Line 39:             setattr(cls, fun, _secure_method(val, check_method_name))

Line 47:     setattr(f, SECURE_FIELD, False)
Line 48:     return f
Line 49: 
Line 50: 
Line 51: def _secure_method(method, check_method_name):
> I agree, but we can do this extra refactoring in another patch.

I actually wanted to remove this. So fine by me.
Line 52:     @wraps(method)
Line 53:     def wrapper(self, *args, **kwargs):
Line 54:         if not getattr(self, SECURABLE_FIELD, False):
Line 55:             raise RuntimeError("Secured object is not a securable")

Change in vdsm[master]: securable: refactor the securable implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 5: Verified+1

Verified using the unit tests only.

Change in vdsm[master]: securable: refactor the securable implementation

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 5:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6745/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6658/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5853/ : SUCCESS

Change in vdsm[master]: securable: refactor the securable implementation

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 5:

(1 comment)

http://gerrit.ovirt.org/#/c/22115/5/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 27: class SecureError(RuntimeError):
Line 28:     pass
Line 29: 
Line 30: 
Line 31: def Secured(check_method_name):
docstring still omitted. intentionally?

By its name, "@Secured" is very specific. It's not a generic wrapper-of-all-methods. I do not anticipate

@Secure('check1')
 @Secure('check2')
 class Bla():

so I'm having a last attempt to convince that expecting an __is_secure__ method is simplest and clearest.

But maybe it's just a naming problem. We have three terms ("secure", "issafe" and "check") basically meaning the same thing. Maybe a rename check_method_name -> security_method_name would make it clearer?

Btw, is it commonplace to have class wrappers start with an uppercase?
Line 32:     def wrapper(cls):
Line 33:         for name, value in cls.__dict__.iteritems():
Line 34:             if (not callable(value) or not getattr(value, SECURE_FIELD, True)
Line 35:                     or name.startswith("__")  # skipping builtins

Change in vdsm[master]: securable: refactor the securable implementation

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 5: Code-Review+1

Change in vdsm[master]: securable: refactor the securable implementation

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 5:

(1 comment)

http://gerrit.ovirt.org/#/c/22115/5/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 27: class SecureError(RuntimeError):
Line 28:     pass
Line 29: 
Line 30: 
Line 31: def Secured(check_method_name):
> docstring still omitted. intentionally?

I never seen CamelCase decorators in other code, but we have them, e.g. monkeypatch.MonkeyPatch and monkeypatch.MonkeyClass.
Line 32:     def wrapper(cls):
Line 33:         for name, value in cls.__dict__.iteritems():
Line 34:             if (not callable(value) or not getattr(value, SECURE_FIELD, True)
Line 35:                     or name.startswith("__")  # skipping builtins

Change in vdsm[master]: securable: refactor the securable implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 5:

(1 comment)

http://gerrit.ovirt.org/#/c/22115/5/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 27: class SecureError(RuntimeError):
Line 28:     pass
Line 29: 
Line 30: 
Line 31: def Secured(check_method_name):
> I never seen CamelCase decorators in other code, but we have them, e.g. mon

I liked @secured better. It was capitalized to satisfy this request:

http://gerrit.ovirt.org/#/c/22115/2/vdsm/storage/sp.py
Line 32:     def wrapper(cls):
Line 33:         for name, value in cls.__dict__.iteritems():
Line 34:             if (not callable(value) or not getattr(value, SECURE_FIELD, True)
Line 35:                     or name.startswith("__")  # skipping builtins

Change in vdsm[master]: securable: refactor the securable implementation

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 5:

(1 comment)

http://gerrit.ovirt.org/#/c/22115/5/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 27: class SecureError(RuntimeError):
Line 28:     pass
Line 29: 
Line 30: 
Line 31: def Secured(check_method_name):
> I liked @secured better. It was capitalized to satisfy this request:

This was not my intention. The problem was that having two public decorators, one called "secured" and one "unsecured". One would expect that both are class decorators or method decorators.

If we rename to:

@secured_class
    class Foo:

@unsecured_method
        def somthing(self):
            ...

There is no confusion. But I'm not that happy with those names.

+1 for renaming it to "secured".

Adding module documentation that explain how do you use those decorators will solve the possible confusion. For example, see monkeypatch.py.
Line 32:     def wrapper(cls):
Line 33:         for name, value in cls.__dict__.iteritems():
Line 34:             if (not callable(value) or not getattr(value, SECURE_FIELD, True)
Line 35:                     or name.startswith("__")  # skipping builtins

Change in vdsm[master]: securable: refactor the securable implementation

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/22115/4/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 31: 
Line 32: def Secured(check_method_name):
Line 33:     def wrapper(cls):
Line 34:         for fun, val in cls.__dict__.iteritems():
Line 35:             if (not callable(val) or not getattr(val, SECURE_FIELD, True)
> Actually callable on a class attribute (even a classmethod) returns False.

I was referring to this possible code:

>>> class Foo:
    ...        class Bar:
    ...            pass
    ...        pass
    ... 
    >>> for name, value in Foo.__dict__.iteritems():
    ...     print name, 'is callable' if callable(value) else 'not callable'
    ... 
    __module__ not callable
    Bar is callable
    __doc__ not callable

I guess that we do not want to to wrap the Bar class, do we?
Line 36:                     or fun.startswith("__")  # skipping builtins
Line 37:                     or fun == check_method_name):
Line 38:                 continue
Line 39:             setattr(cls, fun, _secure_method(val, check_method_name))

Change in vdsm[master]: securable: refactor the securable implementation

Adam Litke has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 5: Code-Review+1

Change in vdsm[master]: securable: refactor the securable implementation

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 6: Verified+1

Change in vdsm[master]: securable: refactor the securable implementation

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 6:

(8 comments)

http://gerrit.ovirt.org/#/c/22115/6/tests/main.py
File tests/main.py:

Line 36: 
Line 37:         def __init__(self):
Line 38:             self.secured = False
Line 39: 
Line 40:         def __is_secure__(self):
__issecured__ or __secured__ is more Pythonic here. If you go an define "special" method, make it look like other special methods.
Line 41:             return self.secured
Line 42: 
Line 43:         def securedMethod(self):
Line 44:             "securedMethod docstring"

Line 59:             pass
Line 60: 
Line 61:     def assertUnsecured(self, secureObject):
Line 62:         self.assertRaises(SecureError, secureObject.securedMethod)
Line 63:         secureObject.unsecuredMethod()
I would separate this to two tests - one test for rasing when calling secure method, and other other for not raising when calling unsecure method. This way, when a test fails, you know exactly what is broken by the test name.
Line 64: 
Line 65:     def assertSecured(self, secureObject):
Line 66:         secureObject.securedMethod()
Line 67:         secureObject.unsecuredMethod()

Line 83:         self.assertUnsecured(secureObject)
Line 84: 
Line 85:     def testSecurityOverride(self):
Line 86:         secureObject = TestSecurable.MySecureClass()
Line 87:         secureObject.securedMethod(__securityOverride=True)
I would simply reuse the same constant:

securedObject.secureMethod(__secured__=True)
Line 88: 
Line 89:     def testDocstringWrapping(self):
Line 90:         secureObject = TestSecurable.MySecureClass()
Line 91:

http://gerrit.ovirt.org/#/c/22115/6/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 21: from functools import wraps
Line 22: 
Line 23: OVERRIDE_ARG = "__securityOverride"
Line 24: SECURE_FIELD = "__secured__"
Line 25: SECURE_METHOD_NAME = "__is_secure__"
We can one constant for everything:

SECURED = __secured__
Line 26: 
Line 27: 
Line 28: class SecureError(RuntimeError):
Line 29:     pass

Line 44: 
Line 45:     for name, value in cls.__dict__.iteritems():
Line 46:         # Skipping non callable attributes, special methods (including
Line 47:         # SECURE_METHOD_NAME) and unsecured methods.
Line 48:         if (not callable(value) or not getattr(value, SECURE_FIELD, True)
If we simplify the constants:

getattr(value, SECURED, True)
Line 49:                 or name.startswith("__")):
Line 50:             continue
Line 51:         setattr(cls, name, _secure_method(value))
Line 52:

Line 58: 
Line 59:     This decorator is used to mark the methods (of a @secured class) that
Line 60:     are not subject to the __is_secure__ execution check.
Line 61:     """
Line 62:     setattr(f, SECURE_FIELD, False)
If we simplify the constants:

setattr(f, SECURED, False)
Line 63:     return f
Line 64: 
Line 65: 
Line 66: def _secure_method(method):

Line 65: 
Line 66: def _secure_method(method):
Line 67:     @wraps(method)
Line 68:     def wrapper(self, *args, **kwargs):
Line 69:         override = kwargs.pop(OVERRIDE_ARG, False)
If we unify the constants:

override = kwargs.pop(SECURED, False)
Line 70: 
Line 71:         if not (getattr(self, SECURE_METHOD_NAME)() is True
Line 72:                 or override is True):
Line 73:             raise SecureError("Secured object is not in safe state")

Line 69:         override = kwargs.pop(OVERRIDE_ARG, False)
Line 70: 
Line 71:         if not (getattr(self, SECURE_METHOD_NAME)() is True
Line 72:                 or override is True):
Line 73:             raise SecureError("Secured object is not in safe state")
First, we don't want to test for True, we care about the truth-fullness of the value, not the type.

Second, it is more clear to do this in two steps:

is_secured = getattr(self, SECURED)()

if not (is_secured or override):
        raise ...

But why do we care about is_secured if we override the value? This explain better our intent:

secured = kwargs.pop(SECURED, False) or getattr(self, SECURED)()
    if not secured:
        raise ...
Line 74: 
Line 75:         return method(self, *args, **kwargs)
Line 76:

Change in vdsm[master]: securable: refactor the securable implementation

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 6:

(2 comments)

http://gerrit.ovirt.org/#/c/22115/6/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 21: from functools import wraps
Line 22: 
Line 23: OVERRIDE_ARG = "__securityOverride"
Line 24: SECURE_FIELD = "__secured__"
Line 25: SECURE_METHOD_NAME = "__is_secure__"
> We can one constant for everything:

but __secured__ is an attr of methods, __is_secure__ is of the class. I'd rather have them apart.
Line 26: 
Line 27: 
Line 28: class SecureError(RuntimeError):
Line 29:     pass

Line 69:         override = kwargs.pop(OVERRIDE_ARG, False)
Line 70: 
Line 71:         if not (getattr(self, SECURE_METHOD_NAME)() is True
Line 72:                 or override is True):
Line 73:             raise SecureError("Secured object is not in safe state")
> First, we don't want to test for True, we care about the truth-fullness of

+1 for checking truthfullness.
Line 74: 
Line 75:         return method(self, *args, **kwargs)
Line 76:

Change in vdsm[master]: securable: refactor the securable implementation

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 6: Code-Review+1

I think it is nice enough as is, we can polish it later if the martians complain about the code style.

Change in vdsm[master]: securable: refactor the securable implementation

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

http://gerrit.ovirt.org/#/c/22115/6/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 69:         override = kwargs.pop(OVERRIDE_ARG, False)
Line 70: 
Line 71:         if not (getattr(self, SECURE_METHOD_NAME)() is True
Line 72:                 or override is True):
Line 73:             raise SecureError("Secured object is not in safe state")
> +1 for checking truthfullness.

Over irc Federico told me that he considers "is True" as a bit safer - if a buggy __is_secure__() returns 17 we should not consider ourselves as safe.
Line 74: 
Line 75:         return method(self, *args, **kwargs)
Line 76:

Change in vdsm[master]: securable: refactor the securable implementation

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 6:

(1 comment)

http://gerrit.ovirt.org/#/c/22115/6/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 21: from functools import wraps
Line 22: 
Line 23: OVERRIDE_ARG = "__securityOverride"
Line 24: SECURE_FIELD = "__secured__"
Line 25: SECURE_METHOD_NAME = "__is_secure__"
> but __secured__ is an attr of methods, __is_secure__ is of the class. I'd r

The distinction is not important; and the solution is more elegant with one __secured__ attribute.
Line 26: 
Line 27: 
Line 28: class SecureError(RuntimeError):
Line 29:     pass

Change in vdsm[master]: securable: refactor the securable implementation

Ayal Baron has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 6: Code-Review+2

Change in vdsm[master]: securable: refactor the securable implementation

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: securable: refactor the securable implementation
......................................................................

securable: refactor the securable implementation

In order to remove the storage pool metadata we need more flexibility
in defining the isSafe method.
In fact the different backends (with/without metadata) will rely on the
StoragePool SPMness to protect their methods.

In this patch:
- transform the Securable base class and MetaSecurable in a decorator

Change-Id: Id5a8be7536748481746795e27701fbecc7c3318c
Signed-off-by: Federico Simoncelli 
Reviewed-on: http://gerrit.ovirt.org/22115
Reviewed-by: Nir Soffer 
Reviewed-by: Dan Kenigsberg 
Reviewed-by: Ayal Baron 
---
M tests/main.py
M vdsm/storage/securable.py
M vdsm/storage/sp.py
3 files changed, 115 insertions(+), 62 deletions(-)

Approvals:
  Ayal Baron: Looks good to me, approved
  Nir Soffer: Looks good to me, but someone else must approve
  Federico Simoncelli: Verified
  Dan Kenigsberg: Looks good to me, but someone else must approve

Change in vdsm[master]: securable: refactor the securable implementation

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: securable: refactor the securable implementation
......................................................................

Patch Set 6:

(1 comment)

http://gerrit.ovirt.org/#/c/22115/6/vdsm/storage/securable.py
File vdsm/storage/securable.py:

Line 21: from functools import wraps
Line 22: 
Line 23: OVERRIDE_ARG = "__securityOverride"
Line 24: SECURE_FIELD = "__secured__"
Line 25: SECURE_METHOD_NAME = "__is_secure__"
> The distinction is not important; and the solution is more elegant with one

__is_secure__ is part of the API of the decorator. __secured__ is an implementation detail of how it works. The distinction makes the implementation less confusing.
Line 26: 
Line 27: 
Line 28: class SecureError(RuntimeError):
Line 29:     pass