Skip to content
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

[Feature] add ability to set docker labels on worker nodes #163

Merged
merged 3 commits into from
Jan 17, 2020

Conversation

lionelnicolas
Copy link
Contributor

@lionelnicolas lionelnicolas commented Jan 11, 2020

Some service discovery systems are using docker labels to configure DNS, load-balancing, monitoring, metering ... (ie. dalidock , traefik ...).

This PR introduce ability to set arbitrary docker labels for worker nodes using a new --label argument to k3s create and k3d add-node.

@iwilltry42
Copy link
Member

Hi @lionelnicolas , thank you for contributing!
This seems to be a fairly small change compared to the value it adds when combined with services like the two you mentioned 👍
Just one small note: I think that the flag name --label could be misunderstood in a way that people think it can be used to add Kubernetes node labels. Not sure if we should account for this 🤔

@lionelnicolas
Copy link
Contributor Author

@iwilltry42 That's a good point.

I put "docker labels to every node container" in the help message for that reason, but it may not be sufficient.

We could use --docker-label instead (and drop the -l short flag).

My initial thought was that the existing --volume and --env arguments were not including docker, so I did the same for consistency.

Other suggestion, the docker-run manpage shows:

       -l, --label key=value
          Set metadata on the container (for example, --label com.example.key=value).

So maybe we could use --metadata instead of --label ?

Please tell me what you prefer, and I'll make the changes 😉

@iwilltry42
Copy link
Member

@lionelnicolas , nah, let's stick with --label, it makes more sense in the k3d context, you're right there 👍 We won't set Kubernetes node labels anyway 👍

Copy link
Member

@iwilltry42 iwilltry42 left a comment

Choose a reason for hiding this comment

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

I tested it locally and it works perfectly fine for me. Just left some comments that need some clarification :)
Especially it could be cool to have this feature working with our node-selectors, so that you can apply labels to selected nodes and especially also to the master node.

main.go Outdated Show resolved Hide resolved
cli/commands.go Outdated Show resolved Hide resolved
@lionelnicolas lionelnicolas force-pushed the feature/docker-labels-support branch 3 times, most recently from fe429b3 to 080e6c4 Compare January 16, 2020 05:29
@lionelnicolas
Copy link
Contributor Author

@iwilltry42 I've just pushed node-selector support 😉

--label key[=value][@node-specifier]

Node specifier values are the same as --port:

  • server|master: apply docker label on server container
  • workers|agents: apply docker label on worker containers
  • all: apply docker label on all containers (this is the default specifier if none specified)

Note: It's not possible to combine multiple specifiers (ie. --label key=val@node1@node2@node3). To me it's clearer to repeat the argument (--label key=val@node1 --label key=val@node2 --label key=val@node3)

@lionelnicolas lionelnicolas force-pushed the feature/docker-labels-support branch 2 times, most recently from fbaac5b to de0a925 Compare January 16, 2020 05:45
Copy link
Member

@iwilltry42 iwilltry42 left a comment

Choose a reason for hiding this comment

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

LGTM! I tested many different cases any it works perfectly fine for me 👍
Only confusing case could be for someone trying to use multiple node specifiers on a label, where only the last one (correctly) would be interpreted as an actual node-specifier (as you stated in your last comment).
But this can be added to the Docs or FAQ if there is need 👍

Thanks a lot for your contribution :)

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.

2 participants