k8s: Introduce subscriber package to simplify & consolidate K8s watcher callbacks / event handling #15295

christarazi · 2021-03-10T05:44:53Z

This PR implements a new package which represents a subscriber to K8s watcher
events. This package simplifies the logic of K8s watchers and removes all the
unnecessary logic of the consumer (subscriber) from them.

In order for a subscriber to add their event handling to the K8s watcher, it
must use register itself. Depending on what kind of object it's subscribed to,
it must use implement the ServiceHandler interface if it's handling Service
objects, or use the "Raw" API where no such constraint on the object type
exists.

With this implementation, future subscribers can easily add their own logic
without worrying about polluting K8s watcher code with specific, irrelevant
details only really meant for them. Meanwhile, the K8s watchers themselves
simply call Notify*() depending on the event type. This notifies all the
subscribers to run their logic on the event.

In an upcoming PR, a BGP subscriber will utilize this infrastructure, all the
while, the core service watcher code remains unchanged.

This PR does not convert the rest of the Operator's watchers, but instead
provides the groundwork for future PRs.

See commit msgs.

watchers: Add subscriber package
operator/watchers: Convert service watcher to use subscriber

aanm

It's not entirely clear what are we gaining by introducing another queue to process k8s events. We had queues for k8s events in the past but we have removed them because they are redundant as the k8s watcher library already has a really good implementation to handle them.

operator/watchers/queue/queue.go

operator/watchers/queue/queue_test.go

operator/watchers/queue/queue.go

christarazi · 2021-03-11T05:28:54Z

@aanm I rewrote the PR to remove the use of the queue and instead just directly using a slice. Thanks for the suggestion

pkg/k8s/watchers/callbacks/callbacks.go

operator/watchers/service_cache.go

aditighag · 2021-03-25T21:17:53Z

operator/watchers/service_cache.go

