diff --git a/internal/controller/networkinterface_controller.go b/internal/controller/networkinterface_controller.go index 59a2f5c0..c16bb1f3 100644 --- a/internal/controller/networkinterface_controller.go +++ b/internal/controller/networkinterface_controller.go @@ -50,7 +50,14 @@ func (r *NetworkInterfaceReconciler) Reconcile(ctx context.Context, req mcreconc logger.V(1).Info("reconciling network interface") defer logger.V(1).Info("reconcile complete") - return ctrl.Result{}, r.reconcileInterface(ctx, cl.GetClient(), req.NamespacedName) + err = r.reconcileInterface(ctx, cl.GetClient(), req.NamespacedName) + if retryResult, expected := networkInterfaceClaimWriteRace(err); expected { + // The claim controller writes the same interface. Losing that race + // only means the cached copy was stale, so read it again. + logger.V(1).Info("resource changed while reconciling; retrying") + return retryResult, nil + } + return ctrl.Result{}, err } func (r *NetworkInterfaceReconciler) reconcileInterface( @@ -66,6 +73,10 @@ func (r *NetworkInterfaceReconciler) reconcileInterface( if iface.DeletionTimestamp.IsZero() { if controllerutil.AddFinalizer(&iface, networkInterfaceFinalizer) { if err := cl.Update(ctx, &iface); err != nil { + // Deleted since the cache last saw it: nothing left to guard. + if apierrors.IsNotFound(err) { + return nil + } return fmt.Errorf("failed adding finalizer: %w", err) } } diff --git a/internal/controller/networkinterface_writerace_test.go b/internal/controller/networkinterface_writerace_test.go new file mode 100644 index 00000000..824da05c --- /dev/null +++ b/internal/controller/networkinterface_writerace_test.go @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controller + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// An interface deleted after the cache last saw it needs no finalizer, so +// failing to add one is not an error. +func TestAddingAFinalizerToADeletedInterfaceIsNotAnError(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, networkingv1alpha.AddToScheme(scheme)) + iface := &networkingv1alpha.NetworkInterface{ObjectMeta: metav1.ObjectMeta{Name: "nic", Namespace: "tenant"}} + cl := interceptor.NewClient(fakeclient.NewClientBuilder().WithScheme(scheme).WithObjects(iface).Build(), + interceptor.Funcs{ + Update: func(context.Context, client.WithWatch, client.Object, ...client.UpdateOption) error { + return apierrors.NewNotFound(schema.GroupResource{ + Group: networkingv1alpha.GroupVersion.Group, Resource: "networkinterfaces", + }, "nic") + }, + }) + + r := &NetworkInterfaceReconciler{} + require.NoError(t, r.reconcileInterface(context.Background(), cl, client.ObjectKeyFromObject(iface))) +} diff --git a/internal/galactic/controller/networkinterface_controller.go b/internal/galactic/controller/networkinterface_controller.go index 8c471e4a..273914df 100644 --- a/internal/galactic/controller/networkinterface_controller.go +++ b/internal/galactic/controller/networkinterface_controller.go @@ -34,6 +34,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" cloudv1alpha1 "go.datum.net/galactic/api/cloud/v1alpha1" galacticv1alpha1 "go.datum.net/galactic/api/v1alpha1" @@ -124,6 +125,19 @@ type NetworkInterfaceReconciler struct { // +kubebuilder:rbac:groups=galactic.datumapis.com,resources=trafficpolicies;trafficpolicybindings,verbs=get;list;watch;create;update;patch;delete func (r *NetworkInterfaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + result, err := r.reconcile(ctx, req) + if apierrors.IsConflict(err) || apierrors.IsAlreadyExists(err) { + // The claim controller and galactic write the same interface, claim + // and attachment status, and a create can race the cache. Losing that + // race only means the cached copy was stale, so read it again rather + // than report a controller error. + log.FromContext(ctx).V(1).Info("resource changed while reconciling; retrying", "reason", err.Error()) + return ctrl.Result{Requeue: true}, nil + } + return result, err +} + +func (r *NetworkInterfaceReconciler) reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { var networkInterface networkingv1alpha.NetworkInterface if err := r.Get(ctx, req.NamespacedName, &networkInterface); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) @@ -437,6 +451,10 @@ func (r *NetworkInterfaceReconciler) publishAttachmentStatus( ObservedGeneration: attachment.Generation, }) if err := r.Status().Update(ctx, attachment); err != nil { + // Collected with an interface deleted mid-pass. + if apierrors.IsNotFound(err) { + return nil + } return fmt.Errorf("update VPC attachment %s status: %w", client.ObjectKeyFromObject(attachment), err) } return nil @@ -490,6 +508,10 @@ func (r *NetworkInterfaceReconciler) markPrepared( } meta.SetStatusCondition(&networkInterface.Status.Conditions, condition) if err := r.Status().Update(ctx, networkInterface); err != nil { + // A guest torn down mid-pass leaves nothing to report on. + if apierrors.IsNotFound(err) { + return nil + } return fmt.Errorf("update network interface %s status: %w", client.ObjectKeyFromObject(networkInterface), err) } @@ -533,6 +555,9 @@ func (r *NetworkInterfaceReconciler) markPrepared( }) } if err := r.Status().Update(ctx, &claim); err != nil { + if apierrors.IsNotFound(err) { + return nil + } return fmt.Errorf("update network interface claim %s status: %w", key, err) } return nil diff --git a/internal/galactic/controller/networkinterface_writerace_test.go b/internal/galactic/controller/networkinterface_writerace_test.go new file mode 100644 index 00000000..5e45c378 --- /dev/null +++ b/internal/galactic/controller/networkinterface_writerace_test.go @@ -0,0 +1,102 @@ +/* +Copyright © 2026 Datum Technology, Inc. All rights reserved. + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU Affero General Public License as +published by the Free Software Foundation, either version 3 of the +License, or (at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Affero General Public License for more details. + +You should have received a copy of the GNU Affero General Public License +along with this program. If not, see . +*/ + +package controller + +import ( + "context" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + cloudv1alpha1 "go.datum.net/galactic/api/cloud/v1alpha1" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// reconcileWithFailingInterfaceStatus reconciles a bound interface whose VPC +// does not exist yet, so the pass ends on an interface status write, and makes +// that write return statusErr. +func reconcileWithFailingInterfaceStatus(t *testing.T, statusErr error) (ctrl.Result, error) { + t.Helper() + scheme := runtime.NewScheme() + for _, add := range []func(*runtime.Scheme) error{cloudv1alpha1.AddToScheme, networkingv1alpha.AddToScheme} { + if err := add(scheme); err != nil { + t.Fatal(err) + } + } + iface := &networkingv1alpha.NetworkInterface{ObjectMeta: metav1.ObjectMeta{Name: "nic", Namespace: "tenant"}} + iface.Status.Phase = networkingv1alpha.NetworkInterfacePhaseBound + iface.Status.NetworkContextRef = &networkingv1alpha.LocalNetworkContextRef{Name: "vpc"} + apimeta.SetStatusCondition(&iface.Status.Conditions, metav1.Condition{ + Type: networkingv1alpha.NetworkInterfaceAllocated, Status: metav1.ConditionTrue, Reason: "Allocated", + }) + cl := interceptor.NewClient( + fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(iface).WithObjects(iface).Build(), + interceptor.Funcs{ + SubResourceUpdate: func(context.Context, client.Client, string, client.Object, + ...client.SubResourceUpdateOption, + ) error { + return statusErr + }, + }) + r := &NetworkInterfaceReconciler{Client: cl, APIReader: cl, Scheme: scheme} + return r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(iface)}) +} + +var networkInterfaces = schema.GroupResource{Group: networkingv1alpha.GroupVersion.Group, Resource: "networkinterfaces"} + +// Losing a status write to the claim controller or galactic is normal +// convergence: it retries without counting as a reconcile error. +func TestAStatusWriteConflictRetriesWithoutError(t *testing.T) { + result, err := reconcileWithFailingInterfaceStatus(t, apierrors.NewConflict(networkInterfaces, "nic", nil)) + if err != nil { + t.Fatalf("Reconcile error = %v, want nil", err) + } + if result != (ctrl.Result{Requeue: true}) { + t.Fatalf("result = %+v, want a requeue", result) + } +} + +// An interface deleted after it was read has nothing left to report on. +func TestAnInterfaceDeletedMidPassIsNotAnError(t *testing.T) { + if _, err := reconcileWithFailingInterfaceStatus(t, apierrors.NewNotFound(networkInterfaces, "nic")); err != nil { + t.Fatalf("Reconcile error = %v, want nil", err) + } +} + +// Any other failure still surfaces as an error. +func TestOtherStatusWriteFailuresStillSurface(t *testing.T) { + if _, err := reconcileWithFailingInterfaceStatus(t, apierrors.NewForbidden(networkInterfaces, "nic", nil)); err == nil { + t.Fatal("Reconcile error = nil, want the forbidden write to surface") + } +} + +// A create that races the cache retries like a conflict. +func TestACreateRacingTheCacheRetries(t *testing.T) { + result, err := reconcileWithFailingInterfaceStatus(t, apierrors.NewAlreadyExists(networkInterfaces, "nic")) + if err != nil || result != (ctrl.Result{Requeue: true}) { + t.Fatalf("Reconcile = %+v, %v; want a requeue and no error", result, err) + } +}