Add configurable probe timeout and frequency for activator

[bindrad]

Fixes #16249

Proposed Changes

• Add PROBE_TIMEOUT environment variable to configure activator health check timeout (default: 300ms)
• Add PROBE_FREQUENCY environment 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.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: bindrad / name: Dhruv Bindra (45e141b, 51b3c60)

[knative-prow]

[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.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[knative-prow]

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.

[linkvt]

/ok-to-test

[bindrad]

/test unit-tests

[bindrad]

I'm not sure if istio-latest-no-mesh-tls_serving_main is failing due to my changes. Please suggest.

[linkvt]

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

[linkvt]

@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.

[bindrad]

@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]

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.10%. Comparing base (a94c607) to head (45e141b).
⚠️ Report is 33 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/activator/net/throttler.go	0.00%	2 Missing ⚠️

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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dprotaso]

I'd like more flexibility here to provide a golang duration string and parse it using https://pkg.go.dev/time#ParseDuration to prase

[dprotaso]

Otherwise not having the unit in the name is confusing

[dprotaso]

Where and when is probeFrequency changed on a per revision basis?

If we're not changing things per-revision we can save ourselves all this extra code plumbing these fields down and maybe just use the constants that can be overridden by the env var