cmd/jujud: Ensure valid agent.conf after upgrade

cmd/jujud: Ensure valid agent.conf after upgrade

This fixes the agent.conf files for the machine and unit agents after upgrading
from 1.10 to 1.11. I ran into a few differences, the basics are:

1) Machine Agent: machine 0 always starts the state worker, when
upgrading it has an APIInfo section, but it 
  1a) Didn't have Machine.SetPassword in the DB set so we didn't have a
  way to connect via the api
  1b) Didn't have a password set in apiinfo, and 1.10 had already
  changed the password after connecting to the MongoDB
  1c) Had changed their password already

So to solve this one, we grab the Password from StateInfo.Password,
call Agent.SetPassword(), and set the value in APIInfo.Password.

2) Machine Agent: machine != 0. Were only connecting to the state after
connecting to the API. Because of (1a,b) they couldn't connect to
notice that they had work to do.

So on top of what we did for (1), I added a step of:

  2a) If we are unable to connect to the API, call ensureStateWorker().
  This gives us an opportunity to set our API password and save it in
  agent.conf.

3) Unit agents: 

  3a) Didn't even have an APIInfo section.
  3b) Did have a Mongo password, and had reset their Mongo Password

So we copy the state info into the api info section, and change the
port. I only use the default port (17070). I think it would be possible
to query something in state for what the official API is
(state.State.Environ().APIInfo() comes to mind, but I'm not sure what
the correct value is.)

I felt that changing the port would solve all current 1.10 users, and
if we change things in the future, we can change the code. (I also just
discovered that we have Conf.APIPort which may or may not be set in all
the various permutations, I don't have the energy to track down 3
different versions now.)

4) Unit agents bootstrapped with 1.11.2 tools

  4a) Would have a valid APIInfo section
  4b) But would not have StateInfo.Password or APIInfo.Password set.
  This is because changing the Password now occurs when we connect to
  the API, not when we first connect to the Mongo DB.

I added checks for this, to make sure that 1.11.3 doesn't cause us to
call Unit.SetPassword("").


I did a few upgrades from 1.10 => this code, and 1.11.2 => this code,
and straight bootstrapping from this code. It all seems to be up and
running. I don't think I tried bootstrapping 1.10 with this code and
upgrading.

I hopefully captured all of these config edge cases in the test suite,
so we don't have to test all the permutations manually in the future.
We also need to think very carefully when we add stuff about how we
will get upgrade to notice. I'm starting to think we want to tweak how
upgrade works. So instead of just replacing the tools, it has a
"juju upgraded" or some other command that runs the first time you are
upgraded so we can put logic into that place and have it centralized.

https://code.launchpad.net/~jameinel/juju-core/api-set-creds-1199915/+merge/174620

(do not edit description out of merge proposal)

[jameinel, 2013-07-14 16:05:50]

Please take a look.

[mue, 2013-07-15 13:31:42]

LGTM, only few remarks.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go
File cmd/jujud/upgradevalidation.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go...
cmd/jujud/upgradevalidation.go:91: //???
We should at least log the error (which should not happen).

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go...
cmd/jujud/upgradevalidation.go:134: // This is unexpected as all AgentState
objects (Machine and Unit)
Isn't this worth a panic()? How does the system continue with this error?

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_te...
File cmd/jujud/upgradevalidation_test.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_te...
cmd/jujud/upgradevalidation_test.go:47: func (s *UpgradeValidationMachineSuite)
Create1_10Machine(c *gc.C) (*state.Machine, *agent.Conf) {
Needed a few seconds to interpret 1_10 right. ;) How about Create1Dot10Machine?

[fwereade, 2013-07-15 13:53:51]

Looking very nice, thanks for this; I have a couple of questions.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/machine.go#newcode145
cmd/jujud/machine.go:145: // TODO: Once we can reliably trust that we have API
passwords
Give this a TODO(jam), it's nice to be able to know who to ask even when blame
info is lost through unrelated changes.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:73: err = conf.Write()
thanks, good catch

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go
File cmd/jujud/upgradevalidation.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go...
cmd/jujud/upgradevalidation.go:110: }
Can you use State.APIAddresses for this? Or something like that... defined in
state/open.go I think.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go...
cmd/jujud/upgradevalidation.go:134: // This is unexpected as all AgentState
objects (Machine and Unit)
On 2013/07/15 13:31:43, mue wrote:
> Isn't this worth a panic()? How does the system continue with this error?

