X Tutup
Skip to content

Fix service update --host-rm not being granular enough#1054

Merged
vieux merged 1 commit intodocker:masterfrom
thaJeztah:fix-host-rm-being-too-greedy
May 21, 2018
Merged

Fix service update --host-rm not being granular enough#1054
vieux merged 1 commit intodocker:masterfrom
thaJeztah:fix-host-rm-being-too-greedy

Conversation

@thaJeztah
Copy link
Member

fixes moby/moby#35325
fixes moby/moby#37035

Removing a host by <host>:<ip> should only remove occurences of the host with
a matching IP-address, instead of removing all entries for that host.

In addition, combining --host-rm and --host-add for the same host should
result in the new host being added.

This patch fixes the way the diff is calculated to allow combining
removing/adding, and to support entries having both a canonical, and aliases.
Aliases cannot be added by the CLI, but are supported in the Service spec, thus
should be taken into account:

Entries can be removed by either a specific <host-name>:<ip-address>
mapping, or by <host> alone:

  • If both IP-address and host-name is provided, only remove the hostname
    from entries that match the given IP-address.
  • If only a host-name is provided, remove the hostname from any entry it
    is part of (either as canonical host-name, or as alias).
  • If, after removing the host-name from an entry, no host-names remain in
    the entry, the entry itself should be removed.

For example, the list of host-entries before processing could look like this:

hosts = &[]string{
    "127.0.0.2 host3 host1 host2 host4",
    "127.0.0.1 host1 host4",
    "127.0.0.3 host1",
    "127.0.0.1 host1",
}

Removing host1 removes every occurrence:

hosts = &[]string{
    "127.0.0.2 host3 host2 host4",
    "127.0.0.1 host4",
}

Whereas removing host1:127.0.0.1 only remove the host if the IP-address matches:

hosts = &[]string{
    "127.0.0.2 host3 host1 host2 host4",
    "127.0.0.1 host4",
    "127.0.0.3 host1",
}

Before this patch:

$ docker service create --name my-service --host foo:127.0.0.1  --host foo:127.0.0.2 --host foo:127.0.0.3 nginx:alpine
$ docker service update --host-rm foo:127.0.0.1 --host-add foo:127.0.0.4 my-service
$ docker service inspect --format '{{.Spec.TaskTemplate.ContainerSpec.Hosts}}' my-service
[]

After this patch is applied:

$ docker service create --name my-service --host foo:127.0.0.1  --host foo:127.0.0.2 --host foo:127.0.0.3 nginx:alpine
$ docker service update --host-rm foo:127.0.0.1 --host-add foo:127.0.0.5 my-service
$ docker service inspect --format '{{.Spec.TaskTemplate.ContainerSpec.Hosts}}' my-service
[127.0.0.2 foo 127.0.0.3 foo 127.0.0.4 foo]

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

- Description for the changelog

* Fix `docker service update --host-add` does not update existing host entry [docker/cli#1054](https://github.com/docker/cli/pull/1054)

Removing a host by `<host>:<ip>` should only remove occurences of the host with
a matching IP-address, instead of removing all entries for that host.

In addition, combining `--host-rm` and `--host-add` for the same host should
result in the new host being added.

This patch fixes the way the diff is calculated to allow combining
removing/adding, and to support entries having both a canonical, and aliases.
Aliases cannot be added by the CLI, but are supported in the Service spec, thus
should be taken into account:

Entries can be removed by either a specific `<host-name>:<ip-address>`
mapping, or by `<host>` alone:

 - If both IP-address and host-name is provided, only remove the hostname
   from entries that match the given IP-address.
 - If only a host-name is provided, remove the hostname from any entry it
   is part of (either as _canonical_ host-name, or as _alias_).
 - If, after removing the host-name from an entry, no host-names remain in
   the entry, the entry itself should be removed.

For example, the list of host-entries before processing could look like this:

    hosts = &[]string{
        "127.0.0.2 host3 host1 host2 host4",
        "127.0.0.1 host1 host4",
        "127.0.0.3 host1",
        "127.0.0.1 host1",
    }

Removing `host1` removes every occurrence:

    hosts = &[]string{
        "127.0.0.2 host3 host2 host4",
        "127.0.0.1 host4",
    }

Whereas removing `host1:127.0.0.1` only remove the host if the IP-address matches:

    hosts = &[]string{
        "127.0.0.2 host3 host1 host2 host4",
        "127.0.0.1 host4",
        "127.0.0.3 host1",
    }

Before this patch:

    $ docker service create --name my-service --host foo:127.0.0.1  --host foo:127.0.0.2 --host foo:127.0.0.3 nginx:alpine
    $ docker service update --host-rm foo:127.0.0.1 --host-add foo:127.0.0.4 my-service
    $ docker service inspect --format '{{.Spec.TaskTemplate.ContainerSpec.Hosts}}' my-service
    []

After this patch is applied:

    $ docker service create --name my-service --host foo:127.0.0.1  --host foo:127.0.0.2 --host foo:127.0.0.3 nginx:alpine
    $ docker service update --host-rm foo:127.0.0.1 --host-add foo:127.0.0.5 my-service
    $ docker service inspect --format '{{.Spec.TaskTemplate.ContainerSpec.Hosts}}' my-service
    [127.0.0.2 foo 127.0.0.3 foo 127.0.0.4 foo]

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the fix-host-rm-being-too-greedy branch from 4c8bc08 to 27c0858 Compare May 11, 2018 14:35
@thaJeztah
Copy link
Member Author

ping @vdemeester @cpuguy83 @silvin-lubecki PTAL

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🦁

Copy link
Contributor

@vieux vieux left a comment

Choose a reason for hiding this comment

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

LGTM

@vieux vieux merged commit 7bdd820 into docker:master May 21, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.06.0 milestone May 21, 2018
@thaJeztah thaJeztah deleted the fix-host-rm-being-too-greedy branch May 21, 2018 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docker service update --host-rm/--host-add: doesn't persist new value 'docker service update --host-add' does not update existing host entry

4 participants

X Tutup