[node-patches] Change in ovirt-node[master]: add ovirt-upgrade-tool to perform online upgrades with new c...

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: add ovirt-upgrade-tool to perform online upgrades with new codebase
......................................................................


Patch Set 3: (7 inline comments)

A couple of more comments.

....................................................
File scripts/ovirt-upgrade-tool.py
Line 29: import optparse
Line 30: import time
Line 31: import select
Line 32: 
Line 33: def system(command):
What about using ovirt.node.utils.process.system? and improving it if necessary.
Line 34:     system_cmd = subprocess.Popen(command, shell=True, stdout=PIPE, stderr=PIPE)
Line 35:     output, err = system_cmd.communicate()
Line 36:     logger.propagate = False
Line 37:     logger.debug(command)


Line 42:         return False
Line 43: 
Line 44: def fail(msg):
Line 45:     print "%s" % msg
Line 46:     logger.error(msg)
The print statement shouldn't be necessary, as errors are written to stderr
Line 47:     sys.exit(1)
Line 48: 
Line 49: class UpgradeTool():
Line 50: 


Line 43: 
Line 44: def fail(msg):
Line 45:     print "%s" % msg
Line 46:     logger.error(msg)
Line 47:     sys.exit(1)
As Alon also noted, it would be nice if we could work with exceptions, rather than exit and returns - The exceptions could then be handled in the main() function or even on the top-level (where main() is called) - and exit() could be called there.
Line 48: 
Line 49: class UpgradeTool():
Line 50: 
Line 51:     def __init__(self):


Line 63:            %prog
Line 64:            [--reboot]
Line 65:            [--no-existing-hooks]
Line 66:            [--iso]
Line 67:            """)
OptionParser is constructing the help by default, so I don't see the need to maintain this ourselves. Or is there a specific reason?
Line 68: 
Line 69:         parser.add_option("--reboot", type="int", default="10", dest="reboot",
Line 70:                           help="Amount of time before reboot after upgrade")
Line 71: 


Line 80: 
Line 81:     def run_hooks(self, stage):
Line 82:         hooks_path = os.path.join(self.hooks_path, stage)
Line 83:         if os.path.exists(hooks_path):
Line 84:             print "Running %s hooks" % stage
What about using logger.info(…), this would also log the info (if requested) to a file.
I'd use the logger instance everywhere.
Line 85:             for i in os.listdir(hooks_path):
Line 86:                 hook = os.path.join(hooks_path, i)
Line 87:                 print "Running: %s" % i
Line 88:                 if not system(hook):


Line 83:         if os.path.exists(hooks_path):
Line 84:             print "Running %s hooks" % stage
Line 85:             for i in os.listdir(hooks_path):
Line 86:                 hook = os.path.join(hooks_path, i)
Line 87:                 print "Running: %s" % i
As above.
Line 88:                 if not system(hook):
Line 89:                     print "%s hook failed: %s" % (stage, hook)
Line 90:                     return False
Line 91:             print "%s hooks completed" % stage


Line 182:             else:
Line 183:                 fail("Rollback Failed")
Line 184:         if not u.cleanup():
Line 185:             fail("Cleanup Failed")
Line 186:         print "Pausing %s secs for reboot" % options.reboot
Is the reboot always initiated? I thought it is only initiated if --reboot is given.
Line 187:         time.sleep(options.reboot)
Line 188:         system("/sbin/reboot")
Line 189: 
Line 190: # setup logging facility


--
To view, visit http://gerrit.ovirt.org/13264
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I238b49b0a85d910d9db2c2b41e222afcce6b99c5
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Joey Boggs <jboggs at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: Joey Boggs <jboggs at redhat.com>