Merge lp:~axwalk/juju-core/lp1121916-juju-status-filters into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

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://codereview.appspot.com/12235043/

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://codereview.appspot.com/12235043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+178004_code.launchpad.net,

Message:
Please take a look.

Description:
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.

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).

Filters are implemented using using shell file
name pattern matching, like in pyjuju. See:
     http://golang.org/pkg/path/#Match

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)

Please review this at https://codereview.appspot.com/12235043/

Affected files:
   [revision details]
   cmd/juju/cmd_test.go
   cmd/juju/status.go
   cmd/juju/status_test.go

[Martin Packman (gz)]

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.

https://codereview.appspot.com/12235043/

[Andrew Wilkins (axwalk)]

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.

https://codereview.appspot.com/12235043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/12235043/

[William Reade (fwereade)]

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.

https://codereview.appspot.com/12235043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/12235043/

[Andrew Wilkins (axwalk)]

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.

https://codereview.appspot.com/12235043/

[Roger Peppe (rogpeppe)]

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 in...

[Andrew Wilkins (axwalk)]

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 ...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/12235043/

[Roger Peppe (rogpeppe)]

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.

https://codereview.appspot.com/12235043/

[William Reade (fwereade)]

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#newcode34
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#newcode170
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/wordpress/metadata.yaml
File testing/repo/series/wordpress/metadata.yaml (right):

https://codereview.appspot.com/12235043/diff/28001/testing/repo/series/wordpress/metadata.yaml#newcode14
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 :).

https://codereview.appspot.com/12235043/

[Andrew Wilkins (axwalk)]

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.

https://codereview.appspot.com/12235043/

[Andrew Wilkins (axwalk)]

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#newcode34
> 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#newcode170
> 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/wordpress/metadata.yaml
> File testing/repo/series/wordpress/metadata.yaml (right):

https://codereview.appspot.com/12235043/diff/28001/testing/repo/series/wordpress/metadata.yaml#newcode14
> 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.

https://codereview.appspot.com/12235043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/12235043/

[William Reade (fwereade)]

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#newcode170
> > 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/wordpress/metadata.yaml#newcode14
> > 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.

https://codereview.appspot.com/12235043/

[Roger Peppe (rogpeppe)]

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.

https://codereview.appspot.com/12235043/

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/lp1121916-juju-status-filters into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.681s
ok launchpad.net/juju-core/agent/tools 25.280s
ok launchpad.net/juju-core/bzr 6.573s
ok launchpad.net/juju-core/cert 2.508s

----------------------------------------------------------------------
FAIL: meta_test.go:329: MetaSuite.TestMetaHooks

meta_test.go:356:
    c.Assert(hooks, DeepEquals, expectedHooks)
... obtained map[string]bool = map[string]bool{"upgrade-charm":true, "stop":true, "url-relation-joined":true, "url-relation-broken":true, "logging-dir-relation-departed":true, "monitoring-port-relation-changed":true, "cache-relation-changed":true, "config-changed":true, "logging-dir-relation-changed":true, "monitoring-port-relation-joined":true, "monitoring-port-relation-broken":true, "db-relation-departed":true, "cache-relation-joined":true, "cache-relation-broken":true, "start":true, "url-relation-departed":true, "logging-dir-relation-joined":true, "logging-dir-relation-broken":true, "db-relation-changed":true, "install":true, "url-relation-changed":true, "monitoring-port-relation-departed":true, "db-relation-joined":true, "db-relation-broken":true, "cache-relation-departed":true}
... expected map[string]bool = map[string]bool{"start":true, "db-relation-changed":true, "logging-dir-relation-joined":true, "logging-dir-relation-broken":true, "url-relation-departed":true, "install":true, "cache-relation-departed":true, "db-relation-joined":true, "db-relation-broken":true, "url-relation-changed":true, "upgrade-charm":true, "stop":true, "cache-relation-changed":true, "logging-dir-relation-departed":true, "url-relation-joined":true, "url-relation-broken":true, "config-changed":true, "cache-relation-joined":true, "cache-relation-broken":true, "db-relation-departed":true, "logging-dir-relation-changed":true}

OOPS: 76 passed, 1 FAILED
--- FAIL: Test (0.33 seconds)
FAIL
FAIL launchpad.net/juju-core/charm 0.538s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.019s
ok launchpad.net/juju-core/cmd 0.251s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
listing available tools
found 4 tools
found 4 recent tools (version 1.0.0)
listing target bucket
found 3 tools in target; 4 tools to be copied
copying 1.0.0-defaultseries-amd64 from http://127.0.0.1:53845/tools/juju-1.0.0-defaultseries-amd64.tgz
copying tools/juju-1.0.0-defaultseries-amd64.tgz
downloaded tools/juju-1.0.0-defaultseries-amd64.tgz (0kB), uploading
download 0kB, uploading
copying 1.0.0-precise-amd64 from http://127.0.0.1:53845/tools/juju-1.0.0-precise-amd64.tgz
copying tools/juju-1.0.0-precise-amd64.tgz
downloaded tools/juju-1.0.0-precise-amd64.tgz (0kB), uploading
download 0kB, uploading
copying 1.0.0-quantal-amd64 from http://127.0.0.1:53845/tools/juju-1.0.0-quantal-amd64.tgz
copying tools/juju-1.0.0-quantal-amd64.tgz
downloaded tools/juju-1.0.0-quantal-amd64.tgz (0kB), uploading
download 0kB, uploading
copying 1.0.0-quantal-i386...

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/lp1121916-juju-status-filters into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.773s
ok launchpad.net/juju-core/agent/tools 25.298s
ok launchpad.net/juju-core/bzr 6.957s
ok launchpad.net/juju-core/cert 2.051s
ok launchpad.net/juju-core/charm 0.891s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.027s
ok launchpad.net/juju-core/cmd 0.216s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 105.739s
ok launchpad.net/juju-core/cmd/jujud 51.464s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 0.743s
ok launchpad.net/juju-core/constraints 0.027s
ok launchpad.net/juju-core/container/lxc 0.298s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.278s
ok launchpad.net/juju-core/environs 1.161s
? launchpad.net/juju-core/environs/all [no test files]
ok launchpad.net/juju-core/environs/azure 3.838s
ok launchpad.net/juju-core/environs/cloudinit 0.431s
ok launchpad.net/juju-core/environs/config 0.678s
ok launchpad.net/juju-core/environs/dummy 15.907s
ok launchpad.net/juju-core/environs/ec2 181.924s
ok launchpad.net/juju-core/environs/imagemetadata 0.259s
ok launchpad.net/juju-core/environs/instances 0.223s
ok launchpad.net/juju-core/environs/jujutest 0.222s
ok launchpad.net/juju-core/environs/local 1.195s
? launchpad.net/juju-core/environs/local/storage [no test files]
ok launchpad.net/juju-core/environs/localstorage 0.244s
ok launchpad.net/juju-core/environs/maas 2.030s
ok launchpad.net/juju-core/environs/openstack 3.851s
? launchpad.net/juju-core/environs/provider [no test files]
ok launchpad.net/juju-core/environs/sync 0.241s
ok launchpad.net/juju-core/environs/testing 0.012s
? launchpad.net/juju-core/errors [no test files]
ok launchpad.net/juju-core/instance 0.039s
ok launchpad.net/juju-core/juju 11.900s
ok launchpad.net/juju-core/juju/osenv 0.236s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.018s
ok launchpad.net/juju-core/log/syslog 0.019s
ok launchpad.net/juju-core/names 0.030s
ok launchpad.net/juju-core/rpc 0.267s
ok launchpad.net/juju-core/rpc/jsoncodec 0.188s
ok launchpad.net/juju-core/schema 0.030s

----------------------------------------------------------------------
FAIL: service_test.go:1389: ServiceSuite.TestWatchService

[LOG] 9.66034 INFO juju state: opening state; mongo addresses: ["localhost:51241"]; entity ""
[LOG] 9.66789 INFO juju state: connection established
[LOG] 9.69983 INFO juju state: initializing environment
service_test.go:1420:
    testing.NewNotifyWatcherC(c, s.State, w).AssertOneChange()
testing/watcher.go:85:
    c.Fatalf("watcher sent unexpected change: (_, %v)", ok)
... Error: watcher sent unexpected change: (_, true)

OOPS: 308 passed, 1 FAILED
--- FAIL: TestPackage (68.00 seconds)
FAIL
FAIL launchpad.net/juju-core/state 68.190s
? lau...

=== modified file 'charm/meta_test.go'
--- charm/meta_test.go	2013-07-09 10:32:23 +0000
+++ charm/meta_test.go	2013-08-08 09:25:42 +0000
@@ -331,27 +331,31 @@
 	c.Assert(err, IsNil)
 	hooks := meta.Hooks()
 	expectedHooks := map[string]bool{
-		"install":                       true,
-		"start":                         true,
-		"config-changed":                true,
-		"upgrade-charm":                 true,
-		"stop":                          true,
-		"cache-relation-joined":         true,
-		"cache-relation-changed":        true,
-		"cache-relation-departed":       true,
-		"cache-relation-broken":         true,
-		"db-relation-joined":            true,
-		"db-relation-changed":           true,
-		"db-relation-departed":          true,
-		"db-relation-broken":            true,
-		"logging-dir-relation-joined":   true,
-		"logging-dir-relation-changed":  true,
-		"logging-dir-relation-departed": true,
-		"logging-dir-relation-broken":   true,
-		"url-relation-joined":           true,
-		"url-relation-changed":          true,
-		"url-relation-departed":         true,
-		"url-relation-broken":           true,
+		"install":                           true,
+		"start":                             true,
+		"config-changed":                    true,
+		"upgrade-charm":                     true,
+		"stop":                              true,
+		"cache-relation-joined":             true,
+		"cache-relation-changed":            true,
+		"cache-relation-departed":           true,
+		"cache-relation-broken":             true,
+		"db-relation-joined":                true,
+		"db-relation-changed":               true,
+		"db-relation-departed":              true,
+		"db-relation-broken":                true,
+		"logging-dir-relation-joined":       true,
+		"logging-dir-relation-changed":      true,
+		"logging-dir-relation-departed":     true,
+		"logging-dir-relation-broken":       true,
+		"monitoring-port-relation-joined":   true,
+		"monitoring-port-relation-changed":  true,
+		"monitoring-port-relation-departed": true,
+		"monitoring-port-relation-broken":   true,
+		"url-relation-joined":               true,
+		"url-relation-changed":              true,
+		"url-relation-departed":             true,
+		"url-relation-broken":               true,
 	}
 	c.Assert(hooks, DeepEquals, expectedHooks)
 }

