From fcb595a2df7be94abd0d6a63c1b4c9bd16c163f8 Mon Sep 17 00:00:00 2001 From: Daman Arora Date: Sun, 13 Jul 2025 23:59:30 +0530 Subject: [PATCH] service-ip-alloc: delay ip processing on service recreate Service storage implements transactions. It creates an IPAddress object first and then creates the Service object, and if the Service object already exists the complete transaction is reverted. There can be race conditions when the repair loop picks up the new IPAddress object for reconciliation before the transaction is reverted. This leads to spurious IPAddressWrongReference warnings, to suppress these warnings we delay the processing of the new IPAddress object by 5 seconds. The service allocation creates the IPAddress object before creating the Service object, we easily identify this scenario when the IPAddress object creation timestamp is after the Service creation timestamp. We do this only when the IPAddress object is created recently in order to avoid indefinitely requeue/delay in IPAddress cleanup if for some reason the service transaction revert fails. Signed-off-by: Daman Arora --- .../ipallocator/controller/repairip.go | 17 ++++++++++ .../ipallocator/controller/repairip_test.go | 34 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/pkg/registry/core/service/ipallocator/controller/repairip.go b/pkg/registry/core/service/ipallocator/controller/repairip.go index bb7c2d877ec..feb7fce3076 100644 --- a/pkg/registry/core/service/ipallocator/controller/repairip.go +++ b/pkg/registry/core/service/ipallocator/controller/repairip.go @@ -530,6 +530,23 @@ func (r *RepairIPAddress) syncIPAddress(key string) error { return nil } } + + // Service storage implements transactions. It creates an IPAddress object first and then creates + // the Service object, and if the Service object already exists the complete transaction is + // reverted. There can be race conditions when the repair loop picks up the new IPAddress object + // for reconciliation before the transaction is reverted. This leads to spurious + // IPAddressWrongReference warnings, to suppress these warnings we delay the processing of the new + // IPAddress object by 5 seconds. The service allocation creates the IPAddress object before creating + // the Service object, we easily identify this scenario when the IPAddress object creation timestamp + // is after the Service creation timestamp. We do this only when the IPAddress object is created + // recently in order to avoid indefinitely requeue/delay in IPAddress cleanup if for some reason + // the service transaction revert fails. + if ipAddress.CreationTimestamp.After(svc.CreationTimestamp.Time) && + r.clock.Now().Sub(ipAddress.CreationTimestamp.Time) < 5*time.Second { + // requeue after the grace period + r.ipQueue.AddAfter(key, 5*time.Second) + return nil + } runtime.HandleError(fmt.Errorf("the IPAddress: %s for Service %s/%s has a wrong reference %#v; cleaning up", ipAddress.Name, svc.Name, svc.Namespace, ipAddress.Spec.ParentRef)) r.recorder.Eventf(ipAddress, nil, v1.EventTypeWarning, "IPAddressWrongReference", "IPAddressAllocation", "IPAddress: %s for Service %s/%s has a wrong reference; cleaning up", ipAddress.Name, svc.Namespace, svc.Name) err = r.client.NetworkingV1().IPAddresses().Delete(context.Background(), ipAddress.Name, metav1.DeleteOptions{}) diff --git a/pkg/registry/core/service/ipallocator/controller/repairip_test.go b/pkg/registry/core/service/ipallocator/controller/repairip_test.go index f716b6035c1..1e44687f025 100644 --- a/pkg/registry/core/service/ipallocator/controller/repairip_test.go +++ b/pkg/registry/core/service/ipallocator/controller/repairip_test.go @@ -372,6 +372,40 @@ func TestRepairServiceIP(t *testing.T) { actions: [][]string{{"delete", "ipaddresses"}}, events: []string{"Warning IPAddressWrongReference IPAddress: 10.0.1.2 for Service bar/test-svc has a wrong reference; cleaning up"}, }, + { + name: "Two IPAddresses referencing the same service when service create is called again and ip address is in grace period", + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1"}, testTimeNow.Add(1*time.Second)), + }, + ipAddresses: []*networkingv1.IPAddress{ + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc", []string{"10.0.1.1"}), testTimeNow), + // ensure the second IPAddress object has a creation timestamp greater than service creation timestamp + newIPAddressWithCreationTimestamp("10.0.1.2", newService("test-svc", []string{"10.0.1.1"}), testTimeNow.Add(2*time.Second)), + }, + cidrs: []*networkingv1.ServiceCIDR{ + newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), + }, + // update the test time, ensuring 5 seconds have not passed after creation of the second IPAddress Object + testTime: testTimeNow.Add(3 * time.Second), + }, + { + name: "Two IPAddresses referencing the same service when service create is called again and ip address is not in grace period", + svcs: []*v1.Service{ + newServiceWithCreationTimestamp("test-svc", []string{"10.0.1.1"}, testTimeNow.Add(1*time.Second)), + }, + ipAddresses: []*networkingv1.IPAddress{ + newIPAddressWithCreationTimestamp("10.0.1.1", newService("test-svc", []string{"10.0.1.1"}), testTimeNow), + // ensure the second IPAddress object has a creation timestamp greater than service creation timestamp + newIPAddressWithCreationTimestamp("10.0.1.2", newService("test-svc", []string{"10.0.1.1"}), testTimeNow.Add(2*time.Second)), + }, + cidrs: []*networkingv1.ServiceCIDR{ + newServiceCIDR("kubernetes", serviceCIDRv4, serviceCIDRv6), + }, + // update the test time, ensuring 5 seconds have passed after creation of the second IPAddress Object + testTime: testTimeNow.Add(10 * time.Second), + actions: [][]string{{"delete", "ipaddresses"}}, + events: []string{"Warning IPAddressWrongReference IPAddress: 10.0.1.2 for Service bar/test-svc has a wrong reference; cleaning up"}, + }, { name: "Two Services with same ClusterIP", svcs: []*v1.Service{