Nir Soffer has posted comments on this change. Change subject: fencing: Skip fencing if host is connected to storage ......................................................................
Patch Set 2: Code-Review-1 (6 comments) The design looks right, but the original code was in bad shape, and this change make it worse. Please see the comments how this can be improved. http://gerrit.ovirt.org/#/c/30762/2//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-07-28 14:53:23 +0200 Line 4: Commit: Martin Perina <[email protected]> Line 5: CommitDate: 2014-08-01 14:38:09 +0200 Line 6: Line 7: fencing: Skip fencing if host is connected to storage If host is connected to storage -> if host is maintaining its lease We don't know if host is connected or not, we can only assume based on clusterlock info about this host. Line 8: Line 9: 1) Adds fencingPolicy parameter to fenceNode API call. The parameter is Line 10: optional, but it's supposed to be used in oVirt >= 3.5. Line 11: 2) fencingPolicy parameter can contain storageDomainHostIdMap, which Line 9: 1) Adds fencingPolicy parameter to fenceNode API call. The parameter is Line 10: optional, but it's supposed to be used in oVirt >= 3.5. Line 11: 2) fencingPolicy parameter can contain storageDomainHostIdMap, which Line 12: is a map of storage domains and host ids Line 13: 3) If host has live connection to at least one of specified storage If host has live connection to -> If clusterlock reports that host is live on Line 14: domains, the fence agent execution is skipped and host is considered Line 15: alive Line 16: Line 17: Change-Id: I7b1fd9521cebea28d0402e53d46d74b95e73f383 http://gerrit.ovirt.org/#/c/30762/2/vdsm/API.py File vdsm/API.py: Line 1133: APIBase.__init__(self) Line 1134: Line 1135: # General Host functions Line 1136: def fenceNode(self, addr, port, agent, username, password, action, Line 1137: secure=False, options='', fencingPolicy=None): Do we have other types of policy here? No, so name this "policy" Line 1138: """Send a fencing command to a remote node. Line 1139: Line 1140: agent is one of (rsa, ilo, drac5, ipmilan, etc) Line 1141: action can be one of (status, on, off, reboot).""" Line 1159: line = 'passwd=XXXX\n' Line 1160: cleantext += line Line 1161: return cleantext Line 1162: Line 1163: def execute_fence_script(fencingPolicy): This does not execute anything - is is a predicate, and should use name like should_fence(). Line 1164: # skip fence execution if map of storage domains with host id is Line 1165: # entered and at least one storage domain connection from host is Line 1166: # alive Line 1167: execute = True Line 1184: "Error during getHostLeaseStatus: '%s'", Line 1185: result Line 1186: ) Line 1187: Line 1188: return execute This is example how to make the code more complicated to understand or maintain for no reason (single return point is not a reason). Please rewrite this in a more clear way: if fencingPolicy is None: log debug about no policy? return True hostIdMap = fencingPolicy.get('storageDomainHostIdMap') if not hostIdMap: log warning about unsupported policy return True result = self._irs.getHostLeaseStatus(hostIdMap) if result['status']['code'] != 0: log error return True for sd, status in result['domains'].iteritems(): if status == storage.clusterlock.HOST_STATUS_LIVE: log debug why host will not be fenced return False log debug - host is not live on any domain return True Line 1189: Line 1190: self.log.debug('fenceNode(addr=%s,port=%s,agent=%s,user=%s,passwd=%s,' Line 1191: 'action=%s,secure=%s,options=%s,fencingPolicy=%s)', Line 1192: addr, port, agent, username, 'XXXX', action, secure, Line 1198: operationStatus = 'skipped' Line 1199: if ( Line 1200: action == 'status' or Line 1201: execute_fence_script(fencingPolicy) Line 1202: ): With suggested names, this should be now: if action == 'status' or should_fence(policy): ... Line 1203: operationStatus = 'initiated' Line 1204: Line 1205: script = constants.EXT_FENCE_PREFIX + agent Line 1206: -- To view, visit http://gerrit.ovirt.org/30762 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b1fd9521cebea28d0402e53d46d74b95e73f383 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Peřina <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[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
