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

Promote Node Instance Type Label to GA #82049

Merged

Conversation

@andrewsykim
Copy link
Member

andrewsykim commented Aug 28, 2019

What type of PR is this?
/kind api-change

What this PR does / why we need it:
This PR promotes the beta instance type label to GA:

beta.kubernetes.io/instance-type -> node.kubernetes.io/instance-type

In addition, both the kubelet and cloud node controller now backfill node.kubernetes.io/instance-type to existing nodes so existing clusters upgrading will get the new labels as well.

The instance type label is widely adopted and should be renamed to reflect it's general availability. The beta instance-type label will be removed in a future release (see KEP below for more details).

Which issue(s) this PR fixes:
KEP: https://github.com/kubernetes/enhancements/blob/master/keps/sig-cloud-provider/20190215-promoting-cloud-provider-labels.md

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Deprecate the instance type beta label ("beta.kubernetes.io/instance-type") in favor of it's GA equivalent: "node.kubernetes.io/instance-type"

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Aug 28, 2019

/priority important-longterm
/sig cloudprovider node

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Aug 28, 2019

Related to #81431. instance-type was much easier to do than zone/region so I decided to keep it in a separate PR. PTAL

/assign @thockin

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Aug 28, 2019

@andrewsykim andrewsykim force-pushed the andrewsykim:ga-node-instance-type-label branch from 0d5762f to c46b520 Aug 28, 2019
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Aug 28, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

Copy link
Contributor

mattjmcnaughton left a comment

Actual code here looks good to me - I will let someone with slightly more context on the KEP approve.

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Aug 28, 2019

/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 29, 2019
Copy link
Member

thockin left a comment

Thanks!

/lgtm
/approve

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Aug 29, 2019

/milestone v1.16

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 29, 2019
…stance-type labels

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
@andrewsykim andrewsykim force-pushed the andrewsykim:ga-node-instance-type-label branch from b56e53f to be0b115 Nov 8, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Nov 8, 2019
@andrewsykim andrewsykim changed the title [WIP] Promote Node Instance Type Label to GA Promote Node Instance Type Label to GA Nov 8, 2019
… to existing nodes

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
@andrewsykim andrewsykim force-pushed the andrewsykim:ga-node-instance-type-label branch from be0b115 to 761838a Nov 8, 2019
…nstance-type label

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
@andrewsykim andrewsykim force-pushed the andrewsykim:ga-node-instance-type-label branch from 761838a to 094b614 Nov 8, 2019
@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Nov 8, 2019

A lot has changed since this PR was last approved so I'm removing the initial approval. Added backfill logic in the kubelet and cloud node controller so nodes from existing clusters get an update.e

/approve cancel
/assign @liggitt

Should be very similar to #81431

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Nov 8, 2019

NodeRestriction admission unit test needs updating with the final GA label:

--- FAIL: Test_nodePlugin_Admit (0.04s)
    --- FAIL: Test_nodePlugin_Admit/allow_update_of_my_node:_add_allowed_labels (0.00s)
        admission_test.go:1264: nodePlugin.Admit() error = nodes "mynode" is forbidden: is not allowed to modify labels: kubernetes.io/instance-type, expected 
    --- FAIL: Test_nodePlugin_Admit/allow_update_of_my_node:_remove_allowed_labels (0.00s)
        admission_test.go:1264: nodePlugin.Admit() error = nodes "mynode" is forbidden: is not allowed to modify labels: kubernetes.io/instance-type, expected 
    --- FAIL: Test_nodePlugin_Admit/allow_update_of_my_node:_modify_allowed_labels (0.00s)
        admission_test.go:1264: nodePlugin.Admit() error = nodes "mynode" is forbidden: is not allowed to modify labels: kubernetes.io/instance-type, expected 
    --- FAIL: Test_nodePlugin_Admit/allow_update_of_my_node:_add_allowed_labels_while_forbidden_labels_exist_unmodified (0.00s)
        admission_test.go:1264: nodePlugin.Admit() error = nodes "mynode" is forbidden: is not allowed to modify labels: kubernetes.io/instance-type, expected 
    --- FAIL: Test_nodePlugin_Admit/allow_update_of_my_node:_remove_allowed_labels_while_forbidden_labels_exist_unmodified (0.00s)
        admission_test.go:1264: nodePlugin.Admit() error = nodes "mynode" is forbidden: is not allowed to modify labels: kubernetes.io/instance-type, expected
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Nov 8, 2019

lgtm otherwise

…tance-type label

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
@andrewsykim andrewsykim force-pushed the andrewsykim:ga-node-instance-type-label branch from 47665b6 to 560b8ef Nov 8, 2019
@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Nov 8, 2019

Fixed, thanks!

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Nov 8, 2019

/lgtm
/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Nov 8, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@andrewsykim

This comment has been minimized.

Copy link
Member Author

andrewsykim commented Nov 8, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 9cf309e into kubernetes:master Nov 8, 2019
15 checks passed
15 checks passed
cla/linuxfoundation andrewsykim authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
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-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
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.