Change in vdsm[master]: fencing: Skip fencing if host is connected to storage
nsoffer at redhat.com
nsoffer at redhat.com
Fri Aug 1 21:35:46 UTC 2014
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 <mperina at redhat.com>
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 <mperina at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Martin Peřina <mperina at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list