[ipam] Skip redundant CiliumNode status writes in refreshNode

pkg/ipam/crd.go

@@ -77,6 +77,26 @@ type nodeStore struct {
 	restoreFinished  chan struct{}
 	restoreCloseOnce sync.Once
 
+	// lastAppliedUsed is the Status.IPAM.Used map last successfully written
+	// to the apiserver by refreshNode(), taken from the apiserver-returned
+	// object. refreshNode() skips its UpdateStatus() call when the freshly
+	// rebuilt allocation map is unchanged from this value. This is only
+	// safe for Used because the agent is its sole writer (the operator only
+	// initializes it once when nil; see PopulateStatusFields).
+	lastAppliedUsed ipamTypes.AllocationMap
+
+	// pendingReleaseIPsWrite is set by updateLocalNodeResource whenever it
+	// makes a genuine local change to Status.IPAM.ReleaseIPs (the release-IP
+	// ACK/NACK handshake with the operator), and consumed by refreshNode(),
+	// which must not skip its write while this is set. ReleaseIPs is a
+	// shared field with the operator, so unlike Used, a value-equality check
+	// against our own last write is not a reliable no-op signal here: the
+	// operator can re-mark an IP for release with the same string value the
+	// agent already ACKed in a previous round, which would look identical
+	// to our own history even though the current apiserver state (as of the
+	// most recent watch delivery) genuinely requires a fresh ACK/NACK.
+	pendingReleaseIPsWrite bool
+
 	clientset client.Clientset
 
 	conf      *option.DaemonConfig
@@ -377,6 +397,11 @@ func (n *nodeStore) staticIPStatus() (requested bool, assigned string) {
 func (n *nodeStore) deleteLocalNodeResource() {
 	n.mutex.Lock()
 	n.ownNode = nil
+	// The next CiliumNode we observe (e.g. after a delete+recreate while the
+	// agent keeps running) is a distinct object; do not let a stale
+	// no-op-write baseline suppress its first write.
+	n.lastAppliedUsed = nil
+	n.pendingReleaseIPsWrite = false
 	n.mutex.Unlock()
 }
 
@@ -439,6 +464,15 @@ func (n *nodeStore) updateLocalNodeResource(node *ciliumv2.CiliumNode) {
 		configureENIDevices(n.logger, n.ownNode, node, n.mtuConfig, n.sysctl)
 	}
 
+	if n.ownNode != nil && n.ownNode.UID != node.UID {
+		// The object was deleted and recreated (same name, different UID)
+		// without us observing a distinct delete event, e.g. via an
+		// informer relist. Do not let the previous object's no-op-write
+		// baseline suppress this new object's first write.
+		n.lastAppliedUsed = nil
+		n.pendingReleaseIPsWrite = false
+	}
+
 	n.ownNode = node
 	n.allocationPoolSize[IPv4] = 0
 	n.allocationPoolSize[IPv6] = 0
@@ -538,6 +572,10 @@ func (n *nodeStore) updateLocalNodeResource(node *ciliumv2.CiliumNode) {
 	}
 
 	if releaseUpstreamSyncNeeded {
+		// Mark the release-IP handshake state as dirty so that refreshNode()
+		// is guaranteed to write it upstream, even if the resulting
+		// ReleaseIPs content happens to match what we last wrote.
+		n.pendingReleaseIPsWrite = true
 		n.refreshTrigger.TriggerWithReason("excess IP release")
 	}
 }
@@ -575,29 +613,68 @@ func (n *nodeStore) refreshNodeTrigger(reasons []string) {
 // refreshNode updates the custom resource in the apiserver based on the latest
 // information in the local node store
 func (n *nodeStore) refreshNode() error {
-	n.mutex.RLock()
+	n.mutex.Lock()
 	if n.ownNode == nil {
-		n.mutex.RUnlock()
+		n.mutex.Unlock()
 		return nil
 	}
 
 	node := n.ownNode.DeepCopy()
 	staleCopyOfAllocators := make([]*crdAllocator, len(n.allocators))
 	copy(staleCopyOfAllocators, n.allocators)
-	n.mutex.RUnlock()
+	lastAppliedUsed := n.lastAppliedUsed
+	// Consume the release-IP dirty flag now: if the write below fails, it is
+	// restored so the pending handshake state is not lost (see below).
+	forceWrite := n.pendingReleaseIPsWrite
+	n.pendingReleaseIPsWrite = false
+	n.mutex.Unlock()
 
 	node.Status.IPAM.Used = ipamTypes.AllocationMap{}
-
 	for _, a := range staleCopyOfAllocators {
 		a.mutex.RLock()
 		maps.Copy(node.Status.IPAM.Used, a.allocated)
 		a.mutex.RUnlock()
 	}
 
-	var err error
-	_, err = n.clientset.CiliumV2().CiliumNodes().UpdateStatus(context.TODO(), node, metav1.UpdateOptions{})
+	// Skip the UpdateStatus() round-trip entirely when there is no pending
+	// release-IP handshake change (forceWrite) and the freshly rebuilt
+	// allocation map is unchanged from what we last successfully wrote.
+	// refreshNode() is re-triggered on every IP allocate/release, on every
+	// release-IP handshake update, and on every retry-after-error, so
+	// without this check the agent issues a full-object status write even
+	// when nothing actually changed, needlessly contending with the
+	// operator and with itself for the object's shared resourceVersion.
+	if !forceWrite && lastAppliedUsed != nil && maps.Equal(lastAppliedUsed, node.Status.IPAM.Used) {
+		return nil
+	}
 
-	return err
+	updatedNode, err := n.clientset.CiliumV2().CiliumNodes().UpdateStatus(context.TODO(), node, metav1.UpdateOptions{})
+	if err != nil {
+		if forceWrite {
+			// Don't lose track of the pending release-IP handshake change;
+			// the next attempt (triggered by refreshNodeTrigger's own
+			// retry-after-error) must not skip it.
+			n.mutex.Lock()
+			n.pendingReleaseIPsWrite = true
+			n.mutex.Unlock()
+		}
+		return err
+	}
+
+	// Record what the apiserver confirmed, not merely what we sent, so that
+	// any server-side mutation (e.g. a defaulting/mutating webhook) is
+	// reflected in future no-op comparisons. Guarded on ownNode still
+	// referring to the same object we just wrote: if it was deleted or
+	// replaced by a differently-UID'd object while this call was in
+	// flight, committing this baseline would incorrectly suppress that new
+	// object's own first write.
+	n.mutex.Lock()
+	if n.ownNode != nil && n.ownNode.UID == node.UID {
+		n.lastAppliedUsed = updatedNode.Status.IPAM.Used
+	}
+	n.mutex.Unlock()
+
+	return nil
 }
 
 // addAllocator adds a new CRD allocator to the node store

pkg/ipam/crd_test.go

@@ -4,6 +4,7 @@
 package ipam
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"net"
@@ -14,6 +15,9 @@ import (
 	"github.com/cilium/hive/hivetest"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	k8sTesting "k8s.io/client-go/testing"
 
 	eniTypes "github.com/cilium/cilium/pkg/aws/eni/types"
 	azureTypes "github.com/cilium/cilium/pkg/azure/types"
@@ -22,6 +26,7 @@ import (
 	ipamTypes "github.com/cilium/cilium/pkg/ipam/types"
 	"github.com/cilium/cilium/pkg/ipmasq"
 	ciliumv2 "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2"
+	k8sClientTestUtils "github.com/cilium/cilium/pkg/k8s/client/testutils"
 	"github.com/cilium/cilium/pkg/logging"
 	"github.com/cilium/cilium/pkg/logging/logfields"
 	"github.com/cilium/cilium/pkg/node"
@@ -558,3 +563,138 @@ func Test_validateENIConfig(t *testing.T) {
 		})
 	}
 }
+
+// TestRefreshNodeSkipsNoopStatusWrite verifies that refreshNode() only skips
+// the UpdateStatus() apiserver call when Status.IPAM.Used is unchanged from
+// what it last successfully wrote and there is no pending release-IP
+// handshake change -- and, critically, that a pending handshake change is
+// never dropped even if its resulting content happens to be
+// byte-for-byte identical to a value the agent already wrote in a previous
+// round (which a pure content-equality check on ReleaseIPs would miss).
+func TestRefreshNodeSkipsNoopStatusWrite(t *testing.T) {
+	logger := hivetest.Logger(t)
+	fakeClientset, clientset := k8sClientTestUtils.NewFakeClientset(logger)
+
+	cn := newCiliumNode("node1", 0, 0, 0)
+	cn.Spec.IPAM.Pool = ipamTypes.AllocationMap{
+		"1.1.1.1": {Resource: "foo"},
+		"1.1.1.2": {Resource: "foo"},
+	}
+	cn.Status.IPAM.Used = ipamTypes.AllocationMap{
+		"1.1.1.1": {Resource: "foo"},
+	}
+	created, err := clientset.CiliumV2().CiliumNodes().Create(context.Background(), cn, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	var updateStatusCalls int
+	fakeClientset.CiliumFakeClientset.PrependReactor("update", "ciliumnodes", func(action k8sTesting.Action) (bool, runtime.Object, error) {
+		if action.GetSubresource() == "status" {
+			updateStatusCalls++
+		}
+		return false, nil, nil
+	})
+
+	store := newFakeNodeStore(testDaemonConfig(), t)
+	store.clientset = clientset
+	store.ownNode = created.DeepCopy()
+
+	alloc := &crdAllocator{
+		allocated: ipamTypes.AllocationMap{
+			"1.1.1.1": {Resource: "foo"},
+		},
+		family: IPv4,
+	}
+	store.addAllocator(alloc)
+
+	// syncOwnNode re-fetches the object from the fake apiserver and installs
+	// it as ownNode, standing in for the informer watch delivering back the
+	// resourceVersion of our own just-applied write (refreshNode() itself
+	// never updates ownNode).
+	syncOwnNode := func() {
+		fresh, err := clientset.CiliumV2().CiliumNodes().Get(context.Background(), "node1", metav1.GetOptions{})
+		require.NoError(t, err)
+		store.mutex.Lock()
+		store.ownNode = fresh
+		store.mutex.Unlock()
+	}
+
+	// First call: there is no previously-applied baseline yet, so the write
+	// must happen even though the allocator already matches ownNode's Used.
+	require.NoError(t, store.refreshNode())
+	require.Equal(t, 1, updateStatusCalls, "expected the first refreshNode() call to write unconditionally")
+	syncOwnNode()
+
+	// Second call with unchanged allocator state: now that a baseline has
+	// been recorded, this must be skipped.
+	require.NoError(t, store.refreshNode())
+	require.Equal(t, 1, updateStatusCalls, "expected an unchanged refreshNode() call to skip the UpdateStatus call")
+
+	// Change the allocator state: the write should happen again.
+	alloc.mutex.Lock()
+	alloc.allocated = ipamTypes.AllocationMap{
+		"1.1.1.1": {Resource: "foo"},
+		"1.1.1.3": {Resource: "bar"},
+	}
+	alloc.mutex.Unlock()
+
+	require.NoError(t, store.refreshNode())
+	require.Equal(t, 2, updateStatusCalls, "expected a changed allocation map to trigger an UpdateStatus call")
+	syncOwnNode()
+
+	// The operator marks 1.1.1.2 for release. It is not held by the
+	// allocator, so updateLocalNodeResource() ACKs it to ready-for-release.
+	// Status.IPAM.Used does not change in this round, but the handshake
+	// change must still be written.
+	round1 := store.ownNode.DeepCopy()
+	round1.Status.IPAM.ReleaseIPs = map[string]ipamTypes.IPReleaseStatus{
+		"1.1.1.2": ipamOption.IPAMMarkForRelease,
+	}
+	store.updateLocalNodeResource(round1)
+	require.Equal(t,
+		ipamTypes.IPReleaseStatus(ipamOption.IPAMReadyForRelease),
+		store.ownNode.Status.IPAM.ReleaseIPs["1.1.1.2"],
+	)
+
+	require.NoError(t, store.refreshNode())
+	require.Equal(t, 3, updateStatusCalls, "expected the release-IP ACK to trigger an UpdateStatus call")
+	syncOwnNode()
+
+	// Regression case for the bug a pure content-equality check on
+	// ReleaseIPs would have: the operator marks the *same* IP for release
+	// again in a later round. The allocator state is unchanged, so the
+	// agent's ACK is, byte-for-byte, the same "ready-for-release" value it
+	// already wrote in the previous round -- yet the apiserver's current
+	// value (as of this watch delivery) is "marked-for-release" again and
+	// genuinely needs a fresh ACK written back.
+	round2 := store.ownNode.DeepCopy()
+	round2.Status.IPAM.ReleaseIPs = map[string]ipamTypes.IPReleaseStatus{
+		"1.1.1.2": ipamOption.IPAMMarkForRelease,
+	}
+	store.updateLocalNodeResource(round2)
+	require.Equal(t,
+		ipamTypes.IPReleaseStatus(ipamOption.IPAMReadyForRelease),
+		store.ownNode.Status.IPAM.ReleaseIPs["1.1.1.2"],
+	)
+
+	require.NoError(t, store.refreshNode())
+	require.Equal(t, 4, updateStatusCalls,
+		"expected a repeated (but genuinely re-required) release-IP ACK to still trigger an UpdateStatus call")
+	syncOwnNode()
+
+	// Regression guard: a same-name object with a different UID (e.g. a
+	// delete+recreate observed as a plain informer update, without a
+	// distinct delete event) must not inherit the old object's no-op-write
+	// baseline. If Used on the "new" object happens to match the stale
+	// baseline, the write must still happen.
+	recreated := store.ownNode.DeepCopy()
+	recreated.UID = store.ownNode.UID + "-recreated"
+	recreated.Status.IPAM.Used = ipamTypes.AllocationMap{
+		"1.1.1.1": {Resource: "foo"},
+		"1.1.1.3": {Resource: "bar"},
+	}
+	store.updateLocalNodeResource(recreated)
+
+	require.NoError(t, store.refreshNode())
+	require.Equal(t, 5, updateStatusCalls,
+		"expected a recreated (different UID) object to write unconditionally despite a matching Used map")
+}