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