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

Move APIServingWithRoutine to alpha and disabled by default. #126470

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Move APIServingWithRoutine to alpha and disabled by default. #126470

merged 1 commit into from
Jul 31, 2024

Conversation

benluddy
Copy link
Contributor

@benluddy benluddy commented Jul 30, 2024 •
edited by deads2k
Loading

What type of PR is this?

/sig api-machinery
/sig scalability

What this PR does / why we need it:

Two regressions have been discovered related to the interaction between the APIServingWithRoutine feature and other apiserver filters whose deferred functions expect to execute only after handling a request.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Related issues:

Discussion: https://kubernetes.slack.com/archives/C0EG7JC6T/p1722358348712859

Does this PR introduce a user-facing change?

Disabled a previously on-by-default optimization for the API server where each **watch** response used a dedicated goroutine. The `APIServingWithRoutine` feature gate has been demoted from beta to alpha, and is now off by default.

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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/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. labels Jul 30, 2024
@k8s-ci-robot k8s-ci-robot requested review from enj and liggitt July 30, 2024 20:53
@benluddy
Copy link
Contributor Author

/kind regression

@k8s-ci-robot k8s-ci-robot added kind/regression Categorizes issue or PR as related to a regression from a prior release. area/apiserver and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jul 30, 2024
@deads2k
Copy link
Contributor

deads2k commented Jul 30, 2024

This should also be cherry-picked to 1.30. The failure modes are subtle and it's safer to disable entirely versus continue to try to plug gaps.

@benluddy please place an item on the sig-apimachinery agenda next week to discuss the long-term maintenance impacts of this change and try to make contact with the original author to attend.

/lgtm
/approve
/milestone v1.31
/cc @jpbetz

@k8s-ci-robot k8s-ci-robot requested a review from jpbetz July 30, 2024 20:59
@k8s-ci-robot k8s-ci-robot added this to the v1.31 milestone Jul 30, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6d008760969f028f83ea13148c206b213435edfc

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, deads2k

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2024
@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@k8s-ci-robot k8s-ci-robot merged commit f9d2297 into kubernetes:master Jul 31, 2024
15 checks passed
@benluddy benluddy mentioned this pull request Jul 31, 2024
Merged
@linxiulei
Copy link
Contributor

linxiulei commented Jul 31, 2024 •
edited
Loading

/cc @wojtek-t

I will attend sig-apimachinery meeting next week

@k8s-ci-robot k8s-ci-robot requested a review from wojtek-t July 31, 2024 14:30
@liggitt liggitt mentioned this pull request Jul 31, 2024
Closed
@sftim
Copy link
Contributor

sftim commented Jul 31, 2024

Changelog suggestion

-Disabled the feature APIServingWithRoutine by default and changed its stage from Beta to Alpha.
+Disabled a previously on-by-default optimization for the API server where each **watch** response
+used a dedicated goroutine.
+The `APIServingWithRoutine` feature gate has been demoted from beta to alpha, and is now off
+by default.

@seans3
Copy link
Contributor

seans3 commented Jul 31, 2024

/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 Jul 31, 2024
k8s-ci-robot added a commit that referenced this pull request Aug 7, 2024
@k8s-ci-robot
Merge pull request #126481 from benluddy/automated-cherry-pick-of-#12…
d0c02cd 
…6470-kubernetes-release-1.30

Automated cherry pick of #126470: Move APIServingWithRoutine to alpha and disabled by default.
This was referenced Aug 8, 2024
Merged
Merged
@linxiulei linxiulei mentioned this pull request Aug 28, 2024
Open
@chenk008
Copy link
Contributor

It also make APF mutating_work_estimator.go get wrong watchCount.

@benluddy
Copy link
Contributor Author

benluddy commented Sep 17, 2024 •
edited
Loading

It also make APF mutating_work_estimator.go get wrong watchCount.

@chenk008: Are you still seeing this with the feature gate disabled? We just started noticing an issue in APF with watches in our 1.31 distribution (with this feature gate disabled) and it may be due to this feature's related changes to the "get" handler itself. The gate only controls whether or not the extra filter is added to the filter chain.

@linxiulei linxiulei mentioned this pull request Sep 22, 2024
Open
@openshift-ci-robot openshift-ci-robot mentioned this pull request Nov 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enj enj Awaiting requested review from enj

@liggitt liggitt Awaiting requested review from liggitt

@jpbetz jpbetz Awaiting requested review from jpbetz

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. 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/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
[sig-release] Bug Triage
Archived in project
Milestone
v1.31
Development

Successfully merging this pull request may close these issues.
8 participants
@benluddy @deads2k @k8s-ci-robot @k8s-triage-robot @linxiulei @sftim @seans3 @chenk008