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.

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

Requires: https://code.launchpad.net/~thumper/juju-core/agent-config-formatters/+merge/183074

(do not edit description out of merge proposal)

[thumper, 2013-08-30 06:02:19]

Please take a look.

[jameinel, 2013-09-02 06:44:16]

I have some comments but overall this seems pretty good.

The biggest concern is that this breaks cross-version bootstrap without a
specific answer for how we might want to fix it.

Specifically, someone doing "juju bootstrap" with 1.15 will lookup the 1.12/1.14
tools and then write an agent.conf file to it that will only be supported in
1.15-or-newer.

So it feels like we should instead follow the 1-older model. I realize that
Ian's change to be stricter about exact matches may happen here, and then we'll
just require the agent.conf to match exactly the version we are starting.

I feel like if we can have an answer like: "oh you're bootstrapping version X.Y,
I'll write the configs in a format that X.Y understands". Is it hard to find
that answer and allow us to avoid client => server lock step?

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

https://codereview.appspot.com/13421043/diff/1/agent/agent.go#newcode96
agent/agent.go:96: 
I would probably write this as "the lock should be taken, then the config read
from disk, and then data added to it before writing and then finally releasing
the lock".
However, can we not use the FileLock primative instead of just an in-memory
Mutex?

I suppose we only have 1 agent directory per agent, so there shouldn't be 3rd
parties touching that file. Other than cloud-init writing it initially. 

Maybe add a comment as to why a Mutex is sufficient.

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

