Pass config defaults into revision reconciler and add test case to verify #5892

taragu · 2019-10-28T20:22:38Z

/lint

Proposed Changes

Currently we are not passing config-defaults into revision reconciler. The result is we are calling SetDefaults(...) but only use the hard coded values in the code, instead of using what users have set in the config-defaults config map in their cluster.

By running 5296719 locally, I have verified that the config-defaults is not in the context of the revision reconciler.

Slack convo: https://knative.slack.com/archives/CA4DNJ9A4/p1572024996068100

/cc @nimakaviani

Release Note

NONE

knative-prow-robot · 2019-10-28T20:22:40Z

@taragu: GitHub didn't allow me to request PR reviews from the following users: nimakaviani.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/lint

Proposed Changes

Currently we are not passing config-defaults into revision reconciler. The result is we are calling SetDefaults(...) but only use the hard coded values in the code, instead of using what users have set in the config-defaults config map in their cluster.

By running 5296719 locally, I have verified that the config-defaults is not in the context of the revision reconciler.

Slack convo: https://knative.slack.com/archives/CA4DNJ9A4/p1572024996068100

/cc @nimakaviani

Release Note
NONE

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/test-infra repository.

knative-prow-robot

@taragu: 0 warnings.

In response to this:

/lint

Proposed Changes

Currently we are not passing config-defaults into revision reconciler. The result is we are calling SetDefaults(...) but only use the hard coded values in the code, instead of using what users have set in the config-defaults config map in their cluster.

By running 5296719 locally, I have verified that the config-defaults is not in the context of the revision reconciler.

Slack convo: https://knative.slack.com/archives/CA4DNJ9A4/p1572024996068100

/cc @nimakaviani

Release Note
NONE

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/test-infra repository.

nimakaviani · 2019-10-28T21:01:41Z

Thanks @taragu!

@mattmoor this PR follows our slack conversation on passing config-defaults to the controllers. wiring it into revision controller in the PR, since all the config are used in revision spec.

knative-test-reporter-robot · 2019-10-28T21:07:22Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests	pull-knative-serving-integration-tests	1/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

evankanderson · 2019-10-28T21:27:26Z

I don't see a new test case for this behavior (and presumably all the existing tests were passing). Can we add a test where a ConfigMap value is different than the hard-coded defaults which would have failed before this change?

nimakaviani · 2019-10-28T21:40:50Z

pkg/reconciler/revision/config/store_test.go

@@ -96,6 +106,7 @@ func TestStoreImmutableConfig(t *testing.T) {
 	store.OnConfigChanged(ConfigMapFromTestFile(t, pkgmetrics.ConfigMapName()))
 	store.OnConfigChanged(ConfigMapFromTestFile(t, logging.ConfigMapName()))
 	store.OnConfigChanged(ConfigMapFromTestFile(t, pkgtracing.ConfigName))
+	store.OnConfigChanged(ConfigMapFromTestFile(t, apisconfig.DefaultsConfigName))


we should test for immutability of defaults config object too similar to whats done below:

serving/pkg/reconciler/revision/config/store_test.go

Lines 113 to 115 in 18933ce

config.Deployment.QueueSidecarImage = "mutated"

config.Network.IstioOutboundIPRanges = "mutated"

config.Logging.LoggingConfig = "mutated"

nimakaviani · 2019-10-28T21:53:15Z

@evankanderson yup!

@taragu looking at the code, it totally makes sense to add a unit test under revision_test.go to verify the update behavior.

taragu · 2019-10-31T13:03:38Z

/assign @markusthoemmes
Could you take a look when you get a chance?

taragu · 2019-11-12T14:30:41Z

@evankanderson @markusthoemmes just a friendly ping :)

mattmoor

Can we do this for all of the controllers?

taragu · 2019-11-12T14:37:35Z

@mattmoor yes! We should do this for all the controllers. I will submit follow up PRs for them.

mattmoor

/lgtm
/approve

knative-prow-robot · 2019-11-12T14:39:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, taragu

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:

~~pkg/reconciler/OWNERS~~ [mattmoor]

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

Tara Gu added 2 commits October 28, 2019 11:06

Pass config defaults into revision reconciler; add test case to verify

7f80241

add testdata

dee85c3

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 28, 2019

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 28, 2019

knative-prow-robot reviewed Oct 28, 2019

View reviewed changes

knative-prow-robot added the area/API API objects and controllers label Oct 28, 2019

Update existing tests

18933ce

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 28, 2019

nimakaviani reviewed Oct 28, 2019

View reviewed changes

Review: add test case for updating config-defaults

5bb421f

taragu force-pushed the passing-config-defaults branch from 432b2bc to 5bb421f Compare October 29, 2019 15:03

knative-prow-robot assigned markusthoemmes Oct 31, 2019

mattmoor reviewed Nov 12, 2019

View reviewed changes

knative-prow-robot assigned mattmoor Nov 12, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2019

knative-prow-robot merged commit be87d5f into knative:master Nov 12, 2019

taragu deleted the passing-config-defaults branch February 18, 2020 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass config defaults into revision reconciler and add test case to verify #5892

Pass config defaults into revision reconciler and add test case to verify #5892

taragu commented Oct 28, 2019

knative-prow-robot commented Oct 28, 2019

Proposed Changes

knative-prow-robot left a comment

nimakaviani commented Oct 28, 2019

knative-test-reporter-robot commented Oct 28, 2019

evankanderson commented Oct 28, 2019

nimakaviani Oct 28, 2019

nimakaviani commented Oct 28, 2019

taragu commented Oct 31, 2019

taragu commented Nov 12, 2019

mattmoor left a comment

taragu commented Nov 12, 2019

mattmoor left a comment

knative-prow-robot commented Nov 12, 2019

	config.Deployment.QueueSidecarImage = "mutated"
	config.Network.IstioOutboundIPRanges = "mutated"
	config.Logging.LoggingConfig = "mutated"

Pass config defaults into revision reconciler and add test case to verify #5892

Pass config defaults into revision reconciler and add test case to verify #5892

Conversation

taragu commented Oct 28, 2019

Proposed Changes

knative-prow-robot commented Oct 28, 2019

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

nimakaviani commented Oct 28, 2019

knative-test-reporter-robot commented Oct 28, 2019

evankanderson commented Oct 28, 2019

nimakaviani Oct 28, 2019

Choose a reason for hiding this comment

nimakaviani commented Oct 28, 2019

taragu commented Oct 31, 2019

taragu commented Nov 12, 2019

mattmoor left a comment

Choose a reason for hiding this comment

taragu commented Nov 12, 2019

mattmoor left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Nov 12, 2019