Introduce the format 1.16 for agent confg.

Introduce the format 1.16 for agent confg.

With adding the new format, a migrate method was added
to the formatter interface to provide a defined place
to do additional upgrade data munging as needed.

When an old format config is read, it is immediately
migrated and written out using the current format.

The new format allows storing of key/value pairs. These
are now used instead of OS environment variables for
passing on the provider type, lxc bridge, and local
storage information.

Cloudinit no longer writes upstart scripts with extra
environment variables for the machine agents, but instead
writes the extra variables to the agent config. Due to
this change, the MachineEnvironment param was changed
to AgentEnvironment.

Also the StatePort was never read, so it has been removed
from information that is saved.

The cloudinit tests were modified to be agnostic to the
actual format of the agent.conf files.

Previous review found here:
https://codereview.appspot.com/13421043/

https://code.launchpad.net/~thumper/juju-core/new-agent-format/+merge/183558

Requires: https://code.launchpad.net/~thumper/juju-core/pass-through-agent-config/+merge/183543

(do not edit description out of merge proposal)

[thumper, 2013-09-03 02:14:59]

Please take a look.

[fwereade, 2013-09-05 07:05:53]

ISTM that Configs are passed around a bit casually, and certainly not
goroutine-safely, and that this will be a timebomb even if the usage patterns
today (we only write on startup) make it theoretically safe. Discuss...

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

https://codereview.appspot.com/13481043/diff/1/agent/agent.go#newcode85
agent/agent.go:85: // Set the value for the specified key.
It's nice to do complete sentences starting with the name of the thing being
documented.

And, man, the lack of Get prefixes is starting to bug me. It just feels like
wilful character-saving at the cost of consistency and clarity...

https://codereview.appspot.com/13481043/diff/1/agent/agent.go#newcode97
agent/agent.go:97: // written to disk, the mutex should be locked prior to
generating any disk
prior to generating disk state, or prior to reading the source state from which
the future disk state will be generated?

https://codereview.appspot.com/13481043/diff/1/agent/agent.go#newcode217
agent/agent.go:217: // that this should not be called more than once by an
agent, I feel that
Hmm... how far do we expect these to propagate? Will we be cloning them? If so,
surely writes will never be safe... and if not, we'll need to mutex everything
we do with them. Right?

[thumper, 2013-09-12 02:21:06]

Please take a look.

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

https://codereview.appspot.com/13481043/diff/1/agent/agent.go#newcode85
agent/agent.go:85: // Set the value for the specified key.
On 2013/09/05 07:05:54, fwereade wrote:
> It's nice to do complete sentences starting with the name of the thing being
> documented.
> 
> And, man, the lack of Get prefixes is starting to bug me. It just feels like
> wilful character-saving at the cost of consistency and clarity...

Done.

https://codereview.appspot.com/13481043/diff/1/agent/agent.go#newcode97
agent/agent.go:97: // written to disk, the mutex should be locked prior to
generating any disk
On 2013/09/05 07:05:54, fwereade wrote:
> prior to generating disk state, or prior to reading the source state from
which
> the future disk state will be generated?

Yes, this is what I mean.  Effectively noting that it should be locked at the
start of the method, not just prior to writing.

https://codereview.appspot.com/13481043/diff/1/agent/agent.go#newcode217
agent/agent.go:217: // that this should not be called more than once by an
agent, I feel that
On 2013/09/05 07:05:54, fwereade wrote:
> Hmm... how far do we expect these to propagate? Will we be cloning them? If
so,
> surely writes will never be safe... and if not, we'll need to mutex everything
> we do with them. Right?

We only need to mutex the mutable values, which is the values map.  This is now
done.  There is only one agent config, and it isn't regenerated, only read once,
and now passed around.  With the addition of the mutex around the setting and
getting of the arbitrary values, this is now thread (go routine) safe.

[fwereade, 2013-09-12 10:49:27]

On 2013/09/12 02:21:06, thumper wrote:
> Please take a look.
> 
> https://codereview.appspot.com/13481043/diff/1/agent/agent.go
> File agent/agent.go (right):
> 
> https://codereview.appspot.com/13481043/diff/1/agent/agent.go#newcode85
> agent/agent.go:85: // Set the value for the specified key.
> On 2013/09/05 07:05:54, fwereade wrote:
> > It's nice to do complete sentences starting with the name of the thing being
> > documented.
> > 
> > And, man, the lack of Get prefixes is starting to bug me. It just feels like
> > wilful character-saving at the cost of consistency and clarity...
> 
> Done.
> 
> https://codereview.appspot.com/13481043/diff/1/agent/agent.go#newcode97
> agent/agent.go:97: // written to disk, the mutex should be locked prior to
> generating any disk
> On 2013/09/05 07:05:54, fwereade wrote:
> > prior to generating disk state, or prior to reading the source state from
> which
> > the future disk state will be generated?
> 
> Yes, this is what I mean.  Effectively noting that it should be locked at the
> start of the method, not just prior to writing.

