Skip to content
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

Add support for filtering on node labels #37650

Merged
merged 1 commit into from Aug 22, 2018

Conversation

Projects
None yet
6 participants
@anshulpundir
Copy link
Contributor

commented Aug 15, 2018

Full diff: docker/swarmkit@8852e88...496d19b

Relevant changes:

ping @andrewhsu @thaJeztah @dperny

@yongtang
Copy link
Member

left a comment

LGTM once janky is 💚

@thaJeztah
Copy link
Member

left a comment

Thanks! Looks like we need some changes to make the new filter work;

  • update newListNodesFilters() to accept the new filter
    func newListNodesFilters(filter filters.Args) (*swarmapi.ListNodesRequest_Filters, error) {
    accepted := map[string]bool{
    "name": true,
    "id": true,
    "label": true,
    "role": true,
    "membership": true,
    }
    if err := filter.Validate(accepted); err != nil {
    return nil, err
    }
    f := &swarmapi.ListNodesRequest_Filters{
    NamePrefixes: filter.Get("name"),
    IDPrefixes: filter.Get("id"),
    Labels: runconfigopts.ConvertKVStringsToMap(filter.Get("label")),
    }
    for _, r := range filter.Get("role") {
    if role, ok := swarmapi.NodeRole_value[strings.ToUpper(r)]; ok {
    f.Roles = append(f.Roles, swarmapi.NodeRole(role))
    } else if r != "" {
    return nil, fmt.Errorf("Invalid role filter: '%s'", r)
    }
    }
    for _, a := range filter.Get("membership") {
    if membership, ok := swarmapi.NodeSpec_Membership_value[strings.ToUpper(a)]; ok {
    f.Memberships = append(f.Memberships, swarmapi.NodeSpec_Membership(membership))
    } else if a != "" {
    return nil, fmt.Errorf("Invalid membership filter: '%s'", a)
    }
    }
    return f, nil
    }
  • update the Swagger file to document the newly accepted filter; https://github.com/moby/moby/blob/bd06a5ea4df919ff323510fba10801b5673a3af6/api/swagger.yaml?utf8=✓#L8577-L8589
  • add a note to the API changelog to mention the new filter; https://github.com/moby/moby/blob/master/docs/api/version-history.md. This change will be in API 1.39, so either a new entry should be added for that version, or that would come with #37472 (once merged)
@codecov

This comment has been minimized.

Copy link

commented Aug 16, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@2629fe9). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #37650   +/-   ##
=========================================
  Coverage          ?   36.13%           
=========================================
  Files             ?      610           
  Lines             ?    45056           
  Branches          ?        0           
=========================================
  Hits              ?    16282           
  Misses            ?    26531           
  Partials          ?     2243

@anshulpundir anshulpundir force-pushed the anshulpundir:vndr branch from 67cddb5 to 6b2f444 Aug 16, 2018

@anshulpundir

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

addressed comments from @thaJeztah

"label": true,
"role": true,
"membership": true,
"node-labels": true,

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 17, 2018

Member

looks like this should be singular

@@ -19,6 +19,7 @@ keywords: "API, Docker, rcli, REST, documentation"

* `GET /info` now returns an empty string, instead of `<unknown>` for `KernelVersion`
and `OperatingSystem` if the daemon was unable to obtain this information.
* `GET /nodes` now supports a filter type `node-label` ilter to filter nodes based on the label. The format of the label filter could be `label=<key>`/`label=<key>=<value>` to remove those with the specified labels, or `label!=<key>`/`label!=<key>=<value>` to remove those without the specified labels.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 17, 2018

Member

typo s/ilter/filter/

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 17, 2018

Member

also s/label/node-label/

@@ -8585,6 +8585,7 @@ paths:
- `label=<engine label>`
- `membership=`(`accepted`|`pending`)`
- `name=<node name>`
- `node-label=<node label>`

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 17, 2018

Member

also thinking if this should be node.label (full stop, instead of hyphen), as was proposed in #28209

ping @vdemeester @dperny?

@andrewhsu

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

I'd suggest ignoring the z errors because we've had intermittent networking issues with our z infrastructure.

Downloading 'library/busybox:glibc@sha256:0b55a30394294ab23b9afd58fab94e61a923f5834fba7ddbae7f8e0c11ba85e6' (1 layers)...
curl: (28) Operation timed out after 300521 milliseconds with 0 out of 0 bytes received
@@ -8593,6 +8593,7 @@ paths:
- `label=<engine label>`
- `membership=`(`accepted`|`pending`)`
- `name=<node name>`
- `node-label=<node.label>`

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 20, 2018

Member

hm, looks like you changed the example value, instead of the "key"; we could discuss quickly, but I think the idea in #28209 was to have --filter node.label=foobar (so node.label instead of node-label)

@vdemeester ^^

This comment has been minimized.

Copy link
@vdemeester

vdemeester Aug 20, 2018

Member

Yeah, to have node.label, engine.label and label. The dot . was maintly to be consistent with constraint if I rembember correctly 👼

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 20, 2018

Member

Ah; yes; that was the reason

@@ -16,6 +16,7 @@ func newListNodesFilters(filter filters.Args) (*swarmapi.ListNodesRequest_Filter
"label": true,
"role": true,
"membership": true,
"node-label": true,

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 20, 2018

Member

same here (and other places below)

@@ -21,6 +21,7 @@ keywords: "API, Docker, rcli, REST, documentation"
and `OperatingSystem` if the daemon was unable to obtain this information.
* `GET /info` now returns information about the product license, if a license
has been applied to the daemon.
* `GET /nodes` now supports a filter type `node-label` filter to filter nodes based on the node-label. The format of the label filter could be `label=<key>`/`label=<key>=<value>` to remove those with the specified labels, or `label!=<key>`/`label!=<key>=<value>` to remove those without the specified labels.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Aug 20, 2018

Member

The examples here still don't match; e.g. label=<key>=<value> should be either node-label=<key>=<value> or node.label=<key>=<value>

@anshulpundir anshulpundir force-pushed the anshulpundir:vndr branch from b3dacbe to c42a514 Aug 20, 2018

@thaJeztah
Copy link
Member

left a comment

LGTM, thanks!

@andrewhsu
Copy link
Contributor

left a comment

LGTM

ignore the z error:

curl: (56) SSL read: error:00000000:lib(0):func(0):reason(0), errno 104

because z infrastructure issues

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

Failure on experimental (think that's a flaky)

19:17:48 FAIL: docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection
19:17:48 
19:17:48 [dfbd0fadafb65] waiting for daemon to start
19:17:48 [dfbd0fadafb65] daemon started
19:17:48 
19:17:48 [d96abff1621cb] waiting for daemon to start
19:17:48 [d96abff1621cb] daemon started
19:17:48 
19:17:48 [df10c3972bb8f] waiting for daemon to start
19:17:48 [df10c3972bb8f] daemon started
19:17:48 
19:17:48 [dfbd0fadafb65] exiting daemon
19:17:48 assertion failed: error is not nil: Error response from daemon: rpc error: code = DeadlineExceeded desc = context deadline exceeded
19:17:48 [d96abff1621cb] exiting daemon
19:17:48 [df10c3972bb8f] exiting daemon

@thaJeztah thaJeztah changed the title Bump SwarmKit to 496d19be63751d60a55887604683e5cb1a5541aa Add support for filtering on node labels Aug 21, 2018

Changes to cluster/filter, swagger.yaml, version-history.md for filte…
…ring on node labels.

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@thaJeztah thaJeztah force-pushed the anshulpundir:vndr branch from c42a514 to 514ce73 Aug 21, 2018

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

Dropped the swarmkit bump commit, because master is now ahead of that commit, and fixed the merge-conflict in docs/api/version-history.md

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

Argh. Janky failing again on that swagger-check; #36714. I'll restart, but we can likely look at the "experimental" build as well (which should run the same tests)

@anshulpundir

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

thx adjusting this PR! @thaJeztah

@thaJeztah thaJeztah merged commit a0385f7 into moby:master Aug 22, 2018

5 of 6 checks passed

powerpc Jenkins build is being scheduled
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 41741 has succeeded
Details
janky Jenkins build Docker-PRs 50516 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 21821 has succeeded
Details
z Jenkins build Docker-PRs-s390x 10815 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.