Remove duplicate keys in labels of docker info

[yongtang]

- What I did

This fix tries to address the issue raised in #24392 where labels with duplicate keys exist in docker info, which contradicts with the specifications in the docs.

The reason for duplicate keys is that labels are stored as slice of strings in the format of A=B (and the input/output).

- How I did it

This fix tries to address this issue by checking conflict labels when daemon started, and remove duplicate labels (K-V).

The existing /info API has not been changed.

- How to verify it

An additional integration test has been added to cover the changes in this fix.

- Description for the changelog

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

This fix fixes #24392.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

[vdemeester]

Design LGTM for me
/cc @thaJeztah @tiborvass

[thaJeztah]

After some more thinking; I think we should be careful (at least, don't include this in 1.12); even though (e.g.) Swarm removes duplicates, the API (and CLI) so far have always shown duplicates. We don't know how people use these labels. Even though we document that they should be regarded key/value pairs, it's possible people rely on the current behavior and., e.g. treat cpu=intel, cpu=64-bit as cpu=intel,64-bit in their tooling.

If we want to change this; perhaps we should even consider changing it when setting the labels, so remove duplicates when the labels are set, and not when reading/outputing.

The existing /info API has not been changed.

Well, the returned values are;

before;

  "Labels": [
    "foo=bar",
    "foo="
  ],

After

  "Labels": [
    "foo="
  ],

So, although I like this, I think we need more eyes on this to be sure we can change it (at least we may have to version the API changes to not affect older versions of the API)

[bennetimo]

Even though we document that they should be regarded key/value pairs, it's possible people rely on the current behavior and., e.g. treat cpu=intel, cpu=64-bit as cpu=intel,64-bit in their tooling

@thaJeztah that's a fair point. The reason I stumbled into this was because I was experimenting with exactly that, by having multiple 'nodetype=x', 'nodetype=y' labels on the docker daemon to use for swarm container scheduling. From the docker info output I initially took that this was possible, and it wasn't until playing with swarm and seeing that only the last written was taken that I checked the implementation and the docs.

[thaJeztah]

@bennetimo thanks for adding that! So, sounds like it's a valid concern (even though it wasn't originally intended to be used like that)

[yongtang]

Thanks @thaJeztah @bennetimo @vdemeester for the feedbacks. One way to address this issue without unintended impact on existing users is to throw out an error when labels are set. In this way at least the end user will get notified of the change. Or we could remove duplicate when labels are set. Let me take a look and see if I could update the pull request.

[yongtang]

@thaJeztah @bennetimo @vdemeester I updated the pull request so that the daemon will check for conflict label keys at the startup time.

If there are conflicts (same key with different value), the daemon will return an error. In this way, end user at least will be notified.

If there is no conflict, then the daemon will remove any duplicate (same key with same value), so that the output of info will never show conflict or duplicate labels.

All the changes only happen on daemon side.

Please let me know if there are any issues.

[thaJeztah]

Let's make duplicate keys a warning for now, and mark that as "deprecated", then we can change it to an "error" in three releases

[yongtang]

Thanks @thaJeztah I updated the pull request. Now if duplicate keys exists in labels, then in daemon logs a warning message will show up. In addition, a WARNING will show up in the output of docker info.

Not sure if this is enough for deprecation right now. Let me know if there are anything else that needs to be done.

[vdemeester]

Code LGTM 🐸

[cpuguy83]

20:18:43 
20:18:43 ----------------------------------------------------------------------
20:18:43 FAIL: docker_cli_info_test.go:193: DockerSuite.TestInfoLabels
20:18:43 
20:18:43 docker_cli_info_test.go:196:
20:18:43     d := NewDaemon(c)
20:18:43 daemon.go:57:
20:18:43     c.Assert(dest, check.Not(check.Equals), "", check.Commentf("Please set the DEST environment variable"))
20:18:43 ... obtained string = ""
20:18:43 ... expected string = ""
20:18:43 ... Please set the DEST environment variable
20:18:43 
20:18:43 
20:18:43 ----------------------------------------------------------------------

This should probably go in the daemon suite instead of the main one.

[yongtang]

Thanks @cpuguy83 the pull request has been updated. I also create a new pull request #26154 to convert the other related tests in docker_cli_info_test.go to use DockerDaemonSuite, for consistency reasons. Please take a look and let me know if there are any issues.

[thaJeztah]

@yongtang looks like this needs a rebase now 😢

[yongtang]

Thanks @thaJeztah. The pull request has been updated.

[allencloud]

looks like this needs another rebase now 😢 @yongtang

[yongtang]

Thanks @allencloud. The PR has been rebased.

[thaJeztah]

LGTM