=== modified file 'cmd/juju/cmd_test.go'
--- cmd/juju/cmd_test.go	2013-08-06 05:22:59 +0000
+++ cmd/juju/cmd_test.go	2013-08-08 09:25:42 +0000
@@ -115,7 +115,9 @@
 		assertConnName(c, com, "walthamstow")
 
 		com, args = cmdFunc()
-		testInit(c, com, append(args, "hotdog"), "unrecognized args.*")
+		if _, ok := com.(*StatusCommand); !ok {
+			testInit(c, com, append(args, "hotdog"), "unrecognized args.*")
+		}
 	}
 }
 

=== modified file 'cmd/juju/status.go'
--- cmd/juju/status.go	2013-08-06 14:17:24 +0000
+++ cmd/juju/status.go	2013-08-08 09:25:42 +0000
@@ -6,6 +6,8 @@
 import (
 	"encoding/json"
 	"fmt"
+	"path"
+	"regexp"
 	"strings"
 
 	"launchpad.net/gnuflag"
@@ -24,14 +26,27 @@
 
 type StatusCommand struct {
 	cmd.EnvCommandBase
-	out cmd.Output
+	out      cmd.Output
+	patterns []string
 }
 
-var statusDoc = "This command will report on the runtime state of various system entities."
+var statusDoc = `
+This command will report on the runtime state of various system entities.
+
+Service or unit names may be specified to filter the status to only those
+services and units that match, along with the related machines, services
+and units. If a subordinate unit is matched, then its principal unit will
+be displayed. If a principal unit is matched, then all of its subordinates
+will be displayed.
+
+Wildcards ('*') may be specified in service/unit names to match any sequence
+of characters. For example, 'nova-*' will match any service whose name begins
+with 'nova-': 'nova-compute', 'nova-volume', etc.`[1:]
 
 func (c *StatusCommand) Info() *cmd.Info {
 	return &cmd.Info{
 		Name:    "status",
+		Args:    "[pattern ...]",
 		Purpose: "output status information about an environment",
 		Doc:     statusDoc,
 		Aliases: []string{"stat"},
@@ -46,6 +61,11 @@
 	})
 }
 
+func (c *StatusCommand) Init(args []string) error {
+	c.patterns = args
+	return nil
+}
+
 type statusContext struct {
 	instances map[instance.Id]instance.Instance
 	machines  map[string][]*state.Machine
@@ -53,6 +73,95 @@
 	units     map[string]map[string]*state.Unit
 }
 
+type unitMatcher struct {
+	patterns []string
+}
+
+// matchesAny returns true if the unitMatcher will
+// match any unit, regardless of its attributes.
+func (m unitMatcher) matchesAny() bool {
+	return len(m.patterns) == 0
+}
+
+// matchUnit attempts to match a state.Unit to one of
+// a set of patterns, taking into account subordinate
+// relationships.
+func (m unitMatcher) matchUnit(u *state.Unit) bool {
+	if m.matchesAny() {
+		return true
+	}
+
+	// Keep the unit if:
+	//  (a) its name matches a pattern, or
+	//  (b) it's a principal and one of its subordinates matches, or
+	//  (c) it's a subordinate and its principal matches.
+	//
+	// Note: do *not* include a second subordinate if the principal is
+	// only matched on account of a first subordinate matching.
+	if m.matchString(u.Name()) {
+		return true
+	}
+	if u.IsPrincipal() {
+		for _, s := range u.SubordinateNames() {
+			if m.matchString(s) {
+				return true
+			}
+		}
+		return false
+	}
+	principal, valid := u.PrincipalName()
+	if !valid {
+		panic("PrincipalName failed for subordinate unit")
+	}
+	return m.matchString(principal)
+}
+
+// matchString matches a string to one of the patterns in
+// the unit matcher, returning an error if a pattern with
+// invalid syntax is encountered.
+func (m unitMatcher) matchString(s string) bool {
+	for _, pattern := range m.patterns {
+		ok, err := path.Match(pattern, s)
+		if err != nil {
+			// We validate patterns, so should never get here.
+			panic(fmt.Errorf("pattern syntax error in %q", pattern))
+		} else if ok {
+			return true
+		}
+	}
+	return false
+}
+
+// validPattern must match the parts of a unit or service name
+// pattern either side of the '/' for it to be valid.
+var validPattern = regexp.MustCompile("^[a-z0-9-*]+$")
+
+// newUnitMatcher returns a unitMatcher that matches units
+// with one of the specified patterns, or all units if no
+// patterns are specified.
+//
+// An error will be returned if any of the specified patterns
+// is invalid. Patterns are valid if they contain only
+// alpha-numeric characters, hyphens, or asterisks (and one
+// optional '/' to separate service/unit).
+func newUnitMatcher(patterns []string) (unitMatcher, error) {
+	for i, pattern := range patterns {
+		fields := strings.Split(pattern, "/")
+		if len(fields) > 2 {
+			return unitMatcher{}, fmt.Errorf("pattern %q contains too many '/' characters", pattern)
+		}
+		for _, f := range fields {
+			if !validPattern.MatchString(f) {
+				return unitMatcher{}, fmt.Errorf("pattern %q contains invalid characters", pattern)
+			}
+		}
+		if len(fields) == 1 {
+			patterns[i] += "/*"
+		}
+	}
+	return unitMatcher{patterns}, nil
+}
+
 func (c *StatusCommand) Run(ctx *cmd.Context) error {
 	conn, err := juju.NewConnFromName(c.EnvName)
 	if err != nil {
@@ -61,12 +170,26 @@
 	defer conn.Close()
 
 	var context statusContext
-	if context.machines, err = fetchAllMachines(conn.State); err != nil {
-		return err
-	}
-	if context.services, context.units, err = fetchAllServicesAndUnits(conn.State); err != nil {
-		return err
-	}
+	unitMatcher, err := newUnitMatcher(c.patterns)
+	if err != nil {
+		return err
+	}
+	if context.services, context.units, err = fetchAllServicesAndUnits(conn.State, unitMatcher); err != nil {
+		return err
+	}
+
+	// Filter machines by units in scope.
+	var machineIds *set.Strings
+	if !unitMatcher.matchesAny() {
+		machineIds, err = fetchUnitMachineIds(context.units)
+		if err != nil {
+			return err
+		}
+	}
+	if context.machines, err = fetchMachines(conn.State, machineIds); err != nil {
+		return err
+	}
+
 	context.instances, err = fetchAllInstances(conn.Environ)
 	if err != nil {
 		// We cannot see instances from the environment, but
@@ -98,9 +221,11 @@
 	return m, nil
 }
 
-// fetchAllMachines returns a map from top level machine id to machines, where machines[0] is the host
+// fetchMachines returns a map from top level machine id to machines, where machines[0] is the host
 // machine and machines[1..n] are any containers (including nested ones).
-func fetchAllMachines(st *state.State) (map[string][]*state.Machine, error) {
+//
+// If machineIds is non-nil, only machines whose IDs are in the set are returned.
+func fetchMachines(st *state.State, machineIds *set.Strings) (map[string][]*state.Machine, error) {
 	v := make(map[string][]*state.Machine)
 	machines, err := st.AllMachines()
 	if err != nil {
@@ -108,6 +233,9 @@
 	}
 	// AllMachines gives us machines sorted by id.
 	for _, m := range machines {
+		if machineIds != nil && !machineIds.Contains(m.Id()) {
+			continue
+		}
 		parentId, ok := m.ParentId()
 		if !ok {
 			// Only top level host machines go directly into the machine map.
@@ -127,7 +255,7 @@
 
 // fetchAllServicesAndUnits returns a map from service name to service
 // and a map from service name to unit name to unit.
-func fetchAllServicesAndUnits(st *state.State) (map[string]*state.Service, map[string]map[string]*state.Unit, error) {
+func fetchAllServicesAndUnits(st *state.State, unitMatcher unitMatcher) (map[string]*state.Service, map[string]map[string]*state.Unit, error) {
 	svcMap := make(map[string]*state.Service)
 	unitMap := make(map[string]map[string]*state.Unit)
 	services, err := st.AllServices()
@@ -135,20 +263,47 @@
 		return nil, nil, err
 	}
 	for _, s := range services {
-		svcMap[s.Name()] = s
 		units, err := s.AllUnits()
 		if err != nil {
 			return nil, nil, err
 		}
 		svcUnitMap := make(map[string]*state.Unit)
 		for _, u := range units {
+			if !unitMatcher.matchUnit(u) {
+				continue
+			}
 			svcUnitMap[u.Name()] = u
 		}
-		unitMap[s.Name()] = svcUnitMap
+		if unitMatcher.matchesAny() || len(svcUnitMap) > 0 {
+			unitMap[s.Name()] = svcUnitMap
+			svcMap[s.Name()] = s
+		}
 	}
 	return svcMap, unitMap, nil
 }
 
+// fetchUnitMachineIds returns a set of IDs for machines that
+// the specified units reside on, and those machines' ancestors.
+func fetchUnitMachineIds(units map[string]map[string]*state.Unit) (*set.Strings, error) {
+	machineIds := new(set.Strings)
+	for _, svcUnitMap := range units {
+		for _, unit := range svcUnitMap {
+			if !unit.IsPrincipal() {
+				continue
+			}
+			mid, err := unit.AssignedMachineId()
+			if err != nil {
+				return nil, err
+			}
+			for mid != "" {
+				machineIds.Add(mid)
+				mid = state.ParentId(mid)
+			}
+		}
+	}
+	return machineIds, nil
+}
+
 func (context *statusContext) processMachines() map[string]machineStatus {
 	machinesMap := make(map[string]machineStatus)
 	for id, machines := range context.machines {
@@ -275,7 +430,10 @@
 		status.Subordinates = make(map[string]unitStatus)
 		for _, name := range subUnits {
 			subUnit := context.unitByName(name)
-			status.Subordinates[name] = context.processUnit(subUnit)
+			// subUnit may be nil if subordinate was filtered out.
+			if subUnit != nil {
+				status.Subordinates[name] = context.processUnit(subUnit)
+			}
 		}
 	}
 	return

=== modified file 'cmd/juju/status_test.go'
--- cmd/juju/status_test.go	2013-08-06 14:17:24 +0000
+++ cmd/juju/status_test.go	2013-08-08 09:25:42 +0000
@@ -8,6 +8,7 @@
 	"encoding/json"
 	"fmt"
 	"net/url"
+	"strings"
 
 	. "launchpad.net/gocheck"
 	"launchpad.net/goyaml"
@@ -153,6 +154,21 @@
 		"series":      "series",
 		"hardware":    "arch=amd64 cpu-cores=1 mem=1024M",
 	}
+	machine1WithContainersScoped = M{
+		"agent-state": "started",
+		"containers": M{
+			"1/lxc/0": M{
+				"agent-state": "started",
+				"dns-name":    "dummyenv-2.dns",
+				"instance-id": "dummyenv-2",
+				"series":      "series",
+			},
+		},
+		"dns-name":    "dummyenv-1.dns",
+		"instance-id": "dummyenv-1",
+		"series":      "series",
+		"hardware":    "arch=amd64 cpu-cores=1 mem=1024M",
+	}
 	unexposedService = M{
 		"charm":   "local:series/dummy-1",
 		"exposed": false,
@@ -497,6 +513,146 @@
 				},
 			},
 		},
+
+		scopedExpect{
+			"scope status on dummy-service/0 unit",
+			[]string{"dummy-service/0"},
+			M{
+				"environment": "dummyenv",
+				"machines": M{
+					"1": machine1,
+				},
+				"services": M{
+					"dummy-service": M{
+						"charm":   "local:series/dummy-1",
+						"exposed": false,
+						"units": M{
+							"dummy-service/0": M{
+								"machine":          "1",
+								"life":             "dying",
+								"agent-state":      "down",
+								"agent-state-info": "(started)",
+							},
+						},
+					},
+				},
+			},
+		},
+		scopedExpect{
+			"scope status on exposed-service service",
+			[]string{"exposed-service"},
+			M{
+				"environment": "dummyenv",
+				"machines": M{
+					"2": machine2,
+				},
+				"services": M{
+					"exposed-service": M{
+						"charm":   "local:series/dummy-1",
+						"exposed": true,
+						"units": M{
+							"exposed-service/0": M{
+								"machine":          "2",
+								"agent-state":      "error",
+								"agent-state-info": "You Require More Vespene Gas",
+								"open-ports": L{
+									"2/tcp", "3/tcp", "2/udp", "10/udp",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		scopedExpect{
+			"scope status on service pattern",
+			[]string{"d*-service"},
+			M{
+				"environment": "dummyenv",
+				"machines": M{
+					"1": machine1,
+				},
+				"services": M{
+					"dummy-service": M{
+						"charm":   "local:series/dummy-1",
+						"exposed": false,
+						"units": M{
+							"dummy-service/0": M{
+								"machine":          "1",
+								"life":             "dying",
+								"agent-state":      "down",
+								"agent-state-info": "(started)",
+							},
+						},
+					},
+				},
+			},
+		},
+		scopedExpect{
+			"scope status on unit pattern",
+			[]string{"e*posed-service/*"},
+			M{
+				"environment": "dummyenv",
+				"machines": M{
+					"2": machine2,
+				},
+				"services": M{
+					"exposed-service": M{
+						"charm":   "local:series/dummy-1",
+						"exposed": true,
+						"units": M{
+							"exposed-service/0": M{
+								"machine":          "2",
+								"agent-state":      "error",
+								"agent-state-info": "You Require More Vespene Gas",
+								"open-ports": L{
+									"2/tcp", "3/tcp", "2/udp", "10/udp",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		scopedExpect{
+			"scope status on combination of service and unit patterns",
+			[]string{"exposed-service", "dummy-service", "e*posed-service/*", "dummy-service/*"},
+			M{
+				"environment": "dummyenv",
+				"machines": M{
+					"1": machine1,
+					"2": machine2,
+				},
+				"services": M{
+					"dummy-service": M{
+						"charm":   "local:series/dummy-1",
+						"exposed": false,
+						"units": M{
+							"dummy-service/0": M{
+								"machine":          "1",
+								"life":             "dying",
+								"agent-state":      "down",
+								"agent-state-info": "(started)",
+							},
+						},
+					},
+					"exposed-service": M{
+						"charm":   "local:series/dummy-1",
+						"exposed": true,
+						"units": M{
+							"exposed-service/0": M{
+								"machine":          "2",
+								"agent-state":      "error",
+								"agent-state-info": "You Require More Vespene Gas",
+								"open-ports": L{
+									"2/tcp", "3/tcp", "2/udp", "10/udp",
+								},
+							},
+						},
+					},
+				},
+			},
+		},
 	),
 	test(
 		"add a dying service",
@@ -808,6 +964,185 @@
 				},
 			},
 		},
+
+		// scoped on 'logging'
+		scopedExpect{
+			"subordinates scoped on logging",
+			[]string{"logging"},
+			M{
+				"environment": "dummyenv",
+				"machines": M{
+					"1": machine1,
+					"2": machine2,
+				},
+				"services": M{
+					"wordpress": M{
+						"charm":   "local:series/wordpress-3",
+						"exposed": true,
+						"units": M{
+							"wordpress/0": M{
+								"machine":     "1",
+								"agent-state": "started",
+								"subordinates": M{
+									"logging/0": M{
+										"agent-state": "started",
+									},
+								},
+							},
+						},
+						"relations": M{
+							"db":          L{"mysql"},
+							"logging-dir": L{"logging"},
+						},
+					},
+					"mysql": M{
+						"charm":   "local:series/mysql-1",
+						"exposed": true,
+						"units": M{
+							"mysql/0": M{
+								"machine":     "2",
+								"agent-state": "started",
+								"subordinates": M{
+									"logging/1": M{
+										"agent-state":      "error",
+										"agent-state-info": "somehow lost in all those logs",
+									},
+								},
+							},
+						},
+						"relations": M{
+							"server":    L{"wordpress"},
+							"juju-info": L{"logging"},
+						},
+					},
+					"logging": M{
+						"charm":   "local:series/logging-1",
+						"exposed": true,
+						"relations": M{
+							"logging-directory": L{"wordpress"},
+							"info":              L{"mysql"},
+						},
+						"subordinate-to": L{"mysql", "wordpress"},
+					},
+				},
+			},
+		},
+
+		// scoped on wordpress/0
+		scopedExpect{
+			"subordinates scoped on logging",
+			[]string{"wordpress/0"},
+			M{
+				"environment": "dummyenv",
+				"machines": M{
+					"1": machine1,
+				},
+				"services": M{
+					"wordpress": M{
+						"charm":   "local:series/wordpress-3",
+						"exposed": true,
+						"units": M{
+							"wordpress/0": M{
+								"machine":     "1",
+								"agent-state": "started",
+								"subordinates": M{
+									"logging/0": M{
+										"agent-state": "started",
+									},
+								},
+							},
+						},
+						"relations": M{
+							"db":          L{"mysql"},
+							"logging-dir": L{"logging"},
+						},
+					},
+					"logging": M{
+						"charm":   "local:series/logging-1",
+						"exposed": true,
+						"relations": M{
+							"logging-directory": L{"wordpress"},
+							"info":              L{"mysql"},
+						},
+						"subordinate-to": L{"mysql", "wordpress"},
+					},
+				},
+			},
+		},
+	), test(
+		"one service with two subordinate services",
+		addMachine{machineId: "0", job: state.JobManageEnviron},
+		startAliveMachine{"0"},
+		setMachineStatus{"0", params.StatusStarted, ""},
+		addCharm{"wordpress"},
+		addCharm{"logging"},
+		addCharm{"monitoring"},
+
+		addService{"wordpress", "wordpress"},
+		setServiceExposed{"wordpress", true},
+		addMachine{machineId: "1", job: state.JobHostUnits},
+		startAliveMachine{"1"},
+		setMachineStatus{"1", params.StatusStarted, ""},
+		addAliveUnit{"wordpress", "1"},
+		setUnitStatus{"wordpress/0", params.StatusStarted, ""},
+
+		addService{"logging", "logging"},
+		setServiceExposed{"logging", true},
+		addService{"monitoring", "monitoring"},
+		setServiceExposed{"monitoring", true},
+
+		relateServices{"wordpress", "logging"},
+		relateServices{"wordpress", "monitoring"},
+
+		addSubordinate{"wordpress/0", "logging"},
+		addSubordinate{"wordpress/0", "monitoring"},
+
+		setUnitsAlive{"logging"},
+		setUnitStatus{"logging/0", params.StatusStarted, ""},
+
+		setUnitsAlive{"monitoring"},
+		setUnitStatus{"monitoring/0", params.StatusStarted, ""},
+
+		// scoped on monitoring; make sure logging doesn't show up.
+		scopedExpect{
+			"subordinates scoped on:",
+			[]string{"monitoring"},
+			M{
+				"environment": "dummyenv",
+				"machines": M{
+					"1": machine1,
+				},
+				"services": M{
+					"wordpress": M{
+						"charm":   "local:series/wordpress-3",
+						"exposed": true,
+						"units": M{
+							"wordpress/0": M{
+								"machine":     "1",
+								"agent-state": "started",
+								"subordinates": M{
+									"monitoring/0": M{
+										"agent-state": "started",
+									},
+								},
+							},
+						},
+						"relations": M{
+							"logging-dir":     L{"logging"},
+							"monitoring-port": L{"monitoring"},
+						},
+					},
+					"monitoring": M{
+						"charm":   "local:series/monitoring-0",
+						"exposed": true,
+						"relations": M{
+							"monitoring-port": L{"wordpress"},
+						},
+						"subordinate-to": L{"wordpress"},
+					},
+				},
+			},
+		},
 	), test(
 		"machines with containers",
 		addMachine{machineId: "0", job: state.JobManageEnviron},
@@ -862,6 +1197,30 @@
 				},
 			},
 		},
+
+		// once again, with a scope on mysql/1
+		scopedExpect{
+			"machines with nested containers",
+			[]string{"mysql/1"},
+			M{
+				"environment": "dummyenv",
+				"machines": M{
+					"1": machine1WithContainersScoped,
+				},
+				"services": M{
+					"mysql": M{
+						"charm":   "local:series/mysql-1",
+						"exposed": true,
+						"units": M{
+							"mysql/1": M{
+								"machine":     "1/lxc/0",
+								"agent-state": "started",
+							},
+						},
+					},
+				},
+			},
+		},
 	),
 }
 
@@ -1175,19 +1534,26 @@
 	c.Assert(err, IsNil)
 }
 
+type scopedExpect struct {
+	what   string
+	scope  []string
+	output M
+}
+
 type expect struct {
 	what   string
 	output M
 }
 
-func (e expect) step(c *C, ctx *context) {
-	c.Logf("expect: %s", e.what)
+func (e scopedExpect) step(c *C, ctx *context) {
+	c.Logf("expect: %s %s", e.what, strings.Join(e.scope, " "))
 
 	// Now execute the command for each format.
 	for _, format := range statusFormats {
 		c.Logf("format %q", format.name)
 		// Run command with the required format.
-		code, stdout, stderr := runStatus(c, "--format", format.name)
+		args := append([]string{"--format", format.name}, e.scope...)
+		code, stdout, stderr := runStatus(c, args...)
 		c.Assert(code, Equals, 0)
 		c.Assert(stderr, HasLen, 0)
 
@@ -1206,6 +1572,10 @@
 	}
 }
 
+func (e expect) step(c *C, ctx *context) {
+	scopedExpect{e.what, nil, e.output}.step(c, ctx)
+}
+
 func (s *StatusSuite) TestStatusAllFormats(c *C) {
 	for i, t := range statusTests {
 		c.Logf("test %d: %s", i, t.summary)
@@ -1217,3 +1587,32 @@
 		}()
 	}
 }
+
+func (s *StatusSuite) TestStatusFilterErrors(c *C) {
+	steps := []stepper{
+		addMachine{machineId: "0", job: state.JobManageEnviron},
+		addMachine{machineId: "1", job: state.JobHostUnits},
+		addCharm{"mysql"},
+		addService{"mysql", "mysql"},
+		addAliveUnit{"mysql", "1"},
+	}
+	ctx := s.newContext()
+	defer s.resetContext(c, ctx)
+	ctx.run(c, steps)
+
+	// Status filters can only fail if the patterns are invalid.
+	code, _, stderr := runStatus(c, "[*")
+	c.Assert(code, Not(Equals), 0)
+	c.Assert(string(stderr), Equals, `error: pattern "[*" contains invalid characters`+"\n")
+
+	code, _, stderr = runStatus(c, "//")
+	c.Assert(code, Not(Equals), 0)
+	c.Assert(string(stderr), Equals, `error: pattern "//" contains too many '/' characters`+"\n")
+
+	// Pattern validity is checked eagerly; if a bad pattern
+	// proceeds a valid, matching pattern, then the bad pattern
+	// will still cause an error.
+	code, _, stderr = runStatus(c, "*", "[*")
+	c.Assert(code, Not(Equals), 0)
+	c.Assert(string(stderr), Equals, `error: pattern "[*" contains invalid characters`+"\n")
+}

=== modified file 'state/service_test.go'
--- state/service_test.go	2013-07-18 16:46:59 +0000
+++ state/service_test.go	2013-08-08 09:25:42 +0000
@@ -652,6 +652,18 @@
 		},
 	})
 
+	mpEP, err := wordpress.Endpoint("monitoring-port")
+	c.Assert(err, IsNil)
+	c.Assert(mpEP, DeepEquals, state.Endpoint{
+		ServiceName: "wordpress",
+		Relation: charm.Relation{
+			Interface: "monitoring",
+			Name:      "monitoring-port",
+			Role:      charm.RoleProvider,
+			Scope:     charm.ScopeContainer,
+		},
+	})
+
 	dbEP, err := wordpress.Endpoint("db")
 	c.Assert(err, IsNil)
 	c.Assert(dbEP, DeepEquals, state.Endpoint{
@@ -681,7 +693,7 @@
 
 	eps, err := wordpress.Endpoints()
 	c.Assert(err, IsNil)
-	c.Assert(eps, DeepEquals, []state.Endpoint{cacheEP, dbEP, jiEP, ldEP, urlEP})
+	c.Assert(eps, DeepEquals, []state.Endpoint{cacheEP, dbEP, jiEP, ldEP, mpEP, urlEP})
 }
 
 func (s *ServiceSuite) TestServiceRefresh(c *C) {

=== modified file 'state/unit.go'
--- state/unit.go	2013-08-06 17:08:26 +0000
+++ state/unit.go	2013-08-08 09:25:42 +0000
@@ -442,6 +442,12 @@
 	return "", false
 }
 
+// PrincipalName returns the name of the unit's principal.
+// If the unit is not a subordinate, false is returned.
+func (u *Unit) PrincipalName() (string, bool) {
+	return u.doc.Principal, u.doc.Principal != ""
+}
+
 // PublicAddress returns the public address of the unit and whether it is valid.
 func (u *Unit) PublicAddress() (string, bool) {
 	return u.doc.PublicAddress, u.doc.PublicAddress != ""

=== modified file 'state/unit_test.go'
--- state/unit_test.go	2013-07-31 14:19:29 +0000
+++ state/unit_test.go	2013-08-08 09:25:42 +0000
@@ -886,6 +886,36 @@
 	c.Assert(err, IsNil)
 }
 
+func (s *UnitSuite) TestPrincipalName(c *C) {
+	subCharm := s.AddTestingCharm(c, "logging")
+	_, err := s.State.AddService("logging", subCharm)
+	c.Assert(err, IsNil)
+	eps, err := s.State.InferEndpoints([]string{"logging", "wordpress"})
+	c.Assert(err, IsNil)
+	rel, err := s.State.AddRelation(eps...)
+	c.Assert(err, IsNil)
+	ru, err := rel.Unit(s.unit)
+	c.Assert(err, IsNil)
+	err = ru.EnterScope(nil)
+	c.Assert(err, IsNil)
+
+	err = s.unit.Refresh()
+	c.Assert(err, IsNil)
+	subordinates := s.unit.SubordinateNames()
+	c.Assert(subordinates, DeepEquals, []string{"logging/0"})
+
+	su, err := s.State.Unit("logging/0")
+	c.Assert(err, IsNil)
+	principal, valid := su.PrincipalName()
+	c.Assert(valid, Equals, true)
+	c.Assert(principal, Equals, s.unit.Name())
+
+	// Calling PrincipalName on a principal unit yields "", false.
+	principal, valid = s.unit.PrincipalName()
+	c.Assert(valid, Equals, false)
+	c.Assert(principal, Equals, "")
+}
+
 func (s *UnitSuite) TestRemove(c *C) {
 	err := s.unit.Remove()
 	c.Assert(err, ErrorMatches, `cannot remove unit "wordpress/0": unit is not dead`)

=== added directory 'testing/repo/series/monitoring'
=== added directory 'testing/repo/series/monitoring/hooks'
=== added file 'testing/repo/series/monitoring/metadata.yaml'
--- testing/repo/series/monitoring/metadata.yaml	1970-01-01 00:00:00 +0000
+++ testing/repo/series/monitoring/metadata.yaml	2013-08-08 09:25:42 +0000
@@ -0,0 +1,16 @@
+name: monitoring
+summary: "Subordinate monitoring test charm"
+description: |
+    This is a longer description which
+    potentially contains multiple lines.
+subordinate: true
+provides:
+    monitoring-client:
+       interface: monitoring
+requires:
+    monitoring-port:
+       interface: monitoring
+       scope: container
+    info:
+       interface: juju-info
+       scope: container

=== modified file 'testing/repo/series/wordpress/metadata.yaml'
--- testing/repo/series/wordpress/metadata.yaml	2012-10-30 16:14:26 +0000
+++ testing/repo/series/wordpress/metadata.yaml	2013-08-08 09:25:42 +0000
@@ -9,6 +9,9 @@
   logging-dir:
     interface: logging
     scope: container
+  monitoring-port:
+    interface: monitoring
+    scope: container
 requires:
   db:
     interface: mysql