Refactor the Configuration controller to use level-based reconciliation. by mattmoor · Pull Request #1202 · knative/serving

mattmoor · 2018-06-14T04:21:08Z

Based on the guidance from today's working group meeting, this starts to move our controllers towards more conventional reconciliation. Previously, we did work in the callbacks from informers directly. With this, we still listen to these secondary events, but when they occur instead of taking immediate action, we instead queue a reconciliation of any primary resources with which they are associated.

In simpler terms, instead of having the Configuration controller take direct action when Revision events occur we instead queue a reconciliation of the Controller that created it. This core reconciliation loop is now responsible for 100% of the logic so there are changes there as well to accommodate this shift.

Progress towards: #823

knative-metrics-robot · 2018-06-14T04:23:12Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/configuration_types.go	97.7%	97.7%	0.1
pkg/apis/serving/v1alpha1/configuration_types_test.go	97.7%	97.7%	0.1
pkg/controller/configuration/configuration.go	77.7%	75.8%	-1.9
pkg/controller/configuration/configuration_test.go	77.7%	75.8%	-1.9
pkg/controller/route/route.go	78.0%	77.9%	-0.1
pkg/controller/revision/revision.go	80.4%	80.6%	0.1
pkg/controller/revision/revision_test.go	80.4%	80.6%	0.1

*TestCoverage feature is being tested, do not rely on any info here yet

mattmoor · 2018-06-14T04:23:32Z

This is WIP because the baseline change (#1198) needs to land first.

mattmoor · 2018-06-14T04:27:13Z

@smarterclayton I hand rolled this based on spelunking and guesswork. If there are useful utilities for this I'd appreciate any pointers.

knative-metrics-robot · 2018-06-14T04:38:43Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/configuration_types.go	97.7%	97.7%	0.1
pkg/apis/serving/v1alpha1/configuration_types_test.go	97.7%	97.7%	0.1
pkg/controller/configuration/configuration.go	78.3%	75.8%	-2.5
pkg/controller/configuration/configuration_test.go	78.3%	75.8%	-2.5

*TestCoverage feature is being tested, do not rely on any info here yet

knative-metrics-robot · 2018-06-14T04:43:18Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/configuration_types.go	97.7%	97.7%	0.1
pkg/apis/serving/v1alpha1/configuration_types_test.go	97.7%	97.7%	0.1
pkg/controller/configuration/configuration.go	78.3%	75.8%	-2.5
pkg/controller/configuration/configuration_test.go	78.3%	75.8%	-2.5

*TestCoverage feature is being tested, do not rely on any info here yet

vaikas · 2018-06-14T04:43:48Z

can we construct this from one of the api directories instead of hard coding?

vaikas · 2018-06-14T04:44:19Z

nit: The no longer
Configuration no longer...

vaikas · 2018-06-14T04:45:38Z

s/looking up Revision/failed to Get Revision/
? Not a big deal but seems more readable.

vaikas · 2018-06-14T04:52:44Z

I think you can get rid of this and use:
https://github.com/kubernetes/client-go/blob/master/tools/cache/controller.go#L254

That's what I copied this from. Since I want the Obj that is embedded as well (to determine its controller below) I inlines and adapted that same delete-handling logic.

I definitely want something better to use here. I think that my bias would be to actually rip out a lot of the complexity of this function (which we don't need yet) unless there is something better to use.

Based on the guidance from Clayton and others at today's working group meeting, this starts to move our controllers towards more conventional reconciliation. Previously, we did work in the callbacks from informers directly. With this, we still listen to these secondary events, but when they occur instead of taking immediate action, we instead queue a reconciliation of any primary resources with which they are associated. In simpler terms, instead of having the Configuration controller take direct action when Revision events occur we instead queue a reconciliation of the Controller that created it. This core reconciliation loop is now responsible for 100% of the logic so there are changes there as well to accommodate this shift.

knative-metrics-robot · 2018-06-14T12:04:09Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/configuration_types.go	97.7%	97.7%	0.1
pkg/apis/serving/v1alpha1/configuration_types_test.go	97.7%	97.7%	0.1
pkg/controller/configuration/configuration.go	78.3%	78.9%	0.7
pkg/controller/configuration/configuration_test.go	78.3%	78.9%	0.7

*TestCoverage feature is being tested, do not rely on any info here yet

mattmoor · 2018-06-14T12:35:29Z

/assign @vaikas-google
/assign @grantr

vaikas · 2018-06-14T14:53:05Z

/lgtm
/approve

google-prow-robot · 2018-06-14T14:53:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vaikas-google

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/apis/OWNERS~~ [mattmoor,vaikas-google]
~~pkg/controller/OWNERS~~ [mattmoor,vaikas-google]

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

knative-metrics-robot · 2018-06-14T14:55:17Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/configuration_types.go	97.7%	97.7%	0.1
pkg/apis/serving/v1alpha1/configuration_types_test.go	97.7%	97.7%	0.1
pkg/controller/configuration/configuration.go	78.3%	74.7%	-3.5
pkg/controller/configuration/configuration_test.go	78.3%	74.7%	-3.5

*TestCoverage feature is being tested, do not rely on any info here yet

This moves the service controller to the more level-based model started in #1202. This also makes the service controller start to listen to Route and Configuration events to trigger reconciliation, so that we can reflect a basic overall readiness condition based on the combination of these two sub-resources readiness conditions (I don't expect this will be the final model, but is a start). Fixes: #1134

grantr

LGTM. Nice improvement, thanks!

…ase/artifacts` (knative#1202) * Add secret to 1.4 CI yaml * auto generated

…ase/artifacts` (knative#1202) * Add secret to 1.5 CI yaml * auto generated

…ically (knative#1236) * [RELEASE-v1.5] Add manifest patch for internal-tls to `openshift/release/artifacts` (knative#1202) * Add secret to 1.5 CI yaml * auto generated * Support config to deploy internal certificates automatically (knative#13005) * Add certificate reconciler for internal certs * Fix cert path * Temporary use local networking repo * Support internal-encryption configuration * Use const for cert name * Fix lint * rm blank line * Drop unused variable * Use one line style * Use one line code * Update net-kourier nightly bumping knative.dev/net-kourier d758682...b9b1e8b: > b9b1e8b Use `internal-encryption` to deploy internal certificates automatically (# 855) > 427434c bump kind and k8s versions in kind-e2e tests (# 859) Signed-off-by: Knative Automation <automation@knative.team> * Verify SecretPKKey as well * Do not drop activator always in the path * Comment about ctrl-ca suffix Co-authored-by: Knative Automation <automation@knative.team> * Update deps * Enable internal-tls on ocp-tls (knative#1203) * Enable internal-tls on OCP 4.8 * Use tls to match JOB name * Add a target to enable internal-tls in Makefile (knative#1224) * Add a target to enable internal-tls in Makefile * Update CI template for internal-tls enabled * Tests for encryption with Kourier local gateway (knative#13263) * Generate Secrets * Commit generated cert-secret.yaml * httpproxy enables tls client * httpproxy uses https when CA_CERT specified * Pass CA_CERT and SERVER_NAME env variables properly to tests * Avoid using cluster-local certificates for external services * Enable tls tests for cluster-local Kourier gateway * Need to create test resources including the test namespace first before installing Knative so that applying test/config/tls/cert-secret.yaml succeeds * TMP: Enable tls in the standard e2e make target - test purposes * Use knative-serving-ingress ns for deploying server-certs * Deploy certificates at test phase * Separate test and install of installing certs * Wait for knative-serving-ingress to exist * Revert "TMP: Enable tls in the standard e2e make target - test purposes" This reverts commit 5bb3549. Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com> Co-authored-by: Knative Automation <automation@knative.team>

mattmoor commented Jun 14, 2018

View reviewed changes

mattmoor force-pushed the get-thru-informer branch from 7822953 to caac408 Compare June 14, 2018 04:37

google-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 14, 2018

mattmoor force-pushed the get-thru-informer branch from caac408 to 5f0293e Compare June 14, 2018 04:41

vaikas reviewed Jun 14, 2018

View reviewed changes

mattmoor force-pushed the get-thru-informer branch from 5f0293e to 8b3a616 Compare June 14, 2018 12:01

mattmoor changed the title ~~[WIP] Refactor the Configuration controller to use level-based reconciliation.~~ Refactor the Configuration controller to use level-based reconciliation. Jun 14, 2018

google-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2018

google-prow-robot assigned grantr and vaikas Jun 14, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2018

google-prow-robot merged commit 56c7671 into knative:master Jun 14, 2018

mattmoor deleted the get-thru-informer branch June 14, 2018 15:49

grantr reviewed Jun 14, 2018

View reviewed changes

nak3 added a commit to nak3/serving that referenced this pull request Aug 5, 2022

[RELEASE-v1.4] Add manifest patch for internal-tls to `openshift/rele…

9647dc1

…ase/artifacts` (knative#1202) * Add secret to 1.4 CI yaml * auto generated

mgencur pushed a commit to mgencur/serving-1 that referenced this pull request Sep 6, 2022

[RELEASE-v1.5] Add manifest patch for internal-tls to `openshift/rele…

d54ab0e

…ase/artifacts` (knative#1202) * Add secret to 1.5 CI yaml * auto generated

Conversation

mattmoor commented Jun 14, 2018

Uh oh!

knative-metrics-robot commented Jun 14, 2018

Uh oh!

mattmoor commented Jun 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Jun 14, 2018

Uh oh!

knative-metrics-robot commented Jun 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Jun 14, 2018

Uh oh!

mattmoor commented Jun 14, 2018

Uh oh!

vaikas commented Jun 14, 2018

Uh oh!

google-prow-robot commented Jun 14, 2018

Uh oh!

knative-metrics-robot commented Jun 14, 2018

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants