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

Pull NetworkConfig logic out of Revision controller #1242

Closed
mattmoor opened this issue Jun 15, 2018 · 4 comments
Closed

Pull NetworkConfig logic out of Revision controller #1242

mattmoor opened this issue Jun 15, 2018 · 4 comments
Assignees
Labels
area/API API objects and controllers area/networking kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@mattmoor
Copy link
Member

I'd rather have the controllers' use of informers focused on reconciliation. In the case of the NetworkConfig, I feel like we could do this by:

  1. Exposing SetNetworkConfig on the revision controller
  2. In cmd/controller/main.go doing something like:
    configMapInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
        UpdateFunc: func(_, obj interface{}) {
            configMap := obj.(*corev1.ConfigMap)
            if isNetworkConfig(configMap) {
                revisionController.SetNetworkConfig(NewNetworkConfigFromConfigMap(configMap))
                return
            }
            // handle other configmaps being routed to other controllers
        })
@google-prow-robot google-prow-robot added area/API API objects and controllers area/networking kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jun 15, 2018
@mattmoor
Copy link
Member Author

DomainConfig uses a similar pattern.

I have some thoughts on what this might look like, so I may take a look at it this week (if @mdemirhan has not already started on it).

/unassign @mdemirhan
/assign @mattmoor

@mattmoor
Copy link
Member Author

cc @josephburnett

It sounds (even from the docs) like ConfigMap updates through volumes can be very laggy:

As a result, the total delay from the moment when the ConfigMap is updated to the moment when new keys are projected to the pod can be as long as kubelet sync period + ttl of ConfigMaps cache in kubelet.

tl;dr I think I'd propose a simple informer-based configuration model.

I think we'd have something like pkg/configurationmanager (I'll resist the urge to name this pkg/conformer), which exposes:

package configurationmanager

type Watcher func(*corev1.ConfigMap)

type Interface interface {
   Watch(name string, cb Watcher)
}

func New(informer) Interface {...}

Then Controller's would watch the ConfigMaps that they care about:

package route

func New(..., cm configurationmanager.Interface) controller.Interface {
   ...
   // Parse the ConfigMap into DomainConfig, see what's changed and do what setDomainConfig does today.
   cm.Watch("domain-config", c.receiveDomainConfig)
}
package revision

func New(..., cm configurationmanager.Interface) controller.Interface {
   ...
   // Parse the ConfigMap into DomainConfig, see what's changed and do what setDomainConfig does today.
   cm.Watch("network-config", c.receiveNetworkConfig)
}

This is in essence a generalization of what exists through informers directly today, but hides some of that complexity behind a higher-level interface (that we can fake for testing).

This model should avoid the lag of depending on the ConfigMap volumes, and also enable us to be aware of when we've picked up updates to trigger additional work. For example, when we pick up new sidecar images, we may want to trigger a global reconciliation to roll it out (assuming a world where we reconcile the underlying Deployment).

@vaikas
Copy link
Contributor

vaikas commented Jun 25, 2018

Yep, this makes sense to me.

@mattmoor
Copy link
Member Author

I put together the linked PR that implements what's outlined above for {Network,Domain}Config.

Ideally we'd have a single method for handling our Controller configs (including config-autoscaler, config-observability and config-logging), so please consider this through that lens.

google-prow-robot pushed a commit that referenced this issue Jun 27, 2018
* Refactor the way our Controllers watch ConfigMaps

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: #1242

* Drop the kubeclient-based constructors.

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

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

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

* Incorporate feedback from @dprotaso

split Start into a handful of descriptively named helpers.

Don't Start in a go routine, since it doesn't block on SharedInformerFactory.
skonto added a commit to skonto/serving that referenced this issue Sep 21, 2022
skonto added a commit to skonto/serving that referenced this issue Sep 26, 2022
mgencur pushed a commit to openshift-knative/serving that referenced this issue Oct 12, 2022
nak3 pushed a commit to nak3/serving that referenced this issue Nov 10, 2022
nak3 pushed a commit to nak3/serving that referenced this issue Nov 10, 2022
openshift-merge-robot pushed a commit to openshift-knative/serving that referenced this issue Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers area/networking kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

No branches or pull requests

4 participants