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

Revert v1beta1 PodDisruptionBudget selector patchStrategy to pre-1.21 behavior #108138

Merged
merged 2 commits into from Feb 16, 2022
Merged

Revert v1beta1 PodDisruptionBudget selector patchStrategy to pre-1.21 behavior #108138

merged 2 commits into from Feb 16, 2022

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Feb 15, 2022 •
edited

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

Reverts strategic merge patch behavior for the selector field in v1beta1 PodDisruptionBudget objects to match pre-1.21 behavior.

Which issue(s) this PR fixes:

Fixes #108137

Does this PR introduce a user-facing change?

Fixes a regression in 1.21 in v1beta1 PodDisruptionBudget handling of "strategic merge patch"-type API requests for the `selector` field. Prior to 1.21, these requests would merge `matchLabels` content and replace `matchExpressions` content. In 1.21, patch requests touching the `selector` field started replacing the entire selector. This is consistent with server-side apply and the v1 PodDisruptionBudget behavior, but should not have been changed for v1beta1.

/cc @mortent
/cc @jpbetz

@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/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/regression Categorizes issue or PR as related to a regression from a prior release. 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. labels Feb 15, 2022
@liggitt liggitt changed the title Revert v1beta1 PodDisruptionBudget select patchStrategy Revert v1beta1 PodDisruptionBudget selector patchStrategy Feb 15, 2022
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 15, 2022
@k8s-ci-robot
Copy link
Contributor

@liggitt: 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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/apps Categorizes an issue or PR as relevant to SIG Apps. approved Indicates a PR has been approved by an approver from all required OWNERS files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 15, 2022
2rs2ts
2rs2ts approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@2rs2ts 2rs2ts left a comment •
edited

Choose a reason for hiding this comment

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

Thank you for the quick turnaround on this!

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/test and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 15, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2rs2ts, liggitt

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

@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Feb 15, 2022
@liggitt
Copy link
Member Author

liggitt commented Feb 15, 2022 •
edited

added an integration test exercising all strategic merge patch, merge patch, and apply patch types against v1 and v1beta1. Verified the test passes against this PR, passes against release-1.20 (with the v1 cases skipped, since the v1 API didn't exist then), and fails against release-1.21, release-1.22, release-1.23, and master.

@liggitt
Copy link
Member Author

liggitt commented Feb 15, 2022

/retest

@liggitt liggitt changed the title Revert v1beta1 PodDisruptionBudget selector patchStrategy Revert v1beta1 PodDisruptionBudget selector patchStrategy to pre-1.21 behavior Feb 15, 2022
This was referenced Feb 15, 2022
Merged
Merged
@k8s-triage-robot
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.

@liggitt liggitt mentioned this pull request Feb 15, 2022
Merged
@jpbetz
Copy link
Contributor

jpbetz commented Feb 15, 2022

/lgtm

Huge thanks for fixing.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2022
@k8s-ci-robot k8s-ci-robot merged commit a37b6fc into kubernetes:master Feb 16, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Feb 16, 2022
@2rs2ts
Copy link
Contributor

2rs2ts commented Feb 17, 2022

Will this be backported to 1.21-23?

@liggitt
Copy link
Member Author

liggitt commented Feb 17, 2022

yes, cherry-picks already open and linked

k8s-ci-robot added a commit that referenced this pull request Feb 22, 2022
@k8s-ci-robot
Merge pull request #108142 from liggitt/automated-cherry-pick-of-#108…
35758a4 
…138-upstream-release-1.21

Automated cherry pick of #108138: Revert v1beta1 PodDisruptionBudget select patchStrategy
k8s-ci-robot added a commit that referenced this pull request Feb 22, 2022
@k8s-ci-robot
Merge pull request #108141 from liggitt/automated-cherry-pick-of-#108…
4594898 
…138-upstream-release-1.22

Automated cherry pick of #108138: Revert v1beta1 PodDisruptionBudget select patchStrategy
k8s-ci-robot added a commit that referenced this pull request Feb 22, 2022
@k8s-ci-robot
Merge pull request #108139 from liggitt/automated-cherry-pick-of-#108…
9a19f6a 
…138-upstream-release-1.23

Automated cherry pick of #108138: Revert v1beta1 PodDisruptionBudget select patchStrategy
@liggitt liggitt deleted the v1beta1-selector branch May 5, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@2rs2ts 2rs2ts 2rs2ts approved these changes

@jpbetz jpbetz Awaiting requested review from jpbetz

@mortent mortent Awaiting requested review from mortent

Assignees

@jpbetz jpbetz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. 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/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.24
Development

Successfully merging this pull request may close these issues.

policy/v1beta1 PodDisruptionBudget selector patchStrategy changed unexpectedly in 1.21 upgrade
5 participants
@liggitt @k8s-ci-robot @k8s-triage-robot @jpbetz @2rs2ts