Improve default handling for "service create" by aaronlehmann · Pull Request #32284 · moby/moby

aaronlehmann · 2017-03-31T23:09:07Z

Currently, the CLI hardcodes several defaults. This is a problem because it's awkward to keep CLI defaults in sync with swarmkit defaults. Also, when a service is being created, it generally takes on several default values of flags that the user did not specify. If these defaults are changed later in swarmkit, they will not apply to those services, because they will continue to use whatever the CLI hardcoded at the time.

This PR makes two changes across two commits.

Changes docker service inspect to use the new InsertDefaults flag so that fields that are omitted in the service definition show default values in docker service inspect. This paves the way for docker service create to avoid hardcoding its defaults in services without hiding information from inspect.
Changes docker service create to omit fields that are not specified by the user, when possible. This will allow defaults to be applied inside the manager. To provide visibility into defaults in the CLI help, the CLI uses swarmkit's api/defaults package to annotate docker service create --help output. These defaults are left out of docker service update --help, because defaults are misleading in this context - if a flag is not specified, it has no default action.

Fixes #24959
Fixes #32094

ping @aluzzardi @dongluochen @dnephin @thaJeztah

vieux · 2017-04-01T00:02:11Z

ping @dnephin

dnephin

It's unfortunate that the pflag library tries to stay so close to the golang flag package API. It is not well suited for this type of behaviour.

dnephin · 2017-04-03T18:03:16Z

cli/command/service/opts.go

I believe this is always passed the same value defaults.Service.Update.

Does this need to be a parameter? Could it look like the other defaults() functions and accept no params?

Also, this doesn't make any use of opts, which means it doesn't need to be an instance method. It should be just a function.

This is passed either defaults.Service.Update or defaults.Rollback.Update. I agree that it's strange as a method on updateOpts. I'll change it to a function.

Any preference on whether it should be a single function that takes a *api.UpdateConfig (like this method), or separate functions that call a shared implementation with either the Update or Rollback default values?

Ah, I missed that, thanks!

This method moved to a function as-is sgtm.

dnephin · 2017-04-03T18:05:07Z

cli/command/service/opts.go

This doesn't use r, so shouldn't be a method on restartPolicyOptions()

dnephin · 2017-04-03T18:06:17Z

cli/command/service/update.go

With my suggested changes above this function will no longer need a serviceOpts, since these methods become functions.

dnephin · 2017-04-03T18:09:49Z

client/service_inspect.go

I believe the pattern we normally use here is opts ServiceInspectOptions.

It's true that right now this would be a struct with a single field, but the next time we have to go adding a new flag we'll only need to change two lines, instead of every place ServiceInspectWithRaw() is called (for the second time).

Boolean paramaters are also hard to understand from the calling function, having insertDefaults: true is much nicer to read.

SGTM. I noticed that there was a mix of styles, with some functions taking verbose, or force boolean arguments. I can switch to an options structure if that's the preferred style.

dnephin · 2017-04-03T18:38:09Z

cli/command/service/opts.go

This function became a lot harder to read. There's a lot of branching, and string formatting that's not really relevant to the primary responsibility of this function (adding flags).

Instead of a boolean argument, could we pass in a defaults map[string]string ?

All of this logic for formatting defaults can be moved to a buildServiceDefaultFlagMapping(), and passed as the argument from newCreateCommand(). In update we can pass an empty map[string]string{}.

With this new mapping all of the descriptions can become "shared text" + defaults[flagName].

I guess this is a little more complicated in the case where we are actually setting default values, not just error messages. I think that can be handled as well by creating a FlagDefaults struct (instead of just a map[string]string), and adding getInt64(string flagName) int64 accessors, along with another map for default values.

dnephin · 2017-04-03T18:39:30Z

cli/command/service/opts.go

This formatting is duplicated quite a few times:

func formatDefault(value interface{}) string { ... }

dnephin · 2017-04-03T19:15:30Z

cli/command/service/opts.go

Why does the help string claim a default that isn't actually set as the default on the flag?

DurationVar is a little weird. IIRC it shows the default value in the CLI help, but only if it's nonzero. I'd like us to always show the default on service create, even if it's zero, and never show it on service update. So it made sense to manipulate the flag description instead of providing a default value. If we did both, the default value would appear twice when the default is nonzero.

I can add a comment explaing this.

dnephin · 2017-04-03T19:28:00Z

cli/command/service/opts.go

