Add flag `--host` to `service create` and `--host-add/rm` to `service update` #28031

yongtang · 2016-11-03T15:30:04Z

- What I did

This fix tries to address #27902 by adding a flag --host to docker service create and --host-add/--host-rm to docker service update, so that it is possible to specify extra host:ip settings in /etc/hosts.

NOTE:
The flag --add-host is to conform to docker run but other suggestions are welcomed.
No flag has been added to docker service update yet, as I am not sure about what flag should be added for add/rm case.

- How I did it

This fix adds Hosts in swarmkit's ContainerSpec so that it is possible to specify extra hosts during service creation.

Related docs has been updated.

- How to verify it

An integration test has been added.

- Description for the changelog

Add flag --add-host to docker service create so that extra host to ip mappings could be added to /etc/hosts.

- A picture of a cute animal (not mandatory but encouraged)

This fix fixes #27902.

icecrime · 2016-11-03T16:53:29Z

Ping @al @mavenugo @dnephin!

aluzzardi

Great start.

I think we need to support service update and also figure out consistency

/cc @icecrime @dnephin @aanand

aluzzardi · 2016-11-07T22:33:28Z

api/types/swarm/container.go

This should be Hosts like in SwarmKit

aluzzardi · 2016-11-07T22:36:53Z

cli/command/service/create.go

To follow convention, this should be:

--host

Then, docker service update should expose:

--host-add

--host-rm

@dnephin Thoughts?

ya, I agree

yongtang · 2016-11-08T02:00:45Z

Thanks @aluzzardi @dnephin @icecrime @aanand for the review. The PR has been updated. Now service create takes --host flag and service update takes --host-add and --host-rm flag.

Please take a look and let me know for any issues.

vdemeester · 2016-11-08T10:53:47Z

Desgin LGTM 🐸

dnephin · 2016-11-08T22:18:12Z

integration-cli/docker_cli_swarm_test.go

I don't think we should have multiple integration tests for every flag.

Really we only need a couple unit tests for the update function.

Thanks @dnephin. The PR has been updated with this integration test removed. The code change is covered by unit tests now.

stevvooe · 2016-11-08T22:28:58Z

Design LGTM

aluzzardi · 2016-11-08T22:41:02Z

docs/reference/commandline/service_create.md

Needs to be --host
Needs service update docs as well

Thanks @aluzzardi the PR has been updated with docs issue addressed.

aluzzardi · 2016-11-08T22:42:12Z

Code LGTM

Need reference docs update: missing service update CLI docs and API docs

stevvooe · 2016-11-08T22:49:16Z

After some thought, the new hosts API fields should match the man page for /etc/hosts. The syntax for each host entry is the following:

IP_address canonical_hostname [aliases...]

The current format does not correctly represent the concept of aliases, which means we are losing information in transmission. I think we can match this syntax is the swarmkit/services API by insisting on the above syntax.

The approach of keying by hostname should still work here, but ordering control is going to be hard with taking a hosts file as input.

yongtang · 2016-11-09T01:45:39Z

@stevvooe Do we also want to change the cli input? For example, we might use:

docker service create --host "1.2.3.4 myhost" --host "1.1.1.1 example.com example.org"
docker service update --host-rm "example.com" --host-add "5.6.7.8 myhost2"

