worker/deployer: add flag to control syslog fwd

worker/deployer: add flag to control syslog fwd

- Add flag to NewSimpleContext to enable syslog forwarding
- Add util function in api/agent, HasJobs(*Entity, params.MachineJob)
- Update jujud to make use of these

https://code.launchpad.net/~axwalk/juju-core/lp1211147-deployer-check-isstateserver/+merge/180280

(do not edit description out of merge proposal)

[axw, 2013-08-15 04:07:08]

Please take a look.

[fwereade, 2013-08-15 10:04:15]

I'm not sure how unit logs on state servers will get to all-machines.log with
this change. Not saying they won't, but a comment explaining how would be
helpful. A few more thoughts follow:

https://codereview.appspot.com/12852044/diff/1/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/12852044/diff/1/cmd/jujud/agent.go#newcode241
cmd/jujud/agent.go:241: forwardSyslog := !apiagent.HasJob(machine,
params.JobManageState)
It feels like it would be better to calculate this outside this method -- ie not
to add agent-specific code inside deployer (which is otherwise just using the
deployer api) -- and just pass straight through to newDeployContext.

https://codereview.appspot.com/12852044/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (left):

https://codereview.appspot.com/12852044/diff/1/cmd/jujud/machine.go#oldcode174
cmd/jujud/machine.go:174: case params.JobHostUnits:
I can see that it's a little awkward to do the HasJob thing here, but I still
think it's better (because this method is all about agent-level stuff, and it's
nice not to have to spread that concern down into newDeployer).

https://codereview.appspot.com/12852044/diff/1/state/api/agent/utils.go
File state/api/agent/utils.go (right):

https://codereview.appspot.com/12852044/diff/1/state/api/agent/utils.go#newco...
state/api/agent/utils.go:11: func HasJob(e *Entity, job params.MachineJob) bool
{
Might this look better as a method on Entity? YMMV.

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

https://codereview.appspot.com/12852044/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:195: if ctx.forwardSyslog {
Hmmmmmmmm. In the context of machines possibly changing jobs in future, it might
be better to do the removal based only on existence -- we *might* be running a
non-forwarding deployer that had in the distant past installed a unit that does
forward.

It's a bit speculative, but if there's no obvious reason not to do this I think
I'd prefer it.

[axw1, 2013-08-16 02:50:12]

On 2013/08/15 10:04:15, fwereade wrote:
> I'm not sure how unit logs on state servers will get to all-machines.log with
> this change. Not saying they won't, but a comment explaining how would be
> helpful. A few more thoughts follow:

They don't, actually. In my haste to fix the original bug, I missed them
altogether. Thanks for picking that up. I'll look into fixing that and then
request another review.

> https://codereview.appspot.com/12852044/diff/1/cmd/jujud/agent.go
> File cmd/jujud/agent.go (right):
> 
> https://codereview.appspot.com/12852044/diff/1/cmd/jujud/agent.go#newcode241
> cmd/jujud/agent.go:241: forwardSyslog := !apiagent.HasJob(machine,
> params.JobManageState)
> It feels like it would be better to calculate this outside this method -- ie
not
> to add agent-specific code inside deployer (which is otherwise just using the
> deployer api) -- and just pass straight through to newDeployContext.

Done.

> https://codereview.appspot.com/12852044/diff/1/cmd/jujud/machine.go
> File cmd/jujud/machine.go (left):
> 
> https://codereview.appspot.com/12852044/diff/1/cmd/jujud/machine.go#oldcode174
> cmd/jujud/machine.go:174: case params.JobHostUnits:
> I can see that it's a little awkward to do the HasJob thing here, but I still
> think it's better (because this method is all about agent-level stuff, and
it's
> nice not to have to spread that concern down into newDeployer).
> 
> https://codereview.appspot.com/12852044/diff/1/state/api/agent/utils.go
> File state/api/agent/utils.go (right):
> 
>
https://codereview.appspot.com/12852044/diff/1/state/api/agent/utils.go#newco...
> state/api/agent/utils.go:11: func HasJob(e *Entity, job params.MachineJob)
bool
> {
> Might this look better as a method on Entity? YMMV.

I am slightly torn. Originally I was going to make it a method, but my rationale
for not doing so was:
  - The function doesn't need access to anything internal to the Entity struct,
and
  - Entity is pretty minimal, and I didn't want set a precedent for bloating,
just for a utility method.

If you have a strong preference for making it a method, please let me know.

> https://codereview.appspot.com/12852044/diff/1/worker/deployer/simple.go
> File worker/deployer/simple.go (right):
> 
>
https://codereview.appspot.com/12852044/diff/1/worker/deployer/simple.go#newc...
> worker/deployer/simple.go:195: if ctx.forwardSyslog {
> Hmmmmmmmm. In the context of machines possibly changing jobs in future, it
might
> be better to do the removal based only on existence -- we *might* be running a
> non-forwarding deployer that had in the distant past installed a unit that
does
> forward.
> 
> It's a bit speculative, but if there's no obvious reason not to do this I
think
> I'd prefer it.

Good point. I suspect I'll need to change this all a bit anyway, so that a file
is always written out, but the contents differ depending on whether or not it's
a state machine. If it remains the same, I'll work something out.

[axw1, 2013-08-19 02:48:47]

On 2013/08/16 02:50:12, axw1 wrote:
> On 2013/08/15 10:04:15, fwereade wrote:
> > I'm not sure how unit logs on state servers will get to all-machines.log
with
> > this change. Not saying they won't, but a comment explaining how would be
> > helpful. A few more thoughts follow:
> 
> They don't, actually. In my haste to fix the original bug, I missed them
> altogether. Thanks for picking that up. I'll look into fixing that and then
> request another review.
> 
> > https://codereview.appspot.com/12852044/diff/1/cmd/jujud/agent.go
> > File cmd/jujud/agent.go (right):
> > 
> > https://codereview.appspot.com/12852044/diff/1/cmd/jujud/agent.go#newcode241
> > cmd/jujud/agent.go:241: forwardSyslog := !apiagent.HasJob(machine,
> > params.JobManageState)
> > It feels like it would be better to calculate this outside this method -- ie
> not
> > to add agent-specific code inside deployer (which is otherwise just using
the
> > deployer api) -- and just pass straight through to newDeployContext.
> 
> Done.
> 
> > https://codereview.appspot.com/12852044/diff/1/cmd/jujud/machine.go
> > File cmd/jujud/machine.go (left):
> > 
> >
https://codereview.appspot.com/12852044/diff/1/cmd/jujud/machine.go#oldcode174
> > cmd/jujud/machine.go:174: case params.JobHostUnits:
> > I can see that it's a little awkward to do the HasJob thing here, but I
still
> > think it's better (because this method is all about agent-level stuff, and
> it's
> > nice not to have to spread that concern down into newDeployer).
> > 
> > https://codereview.appspot.com/12852044/diff/1/state/api/agent/utils.go
> > File state/api/agent/utils.go (right):
> > 
> >
>
https://codereview.appspot.com/12852044/diff/1/state/api/agent/utils.go#newco...
> > state/api/agent/utils.go:11: func HasJob(e *Entity, job params.MachineJob)
> bool
> > {
> > Might this look better as a method on Entity? YMMV.
> 
> I am slightly torn. Originally I was going to make it a method, but my
rationale
> for not doing so was:
>   - The function doesn't need access to anything internal to the Entity
struct,
> and
>   - Entity is pretty minimal, and I didn't want set a precedent for bloating,
> just for a utility method.
> 
> If you have a strong preference for making it a method, please let me know.
> 
> > https://codereview.appspot.com/12852044/diff/1/worker/deployer/simple.go
> > File worker/deployer/simple.go (right):
> > 
> >
>
https://codereview.appspot.com/12852044/diff/1/worker/deployer/simple.go#newc...
> > worker/deployer/simple.go:195: if ctx.forwardSyslog {
> > Hmmmmmmmm. In the context of machines possibly changing jobs in future, it
> might
> > be better to do the removal based only on existence -- we *might* be running
a
> > non-forwarding deployer that had in the distant past installed a unit that
> does
> > forward.
> > 
> > It's a bit speculative, but if there's no obvious reason not to do this I
> think
> > I'd prefer it.
> 
> Good point. I suspect I'll need to change this all a bit anyway, so that a
file
> is always written out, but the contents differ depending on whether or not
it's
> a state machine. If it remains the same, I'll work something out.

I'm abandoning this, as I've found a vastly simpler fix to the problem. The
original config should have worked, it seems there's either a bug or just weird
behaviour in rsyslog.