juju status: add service/unit filters

juju status: add service/unit filters

Any positional arguments after juju status are
now interpreted as filters for either service or
unit. If the filter includes a '/', then it is
considered a unit filter, else a service filter.
Service filters are transformed to unit filters
by appending '/*'.

A unit passes a filter if one of the following
conditions is true:
 - Its name matches the pattern.
 - The unit is a principal, and one of its
   subordinates matches the pattern.
 - The unit is a subordinate, and its principal
   matches the pattern.
  
If all units of a service are filtered out, then
the service is filtered out; if all units of a
machine are filtered out, the machine is filtered
out (except to fill out the container hierarchy).

Unlike in pyjuju, this implementation allows
only the '*' wildcard in patterns. '?' and
character ranges are not permitted.

Partially fixes bug 1121916

https://code.launchpad.net/~axwalk/juju-core/lp1121916-juju-status-filters/+merge/178004

(do not edit description out of merge proposal)

[axw, 2013-08-01 06:24:33]

Please take a look.

[gz, 2013-08-01 15:39:18]

Rietveld has screwed the diff, you'll need to run `lbox propose -cr -v` again
which should fix it.

One comment from looking at the changes on launchpad, this code ignores a
potential ErrBadPattern from path.Match() - seems like for user sanity it might
be nice to propogate that up to where they can see it. At the least I'd like to
see a test that has a borked pattern (unmatched [ or similar should do) and
asserts that things don't blow up.

[axw1, 2013-08-02 02:31:15]

On 2013/08/01 15:39:18, gz wrote:
> Rietveld has screwed the diff, you'll need to run `lbox propose -cr -v` again
> which should fix it.

Odd - the diffs show up fine to me. I've re-proposed anyway.

> One comment from looking at the changes on launchpad, this code ignores a
> potential ErrBadPattern from path.Match() - seems like for user sanity it
might
> be nice to propogate that up to where they can see it. At the least I'd like
to
> see a test that has a borked pattern (unmatched [ or similar should do) and
> asserts that things don't blow up.

Done. Go's path.Match function checks validity lazily, and I didn't think it
worthwhile expending effort to validate it ahead of time. Thus, pattern validity
will not be checked if it is never considered (i.e. a preceding pattern
matches). I've codified this in a test.

[axw, 2013-08-02 02:34:16]

Please take a look.

[fwereade, 2013-08-02 16:25:03]

Thanks, this is a great start, but we need to think about subordinates in a bit
more detail. Thoughts below.

https://codereview.appspot.com/12235043/diff/7001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/12235043/diff/7001/cmd/juju/status.go#newcode113
cmd/juju/status.go:113: mid, err := unit.AssignedMachineId()
There's something here that demands a bit more thought. 

* principal units have to(?) be included if the filter matches one of their
subordinates (because they're displayed in the principal's "subordinates" field)
* subordinate units should be included if the filter matches their principal
(because otherwise we're stripping relevant info from the requested unit)
* if a principal is included only because one of its subordinates is, we
probably shouldn't display its other subordinates, because they'renot relevant
to the user's request
* AssignedMachineId is expensive for subordinate units, because it roundtrips to
get the machine id from the principal

I *think* that if we tweak unit filtering as above (which remains open to
argument, but is I think consistent with the analogous behaviour for machines)
we can just skip all subordinate units here and still be correct, and not waste
any further time chatting to the database.

[axw, 2013-08-05 10:18:34]

Please take a look.

[axw1, 2013-08-05 10:19:11]

https://codereview.appspot.com/12235043/diff/7001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/12235043/diff/7001/cmd/juju/status.go#newcode113
cmd/juju/status.go:113: mid, err := unit.AssignedMachineId()
On 2013/08/02 16:25:03, fwereade wrote:
> There's something here that demands a bit more thought. 
> 
> * principal units have to(?) be included if the filter matches one of their
> subordinates (because they're displayed in the principal's "subordinates"
field)
> * subordinate units should be included if the filter matches their principal
> (because otherwise we're stripping relevant info from the requested unit)
> * if a principal is included only because one of its subordinates is, we
> probably shouldn't display its other subordinates, because they'renot relevant
> to the user's request
> * AssignedMachineId is expensive for subordinate units, because it roundtrips
to
> get the machine id from the principal
> 
> I *think* that if we tweak unit filtering as above (which remains open to
> argument, but is I think consistent with the analogous behaviour for machines)
> we can just skip all subordinate units here and still be correct, and not
waste
> any further time chatting to the database.

Done. Your suggestions SGTM, so I've implemented them.

[rog, 2013-08-05 14:23:01]

I like the way this is going - I think the matching
semantics are nice and will be useful in practice
(although it might be nice to have a way of showing
info on a single unit, ignoring its machine and principal/subs - that can easily
be done later)

I have a few suggestions below.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode29
cmd/juju/status.go:29: scope []string
"scope" seems like a slightly odd name here.
"patterns" perhaps?

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode32
cmd/juju/status.go:32: var statusDoc = "This command will report on the runtime
state of various system entities."
I think we should add some documentation on the pattern-matching behaviour here.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode36
cmd/juju/status.go:36: Name:    "status",
Args: "[pattern...]",

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode69
cmd/juju/status.go:69: func (m *unitMatcher) match(u *state.Unit, subordinateTo
string) (bool, error) {
I'm not sure we need the subordinateTo argument here.
A unit already knows its own principal unit; we
could either use u.DeployerTag to find it out,
or implement a trivial Unit.PrincipalName
function in state.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode97
cmd/juju/status.go:97: return false, err
we usually use a colon only for a following error.
perhaps:

return false, fmt.Errorf("pattern syntax error in %q", pattern)

?

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode105
cmd/juju/status.go:105: func processScope(scope []string) (m *unitMatcher) {
It'd be nice to see a comment explaining the purpose of this function here.

Rather than have the lazy initialisation, it might be
nicer to have something like this:

// newUnitMatcher returns a unitMatcher
// that matches units with the specified patterns.
func newUnitMatcher(patterns []string) *unitMatcher

// matchesAll reports whether the unit matcher
// matches any unit.
func (um *unitMatcher) matchesAny() bool

Then in fetchAllServicesAndUnits, you can call
um.matchesAny rather than using the somewhat
oblique um!=nil test (assuming I have the
intent correct)

Alternatively continue to use the ==nil test
and only call newUnitMatcher if len(patterns)>0.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode137
cmd/juju/status.go:137: mid, err := unit.AssignedMachineId()
I think we should try to avoid calling for subordinate units, because we have
all the information in the principal units.

Since we are guaranteed to have a principal unit if the unit matcher has matched
its subordinate, we could just do:

if unit.IsPrincipal() {
      mid, err := unit.AssignedMachineId()
      // The only error we can get from AssignedMachineId
      // on a principal unit is if there is no machine id
      // assigned to the unit.
      if err == nil {
          machineIds.Add(mid)
      }
}

You might want to move some or all the logic in this um!=nil
block into a separate function (see also my suggestion in fetchMachines
below)

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode140
cmd/juju/status.go:140: }
I don't think we should be throwing away the error here.
It might represent a genuine network error.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode190
cmd/juju/status.go:190: for _, id := range machineIds.SortedValues() {
Any particular reason we need to call SortedValues here rather than Values?

In fact, I'm not really sure we need to do this
here - can't we do this in the loop where machineIds
is first populated?

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode242
cmd/juju/status.go:242: // the subordinateTo relationships.
I'm not sure I see this. Each principal unit knows its
own subordinates, and each subordinate unit knows its
own principal unit (see my comment in unitMatcher.match)

Given this, I think we can filter the
units before they get put into the map,
and lose the new code below.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode414
cmd/juju/status.go:414: if svcUnitMap, ok := context.units[serviceName]; ok {
I'm not sure this needs to change. The old code works
just fine if context.units[serviceName]==nil, doesn't it?

https://codereview.appspot.com/12235043/diff/18001/testing/repo/series/monito...
File testing/repo/series/monitoring/revision (right):

https://codereview.appspot.com/12235043/diff/18001/testing/repo/series/monito...
testing/repo/series/monitoring/revision:1: 1
is this necessary?

[axw1, 2013-08-06 04:06:09]

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode29
cmd/juju/status.go:29: scope []string
On 2013/08/05 14:23:02, rog wrote:
> "scope" seems like a slightly odd name here.
> "patterns" perhaps?

Done. I was following pyjuju, but I agree that it sounds slightly odd.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode32
cmd/juju/status.go:32: var statusDoc = "This command will report on the runtime
state of various system entities."
On 2013/08/05 14:23:02, rog wrote:
> I think we should add some documentation on the pattern-matching behaviour
here.

Done.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode69
cmd/juju/status.go:69: func (m *unitMatcher) match(u *state.Unit, subordinateTo
string) (bool, error) {
On 2013/08/05 14:23:02, rog wrote:
> I'm not sure we need the subordinateTo argument here.
> A unit already knows its own principal unit; we
> could either use u.DeployerTag to find it out,
> or implement a trivial Unit.PrincipalName
> function in state.

Good point, I've added Unit.PrincipalName.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode97
cmd/juju/status.go:97: return false, err
On 2013/08/05 14:23:02, rog wrote:
> we usually use a colon only for a following error.
> perhaps:
> 
> return false, fmt.Errorf("pattern syntax error in %q", pattern)
> 
> ?

Done.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode105
cmd/juju/status.go:105: func processScope(scope []string) (m *unitMatcher) {
On 2013/08/05 14:23:02, rog wrote:
> It'd be nice to see a comment explaining the purpose of this function here.
> 
> Rather than have the lazy initialisation, it might be
> nicer to have something like this:
> 
> // newUnitMatcher returns a unitMatcher
> // that matches units with the specified patterns.
> func newUnitMatcher(patterns []string) *unitMatcher
> 
> // matchesAll reports whether the unit matcher
> // matches any unit.
> func (um *unitMatcher) matchesAny() bool
> 
> Then in fetchAllServicesAndUnits, you can call
> um.matchesAny rather than using the somewhat
> oblique um!=nil test (assuming I have the
> intent correct)
> 
> Alternatively continue to use the ==nil test
> and only call newUnitMatcher if len(patterns)>0.

Done.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode137
cmd/juju/status.go:137: mid, err := unit.AssignedMachineId()
On 2013/08/05 14:23:02, rog wrote:
> I think we should try to avoid calling for subordinate units, because we have
> all the information in the principal units.
> 
> Since we are guaranteed to have a principal unit if the unit matcher has
matched
> its subordinate, we could just do:
> 
> if unit.IsPrincipal() {
>       mid, err := unit.AssignedMachineId()
>       // The only error we can get from AssignedMachineId
>       // on a principal unit is if there is no machine id
>       // assigned to the unit.
>       if err == nil {
>           machineIds.Add(mid)
>       }
> }
> 
> You might want to move some or all the logic in this um!=nil
> block into a separate function (see also my suggestion in fetchMachines
> below)

Done, created a a new function fetchUnitMachineIds.
The !=nil check is replaced with matchesAny, but remains here (in Run).
Passing the matcher to fetchUnitMachineIds seemed a bit awkward.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode140
cmd/juju/status.go:140: }
On 2013/08/05 14:23:02, rog wrote:
> I don't think we should be throwing away the error here.
> It might represent a genuine network error.

Done.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode190
cmd/juju/status.go:190: for _, id := range machineIds.SortedValues() {
On 2013/08/05 14:23:02, rog wrote:
> Any particular reason we need to call SortedValues here rather than Values?
> 
> In fact, I'm not really sure we need to do this
> here - can't we do this in the loop where machineIds
> is first populated?

Nope, a holdover. Moved to fetchUnitMachineIds.

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode242
cmd/juju/status.go:242: // the subordinateTo relationships.
On 2013/08/05 14:23:02, rog wrote:
> I'm not sure I see this. Each principal unit knows its
> own subordinates, and each subordinate unit knows its
> own principal unit (see my comment in unitMatcher.match)
> 
> Given this, I think we can filter the
> units before they get put into the map,
> and lose the new code below.

Done. Much cleaner, thanks!

https://codereview.appspot.com/12235043/diff/18001/cmd/juju/status.go#newcode414
cmd/juju/status.go:414: if svcUnitMap, ok := context.units[serviceName]; ok {
On 2013/08/05 14:23:02, rog wrote:
> I'm not sure this needs to change. The old code works
> just fine if context.units[serviceName]==nil, doesn't it?

Done.

https://codereview.appspot.com/12235043/diff/18001/testing/repo/series/monito...
File testing/repo/series/monitoring/revision (right):

https://codereview.appspot.com/12235043/diff/18001/testing/repo/series/monito...
testing/repo/series/monitoring/revision:1: 1
On 2013/08/05 14:23:02, rog wrote:
> is this necessary?

Apparently not. I've removed the file, and updated status_test.go to expect
revno=0. Thanks.

[axw, 2013-08-06 04:08:20]

Please take a look.

[rog, 2013-08-07 17:02:30]

LGTM with the second fix below, and one other passing thought.

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode130
cmd/juju/status.go:130: ok, err := path.Match(pattern, s)
It's a pity that because path.Match is lazy
about checking pattern validity, we need to return an
error all the way back.

For consideration: validate the glob patterns
with a regexp, and then we can panic here instead,
and lose the error returns from matchString
and matchUnit (and provide more reliable errors
to the user too)

Something like:

^([^\\[]|\[(\\.|[^-\]]-[^-\]])*\]|\\.)*$

might be sufficient.

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode277
cmd/juju/status.go:277: mid, err := unit.AssignedMachineId()
This still isn't quite right - it should only call AssignedMachineId if the unit
is a principal (that way it doesn't need to do a network call)

just:

if !unit.IsPrincipal() {
    continue
}

should do the job.

[fwereade, 2013-08-07 17:06:53]

LGTM, thanks -- sorry for the delay, it's a busy week.

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode47
cmd/juju/status.go:47: if and when they are tried.`[1:]
Maybe add a little bit more explaining which bits are filtered out (and which
bits are not).

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode52
cmd/juju/status.go:52: Args:    "[pattern...]",
s/n./n ./

?

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode116
cmd/juju/status.go:116: } else {
lose the else?

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status_test.go#ne...
cmd/juju/status_test.go:34: stderr = ctx.Stderr.(*bytes.Buffer).String()
I'd prefer it if stderr and stdout were consistently typed...

https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status_test.go#ne...
cmd/juju/status_test.go:170: "hardware":    "arch=amd64 cpu-cores=1 mem=1024M",
Orthogonal... but this hardware data is not helpfully structured. Please add a
bug.

https://codereview.appspot.com/12235043/diff/28001/testing/repo/series/wordpr...
File testing/repo/series/wordpress/metadata.yaml (right):

https://codereview.appspot.com/12235043/diff/28001/testing/repo/series/wordpr...
testing/repo/series/wordpress/metadata.yaml:14: scope: container
doesn't anything test for endpoints of wordpress elsewhere? If not, cool, I'm
just a bit surprised :).

[axw1, 2013-08-08 02:28:58]

On 2013/08/07 17:02:30, rog wrote:
> LGTM with the second fix below, and one other passing thought.
> 
> https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go
> File cmd/juju/status.go (right):
> 
>
https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode130
> cmd/juju/status.go:130: ok, err := path.Match(pattern, s)
> It's a pity that because path.Match is lazy
> about checking pattern validity, we need to return an
> error all the way back.
> 
> For consideration: validate the glob patterns
> with a regexp, and then we can panic here instead,
> and lose the error returns from matchString
> and matchUnit (and provide more reliable errors
> to the user too)
> 
> Something like:
> 
> ^([^\\[]|\[(\\.|[^-\]]-[^-\]])*\]|\\.)*$
> 
> might be sufficient.

That would be better, but can we simplify this?

The use of path matching is overkill IMHO. I think the only real use-case
for wildcards is to match services with a common prefix or suffix,
e.g. redis-*, which would match redis-master and redis-slave.

How about we simplify it to just allow a '*' wildcard, and nothing else?
Then we can just check that the pattern matches "[a-z0-9-*]". This will
also simplify the documentation.

If we find that people really want complex matching, we could later add a
flag to interpret patterns as REs.

I went ahead and implemented this. If this was wrong, let me know and I'll
revert.

> 
>
https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode277
> cmd/juju/status.go:277: mid, err := unit.AssignedMachineId()
> This still isn't quite right - it should only call AssignedMachineId if the
unit
> is a principal (that way it doesn't need to do a network call)
> 
> just:
> 
> if !unit.IsPrincipal() {
>     continue
> }
> 
> should do the job.

Sorry! I missed that from last time. Done.

[axw1, 2013-08-08 02:46:27]

On 2013/08/07 17:06:53, fwereade wrote:
> LGTM, thanks -- sorry for the delay, it's a busy week.

No worries, thanks for the review.

> https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go
> File cmd/juju/status.go (right):
> 
>
https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode47
> cmd/juju/status.go:47: if and when they are tried.`[1:]
> Maybe add a little bit more explaining which bits are filtered out (and which
> bits are not).

Done.

> 
>
https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode52
> cmd/juju/status.go:52: Args:    "[pattern...]",
> s/n./n ./
> 
> ?

Done.

> 
>
https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status.go#newcode116
> cmd/juju/status.go:116: } else {
> lose the else?

Done.

> https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status_test.go
> File cmd/juju/status_test.go (right):
> 
>
https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status_test.go#ne...
> cmd/juju/status_test.go:34: stderr = ctx.Stderr.(*bytes.Buffer).String()
> I'd prefer it if stderr and stdout were consistently typed...

Done.

> 
>
https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status_test.go#ne...
> cmd/juju/status_test.go:170: "hardware":    "arch=amd64 cpu-cores=1
mem=1024M",
> Orthogonal... but this hardware data is not helpfully structured. Please add a
> bug.

Done: https://bugs.launchpad.net/juju-core/+bug/1209452
(I hope that's what you meant)

>
https://codereview.appspot.com/12235043/diff/28001/testing/repo/series/wordpr...
> File testing/repo/series/wordpress/metadata.yaml (right):
> 
>
https://codereview.appspot.com/12235043/diff/28001/testing/repo/series/wordpr...
> testing/repo/series/wordpress/metadata.yaml:14: scope: container
> doesn't anything test for endpoints of wordpress elsewhere? If not, cool, I'm
> just a bit surprised :).

Sorry, I;m not sure I understand your comment. There are tests for subordinates
relating to wordpress, but there was only a test with one subordinate (logging).
I needed a test involving two subordinates for one principal, hence this
addition.

[axw, 2013-08-08 03:06:08]

Please take a look.

[fwereade, 2013-08-08 07:40:59]

On 2013/08/08 02:46:27, axw1 wrote:
> On 2013/08/07 17:06:53, fwereade wrote:
>
https://codereview.appspot.com/12235043/diff/28001/cmd/juju/status_test.go#ne...
> > cmd/juju/status_test.go:170: "hardware":    "arch=amd64 cpu-cores=1
> mem=1024M",
> > Orthogonal... but this hardware data is not helpfully structured. Please add
a
> > bug.
> 
> Done: https://bugs.launchpad.net/juju-core/+bug/1209452
> (I hope that's what you meant)

Perfect, thanks.

>
https://codereview.appspot.com/12235043/diff/28001/testing/repo/series/wordpr...
> > testing/repo/series/wordpress/metadata.yaml:14: scope: container
> > doesn't anything test for endpoints of wordpress elsewhere? If not, cool,
I'm
> > just a bit surprised :).
> 
> Sorry, I;m not sure I understand your comment. There are tests for
subordinates
> relating to wordpress, but there was only a test with one subordinate
(logging).
> I needed a test involving two subordinates for one principal, hence this
> addition.

I was just expressing mild surprise that this change didn't cause any other
tests to fail. It's not a problem that they didn't :).

Unless there's anything specific you need me to check, my previous LGTM can be
assumed to stand and you can go ahead and merge.

[rog, 2013-08-08 07:52:42]

still LGTM with two final thoughts.

https://codereview.appspot.com/12235043/diff/42001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/12235043/diff/42001/cmd/juju/status.go#newcode137
cmd/juju/status.go:137: const valid = "abcdefghijklmnopqrstuvwxyz0123456789-*"
or:

var validPattern = regexp.MustCompile("^[a-z0-9-*]$")

and use if !validPattern.MatchString(s) instead of validatePattern.

which would make it easier to add extra stuff if, for instance,
we started to allow unicode characters in service names.

https://codereview.appspot.com/12235043/diff/42001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/12235043/diff/42001/state/unit.go#newcode96
state/unit.go:96: var _ AgentEntity = (*Unit)(nil)
This has moved into interface.go now.

[axw1, 2013-08-08 08:13:14]

On 2013/08/08 07:52:42, rog wrote:
> still LGTM with two final thoughts.
> 
> https://codereview.appspot.com/12235043/diff/42001/cmd/juju/status.go
> File cmd/juju/status.go (right):
> 
>
https://codereview.appspot.com/12235043/diff/42001/cmd/juju/status.go#newcode137
> cmd/juju/status.go:137: const valid = "abcdefghijklmnopqrstuvwxyz0123456789-*"
> or:
> 
> var validPattern = regexp.MustCompile("^[a-z0-9-*]$")
> 
> and use if !validPattern.MatchString(s) instead of validatePattern.
> 
> which would make it easier to add extra stuff if, for instance,
> we started to allow unicode characters in service names.

Fair enough, done.

> https://codereview.appspot.com/12235043/diff/42001/state/unit.go
> File state/unit.go (right):
> 
> https://codereview.appspot.com/12235043/diff/42001/state/unit.go#newcode96
> state/unit.go:96: var _ AgentEntity = (*Unit)(nil)
> This has moved into interface.go now.

Thanks, merged.