From 46e2c22fd76643bc985f7e77c99e97c6b7d078fc Mon Sep 17 00:00:00 2001 From: Daman Arora Date: Sun, 11 May 2025 22:48:25 +0530 Subject: [PATCH] kube-proxy: merge OnNodeAdd and OnNodeUpdate into OnNodeChange For kube-proxy, node addition and node update is semantically considered as similar event, we have exactly same handler logic for these two events resulting in duplicate code and unit tests. This merges the `NodeHandler` interface methods OnNodeAdd and OnNodeUpdate into OnNodeChange along with the implementation of the interface. Signed-off-by: Daman Arora --- pkg/proxy/config/config.go | 46 ++++++++--------------- pkg/proxy/healthcheck/healthcheck_test.go | 16 ++++---- pkg/proxy/node.go | 14 +------ pkg/proxy/node_test.go | 4 +- 4 files changed, 28 insertions(+), 52 deletions(-) diff --git a/pkg/proxy/config/config.go b/pkg/proxy/config/config.go index 1f24196e90c..725149013b5 100644 --- a/pkg/proxy/config/config.go +++ b/pkg/proxy/config/config.go @@ -260,12 +260,9 @@ func (c *ServiceConfig) handleDeleteService(obj interface{}) { // NodeHandler is an abstract interface of objects which receive // notifications about node object changes. type NodeHandler interface { - // OnNodeAdd is called whenever creation of new node object - // is observed. - OnNodeAdd(node *v1.Node) - // OnNodeUpdate is called whenever modification of an existing - // node object is observed. - OnNodeUpdate(oldNode, node *v1.Node) + // OnNodeChange is called whenever creation or modification + // of node object is observed. + OnNodeChange(node *v1.Node) // OnNodeDelete is called whenever deletion of an existing node // object is observed. OnNodeDelete(node *v1.Node) @@ -290,8 +287,7 @@ func NewNodeConfig(ctx context.Context, nodeInformer v1informers.NodeInformer, r handlerRegistration, _ := nodeInformer.Informer().AddEventHandlerWithResyncPeriod( cache.ResourceEventHandlerFuncs{ - AddFunc: result.handleAddNode, - UpdateFunc: result.handleUpdateNode, + UpdateFunc: func(_, newObj interface{}) { result.handleChangeNode(newObj) }, DeleteFunc: result.handleDeleteNode, }, resyncPeriod, @@ -321,32 +317,22 @@ func (c *NodeConfig) Run(stopCh <-chan struct{}) { } } -func (c *NodeConfig) handleAddNode(obj interface{}) { +func (c *NodeConfig) handleChangeNode(obj interface{}) { node, ok := obj.(*v1.Node) if !ok { - utilruntime.HandleError(fmt.Errorf("unexpected object type: %v", obj)) - return + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + utilruntime.HandleError(fmt.Errorf("unexpected object type: %v", obj)) + return + } + if node, ok = tombstone.Obj.(*v1.Node); !ok { + utilruntime.HandleError(fmt.Errorf("unexpected object type: %v", obj)) + return + } } for i := range c.eventHandlers { - c.logger.V(4).Info("Calling handler.OnNodeAdd") - c.eventHandlers[i].OnNodeAdd(node) - } -} - -func (c *NodeConfig) handleUpdateNode(oldObj, newObj interface{}) { - oldNode, ok := oldObj.(*v1.Node) - if !ok { - utilruntime.HandleError(fmt.Errorf("unexpected object type: %v", oldObj)) - return - } - node, ok := newObj.(*v1.Node) - if !ok { - utilruntime.HandleError(fmt.Errorf("unexpected object type: %v", newObj)) - return - } - for i := range c.eventHandlers { - c.logger.V(5).Info("Calling handler.OnNodeUpdate") - c.eventHandlers[i].OnNodeUpdate(oldNode, node) + c.logger.V(4).Info("Calling handler.OnNodeChange") + c.eventHandlers[i].OnNodeChange(node) } } diff --git a/pkg/proxy/healthcheck/healthcheck_test.go b/pkg/proxy/healthcheck/healthcheck_test.go index 5f39563e745..e5924296dcd 100644 --- a/pkg/proxy/healthcheck/healthcheck_test.go +++ b/pkg/proxy/healthcheck/healthcheck_test.go @@ -495,7 +495,7 @@ func TestHealthzServer(t *testing.T) { testProxyHealthUpdater(hs, hsTest, fakeClock, ptr.To(true), t) // Should return 200 "OK" if we've synced a node, tainted in any other way - nodeManager.OnNodeUpdate(nil, makeNode(tweakTainted("other"))) + nodeManager.OnNodeChange(makeNode(tweakTainted("other"))) expectedPayload = ProxyHealth{ CurrentTime: fakeClock.Now(), LastUpdated: fakeClock.Now(), @@ -509,7 +509,7 @@ func TestHealthzServer(t *testing.T) { testHTTPHandler(hsTest, http.StatusOK, expectedPayload, t) // Should return 503 "ServiceUnavailable" if we've synced a ToBeDeletedTaint node - nodeManager.OnNodeUpdate(nil, makeNode(tweakTainted(ToBeDeletedTaint))) + nodeManager.OnNodeChange(makeNode(tweakTainted(ToBeDeletedTaint))) expectedPayload = ProxyHealth{ CurrentTime: fakeClock.Now(), LastUpdated: fakeClock.Now(), @@ -523,7 +523,7 @@ func TestHealthzServer(t *testing.T) { testHTTPHandler(hsTest, http.StatusServiceUnavailable, expectedPayload, t) // Should return 200 "OK" if we've synced a node, tainted in any other way - nodeManager.OnNodeUpdate(nil, makeNode(tweakTainted("other"))) + nodeManager.OnNodeChange(makeNode(tweakTainted("other"))) expectedPayload = ProxyHealth{ CurrentTime: fakeClock.Now(), LastUpdated: fakeClock.Now(), @@ -537,7 +537,7 @@ func TestHealthzServer(t *testing.T) { testHTTPHandler(hsTest, http.StatusOK, expectedPayload, t) // Should return 503 "ServiceUnavailable" if we've synced a deleted node - nodeManager.OnNodeUpdate(nil, makeNode(tweakDeleted())) + nodeManager.OnNodeChange(makeNode(tweakDeleted())) expectedPayload = ProxyHealth{ CurrentTime: fakeClock.Now(), LastUpdated: fakeClock.Now(), @@ -575,7 +575,7 @@ func TestLivezServer(t *testing.T) { testProxyHealthUpdater(hs, hsTest, fakeClock, nil, t) // Should return 200 "OK" irrespective of node syncs - nodeManager.OnNodeUpdate(nil, makeNode(tweakTainted("other"))) + nodeManager.OnNodeChange(makeNode(tweakTainted("other"))) expectedPayload = ProxyHealth{ CurrentTime: fakeClock.Now(), LastUpdated: fakeClock.Now(), @@ -588,7 +588,7 @@ func TestLivezServer(t *testing.T) { testHTTPHandler(hsTest, http.StatusOK, expectedPayload, t) // Should return 200 "OK" irrespective of node syncs - nodeManager.OnNodeUpdate(nil, makeNode(tweakTainted(ToBeDeletedTaint))) + nodeManager.OnNodeChange(makeNode(tweakTainted(ToBeDeletedTaint))) expectedPayload = ProxyHealth{ CurrentTime: fakeClock.Now(), LastUpdated: fakeClock.Now(), @@ -601,7 +601,7 @@ func TestLivezServer(t *testing.T) { testHTTPHandler(hsTest, http.StatusOK, expectedPayload, t) // Should return 200 "OK" irrespective of node syncs - nodeManager.OnNodeUpdate(nil, makeNode(tweakTainted("other"))) + nodeManager.OnNodeChange(makeNode(tweakTainted("other"))) expectedPayload = ProxyHealth{ CurrentTime: fakeClock.Now(), LastUpdated: fakeClock.Now(), @@ -614,7 +614,7 @@ func TestLivezServer(t *testing.T) { testHTTPHandler(hsTest, http.StatusOK, expectedPayload, t) // Should return 200 "OK" irrespective of node syncs - nodeManager.OnNodeUpdate(nil, makeNode(tweakDeleted())) + nodeManager.OnNodeChange(makeNode(tweakDeleted())) expectedPayload = ProxyHealth{ CurrentTime: fakeClock.Now(), LastUpdated: fakeClock.Now(), diff --git a/pkg/proxy/node.go b/pkg/proxy/node.go index 1e536a49f27..21ad919616d 100644 --- a/pkg/proxy/node.go +++ b/pkg/proxy/node.go @@ -136,18 +136,8 @@ func (n *NodeManager) NodeInformer() v1informers.NodeInformer { return n.nodeInformer } -// OnNodeAdd is a handler for Node creates. -func (n *NodeManager) OnNodeAdd(node *v1.Node) { - n.onNodeChange(node) -} - -// OnNodeUpdate is a handler for Node updates. -func (n *NodeManager) OnNodeUpdate(_, node *v1.Node) { - n.onNodeChange(node) -} - -// onNodeChange functions helps to implement OnNodeAdd and OnNodeUpdate. -func (n *NodeManager) onNodeChange(node *v1.Node) { +// OnNodeChange is a handler for Node creation and update. +func (n *NodeManager) OnNodeChange(node *v1.Node) { // update the node object n.mu.Lock() oldNodeIPs, _ := utilnode.GetNodeHostIPs(n.node) diff --git a/pkg/proxy/node_test.go b/pkg/proxy/node_test.go index 1f49e89f814..b3cb919f3a0 100644 --- a/pkg/proxy/node_test.go +++ b/pkg/proxy/node_test.go @@ -287,7 +287,7 @@ func TestNodeManagerOnNodeChange(t *testing.T) { nodeManager, err := newNodeManager(ctx, client, 30*time.Second, testNodeName, tc.watchPodCIDRs, exitFunc, 10*time.Millisecond, time.Second) require.NoError(t, err) - nodeManager.onNodeChange(makeNode(tweakNodeIPs(tc.updatedNodeIPs...), tweakPodCIDRs(tc.updatedPodCIDRs...))) + nodeManager.OnNodeChange(makeNode(tweakNodeIPs(tc.updatedNodeIPs...), tweakPodCIDRs(tc.updatedPodCIDRs...))) require.Equal(t, tc.expectedExitCode, exitCode) }) } @@ -321,7 +321,7 @@ func TestNodeManagerNode(t *testing.T) { require.NoError(t, err) require.Equal(t, "1", nodeManager.Node().ResourceVersion) - nodeManager.OnNodeUpdate(nil, makeNode(tweakResourceVersion("2"))) + nodeManager.OnNodeChange(makeNode(tweakResourceVersion("2"))) require.NoError(t, err) require.Equal(t, "2", nodeManager.Node().ResourceVersion) }