Fix configuration timeout defaulting #15617
Conversation
knative-prow
bot
added
the
size/L
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15617 +/- ##
==========================================
- Coverage 80.80% 80.79% -0.01%
==========================================
Files 222 222
Lines 18034 18044 +10
==========================================
+ Hits 14572 14579 +7
- Misses 3090 3093 +3
Partials 372 372 ☔ View full report in Codecov by Sentry. |
|
@dprotaso gentle ping. |
| Name: config.DefaultsConfigName, | ||
| }, | ||
| Data: map[string]string{ | ||
| "revision-timeout-seconds": "423", |
There was a problem hiding this comment.
Why was 423 chosen as the timeout value? Consider using a named constant or variable to make the
intent clear and maintain consistency across test files.
There was a problem hiding this comment.
Just picked a number. I will update with a constant.
| }, | ||
| }, | ||
| }, | ||
| ctx: func() context.Context { |
There was a problem hiding this comment.
The context setup logic is duplicated between configuration_defaults_test.go and
revision_defaults_test.go. Consider extracting this into a test helper function to improve
maintainability.
There was a problem hiding this comment.
Actually they are using a different config store although the func seems similar, which makes things harder because configs are of different types and don't inherit similar methods. I will give it a shot.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: skonto The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
skonto
force-pushed
the
response_time_fix
branch
from
610bdc8 to
308d56c
Compare
|
cc @dprotaso |
|
/retest |
| configurationConfig := cconfig.FromContext(ctx) | ||
| apisConfig := config.Config{} | ||
| if configurationConfig != nil && configurationConfig.Defaults != nil { | ||
| apisConfig.Defaults = configurationConfig.Defaults.DeepCopy() | ||
| } | ||
| if configurationConfig != nil && configurationConfig.Features != nil { | ||
| apisConfig.Features = configurationConfig.Features.DeepCopy() | ||
| } | ||
| ctx = config.ToContext(ctx, &apisConfig) | ||
|
|
There was a problem hiding this comment.
configurationConfig := cconfig.FromContext(ctx)
This will always be nil - the webhook has a config map watcher that updates the api's config store.
There was a problem hiding this comment.
I think we need a different fix - but I haven't dug into why the default of 300 is being applied - when in fact the default is here and it should be using what's in the config map
serving/pkg/apis/config/defaults.go
Lines 140 to 146 in c3f2bfe
There was a problem hiding this comment.
I think this has only been an issue because before when we default we set all the properties - but this one setting is different
There was a problem hiding this comment.
The fix should be to add the apiconfig.Store into the context using the same key that's in the apiconfig package
eg. here
we should call
serving/pkg/apis/config/store.go
Line 69 in c3f2bfe
Other stores in reconciler could have similar issues
Fixes #15616
Proposed Changes
revision-response-start-timeout-seconds,revision-idle-timeout-seconds, so that when they are equal to therevision-timeout-secondsin the defaults cm, they will not be set to 300 (default) at a revision that has no overrides of those values. Instead they are set to nil at the revision (0at the QP side). Semantically this is correct, since when the timeouts are equal we don't need both anyway.Release Note