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.

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

RuntimeClass GA #95718

Merged
merged 1 commit into from
Nov 12, 2020
Merged

RuntimeClass GA #95718

merged 1 commit into from
Nov 12, 2020
+4,377 −305

Conversation

SergeyKanzhelev
Copy link
Member

What type of PR is this?

/kind feature
/sig node

What this PR does / why we need it:

Promote RuntimeClass to GA, including promoting the API to v1. Note, PodOverhead is part of the API and being promoted to v1 as it cannot be separated. We can consider it a beta feature of v1 API. Please advice if this is not recommended and whether there is a workaround.

Which issue(s) this PR fixes:
Related kubernetes/enhancements#585

Special notes for your reviewer:

Inspired by #74433
Still looking into Swagger,json update. May need to introduce new API types there.

Does this PR introduce a user-facing change?:

Promote RuntimeClass feature to GA.
Promote node.k8s.io API groups from v1beta1 to v1.

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

- [KEP]: https://github.com/kubernetes/enhancements/issues/585

/assign @tallclair
/assign @harche

@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/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 20, 2020
@k8s-ci-robot
Copy link
Contributor

@SergeyKanzhelev: GitHub didn't allow me to assign the following users: harche.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

What type of PR is this?

/kind feature
/sig node

What this PR does / why we need it:

Promote RuntimeClass to GA, including promoting the API to v1. Note, PodOverhead is part of the API and being promoted to v1 as it cannot be separated. We can consider it a beta feature of v1 API. Please advice if this is not recommended and whether there is a workaround.

Which issue(s) this PR fixes:
Related kubernetes/enhancements#585

Special notes for your reviewer:

Inspired by #74433
Still looking into Swagger,json update. May need to introduce new API types there.

Does this PR introduce a user-facing change?:

Promote RuntimeClass feature to GA.
Promote node.k8s.io API groups from v1beta1 to v1.

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

- [KEP]: https://github.com/kubernetes/enhancements/issues/585

/assign @tallclair
/assign @harche

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.

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 20, 2020
@k8s-ci-robot
Copy link
Contributor

@SergeyKanzhelev: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver area/kubelet area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Oct 20, 2020
@fejta-bot
Copy link

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.

egernst
egernst reviewed Oct 20, 2020
View reviewed changes
pkg/api/pod/util.go Outdated Show resolved Hide resolved
egernst
egernst reviewed Oct 20, 2020
View reviewed changes
pkg/apis/node/v1/doc.go Outdated Show resolved Hide resolved
egernst
egernst reviewed Oct 20, 2020
View reviewed changes
Copy link
Contributor

@egernst egernst left a comment

Choose a reason for hiding this comment

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

just a couple of superficial nits at this point. LGTM, but I'll take a closer look later this week.

pkg/apis/node/v1/register.go Outdated Show resolved Hide resolved
@SergeyKanzhelev
Copy link
Member Author

just a couple of superficial nits at this point. LGTM, but I'll take a closer look later this week.

@egernst thank you! I should finish dealing with the build failures (working on my machine =)))) by the next review.

Do you know if swagger file is auto-generated? Or I need to add new API there manually?

@fedebongio
Copy link
Contributor

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Oct 20, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2020
@SergeyKanzhelev
Copy link
Member Author

/hold cancel
/lgtm

Thank you again for catching the issue! Will be working on build failures now

/shoutout @liggitt thank you for catching the issue with the code last minute before the PR merge!

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2020
@SergeyKanzhelev
Copy link
Member Author

/test pull-kubernetes-bazel-build

@SergeyKanzhelev
Copy link
Member Author

Something fishy happening:

> GIT_MERGE_VERBOSITY=6 git merge --no-ff d9cecd77c543eb16c858abd7b8aa6fdc62d9124f
> merge: d9cecd77c543eb16c858abd7b8aa6fdc62d9124f - not something we can merge

Its already rebased on latest upstream/master and all sha looks to be correct. Will try to re-base again with the different commit message

@SergeyKanzhelev
Copy link
Member Author

/test pull-kubernetes-bazel-build

@SergeyKanzhelev
Copy link
Member Author

the merge issue seems to be resolved. perhaps some github problem, retesting failed jobs:
/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-gce-alpha-features
/test pull-kubernetes-e2e-gce-ubuntu-containerd
/test pull-kubernetes-node-e2e

tallclair
tallclair reviewed Nov 11, 2020
View reviewed changes
Copy link
Member

@tallclair tallclair 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 Nov 11, 2020
@liggitt
Copy link
Member

liggitt commented Nov 12, 2020 •
edited
Loading

Does this have e2e coverage of all verbs on all endpoints of v1 RuntimeClasses (like ea8f4cb)? If not, this will appear as a regression in conformance coverage of GA endpoints. The expectation is that GA APIs have 100% e2e coverage via tests suitable to promote to conformance tests

@harche
Copy link
Contributor

harche commented Nov 12, 2020

/test pull-kubernetes-e2e-gce-ubuntu-containerd

@k8s-ci-robot k8s-ci-robot merged commit 12d9183 into kubernetes:master Nov 12, 2020
@SergeyKanzhelev SergeyKanzhelev deleted the runtimeClass2 branch November 12, 2020 18:53
@SergeyKanzhelev SergeyKanzhelev mentioned this pull request Nov 12, 2020
Closed
@SergeyKanzhelev
Copy link
Member Author

Does this have e2e coverage of all verbs on all endpoints of v1 RuntimeClasses (like ea8f4cb)? If not, this will appear as a regression in conformance coverage of GA endpoints. The expectation is that GA APIs have 100% e2e coverage via tests suitable to promote to conformance tests

Working on it. Created a tracking issue: #96524

@leilajal
Copy link
Contributor

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 17, 2020
@github-actions github-actions bot mentioned this pull request Nov 18, 2020
Open
@github-actions github-actions bot mentioned this pull request Dec 14, 2020
Open
@floryut floryut mentioned this pull request Jan 18, 2021
Merged
@mortent mortent mentioned this pull request Feb 22, 2021
Merged
@CaoDonghui123 CaoDonghui123 mentioned this pull request Jun 17, 2021
Closed
@pacoxu pacoxu mentioned this pull request Sep 24, 2021
Closed
@SataQiu SataQiu mentioned this pull request Jan 19, 2023
Merged
@tzifudzi tzifudzi mentioned this pull request Nov 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallclair tallclair tallclair left review comments

@egernst egernst egernst left review comments

@thockin thockin thockin left review comments

@liggitt liggitt liggitt left review comments

@brendandburns brendandburns Awaiting requested review from brendandburns

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

Assignees

@mrunalp mrunalp

@lavalamp lavalamp

@liggitt liggitt

@thockin thockin

@tallclair tallclair

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/kubelet area/test 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 kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. 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/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.