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

Improve default handling for "service create" #32284

Merged
merged 2 commits into from Apr 11, 2017

Conversation

@aaronlehmann
Contributor

aaronlehmann commented Mar 31, 2017

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.

  1. 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.

  2. 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

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Apr 1, 2017

Collaborator

ping @dnephin

Collaborator

vieux commented Apr 1, 2017

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.

Show outdated Hide outdated cli/command/service/opts.go
Show outdated Hide outdated cli/command/service/opts.go
Show outdated Hide outdated cli/command/service/update.go
Show outdated Hide outdated client/service_inspect.go
Show outdated Hide outdated cli/command/service/opts.go
Show outdated Hide outdated cli/command/service/opts.go
Show outdated Hide outdated cli/command/service/opts.go
Show outdated Hide outdated cli/command/service/opts.go
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 3, 2017

Contributor

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

Contributor

aaronlehmann commented Apr 3, 2017

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

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 4, 2017

Contributor

Rebased

Contributor

aaronlehmann commented Apr 4, 2017

Rebased

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 4, 2017

Member

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?

Member

thaJeztah commented Apr 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 4, 2017

Contributor

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.

Contributor

aaronlehmann commented Apr 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 4, 2017

Contributor

Rebased

Contributor

aaronlehmann commented Apr 4, 2017

Rebased

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 6, 2017

Contributor

Rebased

Contributor

aaronlehmann commented Apr 6, 2017

Rebased

@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Apr 7, 2017

Contributor

design lgtm.

Contributor

dongluochen commented Apr 7, 2017

design lgtm.

@vdemeester

Design LGTM, moving to code review

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Apr 7, 2017

Member

@aaronlehmann needs a rebase 👼

Member

vdemeester commented Apr 7, 2017

@aaronlehmann needs a rebase 👼

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

Rebased. flagUpdateOrder and flagRollbackOrder are now handled.

Contributor

aaronlehmann commented Apr 7, 2017

Rebased. flagUpdateOrder and flagRollbackOrder are now handled.

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Apr 7, 2017

Collaborator

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

Collaborator

vieux commented Apr 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

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!).

Contributor

aaronlehmann commented Apr 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 7, 2017

Contributor

@vieux: Fixed, thanks!

Contributor

aaronlehmann commented Apr 7, 2017

@vieux: Fixed, thanks!

@vdemeester

LGTM 🐯

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 10, 2017

Member

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": {}
  }
}
Member

thaJeztah commented Apr 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Apr 10, 2017

Contributor

@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.

Contributor

aaronlehmann commented Apr 10, 2017

@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.

Change "service inspect" to show defaults in place of empty fields
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>
Make the CLI show defaults from the swarmkit defaults package
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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 11, 2017

Member

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": {}
        }
    }
]
Member

thaJeztah commented Apr 11, 2017

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

@@ -7584,6 +7584,11 @@ paths:
description: "ID or name of service."
required: true
type: "string"
- name: "insertDefaults"

This comment has been minimized.

@thaJeztah

thaJeztah Apr 11, 2017

Member

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

thaJeztah Apr 11, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 11, 2017

Member

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

Member

thaJeztah commented Apr 11, 2017

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

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32789 has succeeded
Details
janky Jenkins build Docker-PRs 41399 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1587 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12527 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1445 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 11, 2017

@thaJeztah thaJeztah referenced this pull request Apr 11, 2017

Merged

17.05.0 rc1 changelog #32498

@aaronlehmann aaronlehmann deleted the aaronlehmann:fix-service-defaults branch Apr 12, 2017

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
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
Improve default handling for "service create"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment