Don't forward logs if deploying on state server

Don't forward logs if deploying on state server

Previously, whenever a unit was deployed a configuration
file was created in /etc/rsyslog.d to forward logs to
the state servers. This introduced a feedback loop when
the the unit was deployed on a state server machine.

Now, check for the existence of /etc/rsyslog.d/25-juju.conf
before creating an additional configuration.

Fixes bug #1211147

https://code.launchpad.net/~axwalk/juju-core/lp1211147-no-log-forwarding-on-state/+merge/180057

(do not edit description out of merge proposal)

[axw, 2013-08-14 07:59:09]

Please take a look.

[jameinel, 2013-08-14 10:45:13]

I'd like Ian to look over this change, as he's the one that set up the syslog
stuff in the first place.

I'm slightly hesitant to use 'Stat(25-juju-*.conf)' as our method for detecting
that we are forwarding our messages back to ourself, but it might be ok if the
alternatives are even more unpalatable.

https://codereview.appspot.com/12909044/diff/1/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/12909044/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:135: if _, statErr :=
os.Stat(syslogConfigRenderer.ConfigFilePath()); os.IsNotExist(statErr) {
My immediate thought is that this is detecting the case in the wrong way.
It "should" be detecting that it is forwarding the syslog messages to itself and
refuse, rather than detecting the aggregation is being done here.

Is it possible to detect that our local IP Address is in 'stateAddrs' instead of
statting for the file?

I suppose in the HA world we might also need to handle the "I'm now a state
server, and I need to delete 26-juju-x.conf" but I think we can handle that
today with a comment about it. (Since we don't know exactly how HA is going to
look.)

[natefinch, 2013-08-14 19:23:58]

John's point about finding a better way to determine if this machine is
forwarding to itself is a good one, and that's the idea fix. 

However, either way, the knowledge of how to determine that shouldn't live here,
it should live in cloudinit, where this stuff is set up in the first place.
Expose a method from that package that'll do the check for you, so you don't
have that logic leaking across packages.

https://codereview.appspot.com/12909044/diff/1/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/12909044/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:135: if _, statErr :=
os.Stat(syslogConfigRenderer.ConfigFilePath()); os.IsNotExist(statErr) {
On 2013/08/14 10:45:13, jameinel wrote:
> My immediate thought is that this is detecting the case in the wrong way.

At the very least, we shouldn't have this method hard code a string that is also
hard coded in environs/cloudinit. Way way too much knowledge of the internals of
another package.

[wallyworld, 2013-08-15 01:14:01]

not lgtm because there is a better way to determine if the machine is a state
server - see suggestion in comment

https://codereview.appspot.com/12909044/diff/1/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/12909044/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:135: if _, statErr :=
os.Stat(syslogConfigRenderer.ConfigFilePath()); os.IsNotExist(statErr) {
We can detect if a machine is a state server by querying its Jobs and seeing if
it has the JobManageState job. I would introduce a new method on machine
IsStateServer() and use that here rather than check for a (fragile) file name.
When we move to HA, the new method would also likely be generally applicable for
other things as well.

[axw1, 2013-08-15 01:29:48]

Thanks for the reviews all. I'll create two new MPs: one that introduces
IsStateServer to state.Machine, and one that uses it in the deployer code.

https://codereview.appspot.com/12909044/diff/1/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/12909044/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:135: if _, statErr :=
os.Stat(syslogConfigRenderer.ConfigFilePath()); os.IsNotExist(statErr) {
On 2013/08/14 10:45:13, jameinel wrote:
> Is it possible to detect that our local IP Address is in 'stateAddrs' instead
of
> statting for the file?

Fair enough. I did feel a little queasy when I implemented it in terms of file
presence; that should've been my cue to stop ;)

I did consider checking the local IPs, but didn't think it much better. I don't
know if we'd ever do it, but what about state servers behind NAT? Then you're
screwed.

https://codereview.appspot.com/12909044/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:135: if _, statErr :=
os.Stat(syslogConfigRenderer.ConfigFilePath()); os.IsNotExist(statErr) {
On 2013/08/15 01:14:02, wallyworld wrote:
> We can detect if a machine is a state server by querying its Jobs and seeing
if
> it has the JobManageState job. I would introduce a new method on machine
> IsStateServer() and use that here rather than check for a (fragile) file name.
> When we move to HA, the new method would also likely be generally applicable
for
> other things as well.

This sounds good to me. I would've done this originally, but there's no state
objects passed through to the deployer. It sounds like there's no good reason
for there not to be, though, so I'll modify NewSimpleContext to take a
*state.Machine and do what you've suggested.