Code review comment for lp:~chris.macnaughton/openstack-mojo-specs/upgrade-spec

Revision history for this message
Chris MacNaughton (chris.macnaughton) wrote :

A couple of things left to discuss:

> The juju series-update commands, when is it safe to set this for an application that may have multiple units?

At the moment, there is no possible config into the actual callable script, although it will only iterate through units, so, depending on the HA model of the hosted application, there shouldn't be service disruption as we will only be upgrading a single unit at a time for now.

> Individual try/except blocks look good. But it would make sense to have a top level try/except that determines success or failure based on raised exceptions in nested tries.

I'm open to working through doing something like this if we decide that it's necessary; however, I'm nervous to make this code perfect, given that the Juju core team has picked up a roadmap item that will reduce our "series_upgrade" function to something along the lines of `juju series-upgrade $machine_id`, at which point we will be removing almost all of the upgrade code. To make the cleanup of that easier, in the last commit I have comment fenced off the upgrade code, although I propose that it could be moved into a new helper alongside the mojo_utils.py for now to ensure isolation.

> Larger question. At what point does it make more sense to create a bash script and scp it to the unit to run. Rather, than piecemeal commands over ssh? That might be a cleaner approach and it would be re-usable outside mojo.

Rather than scp'ing a bash script, I am considering shifting the series of `juju ssh`'d commands into building a bash script that gets rendered (depends entirely on machine numbers, unit names & numbers, etc) and putting that into /tmp to execute this, as it would ease the run time a bit; however, the amount of time spent running these scripts pales in comparison to the runtime of the `do-release-upgrade`, so I'm not sure of the real value, except to improve readability (a good value, but questionable to me given that this code has been written to be deleted).

« Back to merge proposal