https://codereview.appspot.com/13421043/diff/1/agent/format-1.16.go#newcode27
agent/format-1.16.go:27: type formatter116 struct {
I think this needs to be renamed to formatter_1_16 in keeping with what you did
for the 1_12 formats.

https://codereview.appspot.com/13421043/diff/1/agent/format-1.16.go#newcode59
agent/format-1.16.go:59: // not an empty slice, which is what the DecodeString
returns.
I think the doc string isn't quite complete, as this actually does base64
decoding. We may also want to mention that for an empty string we end up with a
nil slice rather than an empty slice.

However, should we call this decode64 or something to make it clearer that it
isn't a generic decode function, but is about decoding base64 encoded content?

https://codereview.appspot.com/13421043/diff/1/agent/format-1.16.go#newcode67
agent/format-1.16.go:67: func (formatter *formatter116) read(dirName string)
(*configInternal, error) {
I take it your 1.16 format doesn't actually split the content into separate
files. It just is about moving the Certs into base64 encoded strings and pulling
Values from ENV variables into the Values map in the data stream.

https://codereview.appspot.com/13421043/diff/1/agent/format-1.16.go#newcode141
agent/format-1.16.go:141: }
You seem to atomically create the contents of agent.conf but you universally
splat the format file down.

At the very least, we should probably do:
write agent.conf-new, write format file, rename agent.conf-new => target.

That gives us a smaller vulnerability. One could argue that you write the format
file last (in case permissions are such that you can't rename over the
agent.conf file itself.)

But otherwise I would expect writing to fail before renaming.

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

https://codereview.appspot.com/13421043/diff/1/agent/format.go#newcode59
agent/format.go:59: // Once the previousFormat is defined to have a format file
(1.14 or
I think this is now 1.16 or newer

[thumper, 2013-09-02 23:53:25]

On 2013/09/02 06:44:16, jameinel wrote:
> I have some comments but overall this seems pretty good.
> 
> The biggest concern is that this breaks cross-version bootstrap without a
> specific answer for how we might want to fix it.

I'll reply here before I mark this as obsolete as I added another pipe in the
middle,
and we can't change that with lbox, so I need to abandon this review.

> Specifically, someone doing "juju bootstrap" with 1.15 will lookup the
1.12/1.14
> tools and then write an agent.conf file to it that will only be supported in
> 1.15-or-newer.

Um... no it won't.  As you mentioned, Ian has just landed code that means that
tools are selected using major.minor matching.  Also, the format is only handled
client
side for the initial bootstrapping.

When upgrading, the migration process handles the continued readability of the
agent config.

> So it feels like we should instead follow the 1-older model. I realize that
> Ian's change to be stricter about exact matches may happen here, and then
we'll
> just require the agent.conf to match exactly the version we are starting.
> 
> I feel like if we can have an answer like: "oh you're bootstrapping version
X.Y,
> I'll write the configs in a format that X.Y understands". Is it hard to find
> that answer and allow us to avoid client => server lock step?

The lock step is only for the bootstrap.  Once that is done, the client just
talks
the API (or it soon will) to the API servers.

> https://codereview.appspot.com/13421043/diff/1/agent/agent.go
> File agent/agent.go (right):
> 
> https://codereview.appspot.com/13421043/diff/1/agent/agent.go#newcode96
> agent/agent.go:96: 
> I would probably write this as "the lock should be taken, then the config read
> from disk, and then data added to it before writing and then finally releasing
> the lock".

That isn't entirely correct though.  You shouldn't need to reread the config
as there is only a single copy of it, a single process, and all writes to
the config should come from the agent, the thing with the singular copy
of the information.

Added a bit more about how a mutex is sufficient.

> https://codereview.appspot.com/13421043/diff/1/agent/format-1.16.go
> File agent/format-1.16.go (right):
> 
> https://codereview.appspot.com/13421043/diff/1/agent/format-1.16.go#newcode27
> agent/format-1.16.go:27: type formatter116 struct {
> I think this needs to be renamed to formatter_1_16 in keeping with what you
did
> for the 1_12 formats.

This is done.
 
> https://codereview.appspot.com/13421043/diff/1/agent/format-1.16.go#newcode59
> agent/format-1.16.go:59: // not an empty slice, which is what the DecodeString
> returns.
> I think the doc string isn't quite complete, as this actually does base64
> decoding. We may also want to mention that for an empty string we end up with
a
> nil slice rather than an empty slice.

Ummm, this is what I have:

// decode makes sure that for an empty string we have a nil slice,
// not an empty slice, which is what the DecodeString returns.
func (*formatter_1_16) decode(value string) (result []byte, err error) {
	if value != "" {
		result, err = base64.StdEncoding.DecodeString(value)
	}
	return
}


> However, should we call this decode64 or something to make it clearer that it
> isn't a generic decode function, but is about decoding base64 encoded content?

Ok, decode64 is fine with me.

> https://codereview.appspot.com/13421043/diff/1/agent/format-1.16.go#newcode67
> agent/format-1.16.go:67: func (formatter *formatter116) read(dirName string)
> (*configInternal, error) {
> I take it your 1.16 format doesn't actually split the content into separate
> files. It just is about moving the Certs into base64 encoded strings and
pulling
> Values from ENV variables into the Values map in the data stream.

This is correct.  William and I talked about splitting the content into separate
files, and he would like that later, but I decided not to do that here.

I could probably be convinced if he super wants it :-)  and is able to say
which bits he wants where...

> https://codereview.appspot.com/13421043/diff/1/agent/format-1.16.go#newcode141
> agent/format-1.16.go:141: }
> You seem to atomically create the contents of agent.conf but you universally
> splat the format file down.

No... the writing of the format is done in two steps too, just in format.go

> At the very least, we should probably do:
> write agent.conf-new, write format file, rename agent.conf-new => target.
> 
> That gives us a smaller vulnerability. One could argue that you write the
format
> file last (in case permissions are such that you can't rename over the
> agent.conf file itself.)
> 
> But otherwise I would expect writing to fail before renaming.

Sure... I'll do that.  Also using the mutex in the one place where the config
is actually read from the filesystem (even though it is probably not needed).

 
> https://codereview.appspot.com/13421043/diff/1/agent/format.go
> File agent/format.go (right):
> 
> https://codereview.appspot.com/13421043/diff/1/agent/format.go#newcode59
> agent/format.go:59: // Once the previousFormat is defined to have a format
file
> (1.14 or
> I think this is now 1.16 or newer

done.