Nir Soffer has posted comments on this change. Change subject: fencing: Skip fencing if host is maintaining its lease ......................................................................
Patch Set 3: (4 comments) Need few minor cleanups. http://gerrit.ovirt.org/#/c/30762/3/vdsm/API.py File vdsm/API.py: Line 1177: if result['status']['code'] != 0: Line 1178: self.log.error( Line 1179: "Error getting host lease status, error code '%s'", Line 1180: result['status']['code'] Line 1181: ) Don't use this style in vdsm - keep the closing parenthesis on the same line. self.log.error( "long message that cannot fit in the first line", arg) Line 1182: return True Line 1183: Line 1184: for sd, status in result['domains'].iteritems(): Line 1185: if status == storage.clusterlock.HOST_STATUS_LIVE: Line 1179: "Error getting host lease status, error code '%s'", Line 1180: result['status']['code'] Line 1181: ) Line 1182: return True Line 1183: Please document here what is the meaning of live status: host had access to storage when it became non-responsive. Line 1184: for sd, status in result['domains'].iteritems(): Line 1185: if status == storage.clusterlock.HOST_STATUS_LIVE: Line 1186: self.log.debug( Line 1187: "Host has live lease to '%s'", Line 1184: for sd, status in result['domains'].iteritems(): Line 1185: if status == storage.clusterlock.HOST_STATUS_LIVE: Line 1186: self.log.debug( Line 1187: "Host has live lease to '%s'", Line 1188: sd) Please use the same style used elsewhere in vdsm: self.log.debug("part 1 is too long to fit in one line..." "part 2", arg, arg) Line 1189: return False Line 1190: Line 1191: self.log.debug("Host doesn't have any live lease") Line 1192: return True Line 1201: Line 1202: operationStatus = 'skipped' Line 1203: if action == 'status' or should_fence(policy): Line 1204: operationStatus = 'initiated' Line 1205: self.log.debug("Executing action '%s'", action) Instead of indenting all the code bellow, it would be a smaller patch and more clear to bail out now if the operation should be skipped. if action != 'status' or not should_fence(policy): self.log.debug("Skipping execution of action '%s'", action) return {'status': doneCode, 'operationStatus': 'skipped'} And then the rest of the code is kept unchanged (except the addition of the "operationStatus" key), easier to review and to understand later. Line 1206: Line 1207: script = constants.EXT_FENCE_PREFIX + agent Line 1208: Line 1209: inp = ('agent=fence_%s\nipaddr=%s\nlogin=%s\naction=%s\n' -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Peřina <[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: 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
