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

kubelet: promote OS & arch labels to GA #73333

Merged
merged 2 commits into from Feb 16, 2019

Conversation

@yujuhong
Copy link
Member

yujuhong commented Jan 26, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #72929

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kubelet: OS and Arch information is now recorded in `kubernetes.io/os` and `kubernetes.io/arch` labels on Node objects. The previous labels (`beta.kubernetes.io/os` and `beta.kubernetes.io/arch`) are still recorded, but are deprecated and targeted for removal in 1.18.
@yujuhong

This comment has been minimized.

Copy link
Member Author

yujuhong commented Jan 26, 2019

@k8s-ci-robot k8s-ci-robot requested review from bgrant0607 and dchen1107 Jan 26, 2019

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented Jan 26, 2019

Thanks
cc @liggitt

@yujuhong

This comment has been minimized.

Copy link
Member Author

yujuhong commented Jan 29, 2019

/retest

@yujuhong

This comment has been minimized.

Copy link
Member Author

yujuhong commented Jan 29, 2019

/retest

@liggitt are you okay with the change? I'll send a separate PR to update the node controller

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 30, 2019

@liggitt are you okay with the change? I'll send a separate PR to update the node controller

I updated the release note with specific label keys and deprecation timeline for the existing beta labels. Can you add a TODO in the code to remove the beta label reporting in 1.18?

For the node controller, a separate PR is fine, but describing in a KEP how we intend to manage these labels would be worthwhile. Something along the lines of:

  • Starting in 1.14, the node controller will reconcile a mismatch between a present beta label and a present or missing GA label in favor of the beta label (to ensure uniform labeling in the presence of pre-1.14 kubelets that don't report the GA label).
  • Starting in 1.18 (the release we have the kubelet stop reporting the beta labels), the node controller will switch to reconciling a mismatch between a present beta label and a present GA label in favor of the GA label (since pre-1.14 kubelets are not supported against a 1.18+ control plane)
@yujuhong

This comment has been minimized.

Copy link
Member Author

yujuhong commented Jan 30, 2019

/retest

@liggitt I updated the PR according to your comment, but I'm not sure why we have to wait until 1.18. Kubernetes only supports up to 2 version skews based on the policy https://kubernetes.io/docs/setup/version-skew-policy/ The 1.17 master control plane will only support 1.17, 1.16, and 1.15 nodes.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 30, 2019

I'm not sure why we have to wait until 1.18.

We wouldn't have the kubelet stop setting the beta labels until 1.18

@yujuhong

This comment has been minimized.

Copy link
Member Author

yujuhong commented Jan 31, 2019

KEP as requested: kubernetes/enhancements#792

@dims

This comment has been minimized.

Copy link
Member

dims commented Feb 1, 2019

/assign @dchen1107

kubelet: promote OS & arch labels to GA
kubelet now applies both the beta and the GA labels to ensure backward
compatibility.
@yujuhong

This comment has been minimized.

Copy link
Member Author

yujuhong commented Feb 11, 2019

Rebased. All the labels have been moved to staging/src/k8s.io/api/core/v1/well_known_labels.go in another PR, so I had to force push to not cause a mess.

On the other hand, @dims I'm not sure we really want to move the beta labels to v1 api given that we are deprecating them. Should we move them back?

@dims

This comment has been minimized.

Copy link
Member

dims commented Feb 11, 2019

@yujuhong +1 to move back just the beta labels

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 11, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Feb 12, 2019

@yujuhong

This comment has been minimized.

Copy link
Member Author

yujuhong commented Feb 12, 2019

@yujuhong +1 to move back just the beta labels

Added another commit to move those back to the kubelet package

@yujuhong yujuhong force-pushed the yujuhong:os-arch-labels branch from 579e2dc to e64a1e6 Feb 12, 2019

Move beta OS/Arch labels back to the kubelet package
These labels are being deprecated

@yujuhong yujuhong force-pushed the yujuhong:os-arch-labels branch from e64a1e6 to 5fd27c3 Feb 14, 2019

@yujuhong yujuhong referenced this pull request Feb 15, 2019

Open

Promote Node OS/Arch labels to GA #793

0 of 1 task complete
@yujuhong

This comment has been minimized.

Copy link
Member Author

yujuhong commented Feb 15, 2019

Ping @smarterclayton. Do you still want to review this?

@thockin
Copy link
Member

thockin left a comment

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin, yujuhong

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 15, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Feb 15, 2019

lgtm as well

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 15, 2019

@yujuhong: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 03e6d49 link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-local-e2e-containerized 5fd27c3 link /test pull-kubernetes-local-e2e-containerized

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot merged commit 3e74895 into kubernetes:master Feb 16, 2019

16 of 17 checks passed

pull-kubernetes-local-e2e-containerized Job failed.
Details
cla/linuxfoundation yujuhong authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.