Nir Soffer has posted comments on this change. Change subject: fencing: Introduce getHostLeaseStatus API ......................................................................
Patch Set 5: (3 comments) http://gerrit.ovirt.org/#/c/28873/5/tests/testrunner.py File tests/testrunner.py: Line 53 Line 54 Line 55 Line 56 Line 57 > I don't see any test code referring to this. What's the use of this mock? The clustlerlock module define constants using sanlock constants, so using object() breaks this code during the tests. http://gerrit.ovirt.org/#/c/28873/5/vdsm/storage/clusterlock.py File vdsm/storage/clusterlock.py: Line 41: SDM_LEASE_NAME = 'SDM' Line 42: SDM_LEASE_OFFSET = 512 * 2048 Line 43: Line 44: # Host status codes Line 45: HOST_UNKNOWN = sanlock.HOST_UNKNOWN > I suggest using strings instead of numbers. Translate the sanlock constants This is indeed nicer for the log and for the engine, but for code inside vdsm that uses these statuses, it is better that code will use a constant instead of string literals. So we can do: HOST_STATUS = {sanlock.HOST_UNKNOWN: HOST_UNKNOWN, ...} HOST_UNKNOWN = "unknown" And in getHostStatus: return HOST_STATUS[sanlock_status] Waiting for others review with this. Line 46: HOST_FREE = sanlock.HOST_FREE Line 47: HOST_LIVE = sanlock.HOST_LIVE Line 48: HOST_FAIL = sanlock.HOST_FAIL Line 49: HOST_DEAD = sanlock.HOST_DEAD Line 56: class HostStatusNotAvailable(Exception): Line 57: """ Line 58: Raised when host status is not available because the cluster lock does not Line 59: support this operation, or raised an exception. Line 60: """ > supposedly, you are trying to hide sanlock from outside users by replacing Users of this class should not depend on sanlock, unlike os module. Think about the client code using this: import sanlock import foolock import barlock try: return dommain.getHostStatus() except (sanlock.SanlockException, foolock.FooLockException, barlock.BarLockException): ... We don't want to go there. clusterlock.py is like os.py - it returns common errors for multiple clusterlocks and os return common errors for multiple os (e.g. posix, nt). Line 61: Line 62: Line 63: class SafeLease(object): Line 64: log = logging.getLogger("Storage.SafeLease") -- To view, visit http://gerrit.ovirt.org/28873 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iccd62e58a194aa0ceb0f5e2503b8ec7e4349971b Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Itamar Heim <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Xavi Francisco <[email protected]> Gerrit-Reviewer: Yoav Kleinberger <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
