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.

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

revision-response-start-timeout-seconds is set wrongly #15616

Open
skonto opened this issue Nov 14, 2024 · 0 comments · May be fixed by #15617
Open

revision-response-start-timeout-seconds is set wrongly #15616

skonto opened this issue Nov 14, 2024 · 0 comments · May be fixed by #15617
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@skonto
Copy link
Contributor

skonto commented Nov 14, 2024

What version of Knative?

1.16.x and previous versions

Expected Behavior

When revision-timeout-seconds=revision-response-start-timeout-seconds in config defaults a revision without any timeouts should get 0 for revision-response-start-timeout-seconds.

Actual Behavior

revision-response-start-timeout-seconds is set wrongly when its default equals to the default timeoutSeconds in configmap defaults.

Now it gets 300 which is the default if the configmap values are not modified. However, when the user sets those values to be equal, he expects to allow any response time until the revision timeout expires not an arbitrary default value eg. 300.

Steps to Reproduce the Problem

In config defaults we set:

max-revision-timeout-seconds: "584"
revision-response-start-timeout-seconds: "582"
revision-timeout-seconds: "582"

Apply a service without any timeouts and then observe:

kubectl get revision autoscale-go-00001 -ojson | jq .spec.timeoutSeconds
582
kubectl get revision autoscale-go-00001 -ojson | jq .spec.responseStartTimeoutSeconds
300

Given the logic in revision defaults we should see 0 for responseStartTimeoutSeconds.

The reason this falls back to 300 is due to the wrong config store used.
Long ago we split config stores so that the configuration controller
and the revision controller can have a separate one. However the configuration_defaults code here calls the revision_default code here.
The revision defaulting code will get its own config values from here
cfg := config.FromContextOrDefaults(ctx), however that config store has not be initialized as this is the configuration controller and it only gets the default configmap values, thus the value 300.

A fix requires to inject the right values in the ctx as follows:

@@ -67,5 +75,19 @@ func (cs *ConfigurationSpec) SetDefaults(ctx context.Context) {
 			return
 		}
 	}
+
+	cfg := cconfig.FromContext(ctx)
+	cf := config.Config{}
+	if cfg != nil && cfg.Defaults != nil {
+		cf.Defaults = cfg.Defaults.DeepCopy()
+	}
+	if cfg != nil && cfg.Features != nil {
+		cf.Features = cfg.Features.DeepCopy()
+	}
+	if cfg != nil {
+		ctx = config.ToContext(ctx, &cf)
+	}
+
 	cs.Template.SetDefaults(ctx)
 }

To debug further you can use this branch.

@skonto skonto added the kind/bug Categorizes issue or PR as related to a bug. label Nov 14, 2024
@skonto skonto changed the title revision-response-start-timeout-seconds is set wrongly when its default equals to the default timeoutSeconds in configmap defaults revision-response-start-timeout-seconds is set wrongly Nov 14, 2024
@skonto skonto linked a pull request Nov 15, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant