Use labels instead of names to identify containers

[dnephin]

Resolves#1066

Should be compatible with #1269

Also includes:

• a fix for the integration test suite, it was leaving behind one_off containers (the first commit)
• a small refactor of create_container and next_container_number(). I moved the logging into create_container() instead of having it happen in all the callers, and changed it so that find_next_number() only happens once.

[thaJeztah]

Not very familiar with python, but does this currently require to inspect every container, or the containers after filtering by project (and service name)? Trying to determine how important moby/moby#11943 is for this.

[dnephin]

Only the containers after filtering, so it's not so bad

[thaJeztah]

Ah thanks, I was expecting that, but wasn't sure 😄

[aanand]

Fantastic stuff, thank you. Will merge once docker-py label support has landed.

[aanand]

Will this be backwards-compatible with existing Compose projects? i.e. if I've got containers running that I started with the current stable version, then I upgrade and run docker-compose up, will it recreate or will it ignore them and start new ones?

[thaJeztah]

Hm, perhaps interesting thought to add the version of compose that was used to create the containers as a label as well, in case there's some migration needed in future.

[dnephin]

Ah, good point. I think that might be a problem. I suspect it will look for containers using labels, find none, then attempt to create them, running into a 409 conflict error from docker.

I think we'll need to do something to address that.

We could release one version that falls back to trying to lookup by name, or
we could add labels to create_container in one version, and not actually use them to lookup containers until the next version.

Both of those run into the issue where it requires users to upgrade one version at a time though, skipping the intermediate version would still break things.

Another option, which is usually my preference, is to provide a migration script, which would read a docker-compose.yml and recreate the containers adding labels. We could document that the options are either:

1. docker-compose rm using the old version, to get a fresh create
2. use the migration script to add labels to existing containers

@thaJeztah I think a compose version label would be a good idea

[aanand]

Perhaps if Project.containers or Service.containers returns an empty list, we could then check for containers using the old method and prompt the user to upgrade (using a migration script/command) if we find any. Something like:

defcontainers(self, *args, **kwargs):
containers=self._containers_using_labels(*args, **kwargs)
ifnotcontainers:
self._check_for_old_containers()
returncontainersdef_check_for_old_containers(self):
containers=self._containers_using_names()
ifcontainers:
names= [c.nameforcincontainers]
raiseUserError(
""" Found containers that were created using an old version of Compose: {} You have two options: - Migrate them with `docker-compose migrate-containers-1.3` - Remove them with `docker rm -f {}` """.format("\n".join(names), " ".join(names))
)

[aanand]

This would mean checking for containers would still be potentially slow when there aren't any in the current project, but compared to how slow it is now we're only adding a single API call (doing a ps with a label filter), which shouldn't be too costly if it's not going to find anything.

We can remove it in a later version once we're satisfied that most people have upgraded.

[thaJeztah]

@aanand I like that idea. If compose had a configuration file (~/.docker/compose.json), it could be configurable and disabled if people don't need it anymore :)

(Docker itself is working on some changes in the config files / structure)

[ahmetb]

@dnephin liked the idea a lot. Does this PR remove service name entirely from the container name or still keep it around? I guess for readability of 'docker ps' output it might be nice to have them. How do the labels on the containers look like when containers are started from compose?

[aanand]

We keep container names, but they're now "write-only" - we don't look at them, except to figure out what the next numeric suffix should be. (I think eventually we should drop numeric suffixes in favour of something random, so that generating the next one doesn't involve an API call).

Also, once this is merged, we'll be able to implement support for specifying custom container names.

For that reason, it might make sense to revisit the output of docker-compose ps. Since container names are no longer going to be consistently structured, there should probably be a "Service" column:

Service Name Command State Ports
----------------------------------------------------------------------------------------
redis MyCoolContainerName /usr/local/bin/run Up
web composetest_web_a1b2c3 /bin/sh -c python app.py Up 5000->5000/tcp

How do the labels on the containers look like when containers are started from compose?

Something like this:

com.docker.compose.project=composetest
com.docker.compose.service=web
com.docker.compose.container_number=1
com.docker.compose.oneoff=False

[thaJeztah]

except to figure out what the next numeric suffix should be

MAX(com.docker.compose.container_number) + 1 ? Don't think the container-name is required for that?

there should probably be a "Service" column

Or even a "Project" column; compose should now be able to get all containers that are managed by compose (filter by --filter label=com.docker.compose.project should probably work), so that even all services for all projects could be shown and (partly) managed.

[aanand]

Don't think the container-name is required for that?

True - we can just look at the labels - but it's still an extra API call versus generating something random.

Or even a "Project" column

That might be cool: docker-compose ps --all-projects or something.

[thaJeztah]

That might be cool: docker-compose ps --all-projects or something.

Yes! Thinking if we should keep track of the location of the yaml file where the project was started from (the project "home" directory)

[aanand]

Actually, yes, that's a good idea and one I meant to suggest earlier:

com.docker.compose.project.name=composetest
com.docker.compose.project.path=/Users/me/src/composetest

[thaJeztah]

com.docker.compose.project.path=/Users/me/src/composetest

Think that should be the location of the config (yaml), now that all paths are relative to that file (iirc), but yeah, something like that.

On a side-note, there's a breaking change being worked on in Docker currently. I'm personally not really happy about that. Not sure if compose itself is affected directly, but compose users definitely, if they use bind-mounted volumes to mount their source code during development. See:
moby/moby#13025 (comment)

[thaJeztah]

com.docker.compose.project.path=/Users/me/src/composetest

Think that should be the location of the config (yaml), now that all paths are relative to that file (iirc), but yeah, something like that.

On a side-note, there's a breaking change being worked on in Docker currently. I'm personally not really happy about that. Not sure if compose itself is affected directly, but compose users definitely, if they use bind-mounted volumes to mount their source code during development. See:
moby/moby#13025 (comment)

[dnephin]

I'm thinking com.docker.compose.config=/full/path/to/compose.yml. It's possible to specify a different name for the compose file, so pointing at the path isn't really enough information, where as you can always get the root path used by the project by doing dirname <config file>.

[dnephin]

We don't actually have access to the config path from Service right now, so I'm thinking we should wait to add com.docker.compose.config. It will be a larger change to pass the path around through Project, into the Service, and this change is already pretty large as it is. It's also not actually used for anything yet.

I've added the compose version, that was easy enough.

I'm going to focus on migration and backwards compatibility.

[aanand]

👍

[thaJeztah]

this change is already pretty large as it is

Fully agree! one step at a time, no need to mingle multiple features into one.
The suggestions are nice for a future feature being added and don't relate to the Switch to using labels directly, just depend on it.

[dnephin]

Alright, migration test added, and appears to be passing. There was actually a bug in the previous version, it would try to add labels to all projects, instead of just the current one.

[thaJeztah]

but it also seemed a bit weird to make it a full command, since it will be removed eventually.

True, not a big issue I think, as long as the docs mention it's a temporary method. The migration is a nice extra and will only be used once for a project, so "shrugs"

Changes look good to me (but again, I'm not a python coder, so just a general "feel" by glancing over it). I think what's remaining is;

• Updates to the docs (a mention of the new approach, description of the labels, perhaps a how-to for manually creating containers to be included in a project)
• I wonder if we need tests for containers with an incomplete set of labels (thinking of this, if people manually create containers for a project - do we support that?)
• Updates to the shell-completion scripts where needed (can be another PR, and I'm sure @albers is willing to help out if needed :))

Again, great work!

[aanand]

thinking of this, if people manually create containers for a project - do we support that?

Definitely not.

[thaJeztah]

Definitely not.

👍 definitely worth a mention in the docs. People will try, which is fine, but it should be clear that they're on their own in that case.

[aanand]

Is it ever expected/OK for a container not to have a number label? I feel like we should error out if we don't find one, unless I've missed a case. Same for the service label.

[aanand]

@moxiegirl@fredlf - your opinions on docs would be useful here. For context, we're making a change to Compose that means people will need to migrate their containers to a new format - but we're only going to keep it around for one version before dropping support for old containers.

Compose will show a warning if it detects old-style containers, prompt you to run docker-compose migrate_to_labels, and show the URL of a document that explains what's going on.

My question is: where should the document go? We don't plan to keep it around for a long time, so perhaps docs.docker.com isn't the right place and it should just go in a Markdown file on GitHub.

[moxiegirl]

@aanand My recommendations would be:

• include an Upgrade Compose section in the compose release notes with instructions
• add an Upgrade Compose section to the installation docs (http://docs.docker.com/compose/install/) with also a mention

I would recommend the section in the release notes would only exist if we know there can be confusion. So, if no issues exist in the next release, the section goes away. For the Upgrade Compose section in the install --- that would remain a stable touchpoint even if it has a single line to rerun the curl in the next release.

Unfortunately, I have no cycles this release to add the section to install, if you agree and want to add it, I'll have time to review.

[aanand]

@moxiegirl Thanks - I'll do that. I've opened an issue for it: #1425

[dnephin]

Rebased, and added an error to Container.number

[aanand]

LGTM

[thaJeztah]

Great job guys! Thanks

[funkyfuture]

bravo! 👯 💃 👯

(only a matter of gusto atm, but shouldn't anything that is used package-wide like the constants in const.py not rather be defined in __init__.py? so everything is accessible just by an import compose.)