I would love to find a way to avoid needing flags here, but I can't find any right now.

Can we move this these back to the ToRestartPolicy() function, and add a flags arg ? (and the same with the few below this)

aaronlehmann · 2017-04-03T22:33:54Z

@dnephin: Thanks for the thoughtful review. I've addressed the comments - PTAL

aaronlehmann · 2017-04-04T02:14:26Z

Rebased

thaJeztah · 2017-04-04T15:17:41Z

I like this, finally time to give it a quick look (from a UX perspective), and seeing actual defaults in docker service create --help is really cool.

I haven't had time yet to test against an actually older daemon, but running this;

DOCKER_API_VERSION=1.26 docker service create --help

Seems to still show the defaults in docker service create --help; is that expected?

aaronlehmann · 2017-04-04T17:06:56Z

I haven't had time yet to test against an actually older daemon, but running this;
DOCKER_API_VERSION=1.26 docker service create --help
Seems to still show the defaults in docker service create --help; is that expected?

Yes, it's expected. I've pondered the idea of adding an API endpoint where the client can query what defaults are in place on the manager. But it seems like a lot of complexity for something that is generally not a factor (defaults change rarely, and mixed client/server version setups are unusual, and the worst case outcome is outdated help text). In this implementation, the defaults shown in CLI help are linked into the client at build time.

If you think fetching the defaults via an API is worth it, we could consider going in that direction.

aaronlehmann · 2017-04-04T17:09:22Z

Rebased

dnephin

Design LGTM

aaronlehmann · 2017-04-06T23:56:06Z

Rebased

dongluochen

sgtm

dongluochen · 2017-04-07T00:46:25Z

docs/reference/commandline/service_update.md

remove (default 0s)

dongluochen · 2017-04-07T02:27:45Z

design lgtm.

vdemeester

Design LGTM, moving to code review

vdemeester · 2017-04-07T14:12:17Z

@aaronlehmann needs a rebase 👼

aaronlehmann · 2017-04-07T17:36:50Z

Rebased. flagUpdateOrder and flagRollbackOrder are now handled.

vieux · 2017-04-07T23:39:37Z

@aaronlehmann seems like the windows failure is related
..\..\api\server\router\swarm\helpers.go:42: not enough arguments in call to sr.backend.GetService

aaronlehmann · 2017-04-07T23:54:47Z

Deleted my earlier comment, because it was indeed a problem with this PR. I thought it was an existing problem on master, but it turned out the reason it only showed up on windows was because each builder does a separate merge to master so they're not building the same version of the code (which is kind of a problem IMHO!).

aaronlehmann · 2017-04-07T23:56:12Z

@vieux: Fixed, thanks!

vdemeester

LGTM 🐯

thaJeztah · 2017-04-10T09:51:05Z

I'm a bit confused, but probably do something wrong..

docker service create --restart-condition="on-failure" --name web nginx:alpine
2i3pres421je9kuo5rh1vf2n4

Makes this request;

{
  "EndpointSpec": {
    "Mode": "vip"
  },
  "Labels": {},
  "Mode": {
    "Replicated": {}
  },
  "Name": "web",
  "TaskTemplate": {
    "ContainerSpec": {
      "DNSConfig": {},
      "Image": "nginx:alpine"
    },
    "ForceUpdate": 0,
    "Placement": {},
    "Resources": {
      "Limits": {},
      "Reservations": {}
    },
    "RestartPolicy": {
      "Condition": "on-failure",
      "Delay": 5000000000,
      "MaxAttempts": 0
    }
  }
}

With the insertDefaults=true option set;

/ # curl --unix-socket /var/run/docker.sock -sL http://localhost/v1.29/services/web?insertDefaults=true | jq .
{
  "ID": "2i3pres421je9kuo5rh1vf2n4",
  "Version": {
    "Index": 31
  },
  "CreatedAt": "2017-04-10T09:36:58.859857088Z",
  "UpdatedAt": "2017-04-10T09:36:58.859857088Z",
  "Spec": {
    "Name": "web",
    "Labels": {},
    "TaskTemplate": {
      "ContainerSpec": {
        "Image": "nginx:alpine@sha256:5aadb68304a38a8e2719605e4e180413f390cd6647602bee9bdedd59753c3590",
        "DNSConfig": {}
      },
      "Resources": {
        "Limits": {},
        "Reservations": {}
      },
      "RestartPolicy": {
        "Condition": "on-failure",
        "Delay": 5000000000,
        "MaxAttempts": 0
      },
      "Placement": {},
      "ForceUpdate": 0
    },
    "Mode": {
      "Replicated": {
        "Replicas": 1
      }
    },
    "EndpointSpec": {
      "Mode": "vip"
    }
  },
  "Endpoint": {
    "Spec": {}
  }
}

