Refactor the way our Controllers watch ConfigMaps by mattmoor · Pull Request #1351 · knative/serving

mattmoor · 2018-06-26T05:05:35Z

Previously the Revision and Route controllers used a ConfigMap informer directly to watch for changes. This generalizes that pattern into a slightly higher level abstraction under pkg/configurationmanager.

With this configurationmanager.Interface abstraction, Controllers simply register to Watch particular ConfigMaps for changes via a callback:

  cm.Watch("configmap-name", c.receiveSomeConfig)

Once Start() is called on the configurationmanager.Interface all of the registered watchers will be invoked with the initial state of their configs, and if any are missing the call to Start() will return an error. This consolidates the setup and update logic for the various configurations behind a single interface.

This transitions the NetworkConfig and DomainConfig logic, which already employ this pattern (sans abstraction) to this abstraction.

Fixes: #1242

dprotaso · 2018-06-26T15:19:57Z

I'd rather this package be called configuration and this interface be called Watcher

What about configmap for the package, Interface => Watcher and Watcher => Observer?

I'd like to avoid confusion with our Configuration resources. Yay, naming!

sounds good

I went with this, but also renamed New[Fixed] => New{Default,Fixed}Watcher for clarity.

dprotaso · 2018-06-26T15:28:03Z

We can follow the client-go convention and not perform a DeepCopy - but I would expect each controller storing said ConfigMap to perform a DeepCopy themselves.

It would be worth documenting this on the configurationmanager interface

mdemirhan · 2018-06-26T15:54:54Z

Might be useful to add a check to see whether this instance is already started or not and return an error in case it is already started.

mdemirhan · 2018-06-26T16:03:53Z

I like the wording here :-) But might since we are logging below and since we are going to be public soon, might be better off to remove this line.

My debugging snuck in! Will remove, good catch. 😊

mdemirhan · 2018-06-26T17:02:38Z

/lgtm
/approve

Previously the Revision and Route controllers used a ConfigMap informer directly to watch for changes. This generalizes that pattern into a slightly higher level abstraction under `pkg/configmap`. With this `configmap.Watcher` abstraction, Controllers simply register to `Watch` particular `ConfigMap`s for changes via a callback: ```go cm.Watch("configmap-name", c.receiveSomeConfig) ``` Once `Start()` is called on the `configmap.Watcher` all of the registered `configmap.Observers` will be invoked with the initial state of their `ConfigMap`s, and if any are not found the call to `Start()` will return an error. This consolidates the setup and update logic for the various configurations behind a single interface. This transitions the `NetworkConfig` and `DomainConfig` logic, which already employ this pattern (sans abstraction) to this abstraction. Fixes: knative/serving#1242

Rename `network_config.go` to have a consistent filename with `domainconfig.go`.

Also renamed defaultImpl's `watchers` field to `observers` to be consistent with the original intent.

mattmoor · 2018-06-26T20:48:28Z

/test pull-knative-serving-go-coverage

steuhs · 2018-06-26T23:15:22Z

/test pull-knative-serving-go-coverage

steuhs · 2018-06-26T23:46:57Z

/test pull-knative-serving-go-coverage

steuhs · 2018-06-27T00:02:05Z

/test pull-knative-serving-go-coverage

dprotaso · 2018-06-27T03:19:42Z

It looks like this doesn't block so no need for go

https://github.com/kubernetes/client-go/blob/master/informers/factory.go#L124

Unless you meant to start the di.informer explicitly?

In that case you can remove the shared informer factory sif from defaultImpl

Interesting. I copied what we do in ./cmd/controller/main.go, which copied: https://github.com/kubernetes/sample-controller/blob/master/main.go#L68-L69.

TIL.

dprotaso · 2018-06-27T03:22:17Z

For readability assign the lambda to a some var

ie.

startInformerFunc := func() error { ... } if err := startInformerFunc(); err != nil { ... }

or maybe it just simply warrants a separate function

I split this into a couple sub-functions which I definitely thinks helps this self-document a bit better.

dprotaso · 2018-06-27T03:33:14Z

Based on prior comments it seems like you could just take in just a single ConfigMapInformer

Given that the Watch method only takes a name (read: no namespace), I'd like the interface for constructing a Watcher to bear the responsibility of filtering to a single namespace, which AFAIK is only possibly if we own the construction of the SharedInformerFactory that creates the informer.

