-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
PodAutoscaler Active Condition should not affect Reachability #14309
Conversation
Besides the feedback loop with active - reachability is also set to unreachable when the revision's deployment is known to be in a failure state (eg. progress deadline hit/imagepullbackoff) I'm not sure that's necessary because it seems like reachability just affects scale bounds and indirectly KPA condition status |
0980a96
to
d8d6166
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #14309 +/- ##
==========================================
- Coverage 86.16% 86.12% -0.04%
==========================================
Files 195 196 +1
Lines 14702 14787 +85
==========================================
+ Hits 12668 12736 +68
- Misses 1729 1745 +16
- Partials 305 306 +1
☔ View full report in Codecov by Sentry. |
@dprotaso I have taken your PR and tested the problematic scenario and simple scaleUp and scaleFrom/ToZero. Is all working. I can close my PR if consensus exists to go your route. Thanks for checking. |
d8d6166
to
04d60a5
Compare
04d60a5
to
6e94600
Compare
/assign @nak3 @ReToCode @skonto cc @jsanin-vmw who's tackling a similar issue where pods don't scale down when we don't have metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment otherwise LGTM.
/lgtm Thank you! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, nak3 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 |
Fixes #14115
PodAutoscaler.Spec.Reachability
was added a long time ago. Since then we've added aScaleTargetInitialized
condition to make it easier to know if we've scaled up from zero successfully - and this simplified various condition checks in the autoscaler etc.Given that the
PodAutoscaler's
Active
condition would toggleReachability
as @SaschaSchwarze0 pointed out.I believe we don't need this circular logic - reachability should be set depending on the revision's routing state with the exception of failing to progress the deployment/start a users container.