uniter: fix intermittent failure

uniter: fix intermittent failure

I couldn't repro it here, but was consistently seeing failures on the bot in
which the verifyFile target had empty content, which seemed to imply a race
between reading and writing it once it was determined to exist. If that's
the problem, this should fix it.

https://code.launchpad.net/~fwereade/juju-core/intermittent-juju-run-failure/+merge/206353

(do not edit description out of merge proposal)

[fwereade, 2014-02-14 08:41:14]

Please take a look.

[rog, 2014-02-14 11:29:44]

LGTM although really I question why asyncRunCommands is asynchronous at all.

https://codereview.appspot.com/63710044/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/63710044/diff/1/worker/uniter/uniter_test.go#n...
worker/uniter/uniter_test.go:852: return fmt.Sprintf("echo juju run
${JUJU_UNIT_NAME} > %s.tmp; mv %s.tmp %s", path, path, path)
alternatively:

return fmt.Sprintf("echo juju run ${JUJU_UNIT_NAME} > %[1]s.tmp; mv %[1]s.tmp
%[1]s", filepath.Join(testDir, name)

[fwereade, 2014-02-14 12:21:17]

https://codereview.appspot.com/63710044/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/63710044/diff/1/worker/uniter/uniter_test.go#n...
worker/uniter/uniter_test.go:852: return fmt.Sprintf("echo juju run
${JUJU_UNIT_NAME} > %s.tmp; mv %s.tmp %s", path, path, path)
On 2014/02/14 11:29:44, rog wrote:
> alternatively:
> 
> return fmt.Sprintf("echo juju run ${JUJU_UNIT_NAME} > %[1]s.tmp; mv %[1]s.tmp
> %[1]s", filepath.Join(testDir, name)
> 

nicer, thanks

[jameinel, 2014-02-17 07:05:08]

some questions about rationale, but just discussion points.

https://codereview.appspot.com/63710044/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/63710044/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:75: _, name, err := names.ParseTag(tag,
names.UnitTagKind)
This seems like you could end up missing NotFound errors that you might want to
get. Is that a safe change?

I guess you could assume units won't be calling this without knowing what it is,
and maybe we've already validated it elsewhere?

[william.reade, 2014-02-17 09:03:42]

https://codereview.appspot.com/63710044/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/63710044/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:75: _, name, err := names.ParseTag(tag,
names.UnitTagKind)
On 2014/02/17 07:05:09, jameinel wrote:
> This seems like you could end up missing NotFound errors that you might want
to
> get. Is that a safe change?
> 
> I guess you could assume units won't be calling this without knowing what it
is,
> and maybe we've already validated it elsewhere?

We'll still get NotFounds from the code below; the point of this change is that
if we haven't validated the tags properly (which we generally haven't) we'll get
nice wrong-entity-type errors instead of panicking the API server.