Skip to content

Conversation

@aanm
Copy link

@aanm aanm commented Mar 17, 2015

yml example file

web:
  labels:
   - ram=2g
  build: .
  command: python app.py
  ports:
   - "5000:5000"
  volumes:
   - .:/code
  links:
   - redis
redis:
  labels:
   - storage=ssd
   - ram=4g
   - production
  image: redis

Do not merge yet.
Depends on docker/docker-py#529
Signed-off-by: André Martins martins@noironetworks.com

@thaJeztah
Copy link
Member

Thanks! Wondering; would it make sense to use a dict/hash-map (whatchamacallit in yaml) in stead of an array? I.e;

web:
    labels:
        ram: 2g
        storage: ssd

@aanm
Copy link
Author

aanm commented Mar 17, 2015

@thaJeztah
While coding I wondered that too.
Also, what about implementing labels via command line for example

docker-compose up web --label ram=8g

That would overwrite the settings in the yaml file, described here moby/moby#11187 (An end-to-end example)

@thaJeztah
Copy link
Member

I like that. Wrt hash/array; I'll leave that to the maintainers for now. both will work, but a hash more closely matches the JSON in the Docker API

Also, the docker labels feature supports a --label-file. I wonder how that should be implemented in compose (1 file per service?)

@dnephin
Copy link

dnephin commented Mar 18, 2015

It was mentioned a few times in the labels PR that labels pretty closely resembles environment variables. I suspect that will be true for our implementation here as well.

For environment variables both lists and dicts are supported in the config, env_file: <path> to point at a file, and -e as a param to run.

I could see labels working the same way: label_file: ... in the config, --label as a param to docker-compose run (not docker-compose up)

Copy link

Choose a reason for hiding this comment

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

We should not be namespacing labels here, it should be left up to the developer.

We will likely use a namespace for our internal labels in #1066, but any user specified labels should be unmodified I think.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed; labels set by the user should be sent to docker as-is.

Copy link

Choose a reason for hiding this comment

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

A motivation for a separate prefix for labels specified in the composition definition I can see is to be able to differ between the labels specified in the composition and the labels specified with docker-compose run --label.

This implements separation of concern between labels specified by the developer and whoever is running the composition in a particular environment. Otherwise docker-compose run invocations are required to always specify and overwrite all possible labels used in an environment if the guy running the workloads wants to avoid a developer mistakenly specifying a label used for operational concerns.

Simple example:
You are using a label profile=production which influences resource allocation and scheduling. You do not want compositions to inherit these privileges unless the label was specified by the operator invoking docker-compose run.

A richer write-up of this separation of concern can be found here:
moby/moby#11187

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, labels used for scheduling (Swarm) should be namespaced and perhaps not even defined as labels in docker-compose.yml (even though they will be set as labels), e.g.

web:
    constraints:
        storage: ssd
        memory: 2g
    labels:
        hello: world

the "constraints" will then result in a --label com.docker.swarm.constraint=storage=ssd (or similar)

I think Swarm is planning on namespacing their labels as well, but I'll have to check.

Copy link

Choose a reason for hiding this comment

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

Agreed that the constraints even though stored as labels should not all share the same namespace. It must be known what is a constraint, what is an identification label, what is a capability label, etc.

An operator should be able to say at docker-compose run time to ignore all previous constraints and use a new set. This should not require the operator to overwrite each constraint separately. Allowing for this separation of concern is absolutely critical. It must be possible to tie applications to infrastructure without storing this glue in the application itself.

So instead of separating compose and runtime labels with a namespace, an alternative could be a --remove-labels option which allows for wildcards:

web:
    labels:
         constraints:
            storage: ssd
            memory: 2
         generic:
            hello: world

To overwrite all constraints:

[...] --remove-labels 'constraints.*' --label constraints.memory=4g

@thaJeztah
Copy link
Member

For environment variables both lists and dicts are supported in the config

True. The reason I thought a dict would be more appropriate because (contrary to env-variables), the storage for "labels" in Docker also is a dict. Label keys (names) are unique and a dict more verbosely expresses that.

An array should work, but would (probably) require more handling, because latter values for the same label should overwrite former ones.

I could see labels working the same way: label_file: ...

Funny fact; Docker actually supports multiple env-files. The contents of those files are combined / latter values overwrite former values here as well.

I was wondering if it should be possible to specify a --label-file on the command line. Problem is that it's not possible to specify which service the file should be applied to (the same obviously is true for --label); will it be applied to all services? Is that the desired result? (not sure)

@aanm
Copy link
Author

aanm commented Mar 18, 2015

Thanks for the feedback guys.
@thaJeztah
Changing to a dict simplified the code, thanks.
Regarding to specifying labels via command line I was thinking something like:

run [options] [--label serv:KEY[=VAL]...] [-e KEY=VAL...] SERVICE [COMMAND] [ARGS...]
   --label serv:KEY[=VAL]   Specifies a label for a given SERVICE. If
                                        no SERVICE is given the label will be used
                                        on every service.
   --no-prefix-label     Don't prefix 'cfg' and 'cmd' when --label is used.
   --label-file               Configuration yml file with all the labels.

The problem I have is, for example, with this command:

docker-compose run --label web:myEnv=draft redis:myEnv=complete

Now, is the redis:myEnv=complete a SERVICE or an argument from label?
Accordingly with the docs a service name is only composed by [a-zA-Z0-9]. So, I'll assume that everything before the : is the SERVICE. Thus, it will not be possible to give a single label via command line for every service started.

I can also create an option to read from a .yml that has the labels predefined by the operator.
labels.yml

web:
    constraints:
        storage: ssd
        memory: 2g
    labels:
        hello: world

That would result in something like:

        "Labels": {
            "io.docker.compose.cmdline:memory": "2g" ->  The one inside labels.yml from the OP
            "io.docker.compose.cfgfile:memory": "99g" -> The one inside compose.yml from the dev
        },

What are your thoughts on this?
@dnephin
You said earlier "--label as a param to docker-compose run (not docker-compose up)" Why can't be both?

@tgraf
Copy link

tgraf commented Mar 18, 2015

@aanm
The --label option could simply support a single path argument to read labels from a .yml instead. This does not fully resolve the developer vs. operator labels though without a way to remove or replace whole label namespaces. An additional --remove-labels might be an easy to understand API for the operator:

  • No option provided
    • Labels are inherited 1:1, no special operator concerns.
  • --remove-labels 'constraints.*'
    • Remove all constraints but keep other labels, operator only wants to provide own constraints
  • --remove-labels
    • Remove all labels, I'm not trusting the labels specified at all.

I'm sure @aanand has his own thoughts on this ;-)

Signed-off-by: André Martins <martins@noironetworks.com>
@aanm
Copy link
Author

aanm commented Mar 19, 2015

$ sudo docker-compose help up
    ...
    --labels-file FILE          Uses all labels, in the given yaml FILE for the
                                SERVICEs to be up.
    --no-prefix-labels          Don't prefix '.cfg' and '.cmd' when --labels-file
                                is used.
    --remove-labels [REGEX]     Removes all labels. If REGEX is given, removes
                                labels that match the given REGular EXpression.
                                (Note: This option has less priority than the
                                given --labels-file)
    ...
$ sudo docker-compose help run
    ...
    --labels-file FILE          Uses all labels, in the given yaml FILE for the
                                SERVICE to be run.
    --no-prefix-labels          Don't prefix '.cfg' and '.cmd' when --labels-file
                                is used.
    --remove-labels [REGEX]     Removes all labels. If REGEX is given, removes
                                labels that match the given REGular EXpression.
                                (Note: This option has less priority than the
                                given --labels-file)
    ...
$ cat docker-compose.yml
web:
  labels:
    dev.attrib.color: white
    dev.attrib.shape: square
  build: .
...
redis:
  labels:
    storage: ssd
    ram: 4g
    profile: production
  image: redis

$ cat labels.yml 
web:
  labels:
    color: green
redis:
  labels:
    storage: ssd
    ram: 2g
    profile: production
$ sudo docker-compose up --labels-file labels.yml --remove-labels 'dev.*'
$ sudo docker inspect dockercompose_web_1 | grep Labels -B1 -A6
        "Image": "dockercompose_web",
        "Labels": {
            ".cmd:color": "green"
        },
        "MacAddress": "",
        "Memory": 0,
        "MemorySwap": 0,
        "NetworkDisabled": false,
$ sudo docker inspect dockercompose_redis_1 | grep Labels -B1 -A8
        "Image": "redis:latest",
        "Labels": {
            ".cfg:profile": "production",
            ".cfg:ram": "4g",
            ".cfg:storage": "ssd",
            ".cmd:profile": "production",
            ".cmd:ram": "2g",
            ".cmd:storage": "ssd"
        },
        "MacAddress": "",

Note the absence of dev.attrib in dockercompose_web_1 ;-)
Feel free to add more suggestions.
I'm not running tests on my side but I'm not sure if Jenkins is failing because of me...

@thaJeztah
Copy link
Member

Thanks @aanm! Hm, my thoughts;

docker-compose run --label web:myEnv=draft redis:myEnv=complete
Now, is the redis:myEnv=complete a SERVICE or an argument from label

Not sure if you meant write that, but the example should be --label web:foo.... --label redis:foo.... (i.e. the --label flag must be repeated for each label.

I like the idea, though, of using a service: prefix.

Some other thoughts;

  • I don't think the labels should be automatically prefixed/namespaced (with .cfg / .cmd); Docker doesn't do this, so Compose shouldn't do this either. Adding .cfg and .cmd namespaces is (IMO) overcomplicating things; labels should follow the same order of preference as Docker does (e.g. CLI > YAML)
  • The --label-file should accept a newline-delimited file. I think a YAML format will also be covered by
    the new config format (Next-generation configuration format #846)
  • _If_ a shorthand is implemented, I'd prefer --label-ns=my.name.space over --no-prefix-label

Perhaps more importantly; I'm fine with adding more features later, but please in a follow-up PR. Keep the initial implementation simple and only implement what's supported by Docker itself, i.e.;

  • Being able to specify labels in docker-compose.yml (no automatic namespaces/prefixes)
  • Being able to specify labels via the CLI (--label) (initially, applied to all services)
  • Being able to read in a --label-file (initially, applied to all services)

Those should not require a lot of discussion, which makes it easier to merge.

In a follow-up/separate PR, --label [service:]label[=value] could be implemented. This may require some discussion, wrt compatibility

All features wrt moby/moby#11187 etc., should be in a follow up. Currently, moby/moby#11187 is just a proposal and it doesn't make sense to implement anything before it's even accepted in Docker itself. It's fine to discuss options for implementing it in Compose, but best kept in a separate issue.

However, I'm not a maintainer, just my personal opinion!

@aanm
Copy link
Author

aanm commented Mar 19, 2015

I don't think the labels should be automatically prefixed/namespaced (with .cfg / .cmd); Docker doesn't do this, so Compose shouldn't do this either. Adding .cfg and .cmd namespaces is (IMO) overcomplicating things; labels should follow the same order of preference as Docker does (e.g. CLI > YAML)

Yeah, I agree, it doesn't look "good" with .cfg/.cmd. Since docker uses CLI > YAML I'll do it the same way.

The --label-file should accept a newline-delimited file. I think a YAML format will also be covered by the new config format (#846)

What's a "newline-delimited file"? This -> https://en.wikipedia.org/wiki/Line_Delimited_JSON#Example_Output

If a shorthand is implemented, I'd prefer --label-ns=my.name.space over --no-prefix-label

my.name.space would be applied to all labels?

Perhaps more importantly; I'm fine with adding more features later, but please in a follow-up PR. Keep the initial implementation simple and only implement what's supported by Docker itself, i.e.;

Okay ;-) I will:

  • remove the .cfg/.cmd thing
  • specify labels via the CLI (--label) (initially, applied to all services)
  • make tests for everything
  • create a new PR for the '--remove-labels'? (not sure if I should keep this here or move it to a new PR)

Thanks for the feedback!

@thaJeztah
Copy link
Member

What's a "newline-delimited file"?

The label file uses the same format as --env-file. You can find an example in this section; https://docs.docker.com/reference/commandline/cli/#examples_8

Basically, it's just a single key=value per line.

create a new PR for the '--remove-labels'?

I would personally move it to a new PR, yes, so that it can be discussed without upholding this PR.

But again, I'm not a maintainer; they could have a different opinion here :)

@gourao
Copy link

gourao commented Mar 19, 2015

Who consumes (or acts) upon these labels? From what I understand so far, from this PR (and also from moby/moby#9882), I see a way for opaque labels to be passed from the compose yaml spec or from the dockerfile all the way to the docker daemon. But since these are opaque, docker itself is not acting on the labels other than preserving them.

So my question is, who are these labels intended for and how do we go about acting on them?

My interest for example would be around getting the storage specific labels and making the storage implementation honor the requested labels.

@thaJeztah
Copy link
Member

At this moment, labels are only that; a label. No software "acts" on them. Docker enables filtering images/containers based on labels and you are able to read the labels using docker inspect.

It is possible to have software make use of those labels (e.g. Composer itself, to store the project name, or Swarm to schedule containers), but for now that should be "out of scope" for this PR.

@aanand
Copy link

aanand commented Mar 19, 2015

Thanks for making a start on this. I think this PR does much more than the minimum necessary for a useful labels feature:

  • As @thaJeztah said, I don't think we ought to be pre/postfixing label names.
  • I think labels_file can be implemented separately.
  • Any command-line flags are a nicety - labels in docker-compose.yml is the real value-add.
  • remove_labels is a confusing feature. I'm not convinced it's useful.

Furthermore, it puts a lot of logic in main.py and service.py which should really be in config.py.

I've made a start on an MVP labels feature in #1139. I think the next good thing to implement would be labels_file, if you want to take a crack at that - have a look at the implementation of env_file.

@aanand aanand closed this Mar 19, 2015
@aanm aanm deleted the label-support branch April 21, 2015 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants