Services: use ServiceStatus on API v1.41 and up #2157

thaJeztah · 2019-10-23T22:24:35Z

depends on:

~~build static binaries with -tags osusergo #2162 build static binaries with -tags osusergo~~
Revert "homedir: add cgo or osusergo buildtag constraints for unix" moby/moby#40134 Revert "homedir: add cgo or osusergo buildtag constraints for unix"
Fix-up (gometalinter) linting config #2163 Fix-up (gometalinter) linting config
bump docker/docker to a09e6e323e55e1a9b21df9c2c555f5668df3ac9b #2158 bump docker/docker to a09e6e323e55e1a9b21df9c2c555f5668df3ac9b

Services: use ServiceStatus on API v1.41 and up

API v1.41 adds a new option to get the number of desired and running tasks when listing services. This patch enables this functionality, and provides a fallback mechanism when the ServiceStatus is not available, which would be when using an older API version.

Now that the swarm.Service struct captures this information, the ListInfo type is no longer needed, so it is removed, and the related list- and formatting functions have been modified accordingly.

To reduce repetition, sorting the services has been moved to the formatter. This is a slight change in behavior, but all calls to the formatter performed this sort first, so the change will not lead to user-facing changes.

thaJeztah · 2019-10-24T10:30:44Z

~~taking care of the breaking changes in the homedir package first, in #2158~~ done

cli/command/service/list.go

thaJeztah · 2019-10-26T13:48:29Z

cli/command/service/formatter.go

+	return fmt.Sprintf("%d/%d", running, desired)
+}
+
+func (c *serviceContext) maxReplicas() uint64 {


I want to export this one, so that it can be used in --format, but keeping that separate to reduce the changes a bit

thaJeztah · 2019-10-26T13:49:13Z

cli/command/service/formatter_test.go

+02_bar              bar                 replicated          2/4                                        *:80->8090/udp
+01_baz              baz                 global              1/3                                        *:80->8080/tcp
+04_qux2             qux2                replicated          3/3 (max 2 per node)                       
+03_qux10            qux10               replicated          2/3 (max 1 per node)                       


Added test-cases to test the max X per node output

thaJeztah · 2019-10-26T13:49:53Z

cli/command/service/formatter_test.go

 `,
 		},
 	}

 	for _, testcase := range cases {
 		services := []swarm.Service{
 			{
-				ID: "id_baz",
+				ID: "01_baz",


prefixing the ID's with numbers, so that we verify the list is sorted by the service name not the service ID

thaJeztah · 2019-10-26T13:51:37Z

cli/command/service/formatter_test.go

+bar                 replicated
+baz                 global
+qux2                replicated
+qux10               replicated


Sorting moved to the formatter (see the PR description), which is why these tests now have their output in a different order. I added services named qux2 and qux10 to verify the formatter uses naturalsort (otherwise qux2 would come after qux10)

thaJeztah · 2019-10-26T13:52:33Z

cli/command/service/formatter_test.go

 				},
 				Endpoint: swarm.Endpoint{
 					Ports: []swarm.PortConfig{
 						{
 							PublishMode:   "ingress",
 							PublishedPort: 80,
-							TargetPort:    8080,
-							Protocol:      "tcp",
+							TargetPort:    8090,


Not an important change, but multiple services cannot map to the same host-port, so this is just making the test-case more realistic/correct.

thaJeztah · 2019-10-26T13:53:54Z

cli/command/service/formatter_test.go

-id_bar              bar                 replicated          2/4                                     *:80->8080/tcp
+			`ID                  NAME                MODE                REPLICAS               IMAGE               PORTS
+02_bar              bar                 replicated          2/4                                        *:80->8090/udp
+01_baz              baz                 global              1/3                                        *:80->8080/tcp


changed the replica-count to be unique for each service, to verify we're showing the correct status

thaJeztah · 2019-10-28T10:39:19Z

cli/command/service/list.go

+	if err != nil {
+		return nil, err
+	}
+	if len(tasks) == 0 {


This optimisation just tipped the function over Gocyclo 16, requiring me to add a //nolint 😞 not sure if it's worth factoring these lines out to a separate function just for that

thaJeztah · 2019-10-28T10:46:16Z

cli/command/service/list.go

 		}
 	}