+
+func (c *serviceCacheSubscriber) OnAdd(obj *slim_corev1.Service) {
+	log.WithField(logfields.ServiceName, obj.Name).Debugf("Received service addition %+v", obj)
+	K8sSvcCache.UpdateService(obj, c.swgSvcs)


K8sSvcCache.UpdateService sends the events to a buffered channel of some pre-configured capacity. Currently, we have only one buffered channel as K8sSvcCache.UpdateService is directly called from the watcher callback. But with this change there'll be a channel for every subscriber. Won't that increase memory usage of Cilium? Also, it looks like the code to parse the Service object, call correlateEndpoints, etc in K8sSvcCache.UpdateService be duplicated for every subscriber?

No, that's not the intent. I opened #15471 to further address subscribers that depend on the service cache itself. An example is explained there.

Generally , the solution wouldn't be to create a new channel or duplicate anything for that matter for each subscriber, but rather refactor the downstream subscribers that depend on the service cache itself.

aanm

As this is part of the BGP PR, do we still want to keep this PR open?

christarazi · 2021-03-29T17:57:44Z

@aanm I intended for the BGP PR to be based off this one. There's also lots of context inside this PR that might be worth keeping, so let's use this one

This commit implements a package called subscriber for the K8s resource / object watchers. It contains a list of subscribers to notify when handling a K8s add, update, or delete event, respectively. This allows the watchers to be simpler and leaner than they previously were. They no longer need to hold logic specific to the consumers. Instead, they call Notify*() inside the watcher callbacks depending on the event, passing the resource / object from the event. This notifies all the subscribers registered with Register(). Subscribers (also referred to as consumers or handlers) must implement either k8s.io/client-go/tools/cache.ResourceEventHandler and use the "Raw" API of callbacks such as (*RawList).Register(), where there's no strict contraint on the resource / object type, or implement the provided ServiceHandler and use the "Service" specific API, such as (*ServiceList).Register(). Consumers of the K8s Service watcher should implement the ServiceHandler. For now, only the raw and the service API is provided, due to strict necessity. More specific handlers can be added easily by defining a new interface similar to ServiceHandler, but differing on the object type. Signed-off-by: Chris Tarazi <chris@isovalent.com>

This commit converts the service watcher in the Operator to use the new subscriber package to handle K8s object events by defining a ServiceHandler. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi · 2021-03-31T06:34:45Z

test-me-please

aanm · 2021-03-31T14:42:06Z

hit #12891 merging

maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 10, 2021

maintainer-s-little-helper bot added this to In progress in 1.10.0 Mar 10, 2021

christarazi force-pushed the pr/christarazi/refactor-operator-service-watcher branch from 0d07946 to fb02f3c Compare March 10, 2021 05:49

christarazi requested a review from aanm March 10, 2021 05:50

maintainer-s-little-helper bot assigned aanm Mar 10, 2021

christarazi requested a review from brb March 10, 2021 05:50

maintainer-s-little-helper bot assigned brb Mar 10, 2021

christarazi requested review from a team March 10, 2021 05:50

christarazi marked this pull request as ready for review March 10, 2021 05:54

christarazi added the kind/enhancement This would improve or streamline existing functionality. label Mar 10, 2021

aanm requested changes Mar 10, 2021

View reviewed changes

operator/watchers/queue/queue.go Outdated Show resolved Hide resolved

operator/watchers/queue/queue_test.go Outdated Show resolved Hide resolved

operator/watchers/queue/queue.go Outdated Show resolved Hide resolved

operator/watchers/queue/queue.go Outdated Show resolved Hide resolved

christarazi force-pushed the pr/christarazi/refactor-operator-service-watcher branch from fb02f3c to f53283c Compare March 11, 2021 05:18

christarazi requested a review from aanm March 11, 2021 05:27

christarazi changed the title ~~operator: Introduce K8s watcher queue to decouple object monitoring and event processing~~ k8s: Introduce callbacks package to consolidate K8s watcher callbacks Mar 11, 2021

christarazi changed the title ~~k8s: Introduce callbacks package to consolidate K8s watcher callbacks~~ k8s: Introduce callbacks package to simplify & consolidate K8s watcher callbacks / event handling Mar 11, 2021

christarazi commented Mar 11, 2021

View reviewed changes

pkg/k8s/watchers/callbacks/callbacks.go Outdated Show resolved Hide resolved

pkg/k8s/watchers/callbacks/callbacks.go Outdated Show resolved Hide resolved

pkg/k8s/watchers/callbacks/callbacks.go Outdated Show resolved Hide resolved

aanm requested changes Mar 11, 2021

View reviewed changes

pkg/k8s/watchers/callbacks/callbacks.go Outdated Show resolved Hide resolved

pkg/k8s/watchers/callbacks/callbacks.go Outdated Show resolved Hide resolved

operator/watchers/service_cache.go Outdated Show resolved Hide resolved

aanm removed their assignment Mar 11, 2021

christarazi marked this pull request as draft March 11, 2021 17:32

christarazi force-pushed the pr/christarazi/refactor-operator-service-watcher branch from f53283c to 0c7fdd3 Compare March 15, 2021 06:29

christarazi mentioned this pull request Mar 15, 2021

Implement external IP (LoadBalancer) allocation & announcement via BGP for services #15340

Merged

5 tasks

christarazi force-pushed the pr/christarazi/refactor-operator-service-watcher branch from 0c7fdd3 to 8c62a3d Compare March 22, 2021 05:36

christarazi force-pushed the pr/christarazi/refactor-operator-service-watcher branch from a1d5ce0 to 3abdba6 Compare March 25, 2021 18:04

christarazi mentioned this pull request Mar 25, 2021

Refactor K8s watcher to use the new subscriber package #15471

Open

christarazi force-pushed the pr/christarazi/refactor-operator-service-watcher branch 2 times, most recently from 490679e to 44b6c52 Compare March 25, 2021 21:23

aditighag reviewed Mar 25, 2021

View reviewed changes

christarazi changed the title ~~k8s: Introduce callbacks package to simplify & consolidate K8s watcher callbacks / event handling~~ k8s: Introduce subscriber package to simplify & consolidate K8s watcher callbacks / event handling Mar 25, 2021

This comment has been minimized.

Sign in to view

christarazi force-pushed the pr/christarazi/refactor-operator-service-watcher branch from 44b6c52 to d2b4795 Compare March 28, 2021 05:50

This comment has been minimized.

Sign in to view

aanm requested changes Mar 29, 2021

View reviewed changes

aanm removed their assignment Mar 29, 2021

christarazi force-pushed the pr/christarazi/refactor-operator-service-watcher branch from d2b4795 to 9455269 Compare March 29, 2021 18:49

christarazi requested a review from aanm March 29, 2021 18:49

maintainer-s-little-helper bot assigned aanm Mar 29, 2021

christarazi added the release-blocker/1.10 label Mar 30, 2021

christarazi added 2 commits March 30, 2021 23:34

operator/watchers: Convert service watcher to use subscriber

2c5fa4a

This commit converts the service watcher in the Operator to use the new subscriber package to handle K8s object events by defining a ServiceHandler. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi force-pushed the pr/christarazi/refactor-operator-service-watcher branch from 9455269 to 2c5fa4a Compare March 31, 2021 06:34

aanm approved these changes Mar 31, 2021

View reviewed changes

aanm merged commit 117ec3c into cilium:master Mar 31, 2021

1.10.0 automation moved this from In progress to Done Mar 31, 2021

christarazi mentioned this pull request Apr 8, 2021

BGP followups #15611

Closed

11 tasks

This was referenced Apr 28, 2021

Prepare for release v1.10.0-rc1 #15896

Closed

Prepare for release v1.10.0-rc1 #15897

Merged

christarazi mentioned this pull request Sep 1, 2021

CiliumEndpointBatch support in cilium-agent #17207

Merged

krishgobinath mentioned this pull request Sep 9, 2021

Refactor CEP watch handlers using subscriber model. #17355

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

k8s: Introduce subscriber package to simplify & consolidate K8s watcher callbacks / event handling #15295

k8s: Introduce subscriber package to simplify & consolidate K8s watcher callbacks / event handling #15295

christarazi commented Mar 10, 2021 •

edited

aanm left a comment

christarazi commented Mar 11, 2021

aditighag Mar 25, 2021

christarazi Mar 25, 2021 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aanm left a comment

christarazi commented Mar 29, 2021

christarazi commented Mar 31, 2021

aanm commented Mar 31, 2021

k8s: Introduce subscriber package to simplify & consolidate K8s watcher callbacks / event handling #15295

k8s: Introduce subscriber package to simplify & consolidate K8s watcher callbacks / event handling #15295

Conversation

christarazi commented Mar 10, 2021 • edited

aanm left a comment

Choose a reason for hiding this comment

christarazi commented Mar 11, 2021

aditighag Mar 25, 2021

Choose a reason for hiding this comment

christarazi Mar 25, 2021 • edited

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aanm left a comment

Choose a reason for hiding this comment

christarazi commented Mar 29, 2021

christarazi commented Mar 31, 2021

aanm commented Mar 31, 2021

christarazi commented Mar 10, 2021 •

edited

christarazi Mar 25, 2021 •

edited