New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support node label update #24397

Merged
merged 1 commit into from Jul 19, 2016

Conversation

Projects
None yet
10 participants
@dongluochen
Contributor

dongluochen commented Jul 7, 2016

- What I did

Add CLI to update node labels.

- How to verify it

Use the following commands to validate the change.

docker node update --label-add key1=value1 --label-add key2=value2 mynode
docker node inspect mynode
docker node update --label-rm key1 --label-rm key2 mynode
docker node inspect mynode

- Description for the changelog
Support node label update

cc @diogomonica @aaronlehmann @aluzzardi .

closes #24645

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jul 7, 2016

Contributor

Code LGTM, but docs need to be updated.

Contributor

aaronlehmann commented Jul 7, 2016

Code LGTM, but docs need to be updated.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 7, 2016

Member

+1 on design

Member

thaJeztah commented Jul 7, 2016

+1 on design

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 7, 2016

Member

guess this is for 1.12?

Member

thaJeztah commented Jul 7, 2016

guess this is for 1.12?

@thaJeztah thaJeztah added this to the 1.12.0 milestone Jul 7, 2016

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jul 7, 2016

Member

@dongluochen @aaronlehmann @thaJeztah I think we should probably think of engine label too when update node labels. With this PR, we are going to introduce some inconsistency between the node label and the engine label (for the same node).

How swarm mode should play with the daemon configuration on labels (not even talking about --label flag on dockerd but this is definitely gonna cause some trouble), and the reloading of restarting of the daemon ? How node labels update should deal with daemon configuration of the nodes ? How do we deal with inconsistencies between the label the node has in the cluster and the one the engine has when looked at alone ?

/cc @stevvooe @icecrime @aluzzardi @vieux

(Puting back into desgin-review because I think this needs a little bit more discussion 👼 — Also not sure if this should be 1.12.0 or 1.13.0, but I would tend to 1.13.0 😝)

Member

vdemeester commented Jul 7, 2016

@dongluochen @aaronlehmann @thaJeztah I think we should probably think of engine label too when update node labels. With this PR, we are going to introduce some inconsistency between the node label and the engine label (for the same node).

How swarm mode should play with the daemon configuration on labels (not even talking about --label flag on dockerd but this is definitely gonna cause some trouble), and the reloading of restarting of the daemon ? How node labels update should deal with daemon configuration of the nodes ? How do we deal with inconsistencies between the label the node has in the cluster and the one the engine has when looked at alone ?

/cc @stevvooe @icecrime @aluzzardi @vieux

(Puting back into desgin-review because I think this needs a little bit more discussion 👼 — Also not sure if this should be 1.12.0 or 1.13.0, but I would tend to 1.13.0 😝)

@diogomonica

This comment has been minimized.

Show comment
Hide comment
@diogomonica

diogomonica Jul 7, 2016

Contributor

@vdemeester I agree that this might be confusing, but the objective is to have two distinct types of labels: swarm labels and engine labels.

Engine labels can't be trusted for security sensitive scheduling decisions, since any worker can report any label up to a manager. They are, however, useful for things like "Does this host have an SSD?", or "how much ram/disk space does this host have?".

These swarm labels, on the other hand, are always trustworthy, since they are set by an administrator explicitly. This means that we can label a host as pci-dss, and create a constraint to ensure that credit-card-app only gets scheduled on worker nodes that are labeled as pci-dss, and that no malicious worker can self-label itself as pci-dss.

I hope this makes sense.

Contributor

diogomonica commented Jul 7, 2016

@vdemeester I agree that this might be confusing, but the objective is to have two distinct types of labels: swarm labels and engine labels.

Engine labels can't be trusted for security sensitive scheduling decisions, since any worker can report any label up to a manager. They are, however, useful for things like "Does this host have an SSD?", or "how much ram/disk space does this host have?".

These swarm labels, on the other hand, are always trustworthy, since they are set by an administrator explicitly. This means that we can label a host as pci-dss, and create a constraint to ensure that credit-card-app only gets scheduled on worker nodes that are labeled as pci-dss, and that no malicious worker can self-label itself as pci-dss.

I hope this makes sense.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 7, 2016

Member

Alright, so they're completely independent; should we deprecate the old labels, or only add a note to the docs?

Member

