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

@dchen1107 fyi

this looks totally reasonable to me. Two things need fixing:

separate --node-labels and --node-labels-file

and

think about using an existing file format rather than inventing your own.

@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

LGTM. thanks for the PR!

@k8s-bot ok to test

pr builder appears to be missing, activating due to 'lgtm' label.

Continuous integration appears to have missed, closing and re-opening to trigger it

GCE e2e test build/test passed for commit c2526c9.

• Build Log
• Test Artifacts
• Internal Jenkins Results

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.

• Build Log
• Test Artifacts
• Internal Jenkins Results

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

GCE e2e test build/test passed for commit c2526c9.

• Build Log
• Test Artifacts
• Internal Jenkins Results

@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
The bot stuck because something like lack of quota on github.
Log from merge-bot:

E1124 13:00:30.925418       1 github.go:63] Ran out of github API tokens. Sleeping for 46.884576439233335 minutes
E1124 13:02:11.925046       1 github.go:63] Ran out of github API tokens. Sleeping for 45.201249328816665 minutes

GCE e2e test build/test passed for commit c2526c9.

• Build Log
• Test Artifacts
• Internal Jenkins Results

Automatic merge from submit-queue

@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?
cc @dchen1107 @pwittrock @mikedanese @madhusudancs

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 think that a localhost file should solve most of the use cases. Why do we need a command line option as well?

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.

@mml: #18394 is the reason why I'm weary of adding multiple modes for setting labels.

@vishh ... i'll raise another PR referencing this, drop the command line option and retain the local file only ...

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 -
#18439 (comment)