diff --git a/internal/cloud/controller/bgpadvertisement_controller.go b/internal/cloud/controller/bgpadvertisement_controller.go index 642739ff..573790c7 100644 --- a/internal/cloud/controller/bgpadvertisement_controller.go +++ b/internal/cloud/controller/bgpadvertisement_controller.go @@ -70,10 +70,22 @@ type BGPAdvertisementReconciler struct { // +kubebuilder:rbac:groups=galactic.datumapis.com,resources=vpcattachments/status,verbs=get;update;patch func (r *BGPAdvertisementReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + err := r.reconcile(ctx, req) + if apierrors.IsConflict(err) { + // NSO writes the same attachment, interface and claim status. 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("status changed while reconciling; retrying", "reason", err.Error()) + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, err +} + +func (r *BGPAdvertisementReconciler) reconcile(ctx context.Context, req ctrl.Request) error { var advertisement bgpv1alpha1.BGPAdvertisement if err := r.Get(ctx, req.NamespacedName, &advertisement); err != nil { if !apierrors.IsNotFound(err) { - return ctrl.Result{}, err + return err } if r.APIReader != nil { var live bgpv1alpha1.BGPAdvertisement @@ -82,18 +94,18 @@ func (r *BGPAdvertisementReconciler) Reconcile(ctx context.Context, req ctrl.Req // Recreated faster than the cache saw it. The informer's // create event reconciles it again, so clearing now would // only flap the status. - return ctrl.Result{}, nil + return nil } if !apierrors.IsNotFound(err) { - return ctrl.Result{}, fmt.Errorf("confirm withdrawn advertisement %s: %w", req.NamespacedName, err) + return fmt.Errorf("confirm withdrawn advertisement %s: %w", req.NamespacedName, err) } } - return ctrl.Result{}, r.clearWithdrawnAdvertisement(ctx, req.NamespacedName) + return r.clearWithdrawnAdvertisement(ctx, req.NamespacedName) } vpc, attachmentID, ok := galactic.SplitAdvertisementName(advertisement.Name) if !ok { - return ctrl.Result{}, nil + return nil } // Ownership is checked before any status write: an advertisement on a @@ -101,19 +113,19 @@ func (r *BGPAdvertisementReconciler) Reconcile(ctx context.Context, req ctrl.Req // a tenant attachment, interface or claim, whatever its name looks like. router, err := r.routerFor(ctx, advertisement.Namespace, advertisement.Spec.RouterRef.Name) if err != nil { - return ctrl.Result{}, err + return err } if router != nil && !bgpv1alpha1.IsLegacyOwned(router) { - return ctrl.Result{}, nil + return nil } var attachments cloudv1alpha1.VPCAttachmentList if err := r.List(ctx, &attachments, client.MatchingFields{IndexVPCAttachmentIdentity: galactic.AdvertisementName(vpc, attachmentID)}); err != nil { - return ctrl.Result{}, fmt.Errorf("list VPC attachments for advertisement %s: %w", advertisement.Name, err) + return fmt.Errorf("list VPC attachments for advertisement %s: %w", advertisement.Name, err) } if len(attachments.Items) == 0 { - return ctrl.Result{}, nil + return nil } var node string @@ -136,15 +148,19 @@ func (r *BGPAdvertisementReconciler) Reconcile(ctx context.Context, req ctrl.Req } meta.SetStatusCondition(&attachment.Status.Conditions, programmed) if err := r.Status().Update(ctx, attachment); err != nil { - return ctrl.Result{}, fmt.Errorf("update VPC attachment %s status: %w", + // Collected with an interface deleted mid-pass. + if apierrors.IsNotFound(err) { + continue + } + return fmt.Errorf("update VPC attachment %s status: %w", client.ObjectKeyFromObject(attachment), err) } if err := r.projectOntoInterface(ctx, attachment, programmed); err != nil { - return ctrl.Result{}, err + return err } } - return ctrl.Result{}, nil + return nil } // clearWithdrawnAdvertisement removes delivery status only when no tenant @@ -192,7 +208,7 @@ func (r *BGPAdvertisementReconciler) clearWithdrawnAdvertisement(ctx context.Con if err := r.projectOntoInterface(ctx, attachment, programmed); err != nil { return err } - if err := r.Status().Update(ctx, attachment); err != nil { + if err := r.Status().Update(ctx, attachment); client.IgnoreNotFound(err) != nil { return fmt.Errorf("clear withdrawn attachment %s: %w", client.ObjectKeyFromObject(attachment), err) } } @@ -244,6 +260,10 @@ func (r *BGPAdvertisementReconciler) projectOntoInterface( interfaceProgrammed.ObservedGeneration = networkInterface.Generation meta.SetStatusCondition(&networkInterface.Status.Conditions, interfaceProgrammed) if err := r.Status().Update(ctx, &networkInterface); err != nil { + // A guest torn down mid-pass leaves nothing to certify. + if apierrors.IsNotFound(err) { + return nil + } return fmt.Errorf("update network interface %s status: %w", key, err) } if networkInterface.Spec.ClaimRef == nil { @@ -265,6 +285,9 @@ func (r *BGPAdvertisementReconciler) projectOntoInterface( }) } 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", claimKey, err) } return nil diff --git a/internal/cloud/controller/bgpadvertisement_writerace_test.go b/internal/cloud/controller/bgpadvertisement_writerace_test.go new file mode 100644 index 00000000..42fa502c --- /dev/null +++ b/internal/cloud/controller/bgpadvertisement_writerace_test.go @@ -0,0 +1,90 @@ +/* +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" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + ctrlreconcile "sigs.k8s.io/controller-runtime/pkg/reconcile" + + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// failInterfaceStatus makes every interface status write return err, the way +// a write from a stale cached copy, or to a deleted interface, does. +func failInterfaceStatus(f *ownershipFixture, err error) { + f.reconciler.Client = interceptor.NewClient(f.client.(client.WithWatch), interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c client.Client, sub string, obj client.Object, + opts ...client.SubResourceUpdateOption, + ) error { + if _, ok := obj.(*networkingv1alpha.NetworkInterface); ok { + return err + } + return c.SubResource(sub).Update(ctx, obj, opts...) + }, + }) +} + +// TestStatusConflictRetriesWithoutError: losing the interface status write +// to another writer is normal convergence, so it requeues instead of +// counting as a reconcile error. +func TestStatusConflictRetriesWithoutError(t *testing.T) { + f := newOwnershipFixture(t, false) + failInterfaceStatus(f, apierrors.NewConflict( + schema.GroupResource{Group: networkingv1alpha.GroupVersion.Group, Resource: "networkinterfaces"}, + f.iface.Name, nil)) + result, err := f.reconciler.Reconcile(context.Background(), ctrlreconcile.Request{ + NamespacedName: types.NamespacedName{Namespace: f.tenantAdv.Namespace, Name: f.tenantAdv.Name}, + }) + 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) + } +} + +// TestDeletedInterfaceIsNotAnError: an interface deleted after it was read +// leaves nothing to certify. +func TestDeletedInterfaceIsNotAnError(t *testing.T) { + f := newOwnershipFixture(t, false) + failInterfaceStatus(f, apierrors.NewNotFound( + schema.GroupResource{Group: networkingv1alpha.GroupVersion.Group, Resource: "networkinterfaces"}, + f.iface.Name)) + f.reconcile(t, f.tenantAdv) +} + +// Any other failure still surfaces as an error. +func TestOtherStatusWriteFailuresStillSurface(t *testing.T) { + f := newOwnershipFixture(t, false) + failInterfaceStatus(f, apierrors.NewForbidden( + schema.GroupResource{Group: networkingv1alpha.GroupVersion.Group, Resource: "networkinterfaces"}, + f.iface.Name, nil)) + if _, err := f.reconciler.Reconcile(context.Background(), ctrlreconcile.Request{ + NamespacedName: types.NamespacedName{Namespace: f.tenantAdv.Namespace, Name: f.tenantAdv.Name}, + }); err == nil { + t.Fatal("Reconcile error = nil, want the forbidden write to surface") + } +} diff --git a/internal/trafficpolicy/cell/reconcilers.go b/internal/trafficpolicy/cell/reconcilers.go index 183260aa..3bc890ae 100644 --- a/internal/trafficpolicy/cell/reconcilers.go +++ b/internal/trafficpolicy/cell/reconcilers.go @@ -299,6 +299,11 @@ func (r *BindingReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct } if err := r.Client.Status().Patch(ctx, &b, client.RawPatch(types.ApplyPatchType, body), client.FieldOwner(FieldOwner), client.ForceOwnership); err != nil { + // The binding goes with its interface, often between the read above + // and this apply. A deleted binding has no status to report. + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } return ctrl.Result{}, fmt.Errorf("apply TrafficPolicyBinding %s status: %w", req.NamespacedName, err) } if next > 0 { diff --git a/internal/trafficpolicy/cell/reconcilers_test.go b/internal/trafficpolicy/cell/reconcilers_test.go new file mode 100644 index 00000000..3dbcc319 --- /dev/null +++ b/internal/trafficpolicy/cell/reconcilers_test.go @@ -0,0 +1,50 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package cell + +import ( + "context" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + 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" + galacticv1alpha1 "go.datum.net/galactic/api/v1alpha1" +) + +// TestBindingDeletedBeforeStatusApply: a binding deleted with its interface +// between the read and the status apply is not a reconcile error. +func TestBindingDeletedBeforeStatusApply(t *testing.T) { + scheme := runtime.NewScheme() + for _, add := range []func(*runtime.Scheme) error{cloudv1alpha1.AddToScheme, galacticv1alpha1.AddToScheme} { + if err := add(scheme); err != nil { + t.Fatal(err) + } + } + binding := &galacticv1alpha1.TrafficPolicyBinding{ObjectMeta: metav1.ObjectMeta{Name: "nic", Namespace: "tenant"}} + cl := interceptor.NewClient(fake.NewClientBuilder().WithScheme(scheme).WithObjects(binding).Build(), + interceptor.Funcs{ + SubResourcePatch: func(_ context.Context, _ client.Client, _ string, obj client.Object, _ client.Patch, + _ ...client.SubResourcePatchOption, + ) error { + return apierrors.NewNotFound(galacticv1alpha1.GroupVersion.WithResource("trafficpolicybindings"). + GroupResource(), obj.GetName()) + }, + }) + r := &BindingReconciler{Client: cl} + result, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(binding)}) + if err != nil { + t.Fatalf("Reconcile error = %v, want nil", err) + } + if result != (ctrl.Result{}) { + t.Fatalf("result = %+v, want none", result) + } +}