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

EndpointManager and NodeManager Cells #21746

Merged

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Oct 16, 2022

This PR adds cells for EndpointManager and NodeManager (cilium-agent objects snippet):

          Ⓜ️ endpoint-manager (Manages the collection of local endpoints):
              ⚙️ (endpointmanager.EndpointManagerConfig) {
                  EndpointGCInterval: (time.Duration) 5m0s
              }

              🚧 endpointmanager.newDefaultEndpointManager (cell.go:177):
                  ⇨ client.Clientset, endpointmanager.EndpointManagerConfig, hive.Lifecycle
                  ⇦ endpointmanager.EndpointManager, endpointmanager.EndpointsLookup, 
                      endpointmanager.EndpointsModify

          Ⓜ️ node-manager (Manages the collection of Cilium nodes):
              🚧 manager.newAllNodeManager (cell.go:69):
                  ⇨ hive.Lifecycle
                  ⇦ manager.NodeManager

The endpoint manager is now exposed as 3 different APIs: EndpointsLookup (for read-only lookups of endpoints), EndpointsModify (for adding and removing endpoints), and EndpointManager (for initialization and operations on all endpoints). This division by functionality adds clarity ("ah this module only needs to look up endpoints") and makes integration testing of cells depending on e.g. endpoint lookup simpler.

Node manager's API is kept as in one chunk as it's relatively small. It is still exposed as an interface rather than as struct pointer, again for improved integration testing and for clarity. Providing interfaces at the cell-level does not go against the "return structs, accept interfaces" rule. That rule is all about loose coupling in public APIs, e.g. library functions should be flexible in what they accept (don't take *os.File, but io.Reader). Here we're following the same spirit, but at the macro level. With interfaces in the object graph we can choose the implementation to provide to a cell or augment an existing one (cell.Decorate) if needed. This gives maximum flexibility and loose coupling that comes especially handy in integration testing.

@maintainer-s-little-helper

This comment was marked as outdated.

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 16, 2022
@joamaki joamaki changed the title DRAFT: Modularization of EndpointManager, NodeMAnager and NetConf DRAFT: Modularization of EndpointManager, NodeManager and NetConf Oct 16, 2022
@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch from f42c894 to 2dfcd74 Compare October 16, 2022 13:41
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 16, 2022
@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch from 2dfcd74 to 51c9973 Compare November 14, 2022 13:44
@joamaki joamaki changed the title DRAFT: Modularization of EndpointManager, NodeManager and NetConf DRAFT: Modularization of EndpointManager and NodeManager Nov 14, 2022
@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch 2 times, most recently from a080b1e to 27f95d3 Compare November 17, 2022 10:28
@joamaki joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Nov 17, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 17, 2022
@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch 2 times, most recently from 2a8ace8 to b20a9b6 Compare November 24, 2022 10:38
@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch from b20a9b6 to 37d3916 Compare November 30, 2022 13:30
@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch from 37d3916 to 321948e Compare December 20, 2022 08:22
@joamaki joamaki changed the title DRAFT: Modularization of EndpointManager and NodeManager EndpointManager and NodeManager Cells Dec 20, 2022
@joamaki
Copy link
Contributor Author

joamaki commented Dec 20, 2022

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN and endpoint routes

Failure Output

FAIL: Failed to reach 10.0.0.32:80 from testclient-host-22plg

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

@joamaki joamaki marked this pull request as ready for review December 20, 2022 09:43
@joamaki joamaki requested review from a team as code owners December 20, 2022 09:43
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, like the separate interfaces. Can't find anything to complain about.

@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch from 321948e to a2330d9 Compare December 20, 2022 11:29
@joamaki
Copy link
Contributor Author

joamaki commented Dec 20, 2022

/test

Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks. One comment below about root context passing. Otherwise LGTM.

daemon/cmd/daemon.go Show resolved Hide resolved
pkg/endpointmanager/cell.go Show resolved Hide resolved
@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch from a2330d9 to 9b67c2e Compare December 21, 2022 08:31
@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch from 9b67c2e to 37e3c5f Compare December 22, 2022 19:09
@joamaki
Copy link
Contributor Author

joamaki commented Dec 22, 2022

/test

daemon/cmd/cells.go Show resolved Hide resolved
pkg/endpointmanager/cell.go Outdated Show resolved Hide resolved
pkg/endpointmanager/cell.go Show resolved Hide resolved
pkg/endpointmanager/cell.go Outdated Show resolved Hide resolved
pkg/endpointmanager/cell.go Show resolved Hide resolved
pkg/endpointmanager/cell.go Show resolved Hide resolved
pkg/endpointmanager/cell.go Show resolved Hide resolved
pkg/endpointmanager/cell.go Show resolved Hide resolved
pkg/endpointmanager/cell.go Show resolved Hide resolved
pkg/endpointmanager/cell.go Show resolved Hide resolved
@joamaki
Copy link
Contributor Author

joamaki commented Dec 23, 2022

@tommyp1ckles addressed your comments about the method comments. Your comments about the API itself were good, but I wouldn't want to change the existing API in this PR as this is only about wrapping the endpoint manager into a module and lifting it outside Daemon struct.

@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch from 37e3c5f to 68ff514 Compare December 23, 2022 08:44
@tommyp1ckles
Copy link
Contributor

@tommyp1ckles addressed your comments about the method comments. Your comments about the API itself were good, but I wouldn't want to change the existing API in this PR as this is only about wrapping the endpoint manager into a module and lifting it outside Daemon struct.

Sounds good, seeing the interface written for these is a good opportunity to see where we can improve things. Everything LGTM, thanks!

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we need to apply this pattern for other subsystems steadily ✔️

This moves the creation of the endpoint manager from daemon.go
into pkg/endpointmanager/cell.go.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
Dropped the unused selector cache and policy triggerer from
node manager and moved the node handler subscription out from
the manager creation. NodeDiscovery no longer needs to Close()
the NodeManager as this is now done by a stop hook in NodeManager.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
When printing the lifecycle hooks a left-over debug
print appeared if a start function was not defined
in a hook: "{<nil> 0x223af60} not defined".

Signed-off-by: Jussi Maki <jussi@isovalent.com>
NodeManager's Stop() was earlier only used by tests so it's behavior was to
clean up datapath state by calling NodeDelete for each node. This is not
the desired behavior as we want to preserve the datapath state when shutting
down in order to not disrupt connectivity.

This cleanup behavior was never used in any tests and prior this was only
triggered by control-plane tests via call to Daemon.Close(), so it's safe
to remove this.

In addition Stop() did not wait for the backgroundSync() goroutine to exit
in Stop(). Fix this by using workerpool.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki force-pushed the pr/joamaki/agent-composition-first-lifts branch from 68ff514 to 21cd454 Compare January 9, 2023 10:29
@joamaki
Copy link
Contributor Author

joamaki commented Jan 9, 2023

/test

@joamaki
Copy link
Contributor Author

joamaki commented Jan 10, 2023

test-runtime hit #19093, which is unrelated to changes. Marking ready.

@joamaki joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 10, 2023
@aditighag aditighag merged commit 4ca3856 into cilium:master Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants