kubelet - node labels command line option #17265
Conversation
gambol99
commented
Nov 14, 2015
gambol99
commented
- adding the -node-label flag to the kubelet which allows for a initial tagging / labeling of the node on cluster registration
- the labels can come from a series of key=pair value or file:///path_to_file which contains key pairs
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
1 similar comment
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
|
Labelling this PR as size/L |
k8s-github-robot
added
the
size/L
gambol99
force-pushed
the
rj/node_labels
branch
2 times, most recently
from
507e7ab
to
ae196e1
Compare
| if len(kl.nodeLabels) > 0 { | ||
| labels, err := kl.retrieveNodeLabels(kl.nodeLabels) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to apply the node labels, %s", err) |
There was a problem hiding this comment.
should probably change this to reference --node-label in the error message as not to cause any confusion
There was a problem hiding this comment.
yeah, please do that.
|
@dchen1107 fyi this looks totally reasonable to me. Two things need fixing: separate and think about using an existing file format rather than inventing your own. |
k8s-github-robot
added
the
needs-rebase
gambol99
force-pushed
the
rj/node_labels
branch
from
ae196e1
to
a41e72d
Compare
…al tagging / labelling of the node on cluster registration - the labels can come from a series of key=pair value or file:///path_to_file which contains key pairs
gambol99
force-pushed
the
rj/node_labels
branch
from
a41e72d
to
c2526c9
Compare
|
@brendandburns .. i've updated the PR separating into two distinct command line options and the node-labels-file can be on either yaml or json format |
k8s-github-robot
removed
the
needs-rebase
brendandburns
added
the
lgtm
|
LGTM. thanks for the PR! |
|
@k8s-bot ok to test |
|
Continuous integration appears to have missed, closing and re-opening to trigger it |
|
GCE e2e test build/test passed for commit c2526c9. |
|
The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label? |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit c2526c9. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit c2526c9. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
@ixdy The bot is stuck in a loop because e2e is taking more than 60 minutes... |
|
@eparis - this is not true |
|
GCE e2e test build/test passed for commit c2526c9. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
|
@wojtek-t seems like you are right, but I thought the bot would avoid this situation. I'll look today. In any case, I can't imagine how the bot would use up all 5000 tokens within 13 minutes. This probably isn't the PR to discuss it though... |
|
@eparis - I don't know what happened, but it seems it did it. Thanks for looking into it! |
|
@gambol99: Can you explain why you introduced two new modes instead of just one? Why not support just file based labels? |
|
We previously had significant discussion on this feature including a design doc AND an implementation (see #13524) in which we decided to put this feature off. There are issues with this implementation that we need to fix before this makes it into a release or we need to revert this. Unfortunately @dchen1107 missed this and it didn't get the proper audience. In the future we should cc @kubernetes/goog-node for changes to the kubelet API. |
|
@vishh "Can you explain why you introduced two new modes instead of just one? Why not support just file based labels?" ... originally it was one option which supported both via -node-label (key=pair|file:///path) which was rejected, rightly so perhaps ... As for why? ... nothing magically, it wasn't a big stretch to support both and it leaves the choice to the user. |
|
@mikedanese is haven't gone through #13524 completely but this is somewhat different, introducing labels/files on host and periodically syncing these labels which would difficult to update and may trip people up. The option of using a pod doesn't sound too appealingly to me, i have to create a token and limit by a auth policy (i "believe" it supported resource 'node', not sure) or use a service account which unless things have changed doesn't yet support token policy, so free for all. Then push a container, write some logic to label and sleep if static, not relabel if restarted, if required etc and have a pod "inside" my cluster start labeling infrastructure outside it, all to add what in 90% of most use case's would be static labels to a node. |
|
@gambol99: Ownership of node labels isn't explicit in k8s as of now. Since we have multiple sources of labels (kube API, kubelet internal plugins, host files, command line flags), it introduces ownership and priority issues. Kubelet, cannot handle labels updates and deletions gracefully. Exposing dynamic labels from kubelet isn't fully supported in k8s as of now. I think that a localhost file should solve most of the use cases. Why do we need a command line option as well? In the future, we want to support only one mechanism for labels and that might be either file based or an exec based plugin model. Unless we can some one fix the node labels handling before the next release, this PR will have to be reverted. |
I can think of a few examples. One is where the label file referenced is constantly overwritten by some outside system or pulled from a read-only filesystem, and for debugging purposes you want to add or modify a label. The flag that lets you specify an arbitrary number of additional labels on the command line makes that easy. |
|
@vishh ... i'll raise another PR referencing this, drop the command line option and retain the local file only ... |
I don't think that solves all the problems with this implementation. For example, if I remove a label from the file (or the command line), will that label ever be removed? |
|
@mml: Take a look at this comment - |