thaJeztah commented Jul 7, 2016

Alright, so they're completely independent; should we deprecate the old labels, or only add a note to the docs?

@diogomonica

This comment has been minimized.

Show comment
Hide comment
@diogomonica

diogomonica Jul 7, 2016

Contributor

I think they serve different purposes. it will always be useful to have a "self-reported" label coming from an engine. So I don't think we should deprecate engine labels.

Imagine that a host gets upgraded, and an SSD gets added. It is perfectly fine (and in fact, desired), for the host to update it's own label to include "I have an SSD". This is the function of the engine label.

However, for any label that never needs/shoudn't be updated by the node itself, swarm labels are the way to go.

Contributor

diogomonica commented Jul 7, 2016

I think they serve different purposes. it will always be useful to have a "self-reported" label coming from an engine. So I don't think we should deprecate engine labels.

Imagine that a host gets upgraded, and an SSD gets added. It is perfectly fine (and in fact, desired), for the host to update it's own label to include "I have an SSD". This is the function of the engine label.

However, for any label that never needs/shoudn't be updated by the node itself, swarm labels are the way to go.

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jul 7, 2016

Member

@diogomonica This does make sence, thanks 👍

Member

vdemeester commented Jul 7, 2016

@diogomonica This does make sence, thanks 👍

@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Jul 7, 2016

Contributor

Following is doc update on constraints from swarmkit README. @thaJeztah Do you know which doc in docker/docker I should update?

Constraints: Operators can limit the set of nodes where a task can be scheduled by defining constraint expressions. Multiple constraints find nodes that satisfy every expression, i.e., an AND match. Constraints can match node attributes in the following table. Note that engine.labels are collected from Docker Engine with information like operating system, drivers, etc. node.labels are added by cluster administrators for operational purpose. For example, some nodes have security compliant labels to run tasks with compliant requirements.

node attribute matches example
node.id node's ID node.id == 2ivku8v2gvtg4
node.hostname node's hostname node.hostname != node-2
node.role node's manager or worker role node.role == manager
node.labels node's labels added by cluster admins node.labels.security == high
engine.labels Docker Engine's labels engine.labels.operatingsystem == ubuntu 14.04
Contributor

dongluochen commented Jul 7, 2016

Following is doc update on constraints from swarmkit README. @thaJeztah Do you know which doc in docker/docker I should update?

Constraints: Operators can limit the set of nodes where a task can be scheduled by defining constraint expressions. Multiple constraints find nodes that satisfy every expression, i.e., an AND match. Constraints can match node attributes in the following table. Note that engine.labels are collected from Docker Engine with information like operating system, drivers, etc. node.labels are added by cluster administrators for operational purpose. For example, some nodes have security compliant labels to run tasks with compliant requirements.

node attribute matches example
node.id node's ID node.id == 2ivku8v2gvtg4
node.hostname node's hostname node.hostname != node-2
node.role node's manager or worker role node.role == manager
node.labels node's labels added by cluster admins node.labels.security == high
engine.labels Docker Engine's labels engine.labels.operatingsystem == ubuntu 14.04
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 7, 2016

Member

@dongluochen I don't think it's properly documented yet; see #24104

/cc @sfsmithcha

Member

thaJeztah commented Jul 7, 2016

@dongluochen I don't think it's properly documented yet; see #24104

/cc @sfsmithcha

@sfsmithcha

This comment has been minimized.

Show comment
Hide comment
@sfsmithcha

sfsmithcha Jul 8, 2016

Contributor

@dongluochen I added your information above to the service_create.md See the following PR.

#24432

Contributor

sfsmithcha commented Jul 8, 2016

@dongluochen I added your information above to the service_create.md See the following PR.

#24432

@sfsmithcha

This comment has been minimized.

Show comment
Hide comment
@sfsmithcha

sfsmithcha Jul 11, 2016

Contributor

@dongluochen , added information about node update. See #24507.

Contributor

sfsmithcha commented Jul 11, 2016

@dongluochen , added information about node update. See #24507.

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jul 11, 2016

Member

Oh never actually did it…
Design LGTM, and given @thaJeztah @aaronlehmann so putting this in code-review 👼

Member

vdemeester commented Jul 11, 2016

Oh never actually did it…
Design LGTM, and given @thaJeztah @aaronlehmann so putting this in code-review 👼

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jul 11, 2016

Member

Flags are consistent with #23773 😉
Code LGTM too 🐸

Member

vdemeester commented Jul 11, 2016

Flags are consistent with #23773 😉
Code LGTM too 🐸

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 11, 2016

Member

I'm moving this back to design review, for #24397 (comment)

Actually overlooked this in the top description, or thought we did support this on other flags, but we don't (sorry)

Member

thaJeztah commented Jul 11, 2016

I'm moving this back to design review, for #24397 (comment)

Actually overlooked this in the top description, or thought we did support this on other flags, but we don't (sorry)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 12, 2016

Member

ping @dongluochen PTAL ^^

Member

thaJeztah commented Jul 12, 2016

ping @dongluochen PTAL ^^

@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Jul 12, 2016

Contributor

Thanks @thaJeztah. I have a question about the format. I tried to follow #23773. But --label-add and --label-rm handling are different there.

Contributor

dongluochen commented Jul 12, 2016

Thanks @thaJeztah. I have a question about the format. I tried to follow #23773. But --label-add and --label-rm handling are different there.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 12, 2016

Member

@dongluochen ah, yes, I saw your comment; I think we should not support it there as well (#23773 (comment)). It's weird to add a new syntax/format on some flags, so better not have it (and possibly add it to all flags in a later stage if we really want it)

Member

thaJeztah commented Jul 12, 2016

@dongluochen ah, yes, I saw your comment; I think we should not support it there as well (#23773 (comment)). It's weird to add a new syntax/format on some flags, so better not have it (and possibly add it to all flags in a later stage if we really want it)

@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Jul 13, 2016

Contributor

Change label-add/label-rm to update one label. PTAL.

docker node update --label-add key1=value1 --label-add key2=value2 mynode
docker node update --label-rm key1 --label-rm key2 mynode
Contributor

dongluochen commented Jul 13, 2016

Change label-add/label-rm to update one label. PTAL.

docker node update --label-add key1=value1 --label-add key2=value2 mynode
docker node update --label-rm key1 --label-rm key2 mynode
@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jul 13, 2016

Member

@thaJeztah putting it back to code-review then 😉
@dongluochen This will need some documentation updates (in docs/references/commandline/node_update.mdat least)

Member

vdemeester commented Jul 13, 2016

@thaJeztah putting it back to code-review then 😉
@dongluochen This will need some documentation updates (in docs/references/commandline/node_update.mdat least)

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jul 13, 2016

Member

@dongluochen needs a rebase too 😓

Member

vdemeester commented Jul 13, 2016

@dongluochen needs a rebase too 😓

@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Jul 13, 2016

Contributor

Update doc. Rebased. Thanks @vdemeester!

Contributor

dongluochen commented Jul 13, 2016

Update doc. Rebased. Thanks @vdemeester!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 14, 2016

Member

Thanks! Found a couple of issues. (I'll check on the service update command as well if that has the same issues)

1. Labels don't accept "no" value

This is inconsistent with other labels; for containers, the =value is optional, and defaults to an empty string;
https://github.com/docker/docker/blob/v1.12.0-rc4/docs/reference/commandline/run.md#set-metadata-on-container--l---label---label-file

The my-label key doesn't specify a value so the label defaults to an empty string("").

$ docker node update --label-add foo=bar --label-add blah 9a72qojtdvjvo59nkrloz1pl8
invalid argument "blah" for --label-add: bad attribute format: blah
See 'docker node update --help'.

Whereas this works;

$ docker run --label foo --name hello hello-world

$ docker inspect --format '{{json .Config.Labels }}' hello
{"foo":""}

2. "label-rm" accepts "key=value"

The --label-rm flag accepts key=value, and silently ignores the flag;

$ docker node update --label-rm foo=bar 9a72qojtdvjvo59nkrloz1pl8
9a72qojtdvjvo59nkrloz1pl8

$ docker node inspect --format '{{ json .Spec.Labels}}' 9a72qojtdvjvo59nkrloz1pl8
{"foo":"bar"}

Using the "wrong" key/value:

$ docker node update --label-rm foo=hahaha 9a72qojtdvjvo59nkrloz1pl8
9a72qojtdvjvo59nkrloz1pl8

$ docker node inspect --format '{{ json .Spec.Labels}}' 9a72qojtdvjvo59nkrloz1pl8
{"foo":"bar"}

Using the "right" key/value:

```bash
$ docker node update --label-rm foo=bar 9a72qojtdvjvo59nkrloz1pl8
9a72qojtdvjvo59nkrloz1pl8

$ docker node inspect --format '{{ json .Spec.Labels}}' 9a72qojtdvjvo59nkrloz1pl8
{"foo":"bar"}

None of the above remove the label. We should either produce an error if --label-rm is a key/value pair, or "split" the key (but producing an error sounds like the better option here).

3. Nodes without labels return null, not {}

Perhaps for a separate PR, but if no labels are set, Spec.Labels appears to be null, not an empty map;

docker node inspect --format '{{ json .Spec.Labels}}' 9a72qojtdvjvo59nkrloz1pl8
null

But for containers, it's always a map/object;

$ docker run --name nolabels busybox sh
$ docker inspect --format '{{json .Config.Labels }}' nolabels
{}

I think we should fix that. I'll open a separate issue

Member

thaJeztah commented Jul 14, 2016

Thanks! Found a couple of issues. (I'll check on the service update command as well if that has the same issues)

1. Labels don't accept "no" value

This is inconsistent with other labels; for containers, the =value is optional, and defaults to an empty string;
https://github.com/docker/docker/blob/v1.12.0-rc4/docs/reference/commandline/run.md#set-metadata-on-container--l---label---label-file

The my-label key doesn't specify a value so the label defaults to an empty string("").

$ docker node update --label-add foo=bar --label-add blah 9a72qojtdvjvo59nkrloz1pl8
invalid argument "blah" for --label-add: bad attribute format: blah
See 'docker node update --help'.

Whereas this works;

$ docker run --label foo --name hello hello-world

$ docker inspect --format '{{json .Config.Labels }}' hello
{"foo":""}

2. "label-rm" accepts "key=value"

The --label-rm flag accepts key=value, and silently ignores the flag;

$ docker node update --label-rm foo=bar 9a72qojtdvjvo59nkrloz1pl8
9a72qojtdvjvo59nkrloz1pl8

$ docker node inspect --format '{{ json .Spec.Labels}}' 9a72qojtdvjvo59nkrloz1pl8
{"foo":"bar"}

Using the "wrong" key/value:

$ docker node update --label-rm foo=hahaha 9a72qojtdvjvo59nkrloz1pl8
9a72qojtdvjvo59nkrloz1pl8

$ docker node inspect --format '{{ json .Spec.Labels}}' 9a72qojtdvjvo59nkrloz1pl8
{"foo":"bar"}

Using the "right" key/value:

```bash
$ docker node update --label-rm foo=bar 9a72qojtdvjvo59nkrloz1pl8
9a72qojtdvjvo59nkrloz1pl8

$ docker node inspect --format '{{ json .Spec.Labels}}' 9a72qojtdvjvo59nkrloz1pl8
{"foo":"bar"}

None of the above remove the label. We should either produce an error if --label-rm is a key/value pair, or "split" the key (but producing an error sounds like the better option here).

3. Nodes without labels return null, not {}

Perhaps for a separate PR, but if no labels are set, Spec.Labels appears to be null, not an empty map;

docker node inspect --format '{{ json .Spec.Labels}}' 9a72qojtdvjvo59nkrloz1pl8
null

But for containers, it's always a map/object;

$ docker run --name nolabels busybox sh
$ docker inspect --format '{{json .Config.Labels }}' nolabels
{}

I think we should fix that. I'll open a separate issue

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 14, 2016

Member

actually; 3. should probably be done in this PR as well, because this adds the "labels" feature (don't think we can add/remove labels on a node before this PR)

Member

thaJeztah commented Jul 14, 2016

actually; 3. should probably be done in this PR as well, because this adds the "labels" feature (don't think we can add/remove labels on a node before this PR)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 14, 2016

Member

Opened an issue for service, which has the same issue for empty labels; #24631

Member

thaJeztah commented Jul 14, 2016

Opened an issue for service, which has the same issue for empty labels; #24631

@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Jul 15, 2016

Contributor

Thanks @thaJeztah for checking!

1 Labels don't accept "no" value

There are 2 problems here. From what I see engine labels require opts.ValidateLabel, i.e., foo=bar format. So we are not consistent already.

dchen@vm2:~/go/src/github.com/docker/docker$ grep -irnw . -e "ValidateLabel" --include=\*.go
./daemon/config.go:160: cmd.Var(opts.NewNamedListOptsRef("labels", &config.Labels, opts.ValidateLabel), []string{"-label"}, usageFn("Set key=value labels to the daemon"))
./daemon/config.go:414:     if _, err := opts.ValidateLabel(label); err != nil {

The second problem is the meaning of a label with key but no value. Node labels are used by constraints (only for now). A label without value seems to indicate just the existence of a label. But current constraint doesn't support exist check.

2 "label-rm" accepts "key=value"

It takes the whole string "key=value" as key. It silently discards this label-rm because there is no such key. Does Docker support label remove in form of "key=value"? I prefer not to support it.

3 Nodes without labels return null, not {}

I think this should be fixed at node join. An issue is opened at docker/swarmkit#1160.

Contributor

dongluochen commented Jul 15, 2016

Thanks @thaJeztah for checking!

1 Labels don't accept "no" value

There are 2 problems here. From what I see engine labels require opts.ValidateLabel, i.e., foo=bar format. So we are not consistent already.

dchen@vm2:~/go/src/github.com/docker/docker$ grep -irnw . -e "ValidateLabel" --include=\*.go
./daemon/config.go:160: cmd.Var(opts.NewNamedListOptsRef("labels", &config.Labels, opts.ValidateLabel), []string{"-label"}, usageFn("Set key=value labels to the daemon"))
./daemon/config.go:414:     if _, err := opts.ValidateLabel(label); err != nil {

The second problem is the meaning of a label with key but no value. Node labels are used by constraints (only for now). A label without value seems to indicate just the existence of a label. But current constraint doesn't support exist check.

2 "label-rm" accepts "key=value"

It takes the whole string "key=value" as key. It silently discards this label-rm because there is no such key. Does Docker support label remove in form of "key=value"? I prefer not to support it.

3 Nodes without labels return null, not {}

I think this should be fixed at node join. An issue is opened at docker/swarmkit#1160.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 15, 2016

Member

From what I see engine labels require opts.ValidateLabel, i.e., foo=bar format. So we are not consistent already.

The engine labels are the odd one out; all other labels (container, image, volume, network) support --label foo (without explicit value set), which defaults to "" (empty string).

For that reason, I think we should support it, even if there's no use for it currently for constraints (people may have other uses for the labels)

Does Docker support label remove in form of "key=value"? I prefer not to support it.

No, I don't think we should support it, since a label key cannot contain a =, I think it should produce an error.

I think this should be fixed at node join

Not sure; if I do

docker node --label-add foo=bar <node>
docker node --label-rm foo <node>

I end up with null again, not {}. The point is that "no labels" results in null, and not a default empty {}

Member

thaJeztah commented Jul 15, 2016

From what I see engine labels require opts.ValidateLabel, i.e., foo=bar format. So we are not consistent already.

The engine labels are the odd one out; all other labels (container, image, volume, network) support --label foo (without explicit value set), which defaults to "" (empty string).

For that reason, I think we should support it, even if there's no use for it currently for constraints (people may have other uses for the labels)

Does Docker support label remove in form of "key=value"? I prefer not to support it.

No, I don't think we should support it, since a label key cannot contain a =, I think it should produce an error.

I think this should be fixed at node join

Not sure; if I do

docker node --label-add foo=bar <node>
docker node --label-rm foo <node>

I end up with null again, not {}. The point is that "no labels" results in null, and not a default empty {}

@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Jul 16, 2016

Contributor

Does Docker support label remove in form of "key=value"? I prefer not to support it.

No, I don't think we should support it, since a label key cannot contain a =, I think it should produce an error.

In the case of --label-rm k1 --label-rm k2=v2, should the command fail all of them, or fail only k2=v2? What should happen if a user tries to remove a non-existent label? Do we have example how such cases should be handled?

Contributor

dongluochen commented Jul 16, 2016

Does Docker support label remove in form of "key=value"? I prefer not to support it.

No, I don't think we should support it, since a label key cannot contain a =, I think it should produce an error.

In the case of --label-rm k1 --label-rm k2=v2, should the command fail all of them, or fail only k2=v2? What should happen if a user tries to remove a non-existent label? Do we have example how such cases should be handled?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 16, 2016

Member

If the wrong syntax is used, the entire command should fail (same as it currently fails when doing --add-label=foo).

What should happen if a user tries to remove a non-existent label?

We can make the remove idempotent, after all, Swarm mode allows you to set a desired state; removing something that wasn't there is a no-op, and I don't see why we should produce an error in that case

Do we have example how such cases should be handled?

Hm, not sure, can't think of one right now, but ping me if I need to think harder 👍

Member

thaJeztah commented Jul 16, 2016

If the wrong syntax is used, the entire command should fail (same as it currently fails when doing --add-label=foo).

What should happen if a user tries to remove a non-existent label?

We can make the remove idempotent, after all, Swarm mode allows you to set a desired state; removing something that wasn't there is a no-op, and I don't see why we should produce an error in that case

Do we have example how such cases should be handled?

Hm, not sure, can't think of one right now, but ping me if I need to think harder 👍

role string
membership string
availability string
}
type annotations struct {
name string

This comment has been minimized.

@dnephin

dnephin Jul 18, 2016

Member

I think name is never set?

@dnephin

dnephin Jul 18, 2016

Member

I think name is never set?

This comment has been minimized.

@dongluochen

dongluochen Jul 18, 2016

Contributor

Yes the name is never set. It's not clear how node.annotations.name should be used. So far we are using hostname only.

@dongluochen

dongluochen Jul 18, 2016

Contributor

Yes the name is never set. It's not clear how node.annotations.name should be used. So far we are using hostname only.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jul 18, 2016

Member

LGTM

Member

dnephin commented Jul 18, 2016

LGTM

@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Jul 18, 2016

Contributor

removing something that wasn't there is a no-op, and I don't see why we should produce an error in that case

I find it more consistent to report error when --label-rm doesn't remove a label. If label k1 doesn't exist, either --label-rm k1=v1 or --label-rm k1 would return error. PR updated for this.

Contributor

dongluochen commented Jul 18, 2016

removing something that wasn't there is a no-op, and I don't see why we should produce an error in that case

I find it more consistent to report error when --label-rm doesn't remove a label. If label k1 doesn't exist, either --label-rm k1=v1 or --label-rm k1 would return error. PR updated for this.

@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Jul 18, 2016

Contributor

Nodes without labels return null, not {}

Current change doesn't fix this issue. I think it should be fixed in Swarmkit. Will look into it.

Contributor

dongluochen commented Jul 18, 2016

Nodes without labels return null, not {}

Current change doesn't fix this issue. I think it should be fixed in Swarmkit. Will look into it.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 18, 2016

Member

I find it more consistent to report error when --label-rm doesn't remove a label.

my line of thought also was that --label-add adds or updates a label, so --label-rm would remove or (no-op) if not exist. But I'm ok with this.

I just tested the latest version, and LGTM

Member

thaJeztah commented Jul 18, 2016

I find it more consistent to report error when --label-rm doesn't remove a label.

my line of thought also was that --label-add adds or updates a label, so --label-rm would remove or (no-op) if not exist. But I'm ok with this.

I just tested the latest version, and LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 18, 2016

Member

@dongluochen can you squash your commits?

Member

thaJeztah commented Jul 18, 2016

@dongluochen can you squash your commits?

Support node label update.
Signed-off-by: Dong Chen <dongluo.chen@docker.com>
@dongluochen

This comment has been minimized.

Show comment
Hide comment
@dongluochen

dongluochen Jul 19, 2016

Contributor

@thaJeztah Updated, thanks for validating!

Contributor

dongluochen commented Jul 19, 2016

@thaJeztah Updated, thanks for validating!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 19, 2016

Member

thanks!

Member

thaJeztah commented Jul 19, 2016

thanks!

@thaJeztah thaJeztah merged commit dc0d604 into moby:master Jul 19, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 21364 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 8016 has succeeded
Details
janky Jenkins build Docker-PRs 30041 has succeeded
Details
userns Jenkins build Docker-PRs-userns 12099 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 28610 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 604 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment