agent: safer and more flexible

agent: safer and more flexible

For the machine agent, we need a way of safely updating
the agent configuration from multiple places.
The singleton approach wasn't cutting it, and isn't
very Go like (communicating by sharing memory n' all).

So we change agent.Config to split out setting
and writing methods, so that the majority of places
that use it can use read-only methods.

We add a Clone method to make it possible to pass
around a read-only copy of a mutable config.

We also factor out some methods that are really behavioural
and not directly to do with Config (except that they use information
from the Config). This means that any config changes
they make can be made under control of external
code.

https://code.launchpad.net/~rogpeppe/juju-core/533-agent-config-safety/+merge/213134

(do not edit description out of merge proposal)

[rog, 2014-03-27 20:20:33]

Please take a look.

[fwereade, 2014-03-27 21:20:32]

Looking very nice indeed, but a few comments follow:

https://codereview.appspot.com/81570043/diff/20001/agent/agent_test.go
File agent/agent_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/agent/agent_test.go#newcod...
agent/agent_test.go:411: c.Assert(conf.StateInfo(), gc.DeepEquals,
expectStateInfo)
jc.DeepEquals? here and elsewhere

https://codereview.appspot.com/81570043/diff/20001/agent/bootstrap.go
File agent/bootstrap.go (right):

https://codereview.appspot.com/81570043/diff/20001/agent/bootstrap.go#newcode153
agent/bootstrap.go:153: newPassword, err := utils.RandomPassword()
Don't we have to write the new password locally first, lest we fail after
setting it but before recording it, and lock ourselves out?

I guess this is somewhat academic, though, as I don't think we retry bootstrap
failures at all anyway.

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent.go#newcode185
cmd/jujud/agent.go:185: // called if the password changes to set the password.
doc needs updating, I think

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent_test.go
File cmd/jujud/agent_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent_test.go#ne...
cmd/jujud/agent_test.go:332: //}
?

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

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:522: return nil
return err? seems like a test may be missing

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/unit_test.go
File cmd/jujud/unit_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/unit_test.go#new...
cmd/jujud/unit_test.go:210: func (f *fakeUnitAgent)
ChangeConfig(func(agent.ConfigSetter)) error {
I would favour embedding the nil interface for nice instructive panics when code
under test acts in an unexpected way...

[rog, 2014-03-28 08:29:58]

https://codereview.appspot.com/81570043/diff/20001/agent/agent_test.go
File agent/agent_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/agent/agent_test.go#newcod...
agent/agent_test.go:411: c.Assert(conf.StateInfo(), gc.DeepEquals,
expectStateInfo)
On 2014/03/27 21:20:32, fwereade wrote:
> jc.DeepEquals? here and elsewhere

Done.

https://codereview.appspot.com/81570043/diff/20001/agent/bootstrap.go
File agent/bootstrap.go (right):

https://codereview.appspot.com/81570043/diff/20001/agent/bootstrap.go#newcode153
agent/bootstrap.go:153: newPassword, err := utils.RandomPassword()
On 2014/03/27 21:20:32, fwereade wrote:
> Don't we have to write the new password locally first, lest we fail after
> setting it but before recording it, and lock ourselves out?
> 
> I guess this is somewhat academic, though, as I don't think we retry bootstrap
> failures at all anyway.

Yeah, I thought about this case and decided that it wasn't that important. And 
InitializeState wouldn't work if called again anyway, because we connect with an
empty username/password. I think it's reasonable to say that if we create an
agent with a non-writable config file, the instance is borked.

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent.go#newcode185
cmd/jujud/agent.go:185: // called if the password changes to set the password.
On 2014/03/27 21:20:32, fwereade wrote:
> doc needs updating, I think

Done.

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent_test.go
File cmd/jujud/agent_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent_test.go#ne...
cmd/jujud/agent_test.go:332: //}
On 2014/03/27 21:20:32, fwereade wrote:
> ?

it was never called. i will try to work out where it was intended to slot in.

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

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:522: return nil
On 2014/03/27 21:20:32, fwereade wrote:
> return err? seems like a test may be missing

yes, will add a test.

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/unit_test.go
File cmd/jujud/unit_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/unit_test.go#new...
cmd/jujud/unit_test.go:210: func (f *fakeUnitAgent)
ChangeConfig(func(agent.ConfigSetter)) error {
On 2014/03/27 21:20:32, fwereade wrote:
> I would favour embedding the nil interface for nice instructive panics when
code
> under test acts in an unexpected way...

added an explicit panic.

[dimitern, 2014-03-28 08:59:36]

A few comments.

https://codereview.appspot.com/81570043/diff/20001/agent/agent_test.go
File agent/agent_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/agent/agent_test.go#newcod...
agent/agent_test.go:259: c.Assert(agent.ConfigFileExists(newConfig), jc.IsTrue)
Why drop the check that initialConfig's config file was deleted when datadir
changes?

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent_test.go
File cmd/jujud/agent_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent_test.go#ne...
cmd/jujud/agent_test.go:332: //}
On 2014/03/28 08:29:59, rog wrote:
> On 2014/03/27 21:20:32, fwereade wrote:
> > ?
> 
> it was never called. i will try to work out where it was intended to slot in.

Remove it then?

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

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:494: //  p runs the upgrade operations for each job type
and updates the updatedToVersion on success.
p? runUpgrades

https://codereview.appspot.com/81570043/diff/20001/upgrades/agentconfig_test.go
File upgrades/agentconfig_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/upgrades/agentconfig_test....
upgrades/agentconfig_test.go:154: err = s.config.Write()
shouldn't the write actually happen inside the MigrateLocalProviderAgentConfig?
I was like that before, because Migrate calls Write. If it's not like this now
something's wrong..

[rog, 2014-03-28 09:11:06]

https://codereview.appspot.com/81570043/diff/20001/agent/agent_test.go
File agent/agent_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/agent/agent_test.go#newcod...
agent/agent_test.go:259: c.Assert(agent.ConfigFileExists(newConfig), jc.IsTrue)
On 2014/03/28 08:59:36, dimitern wrote:
> Why drop the check that initialConfig's config file was deleted when datadir
> changes?

Because Migrate no longer deletes the old config file (it no longer calls
Write). I reasoned that if you're moving the data directory, there will be a lot
more to delete in the old data directory than just the config file, so it's
reasonable to place that responsibility on whatever is calling Migrate.

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent_test.go
File cmd/jujud/agent_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/agent_test.go#ne...
cmd/jujud/agent_test.go:332: //}
On 2014/03/28 08:59:36, dimitern wrote:
> On 2014/03/28 08:29:59, rog wrote:
> > On 2014/03/27 21:20:32, fwereade wrote:
> > > ?
> > 
> > it was never called. i will try to work out where it was intended to slot
in.
> 
> Remove it then?

I'd prefer to work out a way of making it work as a test, because it looks like
it's testing something useful.

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

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:494: //  p runs the upgrade operations for each job type
and updates the updatedToVersion on success.
On 2014/03/28 08:59:36, dimitern wrote:
> p? runUpgrades

Done.

https://codereview.appspot.com/81570043/diff/20001/upgrades/agentconfig_test.go
File upgrades/agentconfig_test.go (right):

https://codereview.appspot.com/81570043/diff/20001/upgrades/agentconfig_test....
upgrades/agentconfig_test.go:154: err = s.config.Write()
On 2014/03/28 08:59:36, dimitern wrote:
> shouldn't the write actually happen inside the
MigrateLocalProviderAgentConfig?
> I was like that before, because Migrate calls Write. If it's not like this now
> something's wrong..

No, MigreateLocalProviderAgentConfig no longer writes the config, because config
writing is now entirely under the control of the cmd/jujud code. That's why
MigrateLocalProviderAgentConfig takes a ConfigSetter, not a ConfigSetterWriter.

[dimitern, 2014-03-28 09:28:50]

LGTM, assuming you do a live test upgrading 1.16 to 1.18 and the agent config
gets migrated and saved properly.

[wwitzel3, 2014-03-28 12:34:41]

LGTM with a question/suggestion and +1 dimitern

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

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:313: handler := provisioner.NewContainerSetupHandler(
6 arguments for a public method seems like a lot. I know it was already this
way, but should we maybe refactor it to a struct since we are in here anyway?

[fwereade, 2014-03-28 13:16:34]

changes LGTM modulo:

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

https://codereview.appspot.com/81570043/diff/20001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:313: handler := provisioner.NewContainerSetupHandler(
On 2014/03/28 12:34:41, wwitzel3 wrote:
> 6 arguments for a public method seems like a lot. I know it was already this
> way, but should we maybe refactor it to a struct since we are in here anyway?

+1 to that.

[rog, 2014-03-28 13:26:19]

Please take a look.

[rog, 2014-03-30 15:57:44]

On 2014/03/28 13:26:19, rog wrote:
> Please take a look.

Verified that upgrades work live. Uncovered
https://bugs.launchpad.net/juju-core/+bug/1299802
but since that also happens on trunk, it doesn't appear to be relevant to this
change.