Agent config format 1.18

Agent config format 1.18

This introduces a new agent config format - 1.18:
- LogDir and DataDir added as fields
- BootstrapJobs setting promoted to a Jobs field
  (for all machines)
- format file removed and added a "version header"
  to the agent.conf file. Now there is a single conf
  file and the upgrades/migrations can use atomic file
  renaming (fixes bug #1224725).
- format 1.12 removed as obsolete.

Introduced agent.DefaultLogDir constant and changed
hardcoded /var/log/juju to use that, except for the
rsyslog state server config, which needs to use
/var/log/juju-namespace due to apparmor restrictions.

Tested live on EC2 and with the local provider.

https://code.launchpad.net/~dimitern/juju-core/305-agent-confg.1.18/+merge/208856

(do not edit description out of merge proposal)

[dimitern, 2014-02-28 17:43:47]

Please take a look.

[dimitern, 2014-02-28 17:49:55]

Please take a look.

[rog, 2014-02-28 18:54:33]

Looks generally reasonable, but I have some suggestions below. I haven't had
time to go through all of it in detail.

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

https://codereview.appspot.com/70010045/diff/1/agent/agent.go#newcode286
agent/agent.go:286: if format == format_1_18 {
This logic seems wrong to me - it'll become obsolete
as soon as we move beyond 1.18.

I wonder if it might be better to do a little
less interleaving of logic for different versions,
and have two obviously separate paths, so eventually
one path can be deleted without needing us to
follow all the logic again.

How about something like this?

var formats = make(map[string] formatter)

func registerFormat(format formatter) {
    formats[format.version()] = format
}

var currentFormat = format_1_18

func ReadConf(configFilePath string) (Config, error) {
    lock mutex
    var format formatter
    var config Config
    if a legacy format file exists {
        config = read file with legacy logic
        format = nil
    } else {
        data, err := ioutil.ReadFile(location)
        if err != nil {
           ...
        }
        format, config, err := parseConfigData(data)
        if err != nil {
            ...
        }
    }
    if format != currentFormat {
       // migrate from old format

       data := currentFormat.marshal(config)
       write to file: "%s\n%s", currentFormat.version(), data)
       if format == nil { 
           remove old format file
       }
    }
    return config, nil
}

type formatter interface {
    version() string
    unmarshal(data []byte) (Config, error)
}

func parseConfigData(data []byte) (format formatter, config Config, err error) {
    i := bytes.IndexByte(data, '\n')
    if i == -1 {
       return invalid format error
    }
    format, err = getFormatter(string(data[0:i]))
    if err != nil {
        return nil, nil, err
    }
    config, err := format.unmarshal(data[i+1:])
    if err != nil {
        return nil, nil, err
    }
    return format, config, nil
}




and in format-1.18.go

type formatter_1_18 struct {}

var format_1_18 = formatter_1_18{}

func init() {
    register(format_1_18)
}

func (formatter_1_18) unmarshal(data []byte) (Config, error) {
     // yaml.Unmarshal, etc
     return &configInternal{
         ....
     }
}

func (formatter_1_18) version() string {
    return "format 1.18"
}

func (formatter_1_18) marshal(config Config) ([]byte, error) {
     // note that this function only needs to be implemented by the
     // formatter for the current version.
}

Oops, that expanded in scope somewhat, but I think it might
make sense. It means there's less for each formatter to do
(no need for every formatter to implement writeCommands, for
example).

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

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode38
agent/format-1.18.go:38: UpgradedToVersion string   `yaml:"upgradedToVersion"`
Why not:

UpgradedToVersion version.Number  `yaml:"upgradedToVersion"`

?

If version.Number doesn't have YAML marshalling methods,
they should be added.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode41
agent/format-1.18.go:41: CACert         string
Let's not base64 encode this - it's already in base64-encoded
PEM format, so we'll be double-encoding.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode62
agent/format-1.18.go:62: func (*formatter_1_18) decode64(value string) (result
[]byte, err error) {
This can go if we don't base64 encode the cert.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode71
agent/format-1.18.go:71: func (*formatter_1_18) upgradedToVersion(value string)
(version.Number, error) {
This can go if we implement YAML parsing in version.Number.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode90
agent/format-1.18.go:90: if formatVersion != versionLine {
This is wrong - the format should have been parsed before
this code ever gets invoked.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode102
agent/format-1.18.go:102: caCert, err := formatter.decode64(format.CACert)
This can go, as suggested above.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode106
agent/format-1.18.go:106: stateServerCert, err :=
formatter.decode64(format.StateServerCert)
This can go, similarly to CACert.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode110
agent/format-1.18.go:110: stateServerKey, err :=
formatter.decode64(format.StateServerKey)
This can also go.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode114
agent/format-1.18.go:114: upgradedToVersion, err :=
formatter.upgradedToVersion(format.UpgradedToVersion)
This too.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode132
agent/format-1.18.go:132: job, err :=
state.MachineJobFromParams(params.MachineJob(jobName))
Why are we storing state.MachineJob rather than params.MachineJob in the config?

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode141
agent/format-1.18.go:141: if len(format.StateAddresses) > 0 {
I can't really see what the condition adds here.

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

https://codereview.appspot.com/70010045/diff/1/agent/format.go#newcode67
agent/format.go:67: return currentFormat, nil
This doesn't seem quite right - surely if there is no format file,
we don't know the format?

[dimitern, 2014-03-06 12:36:40]

Please take a look.

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

https://codereview.appspot.com/70010045/diff/1/agent/agent.go#newcode286
agent/agent.go:286: if format == format_1_18 {
On 2014/02/28 18:54:34, rog wrote:
> This logic seems wrong to me - it'll become obsolete
> as soon as we move beyond 1.18.
> 
> I wonder if it might be better to do a little
> less interleaving of logic for different versions,
> and have two obviously separate paths, so eventually
> one path can be deleted without needing us to
> follow all the logic again.
> 
> How about something like this?
> 
> var formats = make(map[string] formatter)
> 
> func registerFormat(format formatter) {
>     formats[format.version()] = format
> }
> 
> var currentFormat = format_1_18
> 
> func ReadConf(configFilePath string) (Config, error) {
>     lock mutex
>     var format formatter
>     var config Config
>     if a legacy format file exists {
>         config = read file with legacy logic
>         format = nil
>     } else {
>         data, err := ioutil.ReadFile(location)
>         if err != nil {
>            ...
>         }
>         format, config, err := parseConfigData(data)
>         if err != nil {
>             ...
>         }
>     }
>     if format != currentFormat {
>        // migrate from old format
> 
>        data := currentFormat.marshal(config)
>        write to file: "%s\n%s", currentFormat.version(), data)
>        if format == nil { 
>            remove old format file
>        }
>     }
>     return config, nil
> }
> 
> type formatter interface {
>     version() string
>     unmarshal(data []byte) (Config, error)
> }
> 
> func parseConfigData(data []byte) (format formatter, config Config, err error)
{
>     i := bytes.IndexByte(data, '\n')
>     if i == -1 {
>        return invalid format error
>     }
>     format, err = getFormatter(string(data[0:i]))
>     if err != nil {
>         return nil, nil, err
>     }
>     config, err := format.unmarshal(data[i+1:])
>     if err != nil {
>         return nil, nil, err
>     }
>     return format, config, nil
> }
> 
> 
> 
> 
> and in format-1.18.go
> 
> type formatter_1_18 struct {}
> 
> var format_1_18 = formatter_1_18{}
> 
> func init() {
>     register(format_1_18)
> }
> 
> func (formatter_1_18) unmarshal(data []byte) (Config, error) {
>      // yaml.Unmarshal, etc
>      return &configInternal{
>          ....
>      }
> }
> 
> func (formatter_1_18) version() string {
>     return "format 1.18"
> }
> 
> func (formatter_1_18) marshal(config Config) ([]byte, error) {
>      // note that this function only needs to be implemented by the
>      // formatter for the current version.
> }
> 
> Oops, that expanded in scope somewhat, but I think it might
> make sense. It means there's less for each formatter to do
> (no need for every formatter to implement writeCommands, for
> example).
> 
>     

Done similarly, as agreed.

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

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode38
agent/format-1.18.go:38: UpgradedToVersion string   `yaml:"upgradedToVersion"`
On 2014/02/28 18:54:34, rog wrote:
> Why not:
> 
> UpgradedToVersion version.Number  `yaml:"upgradedToVersion"`
> 
> ?
> 
> If version.Number doesn't have YAML marshalling methods,
> they should be added.

Added YAML (un)marshaling methods to version.Number + tests, and changed as
suggested (except due to a goyaml bug, used *version.Number instead of
version.Number).

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode41
agent/format-1.18.go:41: CACert         string
On 2014/02/28 18:54:34, rog wrote:
> Let's not base64 encode this - it's already in base64-encoded
> PEM format, so we'll be double-encoding.

Done.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode62
agent/format-1.18.go:62: func (*formatter_1_18) decode64(value string) (result
[]byte, err error) {
On 2014/02/28 18:54:34, rog wrote:
> This can go if we don't base64 encode the cert.

Done.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode71
agent/format-1.18.go:71: func (*formatter_1_18) upgradedToVersion(value string)
(version.Number, error) {
On 2014/02/28 18:54:34, rog wrote:
> This can go if we implement YAML parsing in version.Number.

Done.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode90
agent/format-1.18.go:90: if formatVersion != versionLine {
On 2014/02/28 18:54:34, rog wrote:
> This is wrong - the format should have been parsed before
> this code ever gets invoked.

Changed as suggested and fixed this.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode102
agent/format-1.18.go:102: caCert, err := formatter.decode64(format.CACert)
On 2014/02/28 18:54:34, rog wrote:
> This can go, as suggested above.

Done.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode106
agent/format-1.18.go:106: stateServerCert, err :=
formatter.decode64(format.StateServerCert)
On 2014/02/28 18:54:34, rog wrote:
> This can go, similarly to CACert.

Done.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode110
agent/format-1.18.go:110: stateServerKey, err :=
formatter.decode64(format.StateServerKey)
On 2014/02/28 18:54:34, rog wrote:
> This can also go.

Done.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode114
agent/format-1.18.go:114: upgradedToVersion, err :=
formatter.upgradedToVersion(format.UpgradedToVersion)
On 2014/02/28 18:54:34, rog wrote:
> This too.

Done.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode132
agent/format-1.18.go:132: job, err :=
state.MachineJobFromParams(params.MachineJob(jobName))
On 2014/02/28 18:54:34, rog wrote:
> Why are we storing state.MachineJob rather than params.MachineJob in the
config?

Done.

https://codereview.appspot.com/70010045/diff/1/agent/format-1.18.go#newcode141
agent/format-1.18.go:141: if len(format.StateAddresses) > 0 {
On 2014/02/28 18:54:34, rog wrote:
> I can't really see what the condition adds here.

It was like that before, just copied it. I guess what it adds it to have a nil
state/apiDetails when no addresses are given.

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

https://codereview.appspot.com/70010045/diff/1/agent/format.go#newcode67
agent/format.go:67: return currentFormat, nil
On 2014/02/28 18:54:34, rog wrote:
> This doesn't seem quite right - surely if there is no format file,
> we don't know the format?

Done.

[fwereade, 2014-03-06 14:41:25]

a few minor comments, leaving full review for rog

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

https://codereview.appspot.com/70010045/diff/20001/agent/agent.go#newcode310
agent/agent.go:310: formatPath := formatFile(dir)
so if we find a format line in the conf file, do we ignore the external format
file? if so, cool

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

https://codereview.appspot.com/70010045/diff/20001/cmd/jujud/agent.go#newcode45
cmd/jujud/agent.go:45: // use it to locate the conf and the infer the data-dir
not even infer the data dir: we need to *record* the data dir in the config; and
be careful not to ever try to infer the conf file location from the data dir.

https://codereview.appspot.com/70010045/diff/20001/worker/deployer/simple.go
File worker/deployer/simple.go (left):

https://codereview.appspot.com/70010045/diff/20001/worker/deployer/simple.go#...
worker/deployer/simple.go:46: initDir string
this doesn't seem to match...

https://codereview.appspot.com/70010045/diff/20001/worker/deployer/simple.go#...
worker/deployer/simple.go:63: initDir:     InitDir,
...this. any idea what the deal is?

[dimitern, 2014-03-06 14:50:36]

Replies to William's comments:

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

https://codereview.appspot.com/70010045/diff/20001/cmd/jujud/agent.go#newcode45
cmd/jujud/agent.go:45: // use it to locate the conf and the infer the data-dir
On 2014/03/06 14:41:26, fwereade wrote:
> not even infer the data dir: we need to *record* the data dir in the config;
and
> be careful not to ever try to infer the conf file location from the data dir.

Now we have data dir in the config, but there's still some inferring code
(mostly because ReadConf used to accept dataDir, not path to config file, as it
does now; and because NewAgentConfig does not accept a path to where the file
needs to be created), which will be removed once we have --config as argument to
jujud.

https://codereview.appspot.com/70010045/diff/20001/worker/deployer/simple.go
File worker/deployer/simple.go (left):

https://codereview.appspot.com/70010045/diff/20001/worker/deployer/simple.go#...
worker/deployer/simple.go:63: initDir:     InitDir,
On 2014/03/06 14:41:26, fwereade wrote:
> ...this. any idea what the deal is?

initDir field is set to the InitDir var, which is defined as "/etc/init" and
overridden in tests.

[rog, 2014-03-06 15:14:06]

looks great as far as i've got.

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go#newcode283
agent/agent.go:283: err    error
d

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go#newcode293
agent/agent.go:293: formatBytes, err := ioutil.ReadFile(legacyFormatPath)
if err != nil && !os.IsNotExist(err) {
    return nil, fmt.Errorf("cannot read format file: %v", err)
}

?

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go#newcode299
agent/agent.go:299: return nil, fmt.Errorf("malformed agent config format: %q",
formatData)
s/: //

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go#newcode313
agent/agent.go:313: logger.Debugf("read agent config format %q",
format.version())
s/config/config, /
?

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go#newcode320
agent/agent.go:320: err = utils.AtomicWriteFile(configFilePath, data, 0600)
This doesn't look quite right - we're not writing
the header here.

We should have a test for this.

I suggest we just call config.Write here instead (as well as
removing the legacy format file). Since Write locks the
mutex, probably make an internal write method that doesn't
lock, and call that from here and from Write.

https://codereview.appspot.com/70010045/diff/40001/agent/format.go
File agent/format.go (right):

https://codereview.appspot.com/70010045/diff/40001/agent/format.go#newcode13
agent/format.go:13: 
Perhaps describe the actual format here?

https://codereview.appspot.com/70010045/diff/40001/cloudinit/options.go
File cloudinit/options.go (left):

https://codereview.appspot.com/70010045/diff/40001/cloudinit/options.go#oldco...
cloudinit/options.go:349: // rsyslog
?

https://codereview.appspot.com/70010045/diff/40001/version/version_test.go
File version/version_test.go (right):

https://codereview.appspot.com/70010045/diff/40001/version/version_test.go#ne...
version/version_test.go:210: if m.onlyNumber {
Let's make Binary work on YAML too, please.

[dimitern, 2014-03-06 16:10:32]

Please take a look.

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go
File agent/agent.go (right):

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go#newcode283
agent/agent.go:283: err    error
On 2014/03/06 15:14:07, rog wrote:
> d

Done.

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go#newcode293
agent/agent.go:293: formatBytes, err := ioutil.ReadFile(legacyFormatPath)
On 2014/03/06 15:14:07, rog wrote:
> if err != nil && !os.IsNotExist(err) {
>     return nil, fmt.Errorf("cannot read format file: %v", err)
> }
> 
> ?

Done.

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go#newcode299
agent/agent.go:299: return nil, fmt.Errorf("malformed agent config format: %q",
formatData)
On 2014/03/06 15:14:07, rog wrote:
> s/: //

Done.

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go#newcode313
agent/agent.go:313: logger.Debugf("read agent config format %q",
format.version())
On 2014/03/06 15:14:07, rog wrote:
> s/config/config, /
> ?

Done.

https://codereview.appspot.com/70010045/diff/40001/agent/agent.go#newcode320
agent/agent.go:320: err = utils.AtomicWriteFile(configFilePath, data, 0600)
On 2014/03/06 15:14:07, rog wrote:
> This doesn't look quite right - we're not writing
> the header here.
> 
> We should have a test for this.
> 
> I suggest we just call config.Write here instead (as well as
> removing the legacy format file). Since Write locks the
> mutex, probably make an internal write method that doesn't
> lock, and call that from here and from Write.

Done.

https://codereview.appspot.com/70010045/diff/40001/agent/format.go
File agent/format.go (right):

https://codereview.appspot.com/70010045/diff/40001/agent/format.go#newcode13
agent/format.go:13: 
On 2014/03/06 15:14:07, rog wrote:
> Perhaps describe the actual format here?

Done.

https://codereview.appspot.com/70010045/diff/40001/cloudinit/options.go
File cloudinit/options.go (left):

https://codereview.appspot.com/70010045/diff/40001/cloudinit/options.go#oldco...
cloudinit/options.go:349: // rsyslog
On 2014/03/06 15:14:07, rog wrote:
> ?

rsyslog is configured during cloudinit, so it shouldn't be in the TODO list
anymore.

https://codereview.appspot.com/70010045/diff/40001/version/version_test.go
File version/version_test.go (right):

https://codereview.appspot.com/70010045/diff/40001/version/version_test.go#ne...
version/version_test.go:210: if m.onlyNumber {
On 2014/03/06 15:14:07, rog wrote:
> Let's make Binary work on YAML too, please.

Done.

[rog, 2014-03-06 17:08:06]

LGTM with one trivial (applies to formatter_1_18 too, i think)

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

https://codereview.appspot.com/70010045/diff/40001/agent/format-1.16.go#newco...
agent/format-1.16.go:13: var format_1_16 = &formatter_1_16{}
There's no particular reason for the pointer here.
We could define all the methods on the value type
(and that's consistent with the way the version
method is already defined)

https://codereview.appspot.com/70010045/diff/40001/agent/format-1.16.go#newco...
agent/format-1.16.go:20: var _ formatter = (*formatter_1_16)(nil)
var _ formatter = formatter_1_16{}

[dimitern, 2014-03-06 17:16:58]

Please take a look.

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

https://codereview.appspot.com/70010045/diff/40001/agent/format-1.16.go#newco...
agent/format-1.16.go:13: var format_1_16 = &formatter_1_16{}
On 2014/03/06 17:08:07, rog wrote:
> There's no particular reason for the pointer here.
> We could define all the methods on the value type
> (and that's consistent with the way the version
> method is already defined)

Done.

https://codereview.appspot.com/70010045/diff/40001/agent/format-1.16.go#newco...
agent/format-1.16.go:20: var _ formatter = (*formatter_1_16)(nil)
On 2014/03/06 17:08:07, rog wrote:
> var _ formatter = formatter_1_16{}

Done.