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

ServerlessService's initial operation mode is now Proxy #12842

Merged
merged 1 commit into from
Apr 28, 2022
Merged

ServerlessService's initial operation mode is now Proxy #12842

merged 1 commit into from
Apr 28, 2022

Conversation

dprotaso
Copy link
Member

@dprotaso dprotaso commented Apr 12, 2022 •
edited
Loading

Fixes: #12887 #12712

Prior the initial operation mode was Serve. This would create a
narrow window where the revision's pods directly receive requests
from the ingress. The size of this window depends on when the
Revision's Pod becomes ready and the time it takes the autoscaler
to make an initial decision on scale an excess burst capacity.

If there is a continuous traffic this could overwhelm Revisions
with a low container concurrency causing the queue-proxy to return
503s since it's queue is full.

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Apr 12, 2022
@codecov
Copy link

codecov bot commented Apr 12, 2022 •
edited
Loading

Codecov Report

Merging #12842 (1ed8da4) into main (f7deea4) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 1ed8da4 differs from pull request most recent head 9da8be2. Consider uploading reports for the commit 9da8be2 to get more accurate results

@@            Coverage Diff             @@
##             main   #12842      +/-   ##
==========================================
- Coverage   87.03%   87.01%   -0.02%     
==========================================
  Files         197      197              
  Lines       14426    14429       +3     
==========================================
  Hits        12556    12556              
- Misses       1576     1578       +2     
- Partials      294      295       +1
Impacted Files Coverage Δ
pkg/reconciler/autoscaling/kpa/kpa.go 95.26% <100.00%> (+0.07%) ⬆️
pkg/autoscaler/statforwarder/leases.go 72.39% <0.00%> (-1.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7deea4...9da8be2. Read the comment docs.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2022
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2022
@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 14, 2022
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2022
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/autoscale area/networking and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 21, 2022
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 21, 2022
@knative-prow knative-prow bot added the area/API API objects and controllers label Apr 28, 2022
@dprotaso
Copy link
Member Author

/test istio-latest-no-mesh_serving_main

1 similar comment
@dprotaso
Copy link
Member Author

/test istio-latest-no-mesh_serving_main

@dprotaso
Copy link
Member Author

woot 300 runs with no flakes.

@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 28, 2022
@dprotaso
Copy link
Member Author

/test istio-latest-no-mesh_serving_main

1 similar comment
@dprotaso
Copy link
Member Author

/test istio-latest-no-mesh_serving_main

@dprotaso
ServerlessService's initial operation mode is now Proxy
9da8be2 
Prior the initial operation mode was Serve.  This would create a
narrow window where the revision's pods directly receive requests
from the ingress.  The size of this window depends on when the
Revision's Pod becomes ready and the time it takes the autoscaler
to make an initial decision on scale an excess burst capacity.

If there is a continuous traffic this could overwhelm Revisions
with a low container concurrency causing the queue-proxy to return
503s since it's queue is full.
@knative-prow
Copy link

knative-prow bot commented Apr 28, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso

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

@dprotaso dprotaso changed the title [wip] shakeout activator flake ServerlessService's initial operation mode is now Proxy Apr 28, 2022
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2022
psschwei
psschwei reviewed Apr 28, 2022
View reviewed changes
pkg/reconciler/autoscaling/kpa/kpa.go
Comment on lines -133 to -139
// We put activator in the serving path in the following cases:
// 1. The revision is scaled to 0:
// a. want == 0
// b. want == -1 && PA is inactive (Autoscaler has no previous knowledge of
// this revision, e.g. after a restart) but PA status is inactive (it was
// already scaled to 0).
// 2. The excess burst capacity is negative.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I wonder if we should keep this comment block (maybe move it to just before the switch statement)? It's a nice statement on when we want the activator in the path, (though I guess since we're defaulting to Proxy mode, it's maybe not the right want to frame it any more...

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I split this out into the three comments for the three cases - let me know if you think they need more clarity

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, no issues there, I think they're very clear.

psschwei
psschwei reviewed Apr 28, 2022
View reviewed changes
Copy link
Contributor

@psschwei psschwei left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2022
nader-ziada
nader-ziada reviewed Apr 28, 2022
View reviewed changes
Copy link
Member

@nader-ziada nader-ziada left a comment

Choose a reason for hiding this comment

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

looks good

@knative-prow knative-prow bot merged commit 95cfa70 into knative:main Apr 28, 2022
@dprotaso dprotaso deleted the activator-flake branch April 28, 2022 21:34
@psschwei psschwei mentioned this pull request May 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psschwei psschwei psschwei left review comments

@nader-ziada nader-ziada nader-ziada left review comments

Assignees

@psschwei psschwei

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Activator is removed from request path prematurely
4 participants
@dprotaso @nader-ziada @psschwei @knative-prow-robot