agent: implement InitializeState

agent: implement InitializeState

We combine two existing functions to make
one that hopefully simplifies things a little.

The previous logic in cmd/jujud/bootstrap.go used two agent.Configs
(the bootstrap config for the state connection
info, and the machine config to save the password);
we change the logic to use the machine agent's
configuration only, leaving the bootstrap config
for backward compatibility only.

https://code.launchpad.net/~rogpeppe/juju-core/442-agent-initialize-state/+merge/189384

(do not edit description out of merge proposal)

[rog, 2013-10-07 18:08:49]

Please take a look.

[rog, 2013-10-08 09:42:53]

Please take a look.

[mue, 2013-10-08 10:00:23]

LGTM, only minor comments.

https://codereview.appspot.com/14395043/diff/6001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/14395043/diff/6001/agent/agent.go#newcode204
agent/agent.go:204: config0, err := NewAgentConfig(params.AgentConfigParams)
Only a matter of personal taste: I dislike the 0 postfix. I would expect
NewAgentConfig() to return an agentConfig. A bit more to type but IMHO more
readable.

https://codereview.appspot.com/14395043/diff/6001/agent/agent.go#newcode344
agent/agent.go:344: func (c *configInternal) writeNewPassword() (string, error)
{
That name is better, or writeGeneratedPassword().

https://codereview.appspot.com/14395043/diff/6001/agent/bootstrap.go
File agent/bootstrap.go (right):

https://codereview.appspot.com/14395043/diff/6001/agent/bootstrap.go#newcode128
agent/bootstrap.go:128: return fmt.Errorf("bootstrap machine expected id 0, got
%q", m.Id())
The test is against a const, the error message contains the id as text. So
changing the id const would lead to a wrong error message. I would use the const
here too.

https://codereview.appspot.com/14395043/diff/6001/provider/local/environ.go
File provider/local/environ.go (left):

https://codereview.appspot.com/14395043/diff/6001/provider/local/environ.go#o...
provider/local/environ.go:43: const boostrapInstanceId = "localhost"
Oh, good catch.

[fwereade, 2013-10-08 10:31:02]

Looking good, but a few comments/questions

https://codereview.appspot.com/14395043/diff/9001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/14395043/diff/9001/agent/agent.go#newcode144
agent/agent.go:144: // we should remove the state addresses from here.
API server is always going to need state addresses, isn't it?

https://codereview.appspot.com/14395043/diff/9001/agent/bootstrap.go
File agent/bootstrap.go (right):

https://codereview.appspot.com/14395043/diff/9001/agent/bootstrap.go#newcode86
agent/bootstrap.go:86: Constraints:             cfg.Constraints,
You sure we want this? It's pretty misleading.

https://codereview.appspot.com/14395043/diff/9001/agent/bootstrap.go#newcode126
agent/bootstrap.go:126: func (c *configInternal) initBootstrapMachine(m
*state.Machine, cfg BootstrapMachineConfig) error {
Seems a bit odd that we don't have the InjectMachine call in here.

https://codereview.appspot.com/14395043/diff/9001/agent/bootstrap_test.go
File agent/bootstrap_test.go (right):

https://codereview.appspot.com/14395043/diff/9001/agent/bootstrap_test.go#new...
agent/bootstrap_test.go:142: c.Assert(err, gc.IsNil)
Check, unless you want st hanging around

https://codereview.appspot.com/14395043/diff/9001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/14395043/diff/9001/constraints/constraints.go#...
constraints/constraints.go:210: return fmt.Errorf("bad %s constraint %q: %v",
name, str, err)
names are usually quoted here, it seems; please be consistent.

And... I don't see a test change. Did we really completely miss this code path?

https://codereview.appspot.com/14395043/diff/9001/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/14395043/diff/9001/provider/local/environ.go#n...
provider/local/environ.go:132: // TODO(rog) how does this comment pertain to the
statement below?
Don't think it does. It should be, I guess, wherever we set a localhost address
in the first place.

[rog, 2013-10-09 09:18:09]

Please take a look.

https://codereview.appspot.com/14395043/diff/9001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/14395043/diff/9001/agent/agent.go#newcode144
agent/agent.go:144: // we should remove the state addresses from here.
On 2013/10/08 10:31:03, fwereade wrote:
> API server is always going to need state addresses, isn't it?

Yeah - I just made minor adjustments to the comment, but
it's invalid.

https://codereview.appspot.com/14395043/diff/9001/agent/bootstrap.go
File agent/bootstrap.go (right):

https://codereview.appspot.com/14395043/diff/9001/agent/bootstrap.go#newcode86
agent/bootstrap.go:86: Constraints:             cfg.Constraints,
On 2013/10/08 10:31:03, fwereade wrote:
> You sure we want this? It's pretty misleading.

Good question - it's here because this CL was about refactoring
not changing semantics, but if you think the bootstrap machine
shouldn't inherit the environment's constraints, I'll remove
it. What do you think?

https://codereview.appspot.com/14395043/diff/9001/agent/bootstrap.go#newcode126
agent/bootstrap.go:126: func (c *configInternal) initBootstrapMachine(m
*state.Machine, cfg BootstrapMachineConfig) error {
On 2013/10/08 10:31:03, fwereade wrote:
> Seems a bit odd that we don't have the InjectMachine call in here.

It started off like that, then I added some stuff to clean
up the bootstrap machine if initBootstrapMachine fails;
then removed that because I couldn't test that logic or think
of a scenario where it would happen (or matter) in practice.

I'll revert it.

https://codereview.appspot.com/14395043/diff/9001/agent/bootstrap_test.go
File agent/bootstrap_test.go (right):

https://codereview.appspot.com/14395043/diff/9001/agent/bootstrap_test.go#new...
agent/bootstrap_test.go:142: c.Assert(err, gc.IsNil)
On 2013/10/08 10:31:03, fwereade wrote:
> Check, unless you want st hanging around

Done.

https://codereview.appspot.com/14395043/diff/9001/constraints/constraints.go
File constraints/constraints.go (right):

https://codereview.appspot.com/14395043/diff/9001/constraints/constraints.go#...
constraints/constraints.go:210: return fmt.Errorf("bad %s constraint %q: %v",
name, str, err)
On 2013/10/08 10:31:03, fwereade wrote:
> names are usually quoted here, it seems; please be consistent.

I don't see any other messages containing known-good
constraint names. The places we quote names are
when we can't understand them, which seems 
reasonable.

In this case though, we know that the name is ok,
so - I think

bad "mem" constraint "1024M": blah blah

isn't as nice as

bad mem constraint "1024M": blah blah

another possibility might be:

bad constraint "mem=1024M": blah blah

which I marginally prefer actually, and changed accordingly.
What do you think?

> And... I don't see a test change. Did we really completely miss this code
path?

No - I missed the failure in the test run output.

https://codereview.appspot.com/14395043/diff/9001/provider/local/environ.go
File provider/local/environ.go (right):

https://codereview.appspot.com/14395043/diff/9001/provider/local/environ.go#n...
provider/local/environ.go:132: // TODO(rog) how does this comment pertain to the
statement below?
On 2013/10/08 10:31:03, fwereade wrote:
> Don't think it does. It should be, I guess, wherever we set a localhost
address
> in the first place.

I've deleted the comment.

[rog, 2013-10-09 09:23:33]

Please take a look.

[rog, 2013-10-09 09:26:54]

Please take a look.

[fwereade, 2013-10-09 10:04:06]

LGTM with the below. If you want to argue about constraint error strings, let's
keep them to a separate CL.

https://codereview.appspot.com/14395043/diff/13001/agent/bootstrap.go
File agent/bootstrap.go (right):

https://codereview.appspot.com/14395043/diff/13001/agent/bootstrap.go#newcode119
agent/bootstrap.go:119: Constraints:             cfg.Constraints,
OK, let's keep it in, it's arguably more consistent. It won't be used... but it
is what *was* used.

https://codereview.appspot.com/14395043/diff/13001/constraints/constraints_te...
File constraints/constraints_test.go (right):

https://codereview.appspot.com/14395043/diff/13001/constraints/constraints_te...
constraints/constraints_test.go:103: err:     `bad constraint "arch=": already
set`,
Eh, actually, I don't like this. It introduces stuttering in one case,and
irrelevant info in the other. Please just restore what we had originally.

[rog, 2013-10-09 12:36:27]

Please take a look.

https://codereview.appspot.com/14395043/diff/13001/constraints/constraints_te...
File constraints/constraints_test.go (right):

https://codereview.appspot.com/14395043/diff/13001/constraints/constraints_te...
constraints/constraints_test.go:103: err:     `bad constraint "arch=": already
set`,
On 2013/10/09 10:04:06, fwereade wrote:
> Eh, actually, I don't like this. It introduces stuttering in one case,and
> irrelevant info in the other. Please just restore what we had originally.

The reason I was making this change is that I was confused by
the error message when using it for real in one case (AFAIR
I used 1024m rather than 1024M) and wanted to see the
actual value.

The value is not necessarily irrelevant in the "already set"
case - the value might give the user context and let them
know where that constraint value is coming from.

We could lose the value from all the subsidiary errors - then we'd always show
all the relevant data without stutter.

I'll do that, if that's ok, but will leave it to another CL.