split Start into a handful of descriptively named helpers. Don't Start in a go routine, since it doesn't block on SharedInformerFactory.

knative-metrics-robot · 2018-06-27T14:10:04Z

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/configmap/default.go	File not exists	97.3%
pkg/configmap/fixed.go	File not exists	100.0%
pkg/configmap/interface.go	File not exists	100.0%
pkg/controller/revision/networkconfig.go	File not exists	100.0%
pkg/controller/route/route.go	78.9%	78.7%	-0.2
pkg/controller/route/domainconfig.go	96.9%	96.4%	-0.4
pkg/controller/revision/revision.go	72.1%	71.8%	-0.4

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

mattmoor · 2018-06-27T15:05:23Z

Going to remove WIP. I'd started to migrate the Controller usage of /etc/config-foo, but I'll do that in a follow-up PR as it was growing unwieldy and could use some refactoring to make that migration more manageable (e.g. pkg/logging shouldn't directly depend on /etc/config-logging existing).

(my current thinking)
I will likely expand the surface of pkg/configmap to include:

// Load reads a mounted ConfigMap from disk, returns the equivalent of it's `Data` field.
func Load(path string) (map[string]string, error) {...}

Then as I refactor the various parts of ControllerConfig into things like logging.Config I can expose constructors that take the corev1.ConfigMap (for use with Watch), or its .Data field (for use with Load, and delegated to by the ConfigMap variant).

mdemirhan · 2018-06-27T15:53:22Z

/lgtm
/approve

google-prow-robot · 2018-06-27T15:53:27Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [mattmoor]

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

mattmoor · 2018-06-27T15:59:40Z

/hold remove

mattmoor · 2018-06-27T15:59:53Z

/hold cancel

mattmoor assigned grantr, mdemirhan and vaikas Jun 26, 2018

dprotaso reviewed Jun 26, 2018

View reviewed changes

mdemirhan approved these changes Jun 26, 2018

View reviewed changes

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

mattmoor force-pushed the config-manager branch from 3198c12 to 5795e19 Compare June 26, 2018 18:00

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

mattmoor force-pushed the config-manager branch 2 times, most recently from a6ceb7a to 58817da Compare June 26, 2018 19:57

mattmoor added 3 commits June 26, 2018 20:07

Drop the kubeclient-based constructors.

26851b9

Rename `network_config.go` to have a consistent filename with `domainconfig.go`.

Check that configmap.defaulImpl.Start is only called once.

e50e5c9

Also renamed defaultImpl's `watchers` field to `observers` to be consistent with the original intent.

mattmoor force-pushed the config-manager branch from 58817da to e50e5c9 Compare June 26, 2018 20:08

dprotaso reviewed Jun 27, 2018

View reviewed changes

google-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 27, 2018

mattmoor mentioned this pull request Jun 27, 2018

Do not log in log library #1364

Closed

mattmoor force-pushed the config-manager branch from ecbb538 to e50e5c9 Compare June 27, 2018 13:57

google-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 27, 2018

Incorporate feedback from @dprotaso

b90d1ad

split Start into a handful of descriptively named helpers. Don't Start in a go routine, since it doesn't block on SharedInformerFactory.

mattmoor changed the title ~~[WIP] Refactor the way our Controllers watch ConfigMaps~~ Refactor the way our Controllers watch ConfigMaps Jun 27, 2018

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

mattmoor removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2018

google-prow-robot merged commit 0999d92 into knative:master Jun 27, 2018

mattmoor deleted the config-manager branch June 27, 2018 16:08

dprotaso mentioned this pull request Jun 19, 2019

Add @dprotaso as a serving approver #4426

Merged

Conversation

mattmoor commented Jun 26, 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!

Choose a reason for hiding this comment

Uh oh!

mdemirhan commented Jun 26, 2018

Uh oh!

mattmoor commented Jun 26, 2018

Uh oh!

steuhs commented Jun 26, 2018

Uh oh!

steuhs commented Jun 26, 2018

Uh oh!

steuhs commented Jun 27, 2018

Uh oh!

dprotaso Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 27, 2018

Uh oh!

mattmoor commented Jun 27, 2018

Uh oh!

mdemirhan commented Jun 27, 2018

Uh oh!

google-prow-robot commented Jun 27, 2018

Uh oh!

mattmoor commented Jun 27, 2018

Uh oh!

mattmoor commented Jun 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dprotaso Jun 27, 2018 •

edited

Loading