-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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 sending down service Running and Desired task counts #39231
Conversation
includes versioning done with an improper version of |
277be14
to
f9fdf44
Compare
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.
thx! left some comments
@@ -167,7 +167,16 @@ func (sr *swarmRouter) getServices(ctx context.Context, w http.ResponseWriter, r | |||
return errdefs.InvalidParameter(err) | |||
} | |||
|
|||
services, err := sr.backend.GetServices(basictypes.ServiceListOptions{Filters: filter}) | |||
var status bool | |||
if value := r.URL.Query().Get("status"); value != "" { |
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.
Can you gate this by API version, so that setting this query-parameter does not have an effect on older API versions?
api/swagger.yaml
Outdated
properties: | ||
RunningTasks: | ||
description: "The number of tasks for the service currently in the Running state" | ||
type: "number" |
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.
Can you add an example value here, and set the correct format/type?
type: "number" | |
type: "integer" | |
format: "uint64" | |
example: 7 |
api/swagger.yaml
Outdated
service spec. For global services, this is computed by taking | ||
count of all tasks for the service with a Desired State other | ||
than Shutdown. | ||
type: "number" |
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.
Can you add an example value here, and set the correct format/type?
type: "number" | |
type: "integer" | |
format: "uint64" | |
example: 10 |
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.
Can you also add a mention in the API history doc; https://github.com/moby/moby/blob/master/docs/api/version-history.md
// the Status field is not supported when retrieving an individual service | ||
// because the Backend API changes required to accomodate it would be too | ||
// disruptive, and because that field is so rarely needed as part of an | ||
// individual service inspection. |
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.
Position of this comment is a bit confusing (as there is no code visible here for status
?)
f9fdf44
to
a1cabfe
Compare
Codecov Report
@@ Coverage Diff @@
## master #39231 +/- ##
=========================================
Coverage ? 37.28%
=========================================
Files ? 609
Lines ? 45261
Branches ? 0
=========================================
Hits ? 16877
Misses ? 26090
Partials ? 2294 |
312f4ec
to
51cdc4c
Compare
to the best of my knowledge, the failure in the experimental build is unrelated. @thaJeztah updated to reflect your comments. |
051f5e2
to
b30f83e
Compare
e87c3e1
to
b614f25
Compare
b614f25
to
a7d5d6c
Compare
unreal tournament announcer voice: re-re-re-rebase |
RunningTasks: | ||
description: "The number of tasks for the service currently in the Running state" | ||
type: "number" | ||
DesiredTasks: |
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.
Isn't desired in the service spec?
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.
Not necessarily, for two reasons:
- As mentioned in the description, for global services, this must be computed by looking at all tasks in desired state
Running
, which cannot be done without getting all tasks for the service, and avoiding getting all tasks for the service is the point of this PR. - The implementation of this in Swarmkit was done in such a way that it's a separate gRPC call, which means there could be races with service updates, where the desired task count changes while the status is being computed. This is no worse than the existing behavior, as the ServiceStatus is consistent within itself, which was also true of these values as computed by looking at tasks (all tasks were retrieved at once and the values were computed client-side, which could in the same way be inconsistent with the service spec.).
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.
Thanks for explaining, that makes sense.
if value := r.URL.Query().Get("insertDefaults"); value != "" { | ||
var err error | ||
insertDefaults, err = strconv.ParseBool(value) | ||
if err != nil { | ||
err := fmt.Errorf("invalid value for insertDefaults: %s", value) |
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.
Why remove the error context?
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.
unsure. It must've gotten removed by accident.
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.
Oh, because directly below, it would get wrapped again, and the result (I think) would be something like Error: invalid value for insertDefaults: invalid value for insertDefaults: {{ strconv error }}
. Am I incorrect on that?
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.
Oh of course :)
services, err := sr.backend.GetServices(basictypes.ServiceListOptions{Filters: filter}) | ||
// the status query parameter is only support in API versions >= 1.41. If | ||
// the client is using a lesser version, ignore the parameter. | ||
cliVersion := r.Header.Get("version") |
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.
Should be able to use httputils.VersionFromContext
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.
Fixing this.
bf4cb85
to
f016519
Compare
f016519
to
b817424
Compare
a test seems to have flaked. i'm going to rebase and push and see if it fails again |
38e2f25
to
f3b9a3a
Compare
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.
LGTM, thanks!
This will need a follow-up in the docker/cli repository to make use of the new field |
@dperny looks like there's a linting failure;
Can you also rebase, to get https://github.com/moby/moby/pulls in (flaky test, which has been disabled for Windows) |
151a3c6
to
9caf9a1
Compare
I've rebased this PR but I'm not clear on what's wrong. Something with swagger is failing, but it's panicking, not outputting useful information about the failure. |
Hm... haven't seen that one before;
Let me have a look if I see what's causing it 🤔 |
api/swagger.yaml
Outdated
in: "query" | ||
type: "boolean" | ||
description: "Include service status, with count of running and desired tasks" | ||
default: false |
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.
Wonder if this is what's causing the panic. I guess a query-string may not support a "default" value? https://tools.ietf.org/html/draft-fge-json-schema-validation-00#section-6.2
This keyword can be used to supply a default JSON value associated with a particular schema. It is RECOMMENDED that a default value be valid against the associated schema.
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.
Perhaps "required": false
would achieve the same
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.
Thanks for looking into this @dperny
9caf9a1
to
87ba762
Compare
Adds a new ServiceStatus field to the Service object, which includes the running and desired task counts. This new field is gated behind a "status" query parameter. Signed-off-by: Drew Erny <drew.erny@docker.com>
87ba762
to
f36042d
Compare
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's green again, with the go-swagger problem fixed 🎉
LGTM
bless you sebastiaan you magnificent man |
full diff: moby/moby@b6684a4...a30990b relevant changes: - moby/moby#39995 Update containerd binary to v1.2.10 - moby/moby#40001 Update runc to v1.0.0-rc8-92-g84373aaa (CVE-2019-16884) - moby/moby#39999 bump golang 1.13.1 (CVE-2019-16276) - moby/moby#40102 bump golang 1.13.3 (CVE-2019-17596) - moby/moby#39994 homedir: add cgo or osusergo buildtag constraints for unix. This is to ensure that users of the homedir package cannot compile statically (`CGO_ENABLED=0`) without also setting the `osusergo` build tag. - moby/moby#39983 builder: remove legacy build's session handling This feature was used by docker build --stream and it was kept experimental. Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit. - Related: docker#2105 build: remove --stream (was experimental) - moby/moby #40045 Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty, golang/gddo, gorilla/mux - moby/moby#39713 bump containerd and dependencies to v1.3.0 - moby/moby#39987 Add ability to handle index acknowledgment with splunk log driver - moby/moby#40070 Use ocischema package instead of custom handler - relates to moby/moby#39727 Docker 19.03 doesn't support OCI image - relates to docker/hub-feedback#1871 - relates to distribution/distribution#3024 - moby/moby#39231 Add support for sending down service Running and Desired task counts - moby/moby#39822 daemon: Use short libnetwork ID in exec-root Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
full diff: moby/moby@b6684a4...a09e6e3 relevant changes: - moby/moby#39995 Update containerd binary to v1.2.10 - moby/moby#40001 Update runc to v1.0.0-rc8-92-g84373aaa (CVE-2019-16884) - moby/moby#39999 bump golang 1.13.1 (CVE-2019-16276) - moby/moby#40102 bump golang 1.13.3 (CVE-2019-17596) - moby/moby#40134 Revert "homedir: add cgo or osusergo buildtag constraints for unix" - reverts moby/moby#39994 homedir: add cgo or osusergo buildtag constraints for unix, in favor of documenting when to set the `osusergo` build tag. The `osusergo` build-flag must be used when compiling a static binary with `cgo` enabled, and linking against `glibc`. - moby/moby#39983 builder: remove legacy build's session handling This feature was used by docker build --stream and it was kept experimental. Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit. - Related: docker#2105 build: remove --stream (was experimental) - moby/moby #40045 Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty, golang/gddo, gorilla/mux - moby/moby#39713 bump containerd and dependencies to v1.3.0 - moby/moby#39987 Add ability to handle index acknowledgment with splunk log driver - moby/moby#40070 Use ocischema package instead of custom handler - relates to moby/moby#39727 Docker 19.03 doesn't support OCI image - relates to docker/hub-feedback#1871 - relates to distribution/distribution#3024 - moby/moby#39231 Add support for sending down service Running and Desired task counts - moby/moby#39822 daemon: Use short libnetwork ID in exec-root - moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion() - updates/requires Microsoft/hscshim@2226e083fc390003ae5aa8325c3c92789afa0e7a Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
full diff: moby/moby@b6684a4...a09e6e3 relevant changes: - moby/moby#39995 Update containerd binary to v1.2.10 - moby/moby#40001 Update runc to v1.0.0-rc8-92-g84373aaa (CVE-2019-16884) - moby/moby#39999 bump golang 1.13.1 (CVE-2019-16276) - moby/moby#40102 bump golang 1.13.3 (CVE-2019-17596) - moby/moby#40134 Revert "homedir: add cgo or osusergo buildtag constraints for unix" - reverts moby/moby#39994 homedir: add cgo or osusergo buildtag constraints for unix, in favor of documenting when to set the `osusergo` build tag. The `osusergo` build-flag must be used when compiling a static binary with `cgo` enabled, and linking against `glibc`. - moby/moby#39983 builder: remove legacy build's session handling This feature was used by docker build --stream and it was kept experimental. Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit. - Related: docker#2105 build: remove --stream (was experimental) - moby/moby #40045 Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty, golang/gddo, gorilla/mux - moby/moby#39713 bump containerd and dependencies to v1.3.0 - moby/moby#39987 Add ability to handle index acknowledgment with splunk log driver - moby/moby#40070 Use ocischema package instead of custom handler - relates to moby/moby#39727 Docker 19.03 doesn't support OCI image - relates to docker/hub-feedback#1871 - relates to distribution/distribution#3024 - moby/moby#39231 Add support for sending down service Running and Desired task counts - moby/moby#39822 daemon: Use short libnetwork ID in exec-root - moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion() - updates/requires Microsoft/hscshim@2226e083fc390003ae5aa8325c3c92789afa0e7a Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
full diff: moby/moby@b6684a4...a09e6e3 relevant changes: - moby/moby#39995 Update containerd binary to v1.2.10 - moby/moby#40001 Update runc to v1.0.0-rc8-92-g84373aaa (CVE-2019-16884) - moby/moby#39999 bump golang 1.13.1 (CVE-2019-16276) - moby/moby#40102 bump golang 1.13.3 (CVE-2019-17596) - moby/moby#40134 Revert "homedir: add cgo or osusergo buildtag constraints for unix" - reverts moby/moby#39994 homedir: add cgo or osusergo buildtag constraints for unix, in favor of documenting when to set the `osusergo` build tag. The `osusergo` build-flag must be used when compiling a static binary with `cgo` enabled, and linking against `glibc`. - moby/moby#39983 builder: remove legacy build's session handling This feature was used by docker build --stream and it was kept experimental. Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit. - Related: #2105 build: remove --stream (was experimental) - moby/moby #40045 Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty, golang/gddo, gorilla/mux - moby/moby#39713 bump containerd and dependencies to v1.3.0 - moby/moby#39987 Add ability to handle index acknowledgment with splunk log driver - moby/moby#40070 Use ocischema package instead of custom handler - relates to moby/moby#39727 Docker 19.03 doesn't support OCI image - relates to docker/hub-feedback#1871 - relates to distribution/distribution#3024 - moby/moby#39231 Add support for sending down service Running and Desired task counts - moby/moby#39822 daemon: Use short libnetwork ID in exec-root - moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion() - updates/requires Microsoft/hscshim@2226e083fc390003ae5aa8325c3c92789afa0e7a Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 7f6cd64335dc631efaa8204c01f92aa40939073a Component: cli
- What I did
Add an optional
ServiceStatus
field in Service objects, which is available as part of List operations, and gives the Running and Desired task counts. This is essentially performing a UI operation on the server side, but as this is probably the most common UI operation and it is rather expensive to perform client side, it's worth doing.- How I did it
Plumbed through the use of a new swarmkit gRPC endpoint (see moby/swarmkit#2856) which allows getting service statuses. This is a separate swarmkit API call because swarmkit's protos function as both the wire format and the persistence format of the objects, and I did not want to add a
Status
field to the swarmkitService
object. However, because this is much less of a concern in the engine, theServiceStatus
field is present on the engine API type.Includes updates to swagger.yml, of course.
- How to verify it
The swarmkit code is well-tested to make sure the right values are computed. The Docker code includes an integration test confirming that the values are correctly plumbed through to the API.
- Description for the changelog
GET
/services
now accepts astatus
parameter, which instructs the engine to return the number of Running and Desired tasks as part of a service.