+	if len(status) == 0 {
+		// All services have their ServiceStatus set, so we're done


Theoretically, we could skip this whole function based on API version, but in situations where the API version is unknown/not set, we would not be propagating ServiceStatus (which happens, e.g., in our unit tests). To be on the safe side, the loop above, and this check, make sure that we always check if ServiceStatus is present, and otherwise fall back to the old behaviour of fetching tasks and nodes to get this information.

thaJeztah · 2019-10-28T11:27:42Z

cli/command/service/list_test.go

@@ -22,7 +29,315 @@ func TestServiceListOrder(t *testing.T) {
 		},
 	})
 	cmd := newListCommand(cli)
+	cmd.SetArgs([]string{})


This was to allow running the test separately in my IDE; when pre-compiling the test and running it, this test would otherwise pick-up the -test.run flags as arguments, and fail the test;

go test -c -o /tmp/foo.test github.com/docker/cli/cli/command/service \ go tool test2json -t /tmp/foo.test -test.v -test.run '^TestServiceListOrder$' ... list_test.go:34: assertion failed: error is not nil: \"ls\" accepts no arguments.\n"}

thaJeztah · 2019-10-28T11:41:34Z

okay, cleaned up the commits; this should be ready for review

ping @silvin-lubecki @dperny @rumpl @vdemeester PTAL

silvin-lubecki · 2019-10-28T13:48:07Z

cli/command/service/formatter_test.go

