Fix:#1295650 Failproofed rsyslog restoring script

Fix:#1295650 Failproofed rsyslog restoring script

This bash script failure interrupts provisioning of
machines on juju-restore.
After making manual tests on the a half restored
env, I noticed that the errors in the process and
in the scripts that used this are caused by sed
being called in files that might not exist in all
of the versions, Andrew Wilkins informed me, as
stated in the bug, that those where not necessary in
newer versions of juju-core and it was ok to ignore
them.
I added failsafes with informative messages for
the script (error catching not possible because
of -e flag).
I also added checks for another sed call that
was just handling expected errors using ||
now those errors should not be triggered and
only an echo for the expected missing rsyslog
file will be output.

https://code.launchpad.net/~hduran-8/juju-core/1295650_rsyslog_on_restore/+merge/212207

(do not edit description out of merge proposal)

[hduran, 2014-03-21 17:21:47]

Please take a look.

[rog, 2014-03-21 17:52:01]

Looks good in general, with some suggestions below.

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
cmd/plugins/juju-restore/restore.go:340: // Redirections to /dev/null are to
avoid confusing non fatal errors printed
In general, comments should be next to the code that they pertain to. I've made
some suggestions below.

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
cmd/plugins/juju-restore/restore.go:351: if [[ $agent = unit-* ]]
I think it might be reasonable to fold the tests into
one condition, with a more direct test for directory
existence:

# If we're processing a unit agent's directly
# and it has some relations, reset
# the stored version of all of them to
# ensure that any relation hooks will
# fire.
if [[ $agent = unit-* ]] && [ -d "$agent/state/relations" ]

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
cmd/plugins/juju-restore/restore.go:360: sed -i -r 's/^(:syslogtag, startswith,
"juju-" @)(.*)(:[0-9]+.*)$/\1{{.Address}}\3/' /etc/rsyslog.d/*-juju*.conf
2>/dev/null || echo "WARN: rsyslog live editing failed"
I think we probably do want things to fail if the sed fails.

how about this:

# Change the rsyslog destination. Note that some versions of
# juju have no rsyslog files, so we make sure that
# we do not fail in that case.
find /etc/rsyslog.d -name '*-juju*.conf' | xargs sed -i -r 's/^(:syslogtag,
startswith, "juju-" @)(.*)(:[0-9]+.*)$/\1{{.Address}}\3/'

?

[axw, 2014-03-22 13:24:32]

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
cmd/plugins/juju-restore/restore.go:360: sed -i -r 's/^(:syslogtag, startswith,
"juju-" @)(.*)(:[0-9]+.*)$/\1{{.Address}}\3/' /etc/rsyslog.d/*-juju*.conf
2>/dev/null || echo "WARN: rsyslog live editing failed"
On 2014/03/21 17:52:02, rog wrote:
> I think we probably do want things to fail if the sed fails.
> 
> how about this:
> 
> # Change the rsyslog destination. Note that some versions of
> # juju have no rsyslog files, so we make sure that
> # we do not fail in that case.

I may not have been clear in my communication to Horacio about how it works now.
The rsyslog configuration will still exist, but it is now written by the machine
agent (by a the rsyslog worker), as opposed to during cloud-init. Previously the
file would exist once bootstrap completed, now it will be written by the machine
agent some time after it starts up.

We shouldn't run this part of the script at all for versions of Juju >= 1.18 (or
whatever 1.17.x it was). The rsyslog worker will write out the file anew every
time the machine agent starts up, and is rewritten whenever relevant environment
configuration changes. Since the file is rewritten on machine agent startup,
there's no need to an external rewrite (with sed) at all.

> find /etc/rsyslog.d -name '*-juju*.conf' | xargs sed -i -r 's/^(:syslogtag,
> startswith, "juju-" @)(.*)(:[0-9]+.*)$/\1{{.Address}}\3/'
> 
> ?

[hduran, 2014-03-25 15:10:01]

Please take a look.

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
cmd/plugins/juju-restore/restore.go:340: // Redirections to /dev/null are to
avoid confusing non fatal errors printed
On 2014/03/21 17:52:02, rog wrote:
> In general, comments should be next to the code that they pertain to. I've
made
> some suggestions below.

Done.

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
cmd/plugins/juju-restore/restore.go:351: if [[ $agent = unit-* ]]
On 2014/03/21 17:52:02, rog wrote:
> I think it might be reasonable to fold the tests into
> one condition, with a more direct test for directory
> existence:
> 
> # If we're processing a unit agent's directly
> # and it has some relations, reset
> # the stored version of all of them to
> # ensure that any relation hooks will
> # fire.
> if [[ $agent = unit-* ]] && [ -d "$agent/state/relations" ]

I Changed this to an approach that uses find to get the files in that folder, if
ay

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
cmd/plugins/juju-restore/restore.go:360: sed -i -r 's/^(:syslogtag, startswith,
"juju-" @)(.*)(:[0-9]+.*)$/\1{{.Address}}\3/' /etc/rsyslog.d/*-juju*.conf
2>/dev/null || echo "WARN: rsyslog live editing failed"
axw 's comment obsoletes this problem

https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
cmd/plugins/juju-restore/restore.go:360: sed -i -r 's/^(:syslogtag, startswith,
"juju-" @)(.*)(:[0-9]+.*)$/\1{{.Address}}\3/' /etc/rsyslog.d/*-juju*.conf
2>/dev/null || echo "WARN: rsyslog live editing failed"
On 2014/03/22 13:24:32, axw wrote:
> On 2014/03/21 17:52:02, rog wrote:
> > I think we probably do want things to fail if the sed fails.
> > 
> > how about this:
> > 
> > # Change the rsyslog destination. Note that some versions of
> > # juju have no rsyslog files, so we make sure that
> > # we do not fail in that case.
> 
> I may not have been clear in my communication to Horacio about how it works
now.
> The rsyslog configuration will still exist, but it is now written by the
machine
> agent (by a the rsyslog worker), as opposed to during cloud-init. Previously
the
> file would exist once bootstrap completed, now it will be written by the
machine
> agent some time after it starts up.
> 
> We shouldn't run this part of the script at all for versions of Juju >= 1.18
(or
> whatever 1.17.x it was). The rsyslog worker will write out the file anew every
> time the machine agent starts up, and is rewritten whenever relevant
environment
> configuration changes. Since the file is rewritten on machine agent startup,
> there's no need to an external rewrite (with sed) at all.
> 
> > find /etc/rsyslog.d -name '*-juju*.conf' | xargs sed -i -r 's/^(:syslogtag,
> > startswith, "juju-" @)(.*)(:[0-9]+.*)$/\1{{.Address}}\3/'
> > 
> > ?

Done.

[rog, 2014-03-25 15:15:18]

On 2014/03/25 15:10:01, hduran wrote:
> Please take a look.
> 
>
https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
> File cmd/plugins/juju-restore/restore.go (right):
> 
>
https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
> cmd/plugins/juju-restore/restore.go:340: // Redirections to /dev/null are to
> avoid confusing non fatal errors printed
> On 2014/03/21 17:52:02, rog wrote:
> > In general, comments should be next to the code that they pertain to. I've
> made
> > some suggestions below.
> 
> Done.
> 
>
https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
> cmd/plugins/juju-restore/restore.go:351: if [[ $agent = unit-* ]]
> On 2014/03/21 17:52:02, rog wrote:
> > I think it might be reasonable to fold the tests into
> > one condition, with a more direct test for directory
> > existence:
> > 
> > # If we're processing a unit agent's directly
> > # and it has some relations, reset
> > # the stored version of all of them to
> > # ensure that any relation hooks will
> > # fire.
> > if [[ $agent = unit-* ]] && [ -d "$agent/state/relations" ]
> 
> I Changed this to an approach that uses find to get the files in that folder,
if
> ay
> 
>
https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
> cmd/plugins/juju-restore/restore.go:360: sed -i -r 's/^(:syslogtag,
startswith,
> "juju-" @)(.*)(:[0-9]+.*)$/\1{{.Address}}\3/' /etc/rsyslog.d/*-juju*.conf
> 2>/dev/null || echo "WARN: rsyslog live editing failed"
> axw 's comment obsoletes this problem
> 
>
https://codereview.appspot.com/78870043/diff/1/cmd/plugins/juju-restore/resto...
> cmd/plugins/juju-restore/restore.go:360: sed -i -r 's/^(:syslogtag,
startswith,
> "juju-" @)(.*)(:[0-9]+.*)$/\1{{.Address}}\3/' /etc/rsyslog.d/*-juju*.conf
> 2>/dev/null || echo "WARN: rsyslog live editing failed"
> On 2014/03/22 13:24:32, axw wrote:
> > On 2014/03/21 17:52:02, rog wrote:
> > > I think we probably do want things to fail if the sed fails.
> > > 
> > > how about this:
> > > 
> > > # Change the rsyslog destination. Note that some versions of
> > > # juju have no rsyslog files, so we make sure that
> > > # we do not fail in that case.
> > 
> > I may not have been clear in my communication to Horacio about how it works
> now.
> > The rsyslog configuration will still exist, but it is now written by the
> machine
> > agent (by a the rsyslog worker), as opposed to during cloud-init. Previously
> the
> > file would exist once bootstrap completed, now it will be written by the
> machine
> > agent some time after it starts up.
> > 
> > We shouldn't run this part of the script at all for versions of Juju >= 1.18
> (or
> > whatever 1.17.x it was). The rsyslog worker will write out the file anew
every
> > time the machine agent starts up, and is rewritten whenever relevant
> environment
> > configuration changes. Since the file is rewritten on machine agent startup,
> > there's no need to an external rewrite (with sed) at all.
> > 
> > > find /etc/rsyslog.d -name '*-juju*.conf' | xargs sed -i -r
's/^(:syslogtag,
> > > startswith, "juju-" @)(.*)(:[0-9]+.*)$/\1{{.Address}}\3/'
> > > 
> > > ?
> 
> Done.

LGTM assuming it works live.