Provisioner gets addresses for each new machine.

Provisioner gets addresses for each new machine.

The addresses are retrieved each time we attempt to start a machine.  This
allows the provisioner to take advantage of any new api or state servers that
are started as part of a block of machines being provisioned at one time.

There is some saving of addresses at this stage to make the existing tests
pass, even though this goes a little against what William has mentioned
before.  If we have invalid config, the tests assume that we continue with old
info, whereas William has suggested that we pause provisioning while the
config is broken.

One behavioural tweak in this branch.  If we do fail to setup authentication,
we no longer kill the task, but instead log and continue.  We will attempt to
start it again next time through the loop.

https://code.launchpad.net/~thumper/juju-core/provisioner-reget-state-info/+merge/168562

(do not edit description out of merge proposal)

[thumper, 2013-06-10 23:14:08]

Please take a look.

[fwereade, 2013-06-11 09:08:21]

Strongly -1 on the clever bits here. Just barf on failure and I'll be happy :).

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_task.go:304: // Don't return a real error, just
try again next time.
This is problematic, because the watcher won't report that machine again until
it changes -- which it won't. For this to work, we need the provisioner to be
restarted regularly... but I'm hoping for a provisioner that can run for weeks
without breaking sweat.

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_task.go:360: // info if config becomes invalid.
Honestly, I think this is sufficient justification to nuke the whole
provisioner; it'll be restarted in a few seconds, and for now I'd rather not
even try to provision a machine (which can be expected to cost our users actual
money) unless I'm as sure as I can be that I'll be able to start a functioning
machine agent...

[fwereade, 2013-06-12 09:10:56]

Re what we do without a valid environment... hmm. Thinking aloud:

* we certainly start off with an invalid environ config, and only get a valid
one once whoever started the environment has connected for the first time.

* today, on first startup, there will be one machine present; but it'll be
marked as provisioned already, so it won't cause env access directly; and if
we're tolerant of list-instances failures, we'll be able to complete the first
processMachines without difficulty. But I don't think we can necessarily depend
on that forever...

* in theory, it is impossible to set an invalid environ config in state, except
via the special mechanism at bootstrap time.

* in practice, an environ config can most certainly become invalid -- you can
(1) replace the config with *any* valid config, which might ofc be invalid in
this context (changed environ type, for example, will screw us most
comprehensively) and (2) config update is racy anyway, such that two users
making valid changes concurrently can end up with an environment config that
reflects neither of their desires... and I bet there's a way for that to cause
invalidity. (oh, and, on inspection, (3): it looks like values can't be
dependably unset anyway. we probably need that too. grar.)

* BUT the consequences of an invalid environ affect many more things than just
the provisioner.

* AND SO it would be folly to spend time and effort hardening the provisioner
against catastrophic environ failure when we could be fixing the root cause.

* AND THUS you should (1) wait for a valid config at startup before processing
machines and (2) proceed otherwise as though the environment configuration were
always guaranteed consistent and valid from a juju perspective and (3) use the
time thereby saved to fix SetEnvironConfig.

* BUT ALSO remember that a valid *environ config* does not necessarily contain
valid *provider credentials*. So issues can always arise at runtime anyway, and
should I think be dealt with as follows:

  * StartInstance: record on machine via SetInstanceError.
  * AllInstances: log and ignore -- I don't think there are negative
consequences (out-of-date
    instance list is always possible anyway).
  * StopInstances: log and ignore, they'll be tried again next time we
processMachines. (We should
    add a periodic processMachines call so that we do at least handle them in
something resembling
    a timely fashion. Not too hard, I think?)

[thumper, 2013-06-17 04:15:59]

Please take a look.

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_...
File worker/provisioner/provisioner_task.go (right):

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_task.go:304: // Don't return a real error, just
try again next time.
On 2013/06/11 09:08:21, fwereade wrote:
> This is problematic, because the watcher won't report that machine again until
> it changes -- which it won't. For this to work, we need the provisioner to be
> restarted regularly... but I'm hoping for a provisioner that can run for weeks
> without breaking sweat.

OK, reverted.

https://codereview.appspot.com/9824047/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_task.go:360: // info if config becomes invalid.
On 2013/06/11 09:08:21, fwereade wrote:
> Honestly, I think this is sufficient justification to nuke the whole
> provisioner; it'll be restarted in a few seconds, and for now I'd rather not
> even try to provision a machine (which can be expected to cost our users
actual
> money) unless I'm as sure as I can be that I'll be able to start a functioning
> machine agent...

OK, nuking in progress.

[fwereade, 2013-06-17 13:46:12]

LGTM

[wallyworld, 2013-06-17 23:02:35]

LGTM