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.

Sign up for GitHub

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

Jump to bottom

kubectl: show node label if defined #35901

Merged

k8s-github-robot merged 1 commit into kubernetes:master from justinsb:kubectl_show_labels

Nov 10, 2016

Member

justinsb commented Oct 31, 2016 •

edited

Loading

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 kubernetes.io/role label
a kubeadm.alpha.kubernetes.io/role label
~~a taint with Key 'dedicated'~~

This change is

googlebot added the cla: yes label

justinsb mentioned this pull request

Need a way to show masters #33533

Closed

k8s-github-robot assigned deads2k

k8s-github-robot added size/M release-note-label-needed labels

Member Author

justinsb commented Oct 31, 2016

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:

Add a role column, and include it by default
Add a taints column, and include it by default
Some combination of the above

deads2k reviewed

View reviewed changes

pkg/kubectl/resource_printer.go

+              // * 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 != "" {

Contributor

deads2k Oct 31, 2016

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.

deads2k reviewed

View reviewed changes

pkg/kubectl/resource_printer.go

+              	if role := node.Labels["kubernetes.io/role"]; role != "" {
+              		return role
+              	}
+              	if role := node.Labels["kubeadm.alpha.kubernetes.io/role"]; role != "" {

Contributor

deads2k Oct 31, 2016

Can you point me at docs? I'm only seeing this as having the value "master" (which should probably also be a constant).

deads2k reviewed

View reviewed changes

pkg/kubectl/resource_printer.go

               	if node.Spec.Unschedulable {
               		status = append(status, "SchedulingDisabled")
               	}
+              	if role := findNodeRole(node); role != "" {

Contributor

deads2k Oct 31, 2016

if a role is a top level thing, why doesn't it have a column?

Member Author

justinsb Oct 31, 2016

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...

deads2k reviewed

View reviewed changes

pkg/kubectl/resource_printer.go

+              	taints, err := api.GetTaintsFromNodeAnnotations(node.Annotations)
+              	if err != nil {
+              		glog.Warningf("error parsing node taints for node %q: %v", node.Name, err)

Contributor

deads2k Oct 31, 2016

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.

Member Author

justinsb Oct 31, 2016

Good point. Updated to add a status of InvalidTaints if taints cannot be parsed.

deads2k reviewed

View reviewed changes

pkg/kubectl/resource_printer.go

+              		glog.Warningf("error parsing node taints for node %q: %v", node.Name, err)
+              	} else {
+              		for _, taint := range taints {
+              			if taint.Key == "dedicated" {

Contributor

deads2k Oct 31, 2016

Again, if these are going to be magic or recommended values, there ought to be constants in teh code.

Contributor

deads2k commented Oct 31, 2016

If you're going to update a printer, seems like you should update a describer to match.

Member Author

justinsb commented Oct 31, 2016

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 findNodeRole function/logic)

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.

k8s-github-robot added size/L and removed size/M labels

Contributor

k8s-ci-robot commented Oct 31, 2016

Jenkins Kubemark GCE e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Contributor

k8s-ci-robot commented Oct 31, 2016

Jenkins GCE etcd3 e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Contributor

k8s-ci-robot commented Oct 31, 2016

Jenkins GCI GCE e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Contributor

k8s-ci-robot commented Oct 31, 2016

Jenkins GKE smoke e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Contributor

k8s-ci-robot commented Oct 31, 2016

Jenkins unit/integration failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Contributor

k8s-ci-robot commented Oct 31, 2016

Jenkins GCI GKE smoke e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Contributor

k8s-ci-robot commented Oct 31, 2016

Jenkins GCE Node e2e failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Contributor

k8s-ci-robot commented Oct 31, 2016

Jenkins verification failed for commit 8d6e77bd94511a0559ad8fc7746832a4114de6ab. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

This was referenced Oct 31, 2016

Master is added to NodePort / AWS ELBs #33884

Closed

Add constants for node role labels #35975

Merged

justinsb force-pushed the kubectl_show_labels branch 2 times, most recently from 8fa9735 to a7f9ce6 Compare

November 4, 2016 23:17

Member Author

justinsb commented Nov 4, 2016

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 :-)

justinsb added release-note and removed release-note-label-needed labels

k8s-github-robot added size/M and removed size/L labels

davidopp mentioned this pull request

Stop using taint with key=dedicated to mark master node #36272

Closed

Contributor

k8s-ci-robot commented Nov 4, 2016

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. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

justinsb added this to the v1.5 milestone

Member Author

justinsb commented Nov 5, 2016

@k8s-bot cvm gce e2e test this

Member Author

justinsb commented Nov 7, 2016

@deads2k if you have time to look at this before feature-freeze deadline, that would be appreciated :-) If not, can you reassign please?

deads2k reviewed

View reviewed changes

pkg/api/unversioned/well_known_labels.go

+              	// 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"

Contributor

deads2k Nov 7, 2016

Why didn't kubeadm use the "real" annotation?

Member Author

justinsb Nov 7, 2016

I presume they didn't want to go through the review process

deads2k reviewed

View reviewed changes

pkg/api/unversioned/well_known_labels.go

+              	// NodeLabelRoleNode is the value of a NodeLabelRole or NodeLabelKubeadmAlphaRole label, indicating a "normal" node,
+              	// as opposed to a RoleMaster node.
+              	NodeLabelRoleNode = "node"

Contributor

deads2k Nov 7, 2016

Anything using this?

Member Author

justinsb Nov 7, 2016

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.

Contributor

deads2k Nov 7, 2016

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.

deads2k reviewed

View reviewed changes

pkg/api/unversioned/well_known_labels.go

+              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"

Contributor

deads2k Nov 7, 2016

Ah, this the one that's used as a prefix, isn't it? Update the name to match and code to use it?

Member Author

justinsb Nov 7, 2016

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

Contributor

deads2k Nov 7, 2016

I don't believe so, no. The known uses today are
kubernetes.io/role=master

I can't find this use.

Member Author

justinsb Nov 7, 2016

It is used in kops: https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/kubeletconfig.go#L48


          kubectl: show node label if defined

98f7c39

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

justinsb force-pushed the kubectl_show_labels branch from a7f9ce6 to 98f7c39 Compare

November 7, 2016 23:38

Member Author

justinsb commented Nov 7, 2016

@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!)

Contributor

deads2k commented Nov 8, 2016

@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.

deads2k added the lgtm label

k8s-github-robot commented Nov 10, 2016

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

k8s-github-robot commented Nov 10, 2016

Automatic merge from submit-queue

k8s-github-robot merged commit a551dff into kubernetes:master

chentao1596 mentioned this pull request

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

rosskukulinski mentioned this pull request

kubectl get nodes doesn't identify master heptio/aws-quickstart#61

Closed

qrevel mentioned this pull request

kubectl does not show node role #50010

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm release-note size/M