-
Notifications
You must be signed in to change notification settings - Fork 609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make node ls filtering support both node label and engine label #2062
make node ls filtering support both node label and engine label #2062
Conversation
b46ba7a
to
37c40ee
Compare
Codecov Report
@@ Coverage Diff @@
## master #2062 +/- ##
=========================================
- Coverage 59.62% 55.9% -3.73%
=========================================
Files 118 55 -63
Lines 19544 8771 -10773
=========================================
- Hits 11654 4903 -6751
+ Misses 6541 3342 -3199
+ Partials 1349 526 -823 Continue to review full report at Codecov.
|
37c40ee
to
22e1b87
Compare
manager/controlapi/node.go
Outdated
@@ -136,6 +136,10 @@ func (s *Server) ListNodes(ctx context.Context, request *api.ListNodesRequest) ( | |||
if len(request.Filters.Labels) == 0 { | |||
return true | |||
} | |||
if filterMatchLabels(e.Spec.Annotations.Labels, request.Filters.Labels) == true { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that for the SwarmKit side, this should be a separate filter, or at least a way to distinguish between "node" labels and "engine" labels.
We can still decide to keep the "docker" UX the same (filtering by labels using both "engine" and "node" labels), but at least it gives control over what is filtered on at the API level.
@aluzzardi @allencloud wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the separated thing is reasonable. Updated that.
22e1b87
to
4384ff7
Compare
I think what @thaJeztah had in mind was two separate fields in the API to filter on node labels vs. engine labels. |
4384ff7
to
99c64e5
Compare
Yes; didn't look at the last changes in depth, but they should be "separate" options/filters in SwarmKit |
47139d7
to
b8c67ec
Compare
Signed-off-by: allencloud <allen.sun@daocloud.io>
b8c67ec
to
0b97e37
Compare
repeated NodeRole roles = 5; | ||
map<string, string> node_labels = 3; | ||
map<string, string> engine_labels = 4; | ||
repeated NodeSpec.Membership memberships = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't okay to renumber existing values.
@allencloud are you still working on this PR? We might want to push this relatively soon, so if you're unable to get to it, I'd be happy to carry it for you. |
Hi, @nishanttotla |
This pr tries to fix moby/moby#27231 and moby/moby#28209
Currently in swarmkit
ListNode
only supports engine label filtering, while not node label filtering. But this PR makes both supported.Signed-off-by: allencloud allen.sun@daocloud.io