Without the insertDefaults=true option set;

/ # curl --unix-socket /var/run/docker.sock -sL http://localhost/v1.29/services/web | jq .
{
  "ID": "2i3pres421je9kuo5rh1vf2n4",
  "Version": {
    "Index": 31
  },
  "CreatedAt": "2017-04-10T09:36:58.859857088Z",
  "UpdatedAt": "2017-04-10T09:36:58.859857088Z",
  "Spec": {
    "Name": "web",
    "Labels": {},
    "TaskTemplate": {
      "ContainerSpec": {
        "Image": "nginx:alpine@sha256:5aadb68304a38a8e2719605e4e180413f390cd6647602bee9bdedd59753c3590",
        "DNSConfig": {}
      },
      "Resources": {
        "Limits": {},
        "Reservations": {}
      },
      "RestartPolicy": {
        "Condition": "on-failure",
        "Delay": 5000000000,
        "MaxAttempts": 0
      },
      "Placement": {},
      "ForceUpdate": 0
    },
    "Mode": {
      "Replicated": {
        "Replicas": 1
      }
    },
    "EndpointSpec": {
      "Mode": "vip"
    }
  },
  "Endpoint": {
    "Spec": {}
  }
}

aaronlehmann · 2017-04-10T18:34:16Z

@thaJeztah: You're right - there was a problem. Specifying the service ID led to the correct behavior, but services names were not handled properly, as the service was returned directly from ListServices. I've fixed this and added test coverage.

This adds a new parameter insertDefaults to /services/{id}. When this is set, an empty field (such as UpdateConfig) will be populated with default values in the API response. Make "service inspect" use this, so that empty fields do not result in missing information when inspecting a service. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

If no fields related to an update config or restart policy are specified, these structs should not be created as part of the service, to avoid hardcoding the current defaults. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

thaJeztah · 2017-04-11T10:55:17Z

Ah! That looks better;

docker service inspect web
[
    {
        "ID": "k850seipz5ohpemq1cotenze2",
        "Version": {
            "Index": 10
        },
        "CreatedAt": "2017-04-11T10:51:21.181483381Z",
        "UpdatedAt": "2017-04-11T10:51:21.181483381Z",
        "Spec": {
            "Name": "web",
            "Labels": {},
            "TaskTemplate": {
                "ContainerSpec": {
                    "Image": "nginx:alpine@sha256:5aadb68304a38a8e2719605e4e180413f390cd6647602bee9bdedd59753c3590",
                    "StopGracePeriod": 10000000000,
                    "DNSConfig": {}
                },
                "Resources": {
                    "Limits": {},
                    "Reservations": {}
                },
                "RestartPolicy": {
                    "Condition": "on-failure",
                    "Delay": 5000000000,
                    "MaxAttempts": 0
                },
                "Placement": {},
                "ForceUpdate": 0
            },
            "Mode": {
                "Replicated": {
                    "Replicas": 1
                }
            },
            "UpdateConfig": {
                "Parallelism": 1,
                "FailureAction": "pause",
                "Monitor": 5000000000,
                "MaxFailureRatio": 0,
                "Order": "stop-first"
            },
            "RollbackConfig": {
                "Parallelism": 1,
                "FailureAction": "pause",
                "Monitor": 5000000000,
                "MaxFailureRatio": 0,
                "Order": "stop-first"
            },
            "EndpointSpec": {
                "Mode": "vip"
            }
        },
        "Endpoint": {
            "Spec": {}
        }
    }
]

thaJeztah

LGTM

thaJeztah · 2017-04-11T11:01:49Z

api/swagger.yaml

          description: "ID or name of service."
          required: true
          type: "string"
+        - name: "insertDefaults"


This needs to be added to the API version-history; https://github.com/docker/docker/blob/master/docs/api/version-history.md

(ok with doing that in a follow-up)

thaJeztah · 2017-04-11T11:06:46Z

All green; I'll go ahead and merge so that this is in 17.05. I'll include the API version changes #32284 (comment) in the "changelog" pull request

Improve default handling for "service create"

GordonTheTurtle added the status/0-triage label Mar 31, 2017

