rsyslog: ensure logs accumulate on state machines

rsyslog: ensure logs accumulate on state machines

Added a WatchForRsyslogChanges watcher and extended the API with
GetRsyslogConfig. Changed the rsyslog worker to use these additions.

https://code.launchpad.net/~wwitzel3/juju-core/009-ha-rsyslog-api/+merge/219703

(do not edit description out of merge proposal)

[wwitzel, 2014-05-15 14:26:50]

Please take a look.

[wwitzel3, 2014-05-15 14:37:03]

https://codereview.appspot.com/94510043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/94510043/diff/1/state/api/params/params.go#new...
state/api/params/params.go:684: // RsyslogConfigResult holds the result of an
GetRsyslogConfig
s/an/a

[mfoord, 2014-05-15 15:27:56]

https://codereview.appspot.com/94510043/diff/1/state/api/rsyslog/rsyslog.go
File state/api/rsyslog/rsyslog.go (right):

https://codereview.appspot.com/94510043/diff/1/state/api/rsyslog/rsyslog.go#n...
state/api/rsyslog/rsyslog.go:58: fmt.Printf("tag: %s", agentTag)
That's a debugging Printf I guess...

[wwitzel, 2014-05-15 18:02:07]

Please take a look.

[wwitzel3, 2014-05-15 18:08:25]

Manually tested many scenarios against virtual maas and ec2. Including going
from non-HA to HA with units deployed and not deployed.

[fwereade, 2014-05-16 09:21:24]

I think this is generally solid but I have a few questions.

https://codereview.appspot.com/94510043/diff/20001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/94510043/diff/20001/state/api/params/params.go...
state/api/params/params.go:687: Port      int
/eyebrow

Why do we need both port and hostports?

https://codereview.appspot.com/94510043/diff/20001/state/apiserver/rsyslog/rs...
File state/apiserver/rsyslog/rsyslog.go (right):

https://codereview.appspot.com/94510043/diff/20001/state/apiserver/rsyslog/rs...
state/apiserver/rsyslog/rsyslog.go:90: watch := api.st.WatchAPIHostPorts()
ISTM that this won't trigger on rsyslog-ca-cert changes.

I guess I'm out of date re: rsyslog-ca-cert -- would you ping me when you're
online so we can discuss?

https://codereview.appspot.com/94510043/diff/20001/worker/rsyslog/worker.go
File worker/rsyslog/worker.go (right):

https://codereview.appspot.com/94510043/diff/20001/worker/rsyslog/worker.go#n...
worker/rsyslog/worker.go:129: return h.st.WatchForRsyslogChanges(h.tag)
Do we need tag? I *think* it's implicit in the connection. Probably I'm wrong?

[mfoord, 2014-05-16 10:34:34]

On 2014/05/16 09:21:24, fwereade wrote:
> I think this is generally solid but I have a few questions.
> 
> https://codereview.appspot.com/94510043/diff/20001/state/api/params/params.go
> File state/api/params/params.go (right):
> 
>
https://codereview.appspot.com/94510043/diff/20001/state/api/params/params.go...
> state/api/params/params.go:687: Port      int
> /eyebrow
> 
> Why do we need both port and hostports?
> 

We could lose Port, we kept it mainly because that's how the *existing* system
works.

>
https://codereview.appspot.com/94510043/diff/20001/state/apiserver/rsyslog/rs...
> File state/apiserver/rsyslog/rsyslog.go (right):
> 
>
https://codereview.appspot.com/94510043/diff/20001/state/apiserver/rsyslog/rs...
> state/apiserver/rsyslog/rsyslog.go:90: watch := api.st.WatchAPIHostPorts()
> ISTM that this won't trigger on rsyslog-ca-cert changes.
> 
> I guess I'm out of date re: rsyslog-ca-cert -- would you ping me when you're
> online so we can discuss?
> 

So we had a big discussion about this issue (is watching APIHostPorts alone
sufficient or not)?
No-one wanted to commit to an answer, but the consensus seemed to be if we're
treating the rsyslog config entries as "immutable once set" then watching
APIHostPorts alone to trigger rsyslog config rewriting should be enough. And it
works for the scenarios we've manually tested:

* non-HA with unit deployed
* HA without units
* HA then units deployed
* non-HA with units deployed then ensure-availability

rsyslog config is rewritten in all these cases and logging happens correctly, so
the only wrinkle would be if the rsyslog config can be written out *before* the
certificate is set for the first time on bootstrap - in which case it wouldn't
be rewritten. When we asked nobody thought that was likely, but no-one would
*commit* to it being impossible, however "they" (jam and natefinch) did think
that if we're treating the rsyslog config as "immutable once set" then watching
just APIHostPorts *should* be sufficient. So that's what we did.

> https://codereview.appspot.com/94510043/diff/20001/worker/rsyslog/worker.go
> File worker/rsyslog/worker.go (right):
> 
>
https://codereview.appspot.com/94510043/diff/20001/worker/rsyslog/worker.go#n...
> worker/rsyslog/worker.go:129: return h.st.WatchForRsyslogChanges(h.tag)
> Do we need tag? I *think* it's implicit in the connection. Probably I'm wrong?

It is sent by other code, so we sent it. We don't need it in our API endpoint
itself.

[mfoord, 2014-05-16 14:09:28]

On 2014/05/16 10:34:34, mfoord wrote:
> On 2014/05/16 09:21:24, fwereade wrote:
> > I think this is generally solid but I have a few questions.
> > 
> >
https://codereview.appspot.com/94510043/diff/20001/state/api/params/params.go
> > File state/api/params/params.go (right):
> > 
> >
>
https://codereview.appspot.com/94510043/diff/20001/state/api/params/params.go...
> > state/api/params/params.go:687: Port      int
> > /eyebrow
> > 
> > Why do we need both port and hostports?
> > 
> 
> We could lose Port, we kept it mainly because that's how the *existing* system
> works.
> 

And in fact we store the rsyslogPort in the EnvironConfig, so we probably still
need it for backwards compatibility without a bigger refactor than *strictly*
necessary at this stage. If you'd like us to do that refactor now we can. 

We probably should still use the HostPorts for the template rendering which we
haven't yet done.

[mfoord, 2014-05-19 10:02:47]

https://codereview.appspot.com/94510043/diff/20001/worker/rsyslog/worker.go
File worker/rsyslog/worker.go (right):

