I like the current design. I think that if the label does not exist it should create it but updating one label should not be allowed to remove others. However adding the option to remove and not have an option to add would be a bit confusing, at least for me.

Some quick thoughts;

Do we support label-file as we do on run ? (it might be too quick to think of that but…)

I think eventually we should support that. I can imagine some system generating the label files, and the user wants to update the container accordingly. Should a label-file automatically replace all existing labels?

Do we allow to add/remove labels or just overriding them all at once ? (I'm definitely for the 1st)

I'm for add/remove labels. I think --label foo=bar should act idempotent, i.e. add the label if it doesn't exist, or update it's value if it's already there.

Removing is more tricky, I'm not sure I like --add-label / --remove-label. Perhaps the --label=-foo to remove the label works for removing.

Do we want to merge the labels client-side or server-side ?

I think we should do that client-side, and only send to the server what it needs to "set". i.e., server-side it's always a replace of all existing labels. Obviously, this could lead to a race condition if the data that the client is using is out of date (e.g. the container has been updated in the mean time). Not sure we should be worrying about that though.

Should we copy Labels to HostConfig at start/creation and update those ones ?

That's what I had in mind, yes. As you described, the "Config" labels are what's in the image, and the "RunConfig" is what is set/overridden at runtime. Just my 0.02c though :-)

Note that moving the labels to RunConfig will probably need changes to docker ps and --filter, so that it acts on the current labels.

Is docker going to emit an event on a (label) update?

@rade the current update command already emit an event so, yeah, it should.

the current update command already emit an event so, yeah, it should.

It does? I was looking at #15078 and couldn't find any changes to events. e.g. https://github.com/docker/docker/blob/master/docs/reference/commandline/events.md hasn't been touched.

@rade it has been added with #18888, update is on the list of events (at the end).

If I restart the machine, is the label still there?

@hacpai that's part of the design review and process to decide if it will or not yeah 😉.

If I restart the machine, is the label still there?

that's part of the design review and process to decide if it will or not yeah 😉.

Surely the answer is the same as for resource configs (the only other setting that update can currently modify). The docs say "You can specify these options on a running or a stopped container. [...] When you run docker update on stopped container, the next time you restart it, the container uses those values.". So perhaps from that we can deduce that the settings survive machine restarts, though the wording could be clearer.

If I restart the machine, is the label still there?

I think it should be persisted, yes; just like we do for updating resources.

Ping @vdemeester @thaJeztah: this seems terribly stalled... What are the next steps?

@icecrime I need to rebase it and then we should first see if UX wise the proposition make sence, i.e. Current design is :

$ docker inspect --format="{{json .Config.Labels}}" d61
{}
$ docker update --add-label=foo=bar --add-label=bar=foo d61
d61
$ docker inspect --format="{{json .Config.Labels}}" d61
{"bar":"foo","foo":"bar"}
$ docker update --remove-label=bar d61
d61
$ docker inspect --format="{{json .Config.Labels}}" d61
{"foo":"bar"}

It is possible to do --remove-label=bar=toto, and it will stil remove the label bar even if the value if different. Don't really know what to think about that.

From there, I'll make a PR in engine-api and things will follow :wink:

Just discussed with @tiborvass - we should decide what happens if you commit the container; should the old labels be used, or the new labels?

Given that there's docker commit --change, which supports changing labels, perhaps we should not commit the runtime labels? idk

@thaJeztah oh good point. On the first though I would also think it should not be commited, as the update command only update runtime configurations.

@vdemeester true, so the "config" labels are copied to "runconfig", and those are the ones we modify, right? And "config" is never changed, so committing will not save those changes.

@thaJeztah not in the current naive implementation, but ideally (and in the end) that would be the case 😉.

Agree with @bboreham and @astrostl. The ability to set metadata on the container allows tools working along Docker to function yet still keep Docker at the center for the "source of truth". As @astrostl said, you could have a separate datastore for this but keeping in sync is not trivial and does not play well with other systems.

I've update the PR according to some decision (on the cli side):

Client side, flags are --label and --label-file as the run command. We could add --add-label and --remove-label client side but I'm not even sure we need that now (agreed with @avsm it can but done outside the client). I also choose to use --label instead of a --set-labels to keep consistency with the run command (and how update commands currently maps the same labels).

API side, it's just a new Labels field with the same struct as Config.Labels (map[string]string).

On the daemon side, I do agree with @cpuguy83, it's confusing to have Labels in multiple places (in Config and HostConfig). The current labels are already runtime labels, that happens to inherit from images. Labels of containers comes from image and the one specified by --label or --label-file.
On the question "should updated labels be commited when issueing commits", I also think they should, for consistency with run if nothing else.

$ docker run -itd --label foo=bar --name ohmylabel busybox top
$ docker commit ohmylabel myimage
$ docker inspect --format "{{ json .Config.Labels }}" myimage
{"foo":"bar"}
$ docker inspect --format "{{ json .Config.Labels }}" myimage
{"foo":"bar"}

I think it would make sense (and consistent) for with an update in the middle to update the container labels, that they are commited.

So for now, it is updating Container.Config directly 👼.

Note this design is probably the most simpler to implement (no changes in other commands, less potential side effects) too.

🐸

Is there a PR for the CLI changes?

Cli, and daemon side 😉 (api changes ar on docker-archive-public/docker.engine-api#129)

On the daemon side, I do agree with @cpuguy83, it's confusing to have Labels in multiple places (in Config and HostConfig). The current labels are already runtime labels, that happens to inherit from images.

Just to be clear, the image labels are in Config and the container run-time labels are under HostConfig?

So one blocker on this PR is the fact that labels are not container-specific and we should take the pain of making sure the UX for updating labels is universal across labelable resources.

Please read my comment about update label UX: #20356 (comment) (which turned out to be out of scope for that issue, but it's relevant for the discussion in this PR)

Until we can figure out the right UX this PR is kind-of blocked :(

@tiborvass yep, I'll close for now I think :-)

Blocked tickets are closed as a matter of practice here?

@astrostl Let say that I'm not keeping my PR open as it has become outdate, given the changes in the code since the last time and given the fact that now labels are available for other objects and thus this might need an overall rethink.

I've opened an issue to discuss it (here : #21721), but no matter what, this PR is stalled and outdated, thus I prefer to close it 😉

@dnephin I can immagine a lot of use cases focused into interoperability between containers.

Probably it can be the foundation to implement features similar to kubernetes replication controllers.

rolling updates behind a load-balancer

Imagine loadbalancer is https://github.com/docker/dockercloud-haproxy but in addition it can dynamically include/exclude containers depending on the presence of a label, say lb=true/false.

Graceful update can be achieved as follows:

1. an update task (or a supervisor container) starts a new container scaling up by 1 with updated image. It will be automatically picked by loadbalancer.
2. if new container fails to start it's killed and the update is stopped. Nothing breaks.
3. if new container is up and lively update task set the old container to lb=false
4. in response to lb=false the old container will be put outside routing by load balancer, without killing it
5. update task will monitor the old container till it has no residual network activity and then kill it

Keep alive a number of containers across all cluster

On scaling a container all the containers of the same service can be marked with the desired replica number.

Ie. running:

docker-compose scale web=2 

may cause all the containers for service web to be tagged with `replicas=2``

A supervisor can then monitor the cluster and ensure that exactly 2 instances of the service are always running.

Thanks.