upgrades: Local provider 1.16 to 1.18 migration

upgrades: Local provider 1.16 to 1.18 migration

This adds a couple of utility methods:
- agent.MigrateConfig() - allows to atomically
change and save an existing agent config (only DataDir,
LogDir, Jobs or Values);
- agent.PatchConfig (only for tests) - to facilitate
testing of the above.

Then, we add an upgrade step for migrating existing
1.16 local environments to 1.18, including:
- agent config (DataDir, LogDir, Jobs, Values)
- rsyslog configuration and setup
- environment config (container and namespace settings)
- removing deprecated stuff (shared-storage dir and the
related settings in the agent config).

In addition, a few changes and drive-by fixes were done
to related packages.

Tested extensively live using a 1.16 local environment
upgrading to 1.18.

https://code.launchpad.net/~dimitern/juju-core/340-lp-1291400-1.16-to-1.18-agent-config-migration/+merge/211560

(do not edit description out of merge proposal)

[dimitern, 2014-03-18 15:46:02]

Please take a look.

[fwereade, 2014-03-19 09:36:26]

The actual migration looks great, but I have a few issues; let me know what you
think.

https://codereview.appspot.com/77340044/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode135
agent/agent.go:135: configMutex.Lock()
This locking still seems a bit off-kilter -- for it to really make sense we
essentially need the config to be a singleton (AIUI we coincidentally only
happen to create one, but that's not really the same...).

If it's documented then I can put up with it, though: progress not perfection...

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode156
agent/agent.go:156: }
I'm a bit worried that I can't see the utility of changing most of these
fields... I'd kinda rather write the code for changing them only when we have a
use case.

I suspect this is down to reusing the AgentConfigParams type when we really
ought to have a new type for this case?

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode158
agent/agent.go:158: config.caCert = newParams.CACert[:]
This doesn't do what you think: http://play.golang.org/p/I314cvnMQC -- use
copy() instead

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode164
agent/agent.go:164: config.stateDetails.addresses = newParams.StateAddresses[:]
ditto

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode170
agent/agent.go:170: config.apiDetails.addresses = newParams.APIAddresses[:]
ditto

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode176
agent/agent.go:176: if key == "_DELETE_" {
could we maybe just have a separate field for values to delete?

https://codereview.appspot.com/77340044/diff/1/upgrades/agentconfig.go
File upgrades/agentconfig.go (right):

https://codereview.appspot.com/77340044/diff/1/upgrades/agentconfig.go#newcode46
upgrades/agentconfig.go:46: logger.Debugf("no state connection, skipping 1.16 to
1.18 migration")
"no migration required" instead of the "skipping" text maybe?

https://codereview.appspot.com/77340044/diff/1/upgrades/agentconfig.go#newcode55
upgrades/agentconfig.go:55: logger.Debugf("not a local environment, skipping
1.16 to 1.18 migration")
ditto

[dimitern, 2014-03-19 11:16:28]

Please take a look.

https://codereview.appspot.com/77340044/diff/1/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode135
agent/agent.go:135: configMutex.Lock()
On 2014/03/19 09:36:26, fwereade wrote:
> This locking still seems a bit off-kilter -- for it to really make sense we
> essentially need the config to be a singleton (AIUI we coincidentally only
> happen to create one, but that's not really the same...).
> 
> If it's documented then I can put up with it, though: progress not
perfection...

I'll update the doc comment for configMutex to explain the quasi-singleton
approach.

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode156
agent/agent.go:156: }
On 2014/03/19 09:36:26, fwereade wrote:
> I'm a bit worried that I can't see the utility of changing most of these
> fields... I'd kinda rather write the code for changing them only when we have
a
> use case.
> 
> I suspect this is down to reusing the AgentConfigParams type when we really
> ought to have a new type for this case?

As discussed online, I'll create a new params struct with only the needed things
- DataDir, LogDir, Jobs, and Values (and drop the possibility to change the rest
until we need them).

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode158
agent/agent.go:158: config.caCert = newParams.CACert[:]
On 2014/03/19 09:36:26, fwereade wrote:
> This doesn't do what you think: http://play.golang.org/p/I314cvnMQC -- use
> copy() instead

Hah.. Python habits :) Good to know, although this is now removed.

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode164
agent/agent.go:164: config.stateDetails.addresses = newParams.StateAddresses[:]
On 2014/03/19 09:36:26, fwereade wrote:
> ditto

Removed.

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode170
agent/agent.go:170: config.apiDetails.addresses = newParams.APIAddresses[:]
On 2014/03/19 09:36:26, fwereade wrote:
> ditto

Removed.

https://codereview.appspot.com/77340044/diff/1/agent/agent.go#newcode176
agent/agent.go:176: if key == "_DELETE_" {
On 2014/03/19 09:36:26, fwereade wrote:
> could we maybe just have a separate field for values to delete?

That's what the _DELETE_ key is for, but as discussed online, I'll add a
DeleteValues field in the new params struct.

https://codereview.appspot.com/77340044/diff/1/upgrades/agentconfig.go
File upgrades/agentconfig.go (right):

https://codereview.appspot.com/77340044/diff/1/upgrades/agentconfig.go#newcode46
upgrades/agentconfig.go:46: logger.Debugf("no state connection, skipping 1.16 to
1.18 migration")
On 2014/03/19 09:36:26, fwereade wrote:
> "no migration required" instead of the "skipping" text maybe?

Done.

https://codereview.appspot.com/77340044/diff/1/upgrades/agentconfig.go#newcode55
upgrades/agentconfig.go:55: logger.Debugf("not a local environment, skipping
1.16 to 1.18 migration")
On 2014/03/19 09:36:26, fwereade wrote:
> ditto

Done.

[fwereade, 2014-03-19 11:38:29]

LGTM with a doc tweak, assuming you've done a live upgrade with the new code :).

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

https://codereview.appspot.com/77340044/diff/20001/agent/agent.go#newcode129
agent/agent.go:129: // in a MigrateConfig call. DeleteValues specifies a list of
keys to
...holds agent config values to change... Empty values will be ignored.

[dimitern, 2014-03-19 11:48:05]

Please take a look.

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

https://codereview.appspot.com/77340044/diff/20001/agent/agent.go#newcode129
agent/agent.go:129: // in a MigrateConfig call. DeleteValues specifies a list of
keys to
On 2014/03/19 11:38:30, fwereade wrote:
> ...holds agent config values to change... Empty values will be ignored.

Done.