-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
kubectl: show node label if defined #35901
kubectl: show node label if defined #35901
Conversation
cc @mikedanese for general move to taints, @davidopp for presentation of the 'dedicated' taint Not sure whether we should surface this as a new column. I think it has to show in the default output as this really confuses users (we often get the question "why is the master schedulable"). We've trained users to look for "unschedulable" in the status column as meaning "master", so I think it makes sense to surface it here. Other options:
|
// * a taint with Key 'dedicated' | ||
// If no role is found, ("") is returned | ||
func findNodeRole(node *api.Node) string { | ||
if role := node.Labels["kubernetes.io/role"]; role != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are official, they need to be constants. I can't find mention of this in HEAD, though it looks like someone is using it as a prefix for other annotation in the aws provider only.
if role := node.Labels["kubernetes.io/role"]; role != "" { | ||
return role | ||
} | ||
if role := node.Labels["kubeadm.alpha.kubernetes.io/role"]; role != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point me at docs? I'm only seeing this as having the value "master"
(which should probably also be a constant).
@@ -1476,6 +1476,9 @@ func printNode(node *api.Node, w io.Writer, options PrintOptions) error { | |||
if node.Spec.Unschedulable { | |||
status = append(status, "SchedulingDisabled") | |||
} | |||
if role := findNodeRole(node); role != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if a role is a top level thing, why doesn't it have a column?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was one of the my open questions here. Should I surface it as a column and add that to the default output instead? I do think that would be cleaner if that's OK...
|
||
taints, err := api.GetTaintsFromNodeAnnotations(node.Annotations) | ||
if err != nil { | ||
glog.Warningf("error parsing node taints for node %q: %v", node.Name, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to muddy output on the CLI, particularly since the command will still exit 0. You could change the column content if you wanted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Updated to add a status of InvalidTaints
if taints cannot be parsed.
glog.Warningf("error parsing node taints for node %q: %v", node.Name, err) | ||
} else { | ||
for _, taint := range taints { | ||
if taint.Key == "dedicated" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, if these are going to be magic or recommended values, there ought to be constants in teh code.
If you're going to update a printer, seems like you should update a describer to match. |
Thanks @deads2k! Good call on the docs & need to put these constants into the API. Docs are #35904 and #35211 I'll also work on getting at least the constants into the API (if not the whole I changed the print logic to include "InvalidTaints" if the taints are invalid, and also added a describer output for the role. Can we figure out whether this should be included in the status column, or whether it is better to have a new "role" column? I don't particularly mind either way, but I think it needs to be in the default output if we don't want to be explaining this to users constantly! I'll work concurrently on the docs PRs & getting at least these constants into the api package. |
Jenkins Kubemark GCE e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history. The magic incantation to run this job again is |
Jenkins GCE etcd3 e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history. The magic incantation to run this job again is |
Jenkins GCI GCE e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history. The magic incantation to run this job again is |
Jenkins GKE smoke e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history. The magic incantation to run this job again is |
Jenkins unit/integration failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history. The magic incantation to run this job again is |
Jenkins GCI GKE smoke e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history. The magic incantation to run this job again is |
Jenkins GCE Node e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history. The magic incantation to run this job again is |
Jenkins verification failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history. The magic incantation to run this job again is |
8fa9735
to
a7f9ce6
Compare
The PR with node labels is now LGTMed as #35975, so I rebased this on top of that (the first commit is 35975). I dropped surfacing taints for now, as it sounds like that may have to change. Would love to see this in 1.5, so I don't have to keep explaining where the masters went :-) |
Jenkins GCE e2e failed for commit a7f9ce6. Full PR test history. The magic incantation to run this job again is |
@k8s-bot cvm gce e2e test this |
@deads2k if you have time to look at this before feature-freeze deadline, that would be appreciated :-) If not, can you reassign please? |
|
||
// NodeLabelKubeadmAlphaRole is a label that kubeadm applies to a Node as a hint that it has a particular purpose. | ||
// Use of NodeLabelRole is preferred. | ||
NodeLabelKubeadmAlphaRole = "kubeadm.alpha.kubernetes.io/role" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why didn't kubeadm
use the "real" annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume they didn't want to go through the review process
|
||
// NodeLabelRoleNode is the value of a NodeLabelRole or NodeLabelKubeadmAlphaRole label, indicating a "normal" node, | ||
// as opposed to a RoleMaster node. | ||
NodeLabelRoleNode = "node" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything using this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I know of, but I think kops is likely to do it very soon. I think I've encouraged users to set it themselves, but it should probably be the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I know of, but I think kops is likely to do it very soon. I think I've encouraged users to set it themselves, but it should probably be the default.
They should add it when they start using it. I'm not prepared to "pre-approve" it by creating it as part of this.
|
||
const ( | ||
// NodeLabelRole is the preferred label applied to a Node as a hint that it has a particular purpose (defined by the value). | ||
NodeLabelRole = "kubernetes.io/role" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this the one that's used as a prefix, isn't it? Update the name to match and code to use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so, no. The known uses today are
kubernetes.io/role=master
and kubeadm.alpha.kubernetes.io/role=master
If we start using it as a prefix, we can run that through the API process & then adapt the code to recognize it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so, no. The known uses today are
kubernetes.io/role=master
I can't find this use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are moving towards marking master nodes as tainted, and not necessarily unschedulable. Further now we encourage users to cordon nodes, marking them unschedulable. Thus the reliance on "Unschedulable" is not really a great indicator for the master. So, recognize the existing node 'role' markers, and surface them where Unschedulable is (in the status). We recognize: * a kubernetes.io/role label * a kubeadm.alpha.kubernetes.io/role label Fix kubernetes#33533
a7f9ce6
to
98f7c39
Compare
@deads2k seems the issues were to do with things in the base PR, which has now merged. I think all that's left is what we arrived at originally. OK to LGTM now :-) ? (squeezing it in before feature-freeze!) |
So we now have code which uses the "real" annotation and other, current code, which is still adding an alpha annotation? That just seems buggy. Yes, the printer is fine, but it seems like the API could use some more thought. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
We are moving towards marking master nodes as tainted, and not
necessarily unschedulable. Further now we encourage users to taint
nodes, marking them unschedulable.
Thus the reliance on "Unschedulable" is not really a great indicator for
the master.
Instead, recognize the existing node 'role' markers, and surface them
where Unschedulable is (in the status).
We recognize:
a taint with Key 'dedicated'Fix #33533
This change is![Reviewable](https://camo.githubusercontent.com/2d899f4291d07d3cd2fa4aaae1e3b243f164c23fce87d30a589ace0d496a444c/68747470733a2f2f72657669657761626c652e6b756265726e657465732e696f2f7265766965775f627574746f6e2e737667)