https://codereview.appspot.com/94510043/diff/20001/worker/rsyslog/worker.go#n...
worker/rsyslog/worker.go:152: if cfg.Port == h.syslogPort && rsyslogCACert ==
h.rsyslogCACert {
This will actually screw us over. If APIHosts changes, but not the port nor the
cert, then the conf won't get rewritten. So this needs to change.

[mfoord, 2014-05-19 16:45:35]

On 2014/05/19 10:02:47, mfoord wrote:
> https://codereview.appspot.com/94510043/diff/20001/worker/rsyslog/worker.go
> File worker/rsyslog/worker.go (right):
> 
>
https://codereview.appspot.com/94510043/diff/20001/worker/rsyslog/worker.go#n...
> worker/rsyslog/worker.go:152: if cfg.Port == h.syslogPort && rsyslogCACert ==
> h.rsyslogCACert {
> This will actually screw us over. If APIHosts changes, but not the port nor
the
> cert, then the conf won't get rewritten. So this needs to change.


Note that I've removed this in a separate branch, along with an obsolete test
that now fails, in the following branch:

https://code.launchpad.net/~mfoord/juju-core/ha-rsyslog-shortcut-removal/+mer...

Andrew Wilkins (axw) is fairly certain that by the time the rsyslogworker is
created the port is in the environment, and starting the rsyslog worker already
ensures the ca-cert will be available, so just watching APIHostPorts should be
safe (regarding rsyslog config writing and restarting).

[wwitzel, 2014-05-20 14:17:16]

Please take a look.

[fwereade, 2014-05-21 08:06:30]

Last few changes, otherwise looking good. Thanks.

https://codereview.appspot.com/94510043/diff/40001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/94510043/diff/40001/state/api/params/params.go...
state/api/params/params.go:687: Port      int
please document that Port is just there for compatibility, and capable clients
should trust the HostPorts.

https://codereview.appspot.com/94510043/diff/40001/state/api/rsyslog/rsyslog.go
File state/api/rsyslog/rsyslog.go (right):

https://codereview.appspot.com/94510043/diff/40001/state/api/rsyslog/rsyslog....
state/api/rsyslog/rsyslog.go:20: Port      int
I think you can completely drop Port from this one, right?

https://codereview.appspot.com/94510043/diff/40001/state/apiserver/rsyslog/rs...
File state/apiserver/rsyslog/rsyslog.go (left):

https://codereview.appspot.com/94510043/diff/40001/state/apiserver/rsyslog/rs...
state/apiserver/rsyslog/rsyslog.go:33: func (api *RsyslogAPI)
SetRsyslogCert(args params.SetRsyslogCertParams) (params.ErrorResult, error) {
This still bugs me but it's not actionable right now.

https://codereview.appspot.com/94510043/diff/40001/state/apiserver/rsyslog/rs...
File state/apiserver/rsyslog/rsyslog.go (right):

https://codereview.appspot.com/94510043/diff/40001/state/apiserver/rsyslog/rs...
state/apiserver/rsyslog/rsyslog.go:27: if !authorizer.AuthMachineAgent() &&
!authorizer.AuthUnitAgent() {
do we have an AuthAgent? maybe we should

https://codereview.appspot.com/94510043/diff/40001/state/apiserver/rsyslog/rs...
state/apiserver/rsyslog/rsyslog.go:63: func (api *RsyslogAPI) GetRsyslogConfig()
(params.RsyslogConfigResult, error) {
Heh, almost missed this. Please make it bulky, like WatchForRsyslogChanges
below; I can't see when we'll need it but I'd prefer to remain consistent.

[mfoord, 2014-05-22 10:57:40]

On 2014/05/21 08:06:30, fwereade wrote:
> Last few changes, otherwise looking good. Thanks.
> 
> https://codereview.appspot.com/94510043/diff/40001/state/api/params/params.go
> File state/api/params/params.go (right):
> 
>
https://codereview.appspot.com/94510043/diff/40001/state/api/params/params.go...
> state/api/params/params.go:687: Port      int
> please document that Port is just there for compatibility, and capable clients
> should trust the HostPorts.
> 

The Port is there for state servers to know which port to listen on. It isn't
used by units. I've added a note to this effect.

>
https://codereview.appspot.com/94510043/diff/40001/state/api/rsyslog/rsyslog.go
> File state/api/rsyslog/rsyslog.go (right):
> 
>
https://codereview.appspot.com/94510043/diff/40001/state/api/rsyslog/rsyslog....
> state/api/rsyslog/rsyslog.go:20: Port      int
> I think you can completely drop Port from this one, right?
> 

As discussed we can't drop this until state servers have a mechanism for picking
a port and storing it in the state, so that GetRsyslogConfig can then return the
right information in HostPorts. I've added a note that the Port is only for
state servers to know which port to listen on.

>
https://codereview.appspot.com/94510043/diff/40001/state/apiserver/rsyslog/rs...
> File state/apiserver/rsyslog/rsyslog.go (left):
> 
>
https://codereview.appspot.com/94510043/diff/40001/state/apiserver/rsyslog/rs...
> state/apiserver/rsyslog/rsyslog.go:33: func (api *RsyslogAPI)
> SetRsyslogCert(args params.SetRsyslogCertParams) (params.ErrorResult, error) {
> This still bugs me but it's not actionable right now.
> 

Right.

>
https://codereview.appspot.com/94510043/diff/40001/state/apiserver/rsyslog/rs...
> File state/apiserver/rsyslog/rsyslog.go (right):
> 
>
https://codereview.appspot.com/94510043/diff/40001/state/apiserver/rsyslog/rs...
> state/apiserver/rsyslog/rsyslog.go:27: if !authorizer.AuthMachineAgent() &&
> !authorizer.AuthUnitAgent() {
> do we have an AuthAgent? maybe we should
> 

We don't. Adding it was more trouble that it was worth for a single use case.
John is working on making srvRoot more testable, so if we see another use case
for this method it can be added then.

>
https://codereview.appspot.com/94510043/diff/40001/state/apiserver/rsyslog/rs...
> state/apiserver/rsyslog/rsyslog.go:63: func (api *RsyslogAPI)
GetRsyslogConfig()
> (params.RsyslogConfigResult, error) {
> Heh, almost missed this. Please make it bulky, like WatchForRsyslogChanges
> below; I can't see when we'll need it but I'd prefer to remain consistent.

Ok, working on this. Also some test failures I see with the current state of
this branch.

[mfoord, 2014-05-22 15:25:43]

Unfortunately we mangled this branch, so will repropose from this branch:

https://code.launchpad.net/~mfoord/juju-core/ha-rsyslog-good/+merge/220669

[mfoord, 2014-05-22 16:11:31]

On 2014/05/22 15:25:43, mfoord wrote:
> Unfortunately we mangled this branch, so will repropose from this branch:
> 
> https://code.launchpad.net/~mfoord/juju-core/ha-rsyslog-good/+merge/220669

Superseded by: https://codereview.appspot.com/91630045/