-id_baz              baz                 global              2/4                                     *:80->8080/tcp
-id_bar              bar                 replicated          2/4                                     *:80->8080/tcp
+			`ID                  NAME                MODE                REPLICAS               IMAGE               PORTS
+02_bar              bar                 replicated          2/4                                        *:80->8090/udp


Shouldn't this be sorted by service name? 🤔

Ok just saw your comment below 👍

dperny · 2019-10-28T15:59:06Z

LGTM, thanks! Adding this functionality was actually on my todo list for the sprint, so I'm sorry I didn't communicate that.

thaJeztah · 2019-10-28T16:06:52Z

@dperny no worries; it could've waited, but #2131 came up, where I looked at the changes, and thought: better make use of this new feature, so that we don't have to change the interface / exported functions multiple times.

And with this change, that refactor became quite a bit more simple (last commit on this PR: #2167)

silvin-lubecki · 2019-10-29T14:24:43Z

cli/command/stack/kubernetes/conversion_test.go

-	return swarm.Service{
+func withMode(mode string, replicas uint64) func(*swarm.Service) {
+	return func(service *swarm.Service) {
+		// service.Spec.Mode = swarm.ServiceMode{


nit: commented code?

arf; good catch; I'll update

silvin-lubecki

LGTM, that's a quite good feature there 🤗

rumpl · 2019-10-29T14:40:39Z

LGTM! Thanks @thaJeztah !!

This patch: - Adds new GlobalService and ServiceStatus options - Makes the NodeList() function functional - Minor improvment to the `newService()` function to allow passing options Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This also sets the services to have a Mode set, otherwise they would be invalid. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

API v1.41 adds a new option to get the number of desired and running tasks when listing services. This patch enables this functionality, and provides a fallback mechanism when the ServiceStatus is not available, which would be when using an older API version. Now that the swarm.Service struct captures this information, the `ListInfo` type is no longer needed, so it is removed, and the related list- and formatting functions have been modified accordingly. To reduce repetition, sorting the services has been moved to the formatter. This is a slight change in behavior, but all calls to the formatter performed this sort first, so the change will not lead to user-facing changes. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2019-10-29T14:50:31Z

removed the stray comment; should be good to go now once CI finishes 👍

thaJeztah · 2019-10-29T14:57:28Z

All nice and shiny green; I'll go ahead and merge 👍

GordonTheTurtle added the status/0-triage label Oct 23, 2019

thaJeztah force-pushed the servicestatus branch from dafd247 to 8426186 Compare October 24, 2019 00:06

thaJeztah commented Oct 24, 2019

View reviewed changes

cli/command/service/list.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the servicestatus branch 2 times, most recently from 50f857b to 16a6815 Compare October 25, 2019 13:13

thaJeztah mentioned this pull request Oct 25, 2019

Refactor the stack services command to be uniform [carry 2131] #2167

Merged

6 tasks

thaJeztah force-pushed the servicestatus branch 10 times, most recently from c006d0a to 2d289c9 Compare October 26, 2019 13:47

thaJeztah commented Oct 26, 2019

View reviewed changes

thaJeztah force-pushed the servicestatus branch 7 times, most recently from 79b154b to f9bd3f8 Compare October 28, 2019 10:29

thaJeztah added area/api area/swarm impact/changelog status/2-code-review and removed status/0-triage labels Oct 28, 2019

thaJeztah commented Oct 28, 2019

View reviewed changes

thaJeztah marked this pull request as ready for review October 28, 2019 10:39

thaJeztah requested a review from silvin-lubecki as a code owner October 28, 2019 10:39

thaJeztah commented Oct 28, 2019

View reviewed changes

thaJeztah force-pushed the servicestatus branch 2 times, most recently from 53a41cc to 66bcac6 Compare October 28, 2019 10:53

thaJeztah commented Oct 28, 2019

View reviewed changes

thaJeztah force-pushed the servicestatus branch from 66bcac6 to 4ab76b0 Compare October 28, 2019 11:28

silvin-lubecki reviewed Oct 28, 2019

View reviewed changes

silvin-lubecki reviewed Oct 29, 2019

View reviewed changes

silvin-lubecki approved these changes Oct 29, 2019

View reviewed changes

thaJeztah added 4 commits October 29, 2019 15:47

TestStackServicesErrors: use sub-tests, and return tasks

228e0f5

This also sets the services to have a Mode set, otherwise they would be invalid. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Add test for ServiceStatus

20424e2

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the servicestatus branch from 4ab76b0 to 20424e2 Compare October 29, 2019 14:49

thaJeztah merged commit b3cde35 into docker:master Oct 29, 2019

thaJeztah deleted the servicestatus branch October 29, 2019 14:57

thaJeztah added this to the next milestone Feb 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Services: use ServiceStatus on API v1.41 and up #2157

Services: use ServiceStatus on API v1.41 and up #2157

thaJeztah commented Oct 23, 2019 •

edited

Loading

thaJeztah commented Oct 24, 2019 •

edited

Loading

thaJeztah Oct 26, 2019

thaJeztah Oct 26, 2019

thaJeztah Oct 26, 2019

thaJeztah Oct 26, 2019

thaJeztah Oct 26, 2019 •

edited

Loading

thaJeztah Oct 26, 2019

thaJeztah Oct 28, 2019

thaJeztah Oct 28, 2019

thaJeztah Oct 28, 2019

thaJeztah commented Oct 28, 2019

silvin-lubecki Oct 28, 2019

silvin-lubecki Oct 28, 2019

dperny commented Oct 28, 2019

thaJeztah commented Oct 28, 2019

silvin-lubecki Oct 29, 2019

thaJeztah Oct 29, 2019

silvin-lubecki left a comment

rumpl commented Oct 29, 2019

thaJeztah commented Oct 29, 2019

thaJeztah commented Oct 29, 2019

Services: use ServiceStatus on API v1.41 and up #2157

Services: use ServiceStatus on API v1.41 and up #2157

Conversation

thaJeztah commented Oct 23, 2019 • edited Loading

Services: use ServiceStatus on API v1.41 and up

thaJeztah commented Oct 24, 2019 • edited Loading

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

thaJeztah Oct 26, 2019 • edited Loading

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

thaJeztah commented Oct 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperny commented Oct 28, 2019

thaJeztah commented Oct 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

rumpl commented Oct 29, 2019

thaJeztah commented Oct 29, 2019

thaJeztah commented Oct 29, 2019

thaJeztah commented Oct 23, 2019 •

edited

Loading

thaJeztah commented Oct 24, 2019 •

edited

Loading

thaJeztah Oct 26, 2019 •

edited

Loading