Greg Padgett has posted comments on this change. Change subject: API: setHaMaintenanceMode command ......................................................................
Patch Set 3: (4 comments) Yaniv, thanks for the review--see replies inline. http://gerrit.ovirt.org/#/c/23264/3//COMMIT_MSG Commit Message: Line 6: Line 7: API: setHaMaintenanceMode command Line 8: Line 9: New API for setting hosted engine maintenance mode. Line 10: > Here is the wiki page: Done Line 11: Change-Id: Ic08c5edb0e9b8cc11eb70ef6a66301335c42aad3 Line 12: Bug-Url: https://bugzilla.redhat.com/1053040 http://gerrit.ovirt.org/#/c/23264/3/client/vdsClient.py File client/vdsClient.py: Line 1446: Line 1447: def do_setHaMaintenanceMode(self, args): Line 1448: mode = args[0] Line 1449: enabled = utils.tobool(args[1]) Line 1450: stats = self.s.setHaMaintenanceMode(mode, enabled) > consider: Assert seems like a good addition, done. Naming the arguments looks like the convention in this file (i.e. do_changeCD, do_destroy, monitorCommand, etc), but there are a few exceptions (createStorageDomain is one). We should standardize on a method and document it. IMO naming them makes the code easier to follow. Line 1451: if stats['status']['code']: Line 1452: return stats['status']['code'], stats['status']['message'] Line 1453: return 0, '' Line 1454: Line 2354: 'tuning')), Line 2355: 'setHaMaintenanceMode': (serv.do_setHaMaintenanceMode, Line 2356: ('<type> <enabled>', Line 2357: 'Set "global" or "local" Hosted Engine HA' Line 2358: ' maintenance')), > global local is the type? and enables needs to be True\False or 0\1 or yes\ That is more clear, done. Line 2359: 'deleteImage': (serv.deleteImage, Line 2360: ('<sdUUID> <spUUID> <imgUUID> [<postZero>] [<force>]', Line 2361: 'Delete Image folder with all volumes.', Line 2362: )), http://gerrit.ovirt.org/#/c/23264/3/vdsm/API.py File vdsm/API.py: Line 1505: return errCode['haErr'] Line 1506: Line 1507: try: Line 1508: haClient.HAClient().set_maintenance_mode(mm, enabled) Line 1509: except: > can you be more specific with the catch ? what exceptions can be raised fro Could be one of numerous exceptions considering what the ha client library does. The logger.exception() call should provide a nice hint to go on. (Anyway, this was similar to the setMOMPolicyParameters api.) Line 1510: self.log.exception("error setting HA maintenance mode") Line 1511: return errCode['haErr'] Line 1512: return {'status': doneCode} Line 1513: -- To view, visit http://gerrit.ovirt.org/23264 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic08c5edb0e9b8cc11eb70ef6a66301335c42aad3 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Greg Padgett <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Greg Padgett <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[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
