Fix prefix-matching for service ps by thaJeztah · Pull Request #32800 · moby/moby

thaJeztah · 2017-04-24T23:20:06Z

The docker CLI matches objects either by ID prefix
or a full name match, but not partial name matches.

The correct order of resolution is;

Full ID match (a name should not be able to mask an ID)
Full name
ID-prefix

This patch changes the way services are matched.

Also change to use the first matching service, if there's a full match (by ID or Name) instead of continue looking for other possible matches.

Error handling changed;

Do not error early if multiple services were requested and one or more services were not found. Print the services that were not found after printing those that were found instead
Print an error if ID-prefix matching is ambiguous

fixes #32556

With this patch applied;

Given these services

$ docker service ls
ID                  NAME                      MODE                REPLICAS            IMAGE               PORTS
71yo8k34uxd6        db                        replicated          1/1                 busybox:latest
wt6z677fhoou        db2                       replicated          1/1                 busybox:latest
oygr4njb0qxl        71yo8k34uxd6              replicated          1/1                 busybox:latest
l6ps3579zt0u        hopeful_feynman           replicated          1/1                 busybox:latest
l9ehdvyxkyiy        xenodochial_stonebraker   replicated          1/1                 busybox:latest

Match by name no longer does a "prefix" match

$ docker service ps db
ID                  NAME                IMAGE               NODE                DESIRED STATE       CURRENT STATE           ERROR                              PORTS
ygrefa0tph0c        db.1                busybox:latest      9c8f9fbf54e8        Running             Running 2 minutes ago

Full name match "trumps" partial ID match;

$ docker service ps 71yo8k34uxd6
ID                  NAME                 IMAGE               NODE                DESIRED STATE       CURRENT STATE           ERROR                              PORTS
lgq4j24xl54m        71yo8k34uxd6.1       busybox:latest      9c8f9fbf54e8        Running             Running 2 minutes ago

Ambiguous ID-prefix produces an error

$ docker service ps l
multiple services found with provided prefix: l

$ docker service ps l6
ID                  NAME                    IMAGE               NODE                DESIRED STATE       CURRENT STATE            ERROR                              PORTS
ls2ml2jkxlve        hopeful_feynman.1       busybox:latest      9c8f9fbf54e8        Running             Running 25 seconds ago

Don't error out on first non-existing service, but print errors at the end

$ docker service ps db foo bar
ID                  NAME                IMAGE               NODE                DESIRED STATE       CURRENT STATE           ERROR               PORTS
4b3jlca22dho        db.1                busybox:latest      9c8f9fbf54e8        Running             Running 2 minutes ago
no such service: foo
no such service: bar

AkihiroSuda · 2017-04-25T01:41:27Z

