Added LogDir to agent conf, upgrade steps

Added LogDir to agent conf, upgrade steps

As a preparation for the upcoming debug-log command
and API changes, first we need to get rid of the
hard-coded /var/log/juju strings across the codebase
and add an agent config setting for LogDir.

To make upgrades easier, I added some upgrade steps
to add the log dir if it's missing and an additional
function in the agent package, called UpgradeConfig,
which allows changing any value of the agent config
and write it back.

https://code.launchpad.net/~dimitern/juju-core/310-log-location-agent-conf/+merge/207372

(do not edit description out of merge proposal)

[dimitern, 2014-02-20 09:27:27]

Please take a look.

[dimitern, 2014-02-20 09:35:02]

Please take a look.

[axw, 2014-02-24 09:41:40]

Just stumbled across this...

https://codereview.appspot.com/66540044/diff/20001/upgrades/rsyslogconf.go
File upgrades/rsyslogconf.go (left):

https://codereview.appspot.com/66540044/diff/20001/upgrades/rsyslogconf.go#ol...
upgrades/rsyslogconf.go:28: configRenderer :=
syslog.NewAccumulateConfig(machineTag, syslogPort, namespace)
We need to set LogDir to the right thing on the returned SyslogConfig. See:
https://bugs.launchpad.net/juju-core/+bug/1284020

[dimitern, 2014-02-26 09:13:51]

Please take a look.

[rog, 2014-02-26 09:51:31]

A good direction, but I have a few queries and suggestions.

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

https://codereview.appspot.com/66540044/diff/40001/agent/agent.go#newcode173
agent/agent.go:173: values[LogDir] = logDir
why are we putting LogDir in the values as well as in a parameter?

https://codereview.appspot.com/66540044/diff/40001/agent/format-1.12_whitebox...
File agent/format-1.12_whitebox_test.go (right):

https://codereview.appspot.com/66540044/diff/40001/agent/format-1.12_whitebox...
agent/format-1.12_whitebox_test.go:56: // logDir is sent as part of the values
key.
why?

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

https://codereview.appspot.com/66540044/diff/40001/agent/upgrades.go#newcode29
agent/upgrades.go:29: config := oldConfig.(*configInternal)
This is the kind of thing that makes me sad about
exposing interfaces rather than concrete types.
The signature of this function purports to accept anything that implements the
Config interface, but actually we panic unless it's
something that's been obtained in the right way.

Please can we not do this, and rather use the methods
that are defined on the interface type?

And I'm not that keen on the "only change if non-zero"
thing either - it means that we can't reset an
attribute even if we want to.

https://codereview.appspot.com/66540044/diff/40001/cmd/juju/debuglog.go
File cmd/juju/debuglog.go (right):

https://codereview.appspot.com/66540044/diff/40001/cmd/juju/debuglog.go#newco...
cmd/juju/debuglog.go:86: tailcmd := fmt.Sprintf("tail -n %s -f
%s/all-machines.log", &c.lines, agent.DefaultLogDir)
Why is agentLogDir coming from a global variable here, and not the agent config?

https://codereview.appspot.com/66540044/diff/40001/container/lxc/lxc.go
File container/lxc/lxc.go (right):

https://codereview.appspot.com/66540044/diff/40001/container/lxc/lxc.go#newco...
container/lxc/lxc.go:108: logDir := internalLogDir(name)
why isn't this using the logDir passed in conf.LogDir?

[fwereade, 2014-02-27 13:19:51]

just a quick comment

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

https://codereview.appspot.com/66540044/diff/40001/agent/upgrades.go#newcode29
agent/upgrades.go:29: config := oldConfig.(*configInternal)
On 2014/02/26 09:51:32, rog wrote:
> This is the kind of thing that makes me sad about
> exposing interfaces rather than concrete types.
> The signature of this function purports to accept anything that implements the
> Config interface, but actually we panic unless it's
> something that's been obtained in the right way.
> 
> Please can we not do this, and rather use the methods
> that are defined on the interface type?
> 
> And I'm not that keen on the "only change if non-zero"
> thing either - it means that we can't reset an
> attribute even if we want to.

+100 to all this, apart from the implication that interfaces are the problem.
Can we not just put this func into the Config interface?