(Currently it is `docker run --add-host "myhost:1.2.3.4")

The advantage is that it is possible for docker service create --host to add full record into /etc/hosts (it was not possible for docker run --add-host to take alias).

The drawback is that it is not consistent with docker run --add-host.

vieux · 2016-11-09T10:29:46Z

@yongtang IMO changing the CLI is a good idea. It might not be consistant with docker run but it add more functionalities and we can improve docker run later

vdemeester · 2016-11-09T11:10:05Z

Agreeing with @vieux and @stevvooe, I'm ok to not have the same as docker run for service. I wonder about --host-rm though : for 1.1.1.1 host alias1 alias2, should we support --host-rm 1.1.1.1, --host-rm host or both ? (or even with aliases)

thaJeztah · 2016-11-09T13:52:35Z

I'm not so sure we should change. Changing this to match the format IMO is leaking the underlying implementation, instead of the intent of the user. Now the user has to read the Linux man page for learning the format that /etc/hosts uses (is the implementation on Windows the same?)

What if we want to change to a different implementation in future (e.g. add entries to the embedded DNS, instead of updating /etc/hosts?)

The current format does not correctly represent the concept of aliases, which means we are losing information in transmission.

@stevvooe is there a difference between

1.1.1.1 one two

and

1.1.1.1 one
1.1.1.1 two

Both seem to do the same

justincormack · 2016-11-09T14:33:59Z

Why does docker need to modify the hosts file in swarm mode? Can't we return it to the user to control and stop bind mounting it at all? Docker needs it for --link but it is very problematic, eg racy updates and so on. It is only because we provide it that we need a means to update it.

stevvooe · 2016-11-09T19:48:18Z

@yongtang Do not change the CLI input. I am only advocating that the API adopt the model that represents the problem.

Again, the main problem with the docker API today is that we made it reflect too much of CLI behavior. While they should complement each other, it is more important that the API reflect a realistic data model. The CLI can then bolster that data model with sugar!

stevvooe · 2016-11-10T02:33:27Z

@yongtang ~~Make sure this change is reflected in swarmkit API and services REST API.~~

Looking good!

stevvooe · 2016-11-10T02:33:47Z

api/types/swarm/container.go

stevvooe · 2016-11-10T02:35:08Z

daemon/cluster/executor/container/container.go

This needs to use strings.Fields so it splits on arbitrary whitespace.

stevvooe · 2016-11-10T02:45:19Z

LGTM after strings.Fields fix.

yongtang · 2016-11-10T03:05:13Z

Thanks @stevvooe for the review. The PR has been updated.

This fix is a followup for `Hosts` field in `ContainerSpec`, from the comment in: moby#1729 (review) and from docker PR: moby/moby#28031 In the docker PR, it has been specified that `Hosts` in `ContainerSpec` should only follow the standard format: ``` IP_address canonical_hostname [aliases...] ``` The docker PR 28031 already takes the above into consideration so no implementation updates needed. This fix corrects the comment in the specification of `Hosts`. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

…service update` This fix tries to address 27902 by adding a flag `--host` to `docker service create` and `--host-add/--host-rm` to `docker service update`, so that it is possible to specify extra `host:ip` settings in `/etc/hosts`. This fix adds `Hosts` in swarmkit's `ContainerSpec` so that it is possible to specify extra hosts during service creation. Related docs has been updated. An integration test has been added. This fix fixes 27902. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

This fix is a followup for `Hosts` field in `ContainerSpec`, from the comment in: moby#1729 (review) and from docker PR: moby/moby#28031 In the docker PR, it has been specified that `Hosts` in `ContainerSpec` should only follow the standard format: ``` IP_address canonical_hostname [aliases...] ``` The docker PR 28031 already takes the above into consideration so no implementation updates needed. This fix corrects the comment in the specification of `Hosts`. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

tonistiigi · 2016-11-10T23:37:43Z

LGTM

Pull request moby#27745 added support for the client to talk to older versions of the daemon. Various flags were added to docker 1.13 that are not compatible with older daemons. This PR adds annotations to those flags, so that they are automatically hidden if the daemon is older than docker 1.13 (API 1.25). Not all new flags affect the API (some are client-side only). The following PR's added new flags to docker 1.13 that affect the API; - moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime` - moby#27800 / moby#25317 added `--group` / `--group-add` / `--group-rm` - moby#27702 added `--network` to `docker build` - moby#25962 added `--attachable` to `docker network create` - moby#27998 added `--compose-file` to `docker stack deploy` - moby#22566 added `--stop-timeout` to `docker run` and `docker create` - moby#26061 added `--init` to `docker run` and `docker create` - moby#26941 added `--init-path` to `docker run` and `docker create` - moby#27958 added `--cpus` on `docker run` / `docker create` - moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create` - moby#27596 added `--force` to `docker service update` - moby#27857 added `--hostname` to `docker service create` - moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update` - moby#28076 added `--tty` on `docker service create` / `docker service update` - moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update` - moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update` Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Add flag `--host` to `service create` and `--host-add/rm` to `service update`

Pull request moby/moby#27745 added support for the client to talk to older versions of the daemon. Various flags were added to docker 1.13 that are not compatible with older daemons. This PR adds annotations to those flags, so that they are automatically hidden if the daemon is older than docker 1.13 (API 1.25). Not all new flags affect the API (some are client-side only). The following PR's added new flags to docker 1.13 that affect the API; - moby/moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime` - moby/moby#27800 / moby/moby#25317 added `--group` / `--group-add` / `--group-rm` - moby/moby#27702 added `--network` to `docker build` - moby/moby#25962 added `--attachable` to `docker network create` - moby/moby#27998 added `--compose-file` to `docker stack deploy` - moby/moby#22566 added `--stop-timeout` to `docker run` and `docker create` - moby/moby#26061 added `--init` to `docker run` and `docker create` - moby/moby#26941 added `--init-path` to `docker run` and `docker create` - moby/moby#27958 added `--cpus` on `docker run` / `docker create` - moby/moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create` - moby/moby#27596 added `--force` to `docker service update` - moby/moby#27857 added `--hostname` to `docker service create` - moby/moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update` - moby/moby#28076 added `--tty` on `docker service create` / `docker service update` - moby/moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update` - moby/moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update` Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db Component: cli

GordonTheTurtle added the status/0-triage label Nov 3, 2016

This was referenced Nov 3, 2016

Add ExtraHosts field to DNS settings (DNSConfig) for /etc/hosts moby/swarmkit#1729

Merged

docker compose "extra_hosts" alternative in docker engine swarm mode #27902

Closed

icecrime added status/1-design-review and removed status/0-triage labels Nov 3, 2016

yongtang force-pushed the 27902-extra-hosts branch 2 times, most recently from 02dc164 to 47b063a Compare November 4, 2016 04:21

icecrime added the area/swarm label Nov 4, 2016

aluzzardi suggested changes Nov 7, 2016

View reviewed changes

yongtang force-pushed the 27902-extra-hosts branch from 47b063a to 1716020 Compare November 8, 2016 01:51

yongtang changed the title ~~Add flag --add-host to docker service create~~ Add flag --host to service create and --host-add/rm to service update Nov 8, 2016

yongtang mentioned this pull request Nov 8, 2016

Revendor swarmkit to 29950a4e9912178cadb72abc5a445e3a857cb2b0 #28144

Merged

yongtang force-pushed the 27902-extra-hosts branch from 1716020 to 5e42d49 Compare November 8, 2016 04:11

vdemeester added this to the 1.13.0 milestone Nov 8, 2016

dnephin reviewed Nov 8, 2016

View reviewed changes

aluzzardi reviewed Nov 8, 2016

View reviewed changes

icecrime added status/2-code-review and removed status/1-design-review labels Nov 9, 2016

stevvooe reviewed Nov 10, 2016

View reviewed changes

api/types/swarm/container.go Outdated

Copy link

Contributor

stevvooe Nov 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

stevvooe reviewed Nov 10, 2016

View reviewed changes

daemon/cluster/executor/container/container.go Outdated

Copy link

Contributor

stevvooe Nov 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to use strings.Fields so it splits on arbitrary whitespace.

yongtang force-pushed the 27902-extra-hosts branch from 7da6695 to 6ad36d0 Compare November 10, 2016 03:04

yongtang mentioned this pull request Nov 10, 2016

Update the comment for Hosts field in ContainerSpec moby/swarmkit#1742

Merged

yongtang force-pushed the 27902-extra-hosts branch from 6ad36d0 to ea9a23c Compare November 10, 2016 18:25

thaJeztah added the rebuild/windowsRS1 label Nov 10, 2016

GordonTheTurtle removed the rebuild/windowsRS1 label Nov 10, 2016

aluzzardi approved these changes Nov 10, 2016

View reviewed changes

tonistiigi added status/4-merge and removed status/2-code-review labels Nov 10, 2016

vieux merged commit bed96ce into moby:master Nov 11, 2016

yongtang deleted the 27902-extra-hosts branch November 11, 2016 01:03

thaJeztah added impact/changelog impact/documentation docs/revisit labels Nov 15, 2016

thaJeztah mentioned this pull request Nov 15, 2016

Docker 1.13 changelog updates #28433

Merged

albers mentioned this pull request Nov 27, 2016

Add bash completion for docker service {create,update} host options #28866

Merged

thaJeztah mentioned this pull request Jan 16, 2017

Add version annotation to various flags added in 1.13 #30186

Merged

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

Merge pull request moby#28031 from yongtang/27902-extra-hosts

32beb80

Add flag `--host` to `service create` and `--host-add/rm` to `service update`

thaJeztah mentioned this pull request Jul 4, 2017

Document TaskTemplate > ContainerSpec > Hosts for /services/create and /services/update docker/docs#1530

Closed

Add flag --host to service create and --host-add/rm to service update #28031

Add flag --host to service create and --host-add/rm to service update #28031

Uh oh!

Conversation

yongtang commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

icecrime commented Nov 3, 2016

Uh oh!

aluzzardi 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!

yongtang commented Nov 8, 2016

Uh oh!

vdemeester commented Nov 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Nov 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aluzzardi commented Nov 8, 2016

Uh oh!

stevvooe commented Nov 8, 2016

Uh oh!

yongtang commented Nov 9, 2016

Uh oh!

vieux commented Nov 9, 2016

Uh oh!

vdemeester commented Nov 9, 2016

Uh oh!

thaJeztah commented Nov 9, 2016

Uh oh!

justincormack commented Nov 9, 2016

Uh oh!

stevvooe commented Nov 9, 2016

Uh oh!

stevvooe commented Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Nov 10, 2016

Uh oh!

yongtang commented Nov 10, 2016

Uh oh!

tonistiigi commented Nov 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Add flag `--host` to `service create` and `--host-add/rm` to `service update` #28031

Add flag `--host` to `service create` and `--host-add/rm` to `service update` #28031

yongtang commented Nov 3, 2016 •

edited

Loading

stevvooe commented Nov 10, 2016 •

edited

Loading