off-topic: I know CLI is being ripped out (#32694) from moby/moby, but what is the "current" approach to update the CLI code?

It seems we need to continue to update moby/moby/cli "for a while", and Docker employees eventually cherry-pick up them into docker/docker-cli?

aaronlehmann · 2017-04-25T18:13:13Z

This unfortunately isn't the first time we've had inconsistencies in how names/IDs are resolved. I'd really like to see us create a package that defines the canonical approach. It could take a set of names and IDs as input, and either match one of them or return an appropriate error.

Note that the PR description (but not the code itself) is incorrect: it says that partial name matches are part of the correct order. In fact, we don't do partial name resolution.

aaronlehmann · 2017-04-25T18:14:35Z

cli/command/service/ps.go

Here we're choosing the first service whose ID matches the prefix, and ignoring the others. I think we should error out if the prefix is ambiguous. There are several places in the code where we do this, and again, I think in the long term it would be a good idea to consolidate the code in a central package.

I agree we need to have a standard solution. I was actually considering to error out if matches > 1; this will complicate the logic a bit, because we'll have to check number of matches per keyword, and possibly de-duplicate the keywords (e.g. docker service ps foo foo)

thaJeztah · 2017-04-26T21:01:16Z

@aaronlehmann updated the PR. It's a bit ugly (lots of logic in there), but I think this addresses all situations

aaronlehmann · 2017-04-26T23:57:39Z

cli/command/service/ps.go

This should go above, with "strings"

oh cr*p, it was automatically inserted by my editor. thx for catching

aaronlehmann · 2017-04-26T23:59:52Z

cli/command/service/ps.go

Feel free to ignore this, but I try to avoid using Sprintf for simple string concatenation. With the + operator, there isn't a chance of proving the wrong number or type of arguments to Sprintf.

heh, I actually had that at first, but wasn't sure if we liked plain concatting. I'll change

aaronlehmann · 2017-04-27T00:01:33Z

cli/command/service/ps.go

Seems like matches could just be a boolean, since we only care about whether there was a previous match or not. But it's fine either way.

aaronlehmann · 2017-04-27T00:01:49Z

Overall LGTM

The failures seem unrelated

The docker CLI matches objects either by ID _prefix_ or a full name match, but not partial name matches. The correct order of resolution is; - Full ID match (a name should not be able to mask an ID) - Full name - ID-prefix This patch changes the way services are matched. Also change to use the first matching service, if there's a full match (by ID or Name) instead of continue looking for other possible matches. Error handling changed; - Do not error early if multiple services were requested and one or more services were not found. Print the services that were not found after printing those that _were_ found instead - Print an error if ID-prefix matching is ambiguous Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2017-04-27T01:07:41Z

Addressed nits :)

vdemeester

LGTM 🐯

AkihiroSuda · 2017-05-09T00:02:00Z

Please reopen to docker/CLI?

thaJeztah · 2017-05-11T13:13:12Z

opened docker/cli#72 for the CLI side, I'll bump the CLI version and remove those changes here

boaz0

LGTM 👍

aaronlehmann · 2017-06-14T23:19:26Z

@thaJeztah: Is this needed to fix the test now that the CLI behavior has changed?

thaJeztah · 2017-06-14T23:45:14Z

@aaronlehmann the CLI in this repo still uses an older version, but we need to update it. Problem is currently that I don't know how to get the 17.06 CLI in. I think the plan is to fixate the CLI to 17.06-GA for integration tests, but given that https://github.com/docker/cli does not have a tag or branch for 17.06, we cannot get it from there; it needs to be fetched from https://github.com/docker/docker-ce, but that means getting the entire "docker" repository (which is big), so not idea.

thaJeztah · 2017-06-14T23:47:24Z

Actually, thinking if #33684 will pass, because it will be running against the old CLI

thaJeztah · 2017-06-14T23:48:02Z

Nevermind, it doesn't assume the CLI changes yet

aaronlehmann · 2017-06-14T23:57:24Z

I think the plan is to fixate the CLI to 17.06-GA for integration tests, but given that https://github.com/docker/cli does not have a tag or branch for 17.06, we cannot get it from there; it needs to be fetched from https://github.com/docker/docker-ce, but that means getting the entire "docker" repository (which is big), so not idea.

That is mind-boggling.

Why doesn't docker/cli have a tag or branch for 17.06? That seems very wrong.
If moby is supposed to be independent of released docker versions, why must the CLI be fixed to any particular released version?

thaJeztah · 2017-06-15T00:02:41Z

Why doesn't docker/cli have a tag or branch for 17.06? That seems very wrong.

All cherry-picking is done in docker/docker-ce, so that no branches had to be maintained for the individual repos

If moby is supposed to be independent of released docker versions, why must the CLI be fixed to any particular released version?

After 17.06-GA we won't accept new integration tests that use the Docker CLI (at least, not in this repository), the 17.06 CLI will be used for the existing tests, to catch regressions. Newer versions may be used in a e2e test-suite managed by Docker, Inc., but that's not a responsibility in this repo

aaronlehmann · 2017-06-15T00:08:53Z

All cherry-picking is done in docker/docker-ce, so that no branches had to be maintained for the individual repos

What is wrong with having branches in docker/cli?

I think cherry-picking in docker-ce is a very bad idea because means the unit test coverage is completely lost.

It also has the disadvantage that you pointed out where there is no revision in docker/cli corresponding to what was actually released, and one has to refer to another repository which is less convenient.

After 17.06-GA we won't accept new integration tests that use the Docker CLI (at least, not in this repository), the 17.06 CLI will be used for the existing tests, to catch regressions. Newer versions may be used in a e2e test-suite managed by Docker, Inc., but that's not a responsibility in this repo

This doesn't answer my question of why the cli must be fixed at that particular version, instead of, say, master from docker/cli on the date of the release.

dnephin · 2017-06-15T01:50:23Z

This was merged in docker/cli so closing it here.

We'll need to port this cli test to an API test

GordonTheTurtle added the status/0-triage label Apr 24, 2017

thaJeztah requested review from vdemeester and yongtang April 24, 2017 23:20

thaJeztah added status/2-code-review and removed status/0-triage labels Apr 24, 2017

thaJeztah mentioned this pull request Apr 24, 2017

docker servcie ps <service_name> lists all instances of services with names starting with service_name #32556

Closed

thaJeztah force-pushed the fix-prefix-matching branch from 051951e to 25f4c9d Compare April 24, 2017 23:22

AkihiroSuda added the area/cli Client label Apr 25, 2017

aaronlehmann reviewed Apr 25, 2017

View reviewed changes

thaJeztah force-pushed the fix-prefix-matching branch 2 times, most recently from 10fc3ef to 0044194 Compare April 26, 2017 20:57

aaronlehmann reviewed Apr 26, 2017

View reviewed changes

aaronlehmann reviewed Apr 27, 2017

View reviewed changes

thaJeztah force-pushed the fix-prefix-matching branch from 0044194 to 4242aec Compare April 27, 2017 01:06

vdemeester approved these changes Apr 29, 2017

View reviewed changes

thaJeztah added the impact/cli label May 5, 2017

GordonTheTurtle assigned AkihiroSuda May 9, 2017

thaJeztah mentioned this pull request May 11, 2017

Fix prefix-matching for service ps docker/cli#72

Merged

boaz0 approved these changes May 25, 2017

View reviewed changes

This was referenced May 31, 2017

17.06.0 RC2 tracker docker/for-linux#2

Closed

17.06.0 RC3 tracker docker/for-linux#8

Closed

thaJeztah mentioned this pull request Jun 7, 2017

docker service ps matches prefix #30417

Closed

andrewhsu mentioned this pull request Jun 13, 2017

17.06.0 RC4 tracker docker/for-linux#14

Closed

2 tasks

thaJeztah added the status/needs-vendoring label Jun 14, 2017

thaJeztah mentioned this pull request Jun 15, 2017

[17.06] cherry-pick Fix prefix-matching for service ps docker-archive/docker-ce#77

Closed

dnephin closed this Jun 15, 2017

thaJeztah mentioned this pull request Jun 15, 2017

Update service ps to be an API test #33684

Merged

andrewhsu mentioned this pull request Jun 19, 2017

17.06.0 RC5 tracker docker/for-linux#37

Closed

6 tasks

Conversation

thaJeztah commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Apr 25, 2017

Uh oh!

aaronlehmann commented Apr 25, 2017

Uh oh!

aaronlehmann Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 26, 2017

Uh oh!

aaronlehmann Apr 26, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 27, 2017

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Apr 26, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 27, 2017

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Apr 27, 2017

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Apr 27, 2017

Uh oh!

thaJeztah commented Apr 27, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented May 9, 2017

Uh oh!

thaJeztah commented May 11, 2017

Uh oh!

boaz0 left a comment

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Jun 14, 2017

Uh oh!

thaJeztah commented Jun 14, 2017

Uh oh!

thaJeztah commented Jun 14, 2017

Uh oh!

thaJeztah commented Jun 14, 2017

Uh oh!

aaronlehmann commented Jun 14, 2017

Uh oh!

thaJeztah commented Jun 15, 2017

Uh oh!

aaronlehmann commented Jun 15, 2017

Uh oh!

dnephin commented Jun 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

thaJeztah commented Apr 24, 2017 •

edited

Loading