worker/upgrader: check for tools before getting

worker/upgrader: check for tools before getting

When we moved Upgrader behind the API, we lost one of the checks that
it was doing. Namely, when it saw that an upgrade was requested, it
used to check if we already had that version of the tools available.
This restores that check.

I noticed this while doing scale testing. If you have 1000 units all
trying to upgrade at the same time on a machine, you end up consuming
4.5MB*1000 = 4.5GB of temporary Tar data, plus 20MB*1000=40GB of
'unpacking-' files. Which is more than my VMs have available :).

There is still a race condition, if you have all of the Unit agents
notice at the same time, they will still start trying to download
concurrently. The only thing I could really do there would be to add a
filesystem lock (like uniter-hook-execution). I looked into it, but it
isn't very trivial to set up a fs hook correctly (in uniter.go there
is a lot of code around checking the Tomb to see if we are stopping,
if we held the lock before so we can break it now, etc.) I'd rather
not get the fs locking wrong and end up in a situation where we
deadlock trying to upgrade.

https://code.launchpad.net/~jameinel/juju-core/read-before-download/+merge/193008

(do not edit description out of merge proposal)

[jameinel, 2013-10-29 06:24:04]

Please take a look.

[dimitern, 2013-10-30 13:46:03]

LGTM. At first I thought this fix is not really beneficial except for a very
narrow edge case (hulk-smashing multiple unit agents on the same machine), but
after a discussion with William, I realized we actually have this more often
than not (i.e. 2-3 agents running on the same machine - a machine and a unit
agent, possibly even a subordinate unit agent).

[rog, 2013-10-30 18:03:29]

LGTM with one small thought

https://codereview.appspot.com/19000043/diff/1/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/19000043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:177: if _, err := agenttools.ReadTools(u.dataDir,
agentTools.Version); err == nil {
This could probably be at the start of this function (the "fetching" log message
might be confusing when we're not actually fetching anything)

[jameinel, 2013-10-31 05:26:35]

Please take a look.

[john2, 2013-10-31 05:29:05]

On 2013/10/30 13:46:03, dimitern wrote:
> LGTM. At first I thought this fix is not really beneficial except for a very
> narrow edge case (hulk-smashing multiple unit agents on the same machine), but
> after a discussion with William, I realized we actually have this more often
> than not (i.e. 2-3 agents running on the same machine - a machine and a unit
> agent, possibly even a subordinate unit agent).

Right. We also have a use case where we would stage upgrades by asking the
Machine agent to upgrade first, and then the units, etc. Which would save some
bandwidth downloading the tools multiple times for each machine. But it doesn't
actually help if you always download before checking if the results are going to
be used :).

[john2, 2013-10-31 05:29:22]

https://codereview.appspot.com/19000043/diff/1/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/19000043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:177: if _, err := agenttools.ReadTools(u.dataDir,
agentTools.Version); err == nil {
On 2013/10/30 18:03:30, rog wrote:
> This could probably be at the start of this function (the "fetching" log
message
> might be confusing when we're not actually fetching anything)

Done.