Extend the event.Handler API to provide common state info to implementations #2803

tpantelis · 2023-11-15T19:23:28Z

See the individual for details. To summarize:

Added HandlerState that contains global state common to all event handlers, specifically IsOnGateway and GetRemoteEndpoints which is provided to the handlers via a SetState method.
Modified all handlers to use the HandlerState rather than maintaining such state themselves.
The event controller now provides synchronization across all event callbacks so handlers don't have to.
Added unit tests for handlers that didn't have any prior.

submariner-bot · 2023-11-15T19:23:32Z

🤖 Created branch: z_pr2803/tpantelis/ext_event_hndlr_api
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

skitt · 2023-11-21T12:15:55Z

pkg/event/controller/controller.go

@@ -43,15 +44,36 @@ type specification struct {
 	Namespace string
 }

+type handlerStateType struct {


I found this odd on first read — Type suggests this describes a type, but it’s really the anchor for an implementation of HandlerState, so how about handlerStateImpl?

We could just name it handlerState but it's nice to distinguish it from variables of the same name. I've seen the use of Type in K8s code so I borrowed it (I've used it before as well) - it is actually describing the type. Impl seems Java-ish but I can change it - I'm fine either way (or name it handlerState).

Right, I didn’t bother mentioning handlerState because of the variable name conflict! I prefer Impl personally, especially as more type parameters start appearing in various places. The naming in Kubernetes seems to be shifting too, at least in discussions I’ve been having recently people talk about Impl, and it’s about 50/50 now.

skitt · 2023-11-21T12:27:10Z

pkg/event/controller/endpoint_created.go

-	if err := c.handlers.LocalEndpointCreated(endpoint); err != nil {
-		return err //nolint:wrapcheck  // Let the caller wrap it
+	if endpoint.Spec.Hostname == c.hostname {
+		c.handlerState.isOnGateway.Store(true)


Reads go through IsOnGateway(), so it feels unbalanced to directly access the atomic here.

isOnGateway is private so it really doesn't matter either way. But to balance it we can either add a private function to wrap isOnGateway.Store or not use IsOnGateway() internally.

I agree it’s not important, but a private function would be good (not exposing the atomic for reads — I’d rather keep this encapsulated so we can change the implementation easily).

skitt · 2023-11-21T12:35:16Z

pkg/event/handler.go

 type Handler interface {
 	// Init is called once on startup to let the handler initialize any state it needs.
 	Init() error

+	// SetHandlerState is called once on startup after Init with the HandlerState that can be used to access global data from event callbacks.
+	SetState(handlerCtx HandlerState)


Is it a context, or a state?

Other than the amount of churn involved, is there a reason to have a separate SetState method instead of adding the handler state to Init?

Currently the handlers are set up when the registry is constructed, and then controller construction sets the shared handler state. But AFAICT nothing would stop a handler being added after that, and then it wouldn’t be given the shared state...

Is it a context, or a state?

I went back and forth on that. I finally went with state b/c context is usually used per function and this state persists.

I thought about passing it via Init but that is called before the controller is initialized and it's the controller that owns/implements the state. The registry and controller are completely decoupled. But we could have the registry construct and start the controller - we'd have to have a public method on the controller to get the HandlerState but that seemed awkward, unless we have the controller called Init. Also, each Handler impl would have to explicitly store the state, presumably call SetState on the HandlerBase, but this seemed awkward to me. That's why I went with this approach.

But AFAICT nothing would stop a handler being added after that, and then it wouldn’t be given the shared state...

You're right but that would problematic b/c eventHandlers in Registry is not synchronized. The Registry is designed to add all the handlers first before starting the controller. We could force this by passing all the handlers via NewRegistry and removing AddHandlers. I can do that in a followup.

Right, thanks for the explanation, that makes sense. I like the idea of dropping AddHandlers.

I like the idea of dropping AddHandlers

57cdf7e

Suggested change

SetState(handlerCtx HandlerState)

SetState(handlerState HandlerState)

if you respin this.

skitt · 2023-11-21T13:01:00Z

pkg/routeagent_driver/handlers/kubeproxy/node_handler.go

+		vtepIP, kp.State().IsOnGateway())

-	if kp.isGatewayNode {
+	if kp.State().IsOnGateway() {


This suggests a small (pre-existing) race here, but it’s protected by the lock. However to make the correctness more obvious it would be better to store the current return value of IsOnGateway() and use that both for logging and decision-marking (so it’s apparent that the branch taken matches the log message).

sure, for readability.

The controller was using the syncMutex to protect access to the isGatewayNode field, although not really necessary since the callbacks emanate from the same syncer on the same thread. In addition, some handlers employ their own mutex across multiple callbacks, although unnecessary in most cases. However a hidden caveat with the handler API is that while the Endpoint-related events (ie LocalEndpointCreated, TransitionToGateway etc) are single-threaded wrt one another and don't need synchronization, the Mode-related events emanate from a separate syncer and thread. So if data is accessed across Endpoint and Node events then synchronization is needed. Rather then having each handler implementation be aware of this behavior, it is simpler and safer to have the event controller use the syncMutex to synchronization access to all the callbacks and remove the synchronization in the handlers. Signed-off-by: Tom Pantelis <[email protected]>

This will enable event handlers to query common global state from event callback methods. This will replace the state currently being maintained by many handlers. The state instance is set once for each handler instead of passed to each event callback method to enable the state to be accessed outside of the event callbacks. The HandlerBase implements SetState. Signed-off-by: Tom Pantelis <[email protected]>

Signed-off-by: Tom Pantelis <[email protected]>

...for reuse in other event handler tests. Signed-off-by: Tom Pantelis <[email protected]>

Also added unit tests. Signed-off-by: Tom Pantelis <[email protected]>

Signed-off-by: Tom Pantelis <[email protected]>

Also added unit tests. Signed-off-by: Tom Pantelis <[email protected]>

This enables unit tests to mock the behavior. Signed-off-by: Tom Pantelis <[email protected]>

Define a function so that unit tests can provide a fake implementation. Also consolidate the NewHandler parameters into a HandlerConfig struct. Signed-off-by: Tom Pantelis <[email protected]>

Signed-off-by: Tom Pantelis <[email protected]>

...as AwaitRoutes checks for Dst IP. Also rename AwaitRoutes to AwaitDstRoutes. Signed-off-by: Tom Pantelis <[email protected]>

...instead of hard-coded strings. Signed-off-by: Tom Pantelis <[email protected]>

Signed-off-by: Tom Pantelis <[email protected]>

The mutex was kept to provide atomicity when updating the gateway and host network dataplane since these are also invoked via the route config syncer which runs on its own thread. The synchronization may actually not be necessary but it's better to be safe and the overhead in negligible. Signed-off-by: Tom Pantelis <[email protected]>

This ensures all handlers are registered before starting the controller. Signed-off-by: Tom Pantelis <[email protected]>

skitt · 2023-11-22T10:35:24Z

pkg/event/handler.go

 type Handler interface {
 	// Init is called once on startup to let the handler initialize any state it needs.
 	Init() error

+	// SetHandlerState is called once on startup after Init with the HandlerState that can be used to access global data from event callbacks.
+	SetState(handlerCtx HandlerState)


Suggested change

SetState(handlerCtx HandlerState)

SetState(handlerState HandlerState)

if you respin this.

skitt · 2023-11-22T10:40:03Z

pkg/routeagent_driver/handlers/ovn/fake/ovsdb_client.go

+	"github.com/submariner-io/admiral/pkg/slices"
+)
+
+type OVSDBClient struct {


Writing OVSDB test code, that takes us back!

submariner-bot · 2023-11-22T10:42:11Z

🤖 Closed branches: [z_pr2803/tpantelis/ext_event_hndlr_api]

tpantelis self-assigned this Nov 15, 2023

tpantelis requested review from Oats87, skitt and sridhargaddam as code owners November 15, 2023 19:23

tpantelis force-pushed the ext_event_hndlr_api branch from 3a3d814 to 172115c Compare November 15, 2023 19:41

tpantelis requested review from aswinsuryan and yboaron November 15, 2023 19:41

tpantelis force-pushed the ext_event_hndlr_api branch 2 times, most recently from 3013f75 to 58fbddc Compare November 15, 2023 21:25

tpantelis added the ready-to-test When a PR is ready for full E2E testing label Nov 15, 2023

tpantelis force-pushed the ext_event_hndlr_api branch from 58fbddc to a23e4b7 Compare November 15, 2023 22:28

aswinsuryan approved these changes Nov 18, 2023

View reviewed changes

skitt requested changes Nov 21, 2023

View reviewed changes

tpantelis force-pushed the ext_event_hndlr_api branch from a23e4b7 to 57cdf7e Compare November 21, 2023 19:22

tpantelis added 7 commits November 21, 2023 19:36

Implement HandlerState in the event controller

5d35b3e

Signed-off-by: Tom Pantelis <[email protected]>

Refactor event controller test driver to ControllerSupport

fd1d658

...for reuse in other event handler tests. Signed-off-by: Tom Pantelis <[email protected]>

Use HandlerState in calico IPPool event handler

3da4244

Also added unit tests. Signed-off-by: Tom Pantelis <[email protected]>

Use HandlerState in the kubeproxy SyncHandler

3dd2cf3

Signed-off-by: Tom Pantelis <[email protected]>

Implement Addr* functions in the fake netlink

7fcc9b0

Signed-off-by: Tom Pantelis <[email protected]>

tpantelis force-pushed the ext_event_hndlr_api branch from 57cdf7e to adae9bd Compare November 22, 2023 00:39

tpantelis added 7 commits November 21, 2023 20:16

Use HandlerState in the OVN GatewayRouteHandler

3165455

Also added unit tests. Signed-off-by: Tom Pantelis <[email protected]>

Use HandlerState in the OVN NonGatewayRouteHandler

c36a28b

Also added unit tests. Signed-off-by: Tom Pantelis <[email protected]>

Use the netlink interface abstraction in the OVN handler

338b360

This enables unit tests to mock the behavior. Signed-off-by: Tom Pantelis <[email protected]>

Abstract OVSDB client creation in OVN Handler

0f3f51d

Define a function so that unit tests can provide a fake implementation. Also consolidate the NewHandler parameters into a HandlerConfig struct. Signed-off-by: Tom Pantelis <[email protected]>

Handle Src and Dst in fake netlink Rule functions

64bfc3e

Signed-off-by: Tom Pantelis <[email protected]>

Add fake netlink methods to await Gw IP routes

d3e4661

...as AwaitRoutes checks for Dst IP. Also rename AwaitRoutes to AwaitDstRoutes. Signed-off-by: Tom Pantelis <[email protected]>

Use NATTable and FilterTable constants in the OVN Handler

9aef12e

...instead of hard-coded strings. Signed-off-by: Tom Pantelis <[email protected]>

tpantelis added 4 commits November 21, 2023 20:16

Make some OVN handler constants public for unit tests

302b4b0

Signed-off-by: Tom Pantelis <[email protected]>

Add OVN Handler unit tests

24f8310

Signed-off-by: Tom Pantelis <[email protected]>

Pass Handlers to NewRegistry and remove AddHandlers

60437de

This ensures all handlers are registered before starting the controller. Signed-off-by: Tom Pantelis <[email protected]>

tpantelis force-pushed the ext_event_hndlr_api branch from adae9bd to 60437de Compare November 22, 2023 01:16

skitt approved these changes Nov 22, 2023

View reviewed changes

skitt merged commit fd07457 into submariner-io:devel Nov 22, 2023
35 of 36 checks passed

tpantelis deleted the ext_event_hndlr_api branch December 8, 2023 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend the event.Handler API to provide common state info to implementations #2803

Extend the event.Handler API to provide common state info to implementations #2803

tpantelis commented Nov 15, 2023

submariner-bot commented Nov 15, 2023

skitt Nov 21, 2023

tpantelis Nov 21, 2023

skitt Nov 21, 2023

skitt Nov 21, 2023

tpantelis Nov 21, 2023

skitt Nov 21, 2023

skitt Nov 21, 2023

skitt Nov 21, 2023

tpantelis Nov 21, 2023

skitt Nov 21, 2023

tpantelis Nov 21, 2023

skitt Nov 22, 2023

skitt Nov 21, 2023 •

edited

Loading

tpantelis Nov 21, 2023

skitt Nov 22, 2023

skitt Nov 22, 2023

submariner-bot commented Nov 22, 2023

	SetState(handlerCtx HandlerState)
	SetState(handlerState HandlerState)

Extend the event.Handler API to provide common state info to implementations #2803

Extend the event.Handler API to provide common state info to implementations #2803

Conversation

tpantelis commented Nov 15, 2023

submariner-bot commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skitt Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

submariner-bot commented Nov 22, 2023

skitt Nov 21, 2023 •

edited

Loading