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

update kubernetes.io/os description as it can also be used in pod labels #40332

Merged
merged 2 commits into from
Mar 30, 2023

Conversation

pacoxu
Copy link
Member

@pacoxu pacoxu commented Mar 27, 2023

When I review kubernetes/kubernetes#116892, I find that no document has mentioned it.

	if rejectPodAdmissionBasedOnOSSelector(admitPod, node) {
		return PodAdmitResult{
			Admit:   false,
			Reason:  "PodOSSelectorNodeLabelDoesNotMatch",
			Message: "Failed to admit pod as the `kubernetes.io/os` label doesn't match node label",
		}
	}


...
// rejectPodAdmissionBasedOnOSSelector rejects pod if it's nodeSelector doesn't match
// We expect the kubelet status reconcile which happens every 10sec to update the node labels if there is a mismatch.
func rejectPodAdmissionBasedOnOSSelector(pod *v1.Pod, node *v1.Node) bool {
	labels := node.Labels
	osName, osLabelExists := labels[v1.LabelOSStable]
	if !osLabelExists || osName != runtime.GOOS {
		if len(labels) == 0 {
			labels = make(map[string]string)
		}
		labels[v1.LabelOSStable] = runtime.GOOS
	}
	podLabelSelector, podOSLabelExists := pod.Labels[v1.LabelOSStable]
	if !podOSLabelExists {
		// If the labelselector didn't exist, let's keep the current behavior as is
		return false
	} else if podOSLabelExists && podLabelSelector != labels[v1.LabelOSStable] {
		return true
	}
	return false
}

@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Mar 27, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 27, 2023
@pacoxu
Copy link
Member Author

pacoxu commented Mar 27, 2023

/cc @SergeyKanzhelev @SataQiu

@netlify
Copy link

netlify bot commented Mar 27, 2023

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 24586e4
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/64229bec76db100008430c80
😎 Deploy Preview https://deploy-preview-40332--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.


The Kubelet populates this with `runtime.GOOS` as defined by Go. This can be handy if you are mixing operating systems in your cluster (for example: mixing Linux and Windows nodes).

When the `kubernetes.io/os` label in Pod labels does not match the node label, the kubelet will not admit the pod. However, this is not taken into account by kube scheduler.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When the `kubernetes.io/os` label in Pod labels does not match the node label, the kubelet will not admit the pod. However, this is not taken into account by kube scheduler.
When the `kubernetes.io/os` label value for a Pod does not match the label value on a Node,
the kubelet on the node will not admit the Pod. However, this is not taken into account by
the kube-scheduler.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might also want to link to https://kubernetes.io/docs/concepts/workloads/pods/#pod-os so that readers are aware that this label is one of two relevant mechanisms.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am unsure if this is the recommended approach.

We might also want to link to https://kubernetes.io/docs/concepts/workloads/pods/#pod-os so that readers are aware that this label is one of two relevant mechanisms.

The Pod OS field seems an alternative way for the pod os label.

@sftim
Copy link
Contributor

sftim commented Mar 28, 2023

I guess this should have a tech review. @pacoxu, who can confirm this revision?

@pacoxu
Copy link
Member Author

pacoxu commented Mar 28, 2023

I guess this should have a tech review. @pacoxu, who can confirm this revision?

/cc @ravisantoshgudimetla @jsturtevant
who is the owner of IdentifyPodOS and this is part of kubernetes/enhancements#2802. (I opened #40353 as the feature gate was GAed in v1.25 and removed in v1.27.)

Does anything (in-project) set that label on a Pod / pod template?

kubernetes/kubernetes#104613 this was added in v1.23. I cannot find anywhere that is using this label in pod.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm
as is

Some ideas.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2fe280825c1400046b6fc6cbf5739f752a91987b

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2023
@k8s-ci-robot k8s-ci-robot requested a review from sftim March 28, 2023 07:49
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 28, 2023
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 835b6bf2d89cd4a1f65d872c03c1660088e53e64

Copy link
Member

@SataQiu SataQiu left a comment

Choose a reason for hiding this comment

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

/lgtm

@pacoxu
Copy link
Member Author

pacoxu commented Mar 30, 2023

/assign @sftim

Copy link
Contributor

@divya-mohan0209 divya-mohan0209 left a comment

Choose a reason for hiding this comment

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

LGTM from me!

@sftim
Copy link
Contributor

sftim commented Mar 30, 2023

/approve
based on #40332 (review) as LGTM

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2023
@sftim
Copy link
Contributor

sftim commented Mar 30, 2023

/retitle update kubernetes.io/os description as it can also be used in pod labels

@k8s-ci-robot k8s-ci-robot changed the title update kubernetes.io/os description as it can also be used in pod labels update kubernetes.io/os description as it can also be used in pod labels Mar 30, 2023
@k8s-ci-robot k8s-ci-robot merged commit 72f5bf1 into kubernetes:main Mar 30, 2023
@pacoxu pacoxu deleted the patch-15 branch June 26, 2023 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants