Skip to content

Add policy/v1 Eviction support #100724

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
Merged
k8s-ci-robot merged 6 commits into from
Apr 16, 2021
Merged

Add policy/v1 Eviction support #100724

k8s-ci-robot merged 6 commits into from
Apr 16, 2021

Conversation

@liggitt
Copy link
Member

@liggitt liggitt commented Mar 31, 2021 •
edited
Loading

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Adds support for accepting Eviction policy/v1

To maintain compatibility with existing clients that send policy/v1beta1 Eviction requests to pods/eviction, this requires allowing subresource REST storage to accept multiple versions.

This PR:

  • adds integration test to ensure:
    • policy/v1 requests are accepted
    • policy/v1beta1 requests are accepted (as before)
    • no get or patch requests are accepted (as before)
    • all requests return status objects, not versioned Eviction objects (as before)
  • updates kubectl to use policy/v1 if available, fall back to policy/v1beta1
  • updates e2e tests to use v1
  • promotes eviction test to conformance (flake-free history)

Also tested skew manually:

  • 1.20 kubectl drain against this PR: 
    I0401 23:22:54.885823   41250 request.go:1107] Request Body: {"kind":"Eviction","apiVersion":"policy/v1beta1","metadata":{"name":"testpod","namespace":"default","creationTimestamp":null},"deleteOptions":{}}
I0401 23:22:54.885849   41250 round_trippers.go:425] curl -k -v -XPOST  -H "Content-Type: application/json" -H "Accept: application/json, */*" -H "User-Agent: kubectl-1.20/v1.20.0 (darwin/amd64) kubernetes/af46c47" 'https://localhost:6443/api/v1/namespaces/default/pods/testpod/eviction'
I0401 23:22:54.957882   41250 round_trippers.go:445] POST https://localhost:6443/api/v1/namespaces/default/pods/testpod/eviction 201 Created in 72 milliseconds
  • kubectl built from this PR against a 1.21 server: 
    I0401 23:28:44.729905   47080 request.go:1123] Request Body: {"kind":"Eviction","apiVersion":"policy/v1beta1","metadata":{"name":"testpod","namespace":"default","creationTimestamp":null},"deleteOptions":{}}
I0401 23:28:44.729969   47080 round_trippers.go:435] curl -k -v -XPOST  -H "User-Agent: kubectl-1.22-dev/v1.22.0 (darwin/amd64) kubernetes/64e09ac" -H "Content-Type: application/json" -H "Accept: application/json, */*" 'https://localhost:6443/api/v1/namespaces/default/pods/testpod/eviction'
I0401 23:28:44.805242   47080 round_trippers.go:454] POST https://localhost:6443/api/v1/namespaces/default/pods/testpod/eviction 201 Created in 75 milliseconds

Does this PR introduce a user-facing change?

The pod/eviction subresource now accepts policy/v1 Eviction requests in addition to policy/v1beta1 Eviction requests

/milestone v1.22

cc @mortent

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Mar 31, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Mar 31, 2021
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Mar 31, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 31, 2021
@caesarxuchao
Copy link
Contributor

caesarxuchao commented Apr 1, 2021

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 1, 2021
@liggitt liggitt force-pushed the eviction-v1beta1 branch from a5085fa to af2dced Compare April 2, 2021 03:08
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 2, 2021
@liggitt liggitt changed the title WIP - eviction v1 Add v1 Eviction support Apr 2, 2021
@k8s-ci-robot k8s-ci-robot added area/kubectl sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Apr 2, 2021
@liggitt
Copy link
Member Author

liggitt commented Apr 13, 2021

/hold cancel

@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 Apr 13, 2021
@lavalamp
Copy link
Contributor

lavalamp commented Apr 13, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2021
@johnbelamaric
Copy link
Member

johnbelamaric commented Apr 13, 2021

For conformance tests, ideally we wouldn't rely on the text of the error string. Given that we are looking for a specific reason in this test, I will allow it this time, though.

/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 Apr 13, 2021
@liggitt
Copy link
Member Author

liggitt commented Apr 13, 2021

#101019 flake
/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2021
@liggitt
Copy link
Member Author

liggitt commented Apr 14, 2021

integration failure was related, making the scope AcceptsGroupVersionKind check compare Kind changed the error text returned from a bad kind. Limited the check here to apiVersion, so bad kinds continue to get the existing BadRequest error.

@liggitt
Copy link
Member Author

liggitt commented Apr 14, 2021

delta to scope the new check to just groupVersion is in https://github.com/kubernetes/kubernetes/compare/4925676c62578e09379e504699dfe8aff51c5b1c..02c3a97815f553af4d4d01a6d343c3ec0a692b28

@liggitt
Copy link
Member Author

liggitt commented Apr 14, 2021 •
edited
Loading

For conformance tests, ideally we wouldn't rely on the text of the error string. Given that we are looking for a specific reason in this test, I will allow it this time, though.

That's a good point. I switched the test to look for the error cause (which is intended to be programmatically readable) rather than the error text, and defined a constant in the API package for that cause. See the Define constant for eviction failure cause commit.

@johnbelamaric
Copy link
Member

johnbelamaric commented Apr 14, 2021

That's a good point. I switched the test to look for the error cause (which is intended to be programmatically readable) rather than the error text, and defined a constant in the API package for that cause. See the Define constant for eviction failure cause commit.

Excellent, that's much better.

/approve

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric, lavalamp, liggitt, mortent

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

@lavalamp
Copy link
Contributor

lavalamp commented Apr 16, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2021
@k8s-ci-robot k8s-ci-robot merged commit 27a625c into kubernetes:master Apr 16, 2021
@pacoxu pacoxu mentioned this pull request Apr 19, 2021
Closed
@liggitt liggitt deleted the eviction-v1beta1 branch April 19, 2021 13:26
@github-actions github-actions bot mentioned this pull request Apr 21, 2021
Open
This was referenced Apr 29, 2021
Merged
Closed
@foriequal0 foriequal0 mentioned this pull request Apr 5, 2022
Closed
@zwzhang0107 zwzhang0107 mentioned this pull request Sep 14, 2022
Merged
3 tasks
@quixoticmonk quixoticmonk mentioned this pull request Feb 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deads2k deads2k Awaiting requested review from deads2k

3 more reviewers

@lavalamp lavalamp lavalamp left review comments

@alculquicondor alculquicondor alculquicondor left review comments

@mortent mortent mortent approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mortent mortent

@soltysh soltysh

@lavalamp lavalamp

@deads2k deads2k

@johnbelamaric johnbelamaric

Labels

api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/conformance Issues or PRs related to kubernetes conformance tests area/kubectl 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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Projects

API Reviews
Status: API review completed, 1.22

Milestone

v1.22

Development

Successfully merging this pull request may close these issues.

9 participants

@liggitt @caesarxuchao @lavalamp @johnbelamaric @k8s-ci-robot @mortent @alculquicondor @soltysh @deads2k