cmd: fix name prefix matching #1279

jinuxstyle · 2016-08-01T03:07:44Z

Currently if there are two services, and one service is named
busybox while the other is busybox-top. It is not possible to
remove the busybox service by giving its full name which is also
a common name prefix of both. The only way out is removing the
busybox-top service first or removing it by ID. This looks not an
acceptable behavior.

The same problem applies to clusters, nodes and networks.

Fix it by finding a full name match if prefix matches are more
than one.

Another way to fix it is that start a request with a full name
filter before doing it with a name prefix filter. But I think
this way just presents the same result but adds one more call
overhead to server.

Signed-off-by: Jin Xu jinuxstyle@hotmail.com

codecov-io · 2016-08-01T03:15:30Z

Current coverage is 55.05% (diff: 100%)

Merging #1279 into master will decrease coverage by 0.08%

@@             master      #1279   diff @@
==========================================
  Files            81         81          
  Lines         12850      12850          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           7085       7074    -11   
  Misses         4786       4786          
- Partials        979        990    +11

Powered by Codecov. Last update 13db5d4...dfcd073

dperny · 2016-08-01T18:23:57Z

cmd/swarmctl/node/common.go

+			for _, n := range rl.Nodes {
+				name := n.Spec.Annotations.Name
+				if name == "" && n.Description != nil {
+					name = n.Description.Hostname


Is searching by hostname something that we did before, or is this a new feature?

Searching by hostname is what the server part does.

dperny · 2016-08-01T18:24:14Z

LGTM

aaronlehmann · 2016-08-01T18:41:09Z

cmd/swarmctl/node/common.go

@@ -138,6 +138,16 @@ func getNode(ctx context.Context, c api.ControlClient, input string) (*api.Node,
 		}

 		if l := len(rl.Nodes); l > 1 {
+			for _, n := range rl.Nodes {
+				name := n.Spec.Annotations.Name


I'm not sure I understand why this is looking at n.Spec.Annotations.Name. It looks like the NamePrefixes only filter acts on n.Description.Hostname, so shouldn't this code only be checking Hostname?

Right. The server part only checks hostname when filtering. But the code logic is actually from here in cmd/swarmctl/node/list.go. Should I change them both to keep them consistent or just leave them as they are? I guess the intention of the code is that the Annotations.Name from Spec is always the first place to look for a name, like the names for services, etc.

I see. The use of Annotations.Name in "node ls" seems wrong. It looks like Docker prints the hostname in the list. I think we should update both of these to only use the hostname.

@aaronlehmann We need to be able to let operators set the node name.

@aaronlehmann Right. The Docker doesn't check Annotations.Name. I agree we should keep them consistent.

@stevvooe What do you mean? Do you agree that the client should use n.Description.Hostname directly for displaying node's name?

For now, both Docker client and swarmctl works without problem because node.Spec.Annotations.Name is never set to a valid name string. I don't know whether this will change in future. If yes, then swarmctl's way would be more preferable.

@jinuxstyle This is an oversight. User should be able to set a name on a node and have it work correctly.

We don't seem to index nodes by the name in Annotations.Name. So it wouldn't be possible to look up a node by the Annotations.Name value without some API and store changes. This might be the right direction to go in, but I think it should be a separate PR and should be coordinated with changes to the Docker CLI. In the mean time, it doesn't make sense to me to display a name that can't be typed in to look up the node.

@stevvooe The fact is that there is no way for user to set a name for a node explicitly. Is there any historical reason on this? And should we file a separate issue for this?

In the mean time, it doesn't make sense to me to display a name that can't be typed in to look up the node.

@aaronlehmann What do you mean by saying "a name that can't be typed"? Currently it will always display the Hostname as the node's name because the Annotations.Name is an empty string so it uses the Hostname after doing the check.

aaronlehmann · 2016-08-02T03:14:05Z

The query is by hostname. So displaying Annotations.Name does no good unless there's a way to query by it.

jinuxstyle · 2016-08-02T03:34:46Z

@aaronlehmann It does look a bit odd from implementation's perspective. The clients ask filtering with Name prefix which is usually intended for filtering nodes by their Annotaitons.Name, but server side does it by Hostname. It means clients have to know servers' implementation.

jinuxstyle · 2016-08-05T00:38:24Z

@aaronlehmann I updated PR per your comments. I agree with your comments after thinking a bit more. We should not use the Spec's Annotations.Name of the node at all because it's never set by implementation.

stevvooe · 2016-08-05T20:08:20Z

cmd/swarmctl/network/common.go

@@ -32,6 +32,12 @@ func GetNetwork(ctx context.Context, c api.ControlClient, input string) (*api.Ne
 		}

 		if l := len(rl.Networks); l > 1 {
+			for _, n := range rl.Networks {
+				if n.Spec.Annotations.Name == input {
+					// Found a full name match


This needs to handle the case where there may be two matches. There should only be one exact match.

This code should not assume uniqueness, as that is a server-side policy.

@stevvooe Is it possible that there are two or more matches for the same name? I suppose this would be a bug on the server side if it really happens.

I just found that it is indeed possible that there are multiple nodes with the same name but in different status.
49kmbx2yxf63zn801joyunzm4 node-3 ACCEPTED READY ACTIVE
5a2m77pyawva7b5jjmzmmadjn node-3 ACCEPTED DOWN ACTIVE

Do you know why server side would allow the creation of nodes with the same name? Is it a bug?

When you say "name", you mean "hostname", right? It's definitely possible for nodes to have the same hostname, and that's outside our control.

Even if it is a server-side bug, you need to handle the case where the name is non-unique.

It's a little surprise for me to know that the name could be non-unique. But it should not be a problem since the ID is still unique.

stevvooe · 2016-08-19T20:06:46Z

Here is a description of the correct behavior: #1194 (comment).

stevvooe · 2016-08-19T20:07:30Z

#1194 might be trying to address the same problem, but who know, because there is no description and no examples.

jinuxstyle · 2016-08-20T01:27:12Z

@stevvooe Thanks for referencing to the #1194 which is very informative. I agree with the three cases you listed there. I can improve this PR to make the name prefix matching meet the three cases. If there are multiple full name matches, we should still fail out.

As for ID prefix matching, we can open another PR to do similar things as name prefix matching, or ask @doronp to revise #1194 because the logic is implemented there by getServiceByPrefixedID().

jinuxstyle · 2016-08-20T01:49:59Z

I just read through the code for creating nodes, clusters, services and networks, all of them except creating nodes will check for the uniqueness of the name. So this PR would work after revising the node part. However, I think it's safer and more consistent to revise all of them.

Currently if there are two services, and one service is named busybox while the other is busybox-top. It is not possible to remove the busybox service by giving its fullname which is also a common name prefix of both. The only way out is removing the busybox-top service first. This looks not an acceptable behavior. The same problem applies to clusters, nodes and networks. Fix it by finding a full name match if prefix matches are more than one. Another way to fix it is that start a request with a full name filter before doing it with a name prefix filter. But I think this way just presents the same result but adds one more call overhead to server. Signed-off-by: Jin Xu <jinuxstyle@hotmail.com> v3: fail out if there are multiple full name matches. v2: do not use node's Spec.Annotations.Name as defaut

jinuxstyle · 2016-08-20T02:53:05Z

PR updated per review comments: fail out if there are multiple full name matches.

stevvooe · 2016-08-27T00:58:54Z

@jinuxstyle I think these need to also match the ID by prefix and try the name prefix.

dperny · 2016-08-29T18:08:48Z

Would y'all say that this PR now supercedes #1194 ?

stevvooe · 2016-08-29T21:38:17Z

@dperny That is a fair evaluation.

jinuxstyle · 2016-09-10T09:30:58Z

@stevvooe If we need to support ID prefix match, we would have several more cases to cover which will adds complexity.

ID prefix match (5 cases): err != nil, 0 match, 1 full ID match, 1 prefix match, 2 or more prefix matches.
Name prefix match (6 cases): err != nil, 0 match, 1 full name match, 1 prefix match, 2 or more full name matches (only possible for nodes), 2 or more prefix matches.

To support both name and ID prefix match, we will have 30 (5 * 6) cases to cover.

I spent sometime to understand what you discussed in #1194 and sorted out following points. It can be done by at most two requests with ID and name prefix filters respectively.

First request with ID prefix filter, and look for the only full ID match and return it if found.
Then request with name prefix filter, and look for the only full name match and return it if found.
If both requests return error, fail out, otherwise adds up the total number of matches.
If the total number of matches is 0, return not found error
else If it's 1, return the only match
else return ambiguous error

Any comments?

stevvooe · 2016-09-12T22:44:48Z

@jinuxstyle I'm confused. This is simple stuff.

To support both name and ID prefix match, we will have 30 (5 * 6) cases to cover.

This is an odd way to look at the problem and makes it much more complex than it is. Don't view this as a series of if statements, that will only make this problem untenable.

First step, have 1 function that lists the candidates based on the query. You will hit two indexes using prefix match on id and name. This same function can be used in several contexts to implement different behavior. The rpc that lists items should support OR over separate index sets, ensuring that this can be done in a single RPC. In #1194 (comment), I represented this as a compound index of id and name to the task. Make sure that you understand this point clearly.

All of our algorithms can be defined in terms of the guarantees provided by that matching function. We know that id is always unique match but name may have multiples. Our rules are going to disambiguate over that dataset leveraging that property.

We have two use cases under which we can employ this function:

Identify unique matches. This is for commands that can only find a single match and ambiguity causes them to fail. This would be service update, service rm, etc.
Return all the results matched by the query. This could be employed in listing scenarios, such as service ps, nodes ls, and service ls.

For the purposes of discussion, use case 1 is interesting.

We just need to define simple rules to make these matches. I made this very clear in #1194 (comment), but I'll repeat it here:

Unique match for prefix or exact match on name or id succeeds.
Multiple matches without exact match returns ambiguous error.
Exact match under multiple items gets selected, favoring id.

View each step here as the action after applying a predicate operation on the dataset. If you think about this way, you'll avoid creating a cross-product of conditions that will lead to unmaintainable code.

jinuxstyle · 2016-09-13T01:33:59Z

@stevvooe

This is an odd way to look at the problem and makes it much more complex than it is. Don't view this as a series of if statements, that will only make this problem untenable.

It's not that odd than you might think. With generalization, it could be implemented with at most 5 if statements and 2 loops for iterating over the id and name prefix matching results.

By the way, I have been thinking about why this function would have intended to support both ID and name matching, while the user interface is defined like following:
swarmctl service remove [flags]
By intuition, user would pass a ID but why underlying implementation would also regard it as a name. Would it be better to support name matching by --name or --filter?

The rpc that lists items should support OR over separate index sets, ensuring that this can be done in a single RPC.

This would introduce extensive logical changes to server side code because currently only AND over different filters is supported. Moreover it would not help much on reducing the complexity on client side. The client side will still need several if statements and one or two iterations. Lastly if we are not going to support multiple OR filters from user interface level, it would be meaningless to support OR logic on sever side.

stevvooe · 2016-09-13T02:10:43Z

@jinuxstyle Please go back and read what I said again. I'm not sure you understood me correctly.

if statements really aren't a good way to approach a problem that is ultimately set-based. Typically, an if-based approach won't provide insight into your dataset and will lead to edge cases. A set-based approach ensures correctness. Once correctness is achieved, you can optimize.

This model is already widely used in git and docker, so I am very confused as to why I've had to spend so much time trying to communicate it.

This would introduce extensive logical changes to server side code because currently only AND over different filters is supported. Moreover it would not help much on reducing the complexity on client side. The client side will still need several if statements and one or two iterations. Lastly if we are not going to support multiple OR filters from user interface level, it would be meaningless to support OR logic on sever side.

Not really. Each component must already be OR, as that is the definition of a prefix. You just need to make sure that if both filters are defined, we match the union.

The problem we are dealing with here is correctness, not complexity. Follow the guidelines I've set and this system will be complete and correct.

jinuxstyle · 2016-09-13T02:44:30Z

@stevvooe I understood what you described. I am just more prone to solve problems in a simpler way if other aspects doesn't make big difference. I agree you suggested approach is more general and powerful.

Not really. Each component must already be OR, as that is the definition of a prefix. You just need to make sure that if both filters are defined, we match the union.

Are you sure that each component must already be OR? As I saw in the server side code (e.g. ListServices) and test code, the current implementation only supports intersection for multiple filters.

Do you really think it's worth the changes?

stevvooe · 2016-09-13T18:54:10Z

@jinuxstyle Set-based operations are simple.

From the code you sent, it looks like intersection isn't supported. Prefix is union by definition. You'll need to union the results from each filter. It's really not that complex.

Do you really think it's worth the changes?

Absolutely. The problem here is that we don't have a very powerful way to query the dataset. Also, "if-based" approaches tend to be extremely fragile.

Note that I am not saying "don't use if-statements". I am saying, model the problem as sets of tuples, build an index based on those tuples, query that index and process the results. If this is implemented as a series of server-side and client-side filters, that is fine.

aluzzardi · 2016-09-26T21:54:55Z

@jinuxstyle @stevvooe ping?

jinuxstyle · 2016-09-27T03:37:29Z

@aluzzardi I already have a solution that fixes the problem on client side only. It doesn't touch server side code, neither the desired solution by @stevvooe. Can I propose it?

BTW, I would not be able to do it based on the approach suggested by @stevvooe in short time, because I am no longer doing open source full time. I will still keep contributing but would be driven by my working needs or in spare time.

AkihiroSuda · 2016-11-25T06:16:43Z

@jinuxstyle
Can you show us your solution?
Also, can you also look into moby/moby#27938 if you are interested in?

stevvooe · 2016-11-28T23:29:57Z

@AkihiroSuda We are going to continue to see bugs and inconsistent results if we do not create a solution as described in #1279 (comment). Is there any way we can get this work done?

aaronlehmann · 2017-05-19T01:29:26Z

I don't think this is an issue anymore because swarmctl no longer uses NamePrefixes.

GordonTheTurtle added the status/0-triage label Aug 1, 2016

dperny reviewed Aug 1, 2016
View reviewed changes

aaronlehmann reviewed Aug 1, 2016
View reviewed changes

jinuxstyle force-pushed the fix-name-prefix-matching branch 2 times, most recently from 1e931ab to 2a644ad Compare August 5, 2016 00:29

stevvooe reviewed Aug 5, 2016
View reviewed changes

jinuxstyle force-pushed the fix-name-prefix-matching branch from 2a644ad to dfcd073 Compare August 20, 2016 02:50

dperny mentioned this pull request Aug 29, 2016

Fixes#1105 #1194

Closed

stevvooe mentioned this pull request Nov 3, 2016

api: allow NW name that is the prefix of a swarm NW ID moby/moby#27938

Merged

aaronlehmann closed this May 19, 2017

cmd: fix name prefix matching #1279

cmd: fix name prefix matching #1279

Conversation

jinuxstyle commented Aug 1, 2016

codecov-io commented Aug 1, 2016 • edited

Current coverage is 55.05% (diff: 100%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperny commented Aug 1, 2016

Choose a reason for hiding this comment

jinuxstyle Aug 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Aug 2, 2016

jinuxstyle commented Aug 2, 2016

jinuxstyle commented Aug 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Aug 19, 2016

stevvooe commented Aug 19, 2016

jinuxstyle commented Aug 20, 2016

jinuxstyle commented Aug 20, 2016

jinuxstyle commented Aug 20, 2016

stevvooe commented Aug 27, 2016

dperny commented Aug 29, 2016

stevvooe commented Aug 29, 2016

jinuxstyle commented Sep 10, 2016 • edited

stevvooe commented Sep 12, 2016

jinuxstyle commented Sep 13, 2016

stevvooe commented Sep 13, 2016

jinuxstyle commented Sep 13, 2016

stevvooe commented Sep 13, 2016

aluzzardi commented Sep 26, 2016

jinuxstyle commented Sep 27, 2016

AkihiroSuda commented Nov 25, 2016

stevvooe commented Nov 28, 2016

aaronlehmann commented May 19, 2017

codecov-io commented Aug 1, 2016 •

edited

jinuxstyle Aug 2, 2016 •

edited

jinuxstyle commented Aug 5, 2016 •

edited

jinuxstyle commented Sep 10, 2016 •

edited