k8s: modularize k8s watcher #32878

mhofstetter · 2024-06-04T11:25:51Z

Currently, the global k8swatcher is initialized in the daemon bootstrap function newDaemon.

With the modularization of all its dependencies into their own Hive Cell, it's about time to move the initialization of the k8sWatcher into its own Hive Cell too.

In a first step, the cell only provides the pre-initialized struct, without moving any of the lifecycle aspects into the Cell. For the time being, these are being kept in the daemon initialization (calls function on the k8sWatcher).

In addition the k8sWatcher has been split up into its smaller components/watchers (k8sEventReporter, k8sNamespaceWatcher, k8sPodWatcher, k8sServiceWatcher, k8sEndpointsWatcher, k8sCiliumNodeWatcher, k8sCiliumEndpointsWatcher, k8sCiliumLRPWatcher) - each defining its dependencies. This makes it possible to be dependent on only one specific watcher, which can help to avoid circular dependencies that can occur when depending on the whole k8sWatcher.

Please review the individual commits.

Note: +3,098 −2,824 seems huge, but this comes mostly from the overall-diff from moving / splitting up the watcher_test.go (2640 LoC) into watcher_test.go & service_test.go (with two commits two keep the history: rename & move one test function)

mhofstetter · 2024-06-04T11:34:38Z

/test

pippolo84

Thanks, just some minor comments left inline. 🚀

Regarding the overall approach, I'd say that the long term solution should be to get rid of the k8s watchers cell, as IMHO that is a not so useful layer over the single Resource[T] streams. Instead, we should go for separate well-defined and independent cells that take care of handling their type-specific events without injecting the big k8sWatchers cell in the daemon.
Moreover, IIRC some of these functionalities have been replaced by equivalent ones managed by Resource[T] internally (I'm thinking about the k8s objects related metrics).

Anyway, this PR seems still a nice improvement: it already allows to break some dependencies to the k8sWatcher in favor of the smaller ones, makes some tests easier and blends better with the current hive structure, so I'd say it's a step in the right direction 👍

pkg/k8s/watchers/cell.go

mhofstetter · 2024-06-05T12:08:38Z

Thanks for your fast feedback @pippolo84 . I addressed your feedback 👍

Regarding the overall approach, I'd say that the long term solution should be to get rid of the k8s watchers cell, as IMHO that is a not so useful layer over the single Resource[T] streams. Instead, we should go for separate well-defined and independent cells that take care of handling their type-specific events without injecting the big k8sWatchers cell in the daemon. Moreover, IIRC some of these functionalities have been replaced by equivalent ones managed by Resource[T] internally (I'm thinking about the k8s objects related metrics).

Anyway, this PR seems still a nice improvement: it already allows to break some dependencies to the k8sWatcher in favor of the smaller ones, makes some tests easier and blends better with the current hive structure, so I'd say it's a step in the right direction 👍

I couldn't agree more. That's exactly the intend behind this PR - to pave the way forward.

pippolo84

LGTM, thanks 💯

ldelossa

Nice modularity efforts!

I ran through each commit and the changes look good to me. For the most part this is all code-reorg with no major changes to how Cilium's business logic functions.

Looks good to me!

pkg/k8s/watchers/watcher.go

pkg/k8s/watchers/pod.go

aditighag

This makes it possible to be dependent on only one specific watcher, which can help to avoid circular dependencies that can occur when depending on the whole k8sWatcher.

Very nice improvement! 👍

High-level comment about the design of individual watcher cells: I wonder if there is an opportunity to use generics with interfaces to extract the common methods across all the watcher cells. The interface methods would be init, add/deleteResource[T], stop, etc. There is still lot of duplicated logic in the watcher cells. Wdyt?
(I understand that it's probably outside the scope of this PR.)

k8s: extract k8sCiliumLRPWatcher
k8s: extract k8sCiliumEndpointWatcher

LGTM with a minor improvement suggestion

pkg/k8s/watchers/cilium_local_redirect_policy.go

mhofstetter · 2024-06-07T06:12:43Z

rebased to main to resolve conflicts (without further changes)

derailed

@mhofstetter Nice clean up. Good work Marco!

Currently, the k8swatcher is initialized in the daemon bootstrap function `newDaemon`. With the modularization of all its dependencies into their own Hive Cell, it's about time to move the initialization of the k8sWatcher into its own Hive Cell too. In a first step, the cell only provides the pre-initialized struct, without moving any of the lifecycle aspects into the Cell. For the time being, these are being kept in the daemon initialization. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, k8s event reporting is part of the k8sWatcher. It's used by sub-watchers of the k8swatcher itself, but also by external watchers (e.g. IPAM watcher). As a first step to further modularize the k8swatcher into its smaller components, the k89s event reporting is extracted into an own cell and struct `k8sEventReporter`. This way, other components can depend on it. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, all the k8s watchers of the `k8sWatcher` are defined in the same struct, have access to all the same dependency fields and are provided as one Cell. This commit extracts the k8s Pod watcher into it's own sub-cell that is provided privately. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, all the k8s watchers of the `k8sWatcher` are defined in the same struct, have access to all the same dependency fields and are provided as one Cell. This commit extracts the k8s CiliumNode watcher into it's own sub-cell that is provided privately. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, all the k8s watchers of the `k8sWatcher` are defined in the same struct, have access to all the same dependency fields and are provided as one Cell. This commit extracts the k8s Namespace watcher into it's own sub-cell that is provided privately. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, all the k8s watchers of the `k8sWatcher` are defined in the same struct, have access to all the same dependency fields and are provided as one Cell. This commit extracts the k8s Service watcher into it's own sub-cell that is provided privately. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, all the k8s watchers of the `k8sWatcher` are defined in the same struct, have access to all the same dependency fields and are provided as one Cell. This commit extracts the k8s Endpoints watcher into it's own sub-cell that is provided privately. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, all the k8s watchers of the `k8sWatcher` are defined in the same struct, have access to all the same dependency fields and are provided as one Cell. This commit extracts the k8s CiliumLRP watcher into it's own sub-cell that is provided privately. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, all the k8s watchers of the `k8sWatcher` are defined in the same struct, have access to all the same dependency fields and are provided as one Cell. This commit extracts the k8s CiliumEndpoints watcher into it's own sub-cell that is provided privately. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, during daemon initialization, multiple components access the k8sSvcCache through the corresponding exported field in the k8sWatcher. This commit removes the field from the k8swatcher and forces the daemon to depend on the `k8sSvcCache` directly. In addition, some tests of the k8sWatcher would have been freed up from using the k8sWatcher at all, as they were only testing service logic. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, the file `watcher_test.go` mostly contains service related unit tests. Therefore, the file gets renamed to `service_test.go`. An upcoming commit will extract the only K8sWatcher related test into `watcher_test.go`. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

This commit extracts the k8sWatcher related unit test into it's own file `watcher_test.go`. (Separate commit to keep the git history). Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, the k8sWatcher is set as dependency on the nodeDiscovery during agent initialization by using the method `RegisterK8sSetters`. Trying to add an explicit dependency from the NodeDiscovery to the `K8sWatcher` results in a cyclic dependency via datapath. With the modularization of the k8sWatcher into smaller cells, it's possible to define the explicit dependency only to the `k8sCiliumNodeWatcher`, as this is the only part the NodeDiscovery is intersted in. This way, there's no cyclic dependency. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

With the removal of the k8swatcher initialization from the daemon bootstrap, the dependency to the policyUpdater can be removed from the daemon & daemonParams struct. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

mhofstetter · 2024-06-12T06:33:35Z

rebased to main to resolve conflicts

mhofstetter · 2024-06-12T06:49:38Z

/test

mhofstetter added kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. area/agent Cilium agent related. area/modularization Relates to code modularization and maintenance. labels Jun 4, 2024

github-actions bot added the sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. label Jun 4, 2024

mhofstetter marked this pull request as ready for review June 4, 2024 12:24

mhofstetter requested review from a team as code owners June 4, 2024 12:24

mhofstetter requested review from ldelossa, youngnick, pippolo84, aditighag and derailed June 4, 2024 12:24

pippolo84 requested changes Jun 5, 2024

View reviewed changes

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

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

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

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

mhofstetter force-pushed the pr/mhofstetter/k8s-watcher-cell branch from f3d0f01 to 51f497d Compare June 5, 2024 12:05

mhofstetter requested a review from pippolo84 June 5, 2024 12:29

pippolo84 approved these changes Jun 5, 2024

View reviewed changes

ldelossa approved these changes Jun 6, 2024

View reviewed changes

pkg/k8s/watchers/watcher.go Show resolved Hide resolved

pkg/k8s/watchers/pod.go Show resolved Hide resolved

aditighag approved these changes Jun 7, 2024

View reviewed changes

pkg/k8s/watchers/cilium_local_redirect_policy.go Show resolved Hide resolved

mhofstetter force-pushed the pr/mhofstetter/k8s-watcher-cell branch from 51f497d to c222dc3 Compare June 7, 2024 06:12

derailed approved these changes Jun 7, 2024

View reviewed changes

mhofstetter added 3 commits June 12, 2024 08:24

mhofstetter added 11 commits June 12, 2024 08:24

k8s: move init test to new watcher_test.go

a553423

This commit extracts the k8sWatcher related unit test into it's own file `watcher_test.go`. (Separate commit to keep the git history). Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

mhofstetter force-pushed the pr/mhofstetter/k8s-watcher-cell branch from c222dc3 to bf56cea Compare June 12, 2024 06:33

youngnick approved these changes Jun 12, 2024

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 12, 2024

dylandreimerink added this pull request to the merge queue Jun 12, 2024

Merged via the queue into cilium:main with commit b5ad1e4 Jun 12, 2024

mhofstetter deleted the pr/mhofstetter/k8s-watcher-cell branch June 12, 2024 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

k8s: modularize k8s watcher #32878

k8s: modularize k8s watcher #32878

Uh oh!

mhofstetter commented Jun 4, 2024 •

edited

Loading

Uh oh!

mhofstetter commented Jun 4, 2024

Uh oh!

pippolo84 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhofstetter commented Jun 5, 2024 •

edited

Loading

Uh oh!

pippolo84 left a comment

Uh oh!

ldelossa left a comment

Uh oh!

Uh oh!

Uh oh!

aditighag left a comment

Uh oh!

Uh oh!

mhofstetter commented Jun 7, 2024

Uh oh!

derailed left a comment

Uh oh!

mhofstetter commented Jun 12, 2024

Uh oh!

mhofstetter commented Jun 12, 2024

Uh oh!

Uh oh!

k8s: modularize k8s watcher #32878

k8s: modularize k8s watcher #32878

Uh oh!

Conversation

mhofstetter commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhofstetter commented Jun 4, 2024

Uh oh!

pippolo84 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhofstetter commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pippolo84 left a comment

Choose a reason for hiding this comment

Uh oh!

ldelossa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aditighag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhofstetter commented Jun 7, 2024

Uh oh!

derailed left a comment

Choose a reason for hiding this comment

Uh oh!

mhofstetter commented Jun 12, 2024

Uh oh!

mhofstetter commented Jun 12, 2024

Uh oh!

Uh oh!

mhofstetter commented Jun 4, 2024 •

edited

Loading

mhofstetter commented Jun 5, 2024 •

edited

Loading