aaronlehmann force-pushed the fix-service-defaults branch from 06a40b3 to 55a0368 Compare March 31, 2017 23:24

aaronlehmann added status/1-design-review and removed status/0-triage labels Mar 31, 2017

dnephin reviewed Apr 3, 2017

View reviewed changes

aaronlehmann force-pushed the fix-service-defaults branch 2 times, most recently from bf92ba3 to 6064953 Compare April 3, 2017 22:33

aaronlehmann force-pushed the fix-service-defaults branch from 6064953 to 1c5fb33 Compare April 4, 2017 02:14

aaronlehmann force-pushed the fix-service-defaults branch from 1c5fb33 to 9d94390 Compare April 4, 2017 17:09

dnephin reviewed Apr 5, 2017

View reviewed changes

aaronlehmann force-pushed the fix-service-defaults branch from 9d94390 to 17e9e60 Compare April 6, 2017 23:56

dongluochen reviewed Apr 7, 2017

View reviewed changes

aaronlehmann force-pushed the fix-service-defaults branch from 17e9e60 to 1d6d4d7 Compare April 7, 2017 01:49

vdemeester reviewed Apr 7, 2017

View reviewed changes

vdemeester added status/2-code-review and removed status/1-design-review labels Apr 7, 2017

aaronlehmann force-pushed the fix-service-defaults branch from 1d6d4d7 to bf47928 Compare April 7, 2017 17:35

vieux added the rebuild/windowsRS1 label Apr 7, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Apr 7, 2017

vieux added the rebuild/z label Apr 7, 2017

GordonTheTurtle removed the rebuild/z label Apr 7, 2017

aaronlehmann force-pushed the fix-service-defaults branch from bf47928 to d9331a2 Compare April 7, 2017 23:56

vdemeester approved these changes Apr 10, 2017

View reviewed changes

aaronlehmann force-pushed the fix-service-defaults branch from d9331a2 to c115d68 Compare April 10, 2017 18:33

Aaron Lehmann added 2 commits April 10, 2017 13:41

aaronlehmann force-pushed the fix-service-defaults branch from c115d68 to bbe1202 Compare April 10, 2017 20:41

aaronlehmann added the rebuild/z label Apr 11, 2017

GordonTheTurtle removed the rebuild/z label Apr 11, 2017

thaJeztah approved these changes Apr 11, 2017

View reviewed changes

thaJeztah added impact/api impact/changelog impact/cli labels Apr 11, 2017

thaJeztah merged commit a258ef5 into moby:master Apr 11, 2017

GordonTheTurtle added this to the 17.05.0 milestone Apr 11, 2017

thaJeztah mentioned this pull request Apr 11, 2017

17.05.0 rc1 changelog #32498

Merged

aaronlehmann deleted the fix-service-defaults branch April 12, 2017 01:06

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#32284 from aaronlehmann/fix-service-defaults

35fc4e4

Improve default handling for "service create"

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#32284 from aaronlehmann/fix-service-defaults

f3eacf2

Improve default handling for "service create"

thaJeztah mentioned this pull request Jun 4, 2017

API docs: query parameters missing for the GET /services/{id} endpoint #33515

Closed

thaJeztah mentioned this pull request Nov 9, 2017

CpuShares default value in docker inspect is 0 #35452

Closed

thaJeztah mentioned this pull request Dec 15, 2018

Proposal: Allow user to set own service defaults moby/swarmkit#2794

Open

Conversation

aaronlehmann commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vieux commented Apr 1, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Apr 3, 2017

Uh oh!

aaronlehmann commented Apr 4, 2017

Uh oh!

thaJeztah commented Apr 4, 2017

Uh oh!

aaronlehmann commented Apr 4, 2017

Uh oh!

aaronlehmann commented Apr 4, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Apr 6, 2017

Uh oh!

dongluochen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongluochen commented Apr 7, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Apr 7, 2017

Uh oh!

aaronlehmann commented Apr 7, 2017

Uh oh!

vieux commented Apr 7, 2017

Uh oh!

aaronlehmann commented Apr 7, 2017

Uh oh!

aaronlehmann commented Apr 7, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 10, 2017

Uh oh!

aaronlehmann commented Apr 10, 2017

Uh oh!

thaJeztah commented Apr 11, 2017

Uh oh!

thaJeztah left a comment

aaronlehmann commented Mar 31, 2017 •

edited

Loading