Skip to content

Promote Node Instance Type Label to GA#82049

Merged
k8s-ci-robot merged 6 commits intokubernetes:masterfrom
andrewsykim:ga-node-instance-type-label
Nov 8, 2019
Merged

Promote Node Instance Type Label to GA#82049
k8s-ci-robot merged 6 commits intokubernetes:masterfrom
andrewsykim:ga-node-instance-type-label

Conversation

@andrewsykim
Copy link
Copy Markdown
Member

@andrewsykim andrewsykim commented Aug 28, 2019 •
edited
Loading

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 28, 2019
@k8s-ci-robot k8s-ci-robot requested review from deads2k and hex108 August 28, 2019 00:03
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 28, 2019
@andrewsykim andrewsykim force-pushed the ga-node-instance-type-label branch from 1f2dd87 to 0d5762f Compare August 28, 2019 00:05
@andrewsykim
Copy link
Copy Markdown
Member Author

andrewsykim commented Aug 28, 2019

/priority important-longterm
/sig cloudprovider node

@k8s-ci-robot k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 28, 2019
@andrewsykim
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member Author

andrewsykim commented Aug 28, 2019

cc @MorrisLaw

@andrewsykim andrewsykim force-pushed the ga-node-instance-type-label branch from 0d5762f to c46b520 Compare August 28, 2019 00:22
@fejta-bot
Copy link
Copy Markdown

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.

mattjmcnaughton
mattjmcnaughton reviewed Aug 28, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@mattjmcnaughton mattjmcnaughton left a comment

Choose a reason for hiding this comment

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

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

@andrewsykim
Copy link
Copy Markdown
Member Author

andrewsykim commented Aug 28, 2019

/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2019
thockin
thockin reviewed Aug 29, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2019
@andrewsykim
Copy link
Copy Markdown
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
@andrewsykim
kubelet: add unit tests for backfilling stable os/arch/zone/region/in…
17d8928 
…stance-type labels

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
@andrewsykim andrewsykim force-pushed the ga-node-instance-type-label branch from b56e53f to be0b115 Compare November 8, 2019 15:44
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. 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
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2019
@andrewsykim
cloud node controller: apply stable instance-type label with backfill…
55e40b8 
… to existing nodes

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
@andrewsykim andrewsykim force-pushed the ga-node-instance-type-label branch from be0b115 to 761838a Compare November 8, 2019 15:51
@andrewsykim
scheduler: max attachable volume predicate should also check stable i…
094b614 
…nstance-type label

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
@andrewsykim andrewsykim force-pushed the ga-node-instance-type-label branch from 761838a to 094b614 Compare November 8, 2019 15:52
@andrewsykim
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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
Copy link
Copy Markdown
Member

liggitt commented Nov 8, 2019

lgtm otherwise

@andrewsykim
noderestriction: update node restriction unit tests to use stable ins…
560b8ef 
…tance-type label

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
@andrewsykim andrewsykim force-pushed the ga-node-instance-type-label branch from 47665b6 to 560b8ef Compare November 8, 2019 16:18
@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Nov 8, 2019
@andrewsykim
Copy link
Copy Markdown
Member Author

andrewsykim commented Nov 8, 2019

Fixed, thanks!

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Nov 8, 2019

/lgtm
/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 8, 2019
@k8s-ci-robot
Copy link
Copy Markdown
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

Details 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
Copy link
Copy Markdown
Member Author

andrewsykim commented Nov 8, 2019

/retest

This was referenced Nov 8, 2019
Closed
Merged
@k8s-ci-robot k8s-ci-robot merged commit 9cf309e into kubernetes:master Nov 8, 2019
@mmerrill3 mmerrill3 mentioned this pull request Jan 13, 2020
Closed
@andrewsykim andrewsykim mentioned this pull request Jan 31, 2020
Closed
@andrewsykim andrewsykim mentioned this pull request Feb 24, 2020
Closed
@andrewsykim andrewsykim mentioned this pull request Sep 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thockin thockin thockin left review comments

@deads2k deads2k Awaiting requested review from deads2k

@hex108 hex108 Awaiting requested review from hex108

+1 more reviewer

@mattjmcnaughton mattjmcnaughton mattjmcnaughton left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@liggitt liggitt

@thockin thockin

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

v1.17

Development

Successfully merging this pull request may close these issues.

7 participants

@andrewsykim @fejta-bot @josiahbjorgaard @liggitt @k8s-ci-robot @mattjmcnaughton @thockin