Add configurable probe timeout and frequency for activator
Fixes #16249
Proposed Changes
- Add
PROBE_TIMEOUTenvironment variable to configure activator health check timeout (default: 300ms) - Add
PROBE_FREQUENCYenvironment variable to configure activator probe frequency (default: 200ms)
Release Note
Activator probe timeout and frequency are now configurable via PROBE_TIMEOUT and PROBE_FREQUENCY environment variables.
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: bindrad / name: Dhruv Bindra (45e141bae50885fa71a0ca48300716a19bd8c891, 51b3c60685a8c505d87e9bdf167871b75e10217f)
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: bindrad Once this PR has been reviewed and has the lgtm label, please assign skonto for approval. For more information see the Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Hi @bindrad. Thanks for your PR.
I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
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-sigs/prow repository.
/ok-to-test
/test unit-tests
I'm not sure if istio-latest-no-mesh-tls_serving_main is failing due to my changes. Please suggest.
It's likely a flaky test indicated by a webhook deployment not being ready in the E2E test setup. Let's do a simple retest.
/retest
@bindrad Seems like the flaky test finished without errors.
I just reviewed the code and think it might make sense to only set/overwrite these values through the config-network ConfigMap instead of allowing an overwrite via environment variables. This would seem more in line with how we configure other parameters, I guess the two fields above (MaxIdleProxyConns, ...) are rather outliers.
Another idea would be a new ConfigMap config-activator, this is IMO too much overhead so I wouldn't do it.
I'm not a long-term contributor/approver though so someone else might have a better idea.
@linkvt Thanks for the feedback. I thought to follow the same pattern as MaxIdleProxyConns and MaxIdleProxyConnsPerHost, which are also set via environment variables. I'm fine with either approach, but I think it would be useful to make these values configurable in some form.
Codecov Report
:x: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.10%. Comparing base (a94c607) to head (45e141b).
:warning: Report is 33 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| pkg/activator/net/throttler.go | 0.00% | 2 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #16250 +/- ##
==========================================
+ Coverage 80.02% 80.10% +0.07%
==========================================
Files 215 215
Lines 13327 13328 +1
==========================================
+ Hits 10665 10676 +11
+ Misses 2299 2293 -6
+ Partials 363 359 -4
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.