OK, cool. Thanks.

> https://codereview.appspot.com/13481043/diff/1/agent/agent.go#newcode217
> agent/agent.go:217: // that this should not be called more than once by an
> agent, I feel that
> On 2013/09/05 07:05:54, fwereade wrote:
> > Hmm... how far do we expect these to propagate? Will we be cloning them? If
> so,
> > surely writes will never be safe... and if not, we'll need to mutex
everything
> > we do with them. Right?
> 
> We only need to mutex the mutable values, which is the values map.  This is
now
> done.  There is only one agent config, and it isn't regenerated, only read
once,
> and now passed around.  With the addition of the mutex around the setting and
> getting of the arbitrary values, this is now thread (go routine) safe.

Ah, ok. I'm a little concerned about likely future changes in which we need, eg,
an api worker to write out the state info for a machine that's become a manager
node. I guess it's not something we need to worry about yet, but I'm still
worried that it will not be obvious when the time comes that we *do* need to
worry. Will review properly imminently.

[fwereade, 2013-09-12 13:21:44]

LGTM; comments but no blockers.

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

https://codereview.appspot.com/13481043/diff/6001/agent/agent.go#newcode103
agent/agent.go:103: // when mutliple go-routines may be adding things to the
agent config, and
mutliple

https://codereview.appspot.com/13481043/diff/6001/agent/agent.go#newcode111
agent/agent.go:111: // map.  Retrieving and setting values here are protected by
the mutex.
To get the read/write thing right across the board, I think we have to go with a
model whereby you tell the config to persist by passing in a mutate func... but
even then, I remain nervous about the N cached copies of mutually diverging
state.

I'm starting to get the impression that this is maybe a case for the Forbidden
Design Pattern... but I can also appreciate that what we have here is progress
even if it's not perfection. Would you just rewrite the comment above to make
the limits of the current model clear and explicit?

https://codereview.appspot.com/13481043/diff/6001/agent/format-1.16.go
File agent/format-1.16.go (right):

https://codereview.appspot.com/13481043/diff/6001/agent/format-1.16.go#newcod...
agent/format-1.16.go:147: // directory to ".old".
I agree that we should; or possibly that we should be atomically swapping a
symlink. That's surely out of scope, because of backward compatibility if
nothing else, but it deserves its own bug.

https://codereview.appspot.com/13481043/diff/6001/worker/provisioner/provisio...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/13481043/diff/6001/worker/provisioner/provisio...
worker/provisioner/provisioner.go:71: agentConfig: agentConfig,
I feel like we ought to be able to get the id (tag better perhaps?) out of the
agent config.

https://codereview.appspot.com/13481043/diff/6001/worker/provisioner/provisio...
worker/provisioner/provisioner.go:177: tools, err :=
agenttools.ReadTools(dataDir, version.Current)
I feel like most of agenttools should really be methods on agentConfig. YMMV,
not one for this CL, etc.

[rog, 2013-09-12 14:50:58]

A couple of suggestions based on a very quick skim.

https://codereview.appspot.com/13481043/diff/6001/agent/format-1.16.go
File agent/format-1.16.go (right):

https://codereview.appspot.com/13481043/diff/6001/agent/format-1.16.go#newcod...
agent/format-1.16.go:119: CACert:         
base64.StdEncoding.EncodeToString(config.caCert),
can't we just use string here and below?
the certificates and keys are defined to be in PEM format (it is, or was at
least, even documented in this packages as such), which is ASCII
(and mostly base64 itself).

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

https://codereview.appspot.com/13481043/diff/6001/agent/format.go#newcode33
agent/format.go:33: formatFilename = "format"
Why do we need the file format to be in a separate file?
Can't we just store the format inside the file itself
(some care would obviously need to be taken when first introducing
the format specifier, and that the file format is sufficiently
general to cater for different formats).

Then we don't need to worry about the config file getting
out of step with its purported format.

Something like:

type agentConfig struct {
    Format string
    Contents string
}

should do the job (it's a pity that goyaml doesn't have the equivalent of
json.RawMessage).

[thumper, 2013-09-12 22:52:15]

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

https://codereview.appspot.com/13481043/diff/6001/agent/agent.go#newcode103
agent/agent.go:103: // when mutliple go-routines may be adding things to the
agent config, and
On 2013/09/12 13:21:44, fwereade wrote:
> mutliple

Done.

https://codereview.appspot.com/13481043/diff/6001/agent/agent.go#newcode111
agent/agent.go:111: // map.  Retrieving and setting values here are protected by
the mutex.
On 2013/09/12 13:21:44, fwereade wrote:
> To get the read/write thing right across the board, I think we have to go with
a
> model whereby you tell the config to persist by passing in a mutate func...
but
> even then, I remain nervous about the N cached copies of mutually diverging
> state.
> 
> I'm starting to get the impression that this is maybe a case for the Forbidden
> Design Pattern... but I can also appreciate that what we have here is progress
> even if it's not perfection. Would you just rewrite the comment above to make
> the limits of the current model clear and explicit?

I'm not sure I agree here.  The agent configuration is defined at creation time,
and the only mutable values are in the values map.  Synchronisation around these
and the write methods are sufficient.  Clearly if we are adding other mutating
functions to the interface, these should also be protected.  We don't have N
cached copies mutually diverging.  We have one instance used in multiple places.

I'll add a bit more to the comment here, but I'm not clear on where you feel the
issue is.

https://codereview.appspot.com/13481043/diff/6001/agent/format-1.16.go
File agent/format-1.16.go (right):

https://codereview.appspot.com/13481043/diff/6001/agent/format-1.16.go#newcod...
agent/format-1.16.go:119: CACert:         
base64.StdEncoding.EncodeToString(config.caCert),
On 2013/09/12 14:50:58, rog wrote:
> can't we just use string here and below?
> the certificates and keys are defined to be in PEM format (it is, or was at
> least, even documented in this packages as such), which is ASCII
> (and mostly base64 itself).

This was done to just take the []byte safely into a string.

If it was a string before, why are we passing it around as a byte slice?

We can always change future formats to do this if we decide we want to change.

https://codereview.appspot.com/13481043/diff/6001/agent/format-1.16.go#newcod...
agent/format-1.16.go:147: // directory to ".old".
On 2013/09/12 13:21:44, fwereade wrote:
> I agree that we should; or possibly that we should be atomically swapping a
> symlink. That's surely out of scope, because of backward compatibility if
> nothing else, but it deserves its own bug.

Agree that it is out of scope for this branch, filed a bug and linked it.

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

https://codereview.appspot.com/13481043/diff/6001/agent/format.go#newcode33
agent/format.go:33: formatFilename = "format"
On 2013/09/12 14:50:58, rog wrote:
> Why do we need the file format to be in a separate file?
> Can't we just store the format inside the file itself
> (some care would obviously need to be taken when first introducing
> the format specifier, and that the file format is sufficiently
> general to cater for different formats).
> 
> Then we don't need to worry about the config file getting
> out of step with its purported format.
> 
> Something like:
> 
> type agentConfig struct {
>     Format string
>     Contents string
> }
> 
> should do the job (it's a pity that goyaml doesn't have the equivalent of
> json.RawMessage).

Here we are taking advantage of the knowledge gained in other projects.  This is
what bazaar did, and it worked very well for them.

This also keeps the concepts clean.  Also, William is actually wanting to break
the config apart into more files, not consolidate them.

https://codereview.appspot.com/13481043/diff/6001/worker/provisioner/provisio...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/13481043/diff/6001/worker/provisioner/provisio...
worker/provisioner/provisioner.go:71: agentConfig: agentConfig,
On 2013/09/12 13:21:44, fwereade wrote:
> I feel like we ought to be able to get the id (tag better perhaps?) out of the
> agent config.

We can get the Tag, and then use that to get the machine id.

Perhaps as a trivial follow-up branch.

https://codereview.appspot.com/13481043/diff/6001/worker/provisioner/provisio...
worker/provisioner/provisioner.go:177: tools, err :=
agenttools.ReadTools(dataDir, version.Current)
On 2013/09/12 13:21:44, fwereade wrote:
> I feel like most of agenttools should really be methods on agentConfig. YMMV,
> not one for this CL, etc.

Perhaps, and I agree not for this CL.