Add a worker to update loggo configuration.

Add a worker to update loggo configuration.

With the logging config now specified in the
environment configuration, we can watch to
see if it changes.  If it does, this worker
updates the loggo configuration.

You can now change the logging levels of a
running juju environment using:
  juju set-env 'logging-config=<root>=WARNING;juju=INFO;juju.api=DEBUG'

https://code.launchpad.net/~thumper/juju-core/logger-worker/+merge/185421

(do not edit description out of merge proposal)

[thumper, 2013-09-13 05:34:36]

Please take a look.

[fwereade, 2013-09-13 10:06:59]

LGTM in itself, but I feel the agent.Config stuff is risky, and want to know
what the plan is there.

https://codereview.appspot.com/13587045/diff/1/worker/logger/logger.go
File worker/logger/logger.go (right):

https://codereview.appspot.com/13587045/diff/1/worker/logger/logger.go#newcode57
worker/logger/logger.go:57: if err := logger.agentConfig.Write(); err != nil {
The way I see it, we have now used our one strike. We can't write anything else
to the agent config now, because any two writes can race and cause one to be
overwritten... but nothing has happened around the config itself that might tip
us off to that fact.

And I'm really worried that we *won't* write agent config soon enough that
anyone remembers this pitfall exists when we come to do so. Do you have any
suggestions for mitigation?

[fwereade, 2013-09-13 14:03:19]

NOT LGTM I'm afraid. I know it's just writing a field that's not currently read
by other things, but there's literally no way to tell it's dangerous.

https://codereview.appspot.com/13587045/diff/1/worker/logger/logger.go
File worker/logger/logger.go (right):

https://codereview.appspot.com/13587045/diff/1/worker/logger/logger.go#newcode57
worker/logger/logger.go:57: if err := logger.agentConfig.Write(); err != nil {
On 2013/09/13 10:06:59, fwereade wrote:
> The way I see it, we have now used our one strike. We can't write anything
else
> to the agent config now, because any two writes can race and cause one to be
> overwritten... but nothing has happened around the config itself that might
tip
> us off to that fact.
> 
> And I'm really worried that we *won't* write agent config soon enough that
> anyone remembers this pitfall exists when we come to do so. Do you have any
> suggestions for mitigation?

Actually it's worse... this write is past our zeroth strike, because this object
is shared by multiple goroutines. I think that config needs fixes before this
can go in.

[thumper, 2013-09-15 23:05:14]

Please take a look.

https://codereview.appspot.com/13587045/diff/1/worker/logger/logger.go
File worker/logger/logger.go (right):

https://codereview.appspot.com/13587045/diff/1/worker/logger/logger.go#newcode57
worker/logger/logger.go:57: if err := logger.agentConfig.Write(); err != nil {
On 2013/09/13 14:03:20, fwereade wrote:
> On 2013/09/13 10:06:59, fwereade wrote:
> > The way I see it, we have now used our one strike. We can't write anything
> else
> > to the agent config now, because any two writes can race and cause one to be
> > overwritten... but nothing has happened around the config itself that might
> tip
> > us off to that fact.
> > 
> > And I'm really worried that we *won't* write agent config soon enough that
> > anyone remembers this pitfall exists when we come to do so. Do you have any
> > suggestions for mitigation?
> 
> Actually it's worse... this write is past our zeroth strike, because this
object
> is shared by multiple goroutines. I think that config needs fixes before this
> can go in.

No, I don't see any problem here at all.

In order to avoid the controversy at this stage, I have moved the actual saving
of the updated logging config to a different branch so we can continue the
discussion there and not block the dynamic logging changes.

Ok, so here is what happens to the agent config when changes are made.  There is
exactly one copy of the agent config instance that is passed between the
different go routines and workers inside the unit and machine agents.  Any
update to the values stored in the the agent are protected by the agent mutex
for both writes and reads of that config value.  One worker writing to a value
and another worker reading from that value has no race conditions.  It is
immediately available in the other go routine once the mutex is unlocked.

Since the write is also protected by the mutex, there is no possibility of
overwriting the changes made by a different worker.  Ok, there is one
possibility, where two different workers are trying to modify and save one
value, but that is somewhat outside the scope of the agent config as it is a
normal race condition that needs to be handled by the competing tasks and has
nothing to do with the synchronization of the agent config itself.  Also, not an
issue here.

Why do you think that once we have written something, we can't write something
else?  This doesn't make sense to me and I can't see where you are coming from. 
Can you please explain?

The config has fixes around synchronization of writes and setting and getting
values.  What other fixes do you think are needed?

What are your concerns about this object being used in multiple go routines?

[thumper, 2013-09-15 23:35:28]

Also had a chat with Ian about this just now.

We have decided that we don't actually need to save the new logging config in
the agent config at all.  The main use case for it was for dealing with the
initial debugging of the agent process starting prior to the worker getting the
initial config over the API.  My next branch handles this by passing the
information over the command line in the upstart script.

Since this initial logging config is used only until the worker starts and gets
the updated config, we don't really need to save it at all.  Which does make me
a little frustrated that I spent all that time futzing with the config, but at
least it is in a cleaner state now.

[thumper, 2013-09-15 23:44:53]

Please take a look.

[fwereade, 2013-09-19 09:01:53]

LGTM. Sorry about the frustration -- maybe we can argue about appropriate use of
a Config over beers at some point :).