deployer: Compatibility fixes

deployer: Compatibility fixes

This fixes an omission made in r1347: now the
upstart conf files in /etc/init are created like
this: jujud-<unit-tag>.conf (e.g. jujud-unit-mysql-0.conf).
Before the name used to contain a deployer tag (e.g.
jujud-machine-0:unit-mysql-0.conf). Because of the
change, without this branch after upgrading an
existing juju environment to the current version or
later, previously deployed units won't be visible
to the (now single) deployer.
This fixes that and allows both newly deployed and
previously deployed units to be discovered and
recalled.

https://code.launchpad.net/~dimitern/juju-core/061-deployer-fix-upstart-conf-detection/+merge/172099

(do not edit description out of merge proposal)

[dimitern, 2013-06-28 16:28:17]

Please take a look.

[mue, 2013-06-28 17:24:50]

LGTM, cool work with helpful comments at the right places. ;)

[dimitern, 2013-06-28 18:08:17]

Please take a look.

[fwereade, 2013-06-28 22:24:10]

LGTM modulo quibbles below. I'd rather have this landed for the release and
fixed later than not landed, so I'm off to approve this right now.

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

https://codereview.appspot.com/10746044/diff/1/cmd/jujud/agent.go#oldcode234
cmd/jujud/agent.go:234: // is no distinction between principal and subordinate
deployments)
drop this comment, I think

https://codereview.appspot.com/10746044/diff/1/log/syslog/cmd.go
File log/syslog/cmd.go (right):

https://codereview.appspot.com/10746044/diff/1/log/syslog/cmd.go#newcode17
log/syslog/cmd.go:17: return fmt.Errorf("must be root")
Hmm, can we maybe patch this out for tests?

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

https://codereview.appspot.com/10746044/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:153: func (ctx *SimpleContext)
findCompatibleUpstartJob(unitName string) *upstart.Service {
just findUpstartJob, perhaps?

https://codereview.appspot.com/10746044/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:187: logger.Warningf("installer: cannot restart syslog
daemon: %v", err)
Yeah, definitely better to patch out "restart". We do it for start/stop already,
I think?

That said, I guess we didn't already have that, so this isn't a regression.

https://codereview.appspot.com/10746044/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:220: for unitName, _ := range unitsAndJobs {
for unitName := range unitsAndJobs {

[dimitern, 2013-06-29 11:17:58]

On 2013/06/28 22:24:10, fwereade wrote:
> LGTM modulo quibbles below. I'd rather have this landed for the release and
> fixed later than not landed, so I'm off to approve this right now.
> 
> https://codereview.appspot.com/10746044/diff/1/cmd/jujud/agent.go
> File cmd/jujud/agent.go (left):
> 
> https://codereview.appspot.com/10746044/diff/1/cmd/jujud/agent.go#oldcode234
> cmd/jujud/agent.go:234: // is no distinction between principal and subordinate
> deployments)
> drop this comment, I think
> 
> https://codereview.appspot.com/10746044/diff/1/log/syslog/cmd.go
> File log/syslog/cmd.go (right):
> 
> https://codereview.appspot.com/10746044/diff/1/log/syslog/cmd.go#newcode17
> log/syslog/cmd.go:17: return fmt.Errorf("must be root")
> Hmm, can we maybe patch this out for tests?
> 
> https://codereview.appspot.com/10746044/diff/1/worker/deployer/simple.go
> File worker/deployer/simple.go (right):
> 
>
https://codereview.appspot.com/10746044/diff/1/worker/deployer/simple.go#newc...
> worker/deployer/simple.go:153: func (ctx *SimpleContext)
> findCompatibleUpstartJob(unitName string) *upstart.Service {
> just findUpstartJob, perhaps?
> 
>
https://codereview.appspot.com/10746044/diff/1/worker/deployer/simple.go#newc...
> worker/deployer/simple.go:187: logger.Warningf("installer: cannot restart
syslog
> daemon: %v", err)
> Yeah, definitely better to patch out "restart". We do it for start/stop
already,
> I think?
> 
> That said, I guess we didn't already have that, so this isn't a regression.
> 
>
https://codereview.appspot.com/10746044/diff/1/worker/deployer/simple.go#newc...
> worker/deployer/simple.go:220: for unitName, _ := range unitsAndJobs {
> for unitName := range unitsAndJobs {

Thanks for the review and for approving and landing this.
I'll implement your suggestions in the next CL.

[jameinel, 2013-07-09 06:25:13]

Could this be related to the bug that Frank was investigating (upgrading 1.10 =>
trunk kills everything)?