diff --git a/pkg/ipam/crd.go b/pkg/ipam/crd.go index b6141165232fc..137f65a0738bb 100644 --- a/pkg/ipam/crd.go +++ b/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 diff --git a/pkg/ipam/crd_test.go b/pkg/ipam/crd_test.go index 8faaaad63b5f9..4e6a13f57a671 100644 --- a/pkg/ipam/crd_test.go +++ b/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") +}