Yeah, I think this is a panic. Someone completely screwed up, do not pass go, do
not collect $200.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go...
cmd/jujud/upgradevalidation.go:165: if err := setter.SetPassword(password); err
!= nil {
Hmm. The original idea IIRC was that we save the oldPassword (for recovery
purposes), write the new password to disk, and then really set the new password.
Not quite sure this mechanism and that one are properly joined-up?

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_te...
File cmd/jujud/upgradevalidation_test.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_te...
cmd/jujud/upgradevalidation_test.go:47: func (s *UpgradeValidationMachineSuite)
Create1_10Machine(c *gc.C) (*state.Machine, *agent.Conf) {
On 2013/07/15 13:31:43, mue wrote:
> Needed a few seconds to interpret 1_10 right. ;) How about
Create1Dot10Machine?

-1 myself, but happy to follow consensus

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_te...
cmd/jujud/upgradevalidation_test.go:226: func (s *UpgradeValidationUnitSuite)
TestEnsureAPIInfo111(c *gc.C) {
1_11 for consistency with the above?

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_te...
cmd/jujud/upgradevalidation_test.go:247: func (s *UpgradeValidationUnitSuite)
TestEnsureAPIInfo111Noop(c *gc.C) {
ditto

[jameinel, 2013-07-15 13:57:52]

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go
File cmd/jujud/upgradevalidation.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go...
cmd/jujud/upgradevalidation.go:165: if err := setter.SetPassword(password); err
!= nil {
On 2013/07/15 13:53:51, fwereade wrote:
> Hmm. The original idea IIRC was that we save the oldPassword (for recovery
> purposes), write the new password to disk, and then really set the new
password.
> Not quite sure this mechanism and that one are properly joined-up?

We already have OldPassword available (or StateInfo.Password). We use the fact
that APIInfo.Password isn't set yet to indicate we need to call
state.State.SetPassword().

If we write to disk first, and then crash, the next startup won't call
SetPassword.

I did think carefully about it and had a comment, but if you feel it needs more
clarification I can try to do so.

[jameinel, 2013-07-16 07:41:09]

Please take a look.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/machine.go#newcode145
cmd/jujud/machine.go:145: // TODO: Once we can reliably trust that we have API
passwords
On 2013/07/15 13:53:51, fwereade wrote:
> Give this a TODO(jam), it's nice to be able to know who to ask even when blame
> info is lost through unrelated changes.

Done.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go
File cmd/jujud/upgradevalidation.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go...
cmd/jujud/upgradevalidation.go:110: }
On 2013/07/15 13:53:51, fwereade wrote:
> Can you use State.APIAddresses for this? Or something like that... defined in
> state/open.go I think.

I don't have easy access to a raw state.State object here (I currently take
something like state.Machine or state.Unit)
Looking at the implementation of state.APIAddresses it is just doing what I'm
doing only without "net". Explicitly searching for ":" and splitting on it.

So I will pull in conf.APIPort(), but I'd rather avoid reworking what objects
are in play just to get a function that may or may not properly handle ipv6 in
the future.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation.go...
cmd/jujud/upgradevalidation.go:134: // This is unexpected as all AgentState
objects (Machine and Unit)
On 2013/07/15 13:53:51, fwereade wrote:
> On 2013/07/15 13:31:43, mue wrote:
> > Isn't this worth a panic()? How does the system continue with this error?
> 
> Yeah, I think this is a panic. Someone completely screwed up, do not pass go,
do
> not collect $200.

Done.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_te...
File cmd/jujud/upgradevalidation_test.go (right):

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_te...
cmd/jujud/upgradevalidation_test.go:47: func (s *UpgradeValidationMachineSuite)
Create1_10Machine(c *gc.C) (*state.Machine, *agent.Conf) {
On 2013/07/15 13:53:51, fwereade wrote:
> On 2013/07/15 13:31:43, mue wrote:
> > Needed a few seconds to interpret 1_10 right. ;) How about
> Create1Dot10Machine?
> 
> -1 myself, but happy to follow consensus

I kept it as 1_10 and matched the 111 to 1_11 later in the file.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_te...
cmd/jujud/upgradevalidation_test.go:226: func (s *UpgradeValidationUnitSuite)
TestEnsureAPIInfo111(c *gc.C) {
On 2013/07/15 13:53:51, fwereade wrote:
> 1_11 for consistency with the above?

Done.

https://codereview.appspot.com/11137044/diff/1/cmd/jujud/upgradevalidation_te...
cmd/jujud/upgradevalidation_test.go:247: func (s *UpgradeValidationUnitSuite)
TestEnsureAPIInfo111Noop(c *gc.C) {
On 2013/07/15 13:53:51, fwereade wrote:
> ditto

Done.

[fwereade, 2013-07-16 12:52:16]

LGTM, but please do a little bit of investigation to see if there's a reaosnably
convenient way to reuse the state.APIAddresses code. If not I can live with it
assuming we manage to get rid of this workaround sooner rather than later -- and
I can't see why we *would* keep it around for a long time, so I'm happy
accepting your judgment here.

[jameinel, 2013-07-16 13:42:49]

Please take a look.

[jameinel, 2013-07-17 06:15:10]

On 2013/07/16 13:42:49, jameinel wrote:
> Please take a look.

I did end up doing state.APIAddresses.