From e28116ae6f1fdac52c7e8a9e4d91ef4ab27b43bf Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 26 Aug 2026 14:55:54 -0500 Subject: [PATCH 1/2] feat: report holder availability on interfaces A networking NetworkService needs to know whether a member can serve, but the only health signal on a NetworkInterface was Programmed, written solely by the VPC fabric. Where the fabric is not attached nothing ever writes it, so every member reads unhealthy and no traffic is served even though membership and locality resolve correctly. Compute holds the interface and knows whether the instance behind it is serving, so compute publishes that. A HolderAvailable condition is projected onto every NetworkInterface an instance holds, mirroring the instance's own Available condition. Networking reads it and never learns that the holder is a workload. Anything other than Available=True projects as False carrying the instance's own reason, so Unknown drains the member rather than leaving it silently taking traffic. The projection runs on every reconcile, not only on a transition, so interfaces that predate it converge without their workload being redeployed, and it writes only when the condition moves. Key changes: - Project HolderAvailable onto each interface named by status.networkInterfaces[].networkInterfaceRef - Run the projection on the suspension path too, so a suspended instance drains before the hub is told - Grant the controller networkinterfaces/status write access --- config/components/controller_rbac/role.yaml | 8 + internal/controller/instance_controller.go | 11 + .../controller/networkinterface_holder.go | 127 +++++++ .../networkinterface_holder_test.go | 311 ++++++++++++++++++ 4 files changed, 457 insertions(+) create mode 100644 internal/controller/networkinterface_holder.go create mode 100644 internal/controller/networkinterface_holder_test.go diff --git a/config/components/controller_rbac/role.yaml b/config/components/controller_rbac/role.yaml index 935b4b8a..acedcc22 100644 --- a/config/components/controller_rbac/role.yaml +++ b/config/components/controller_rbac/role.yaml @@ -98,6 +98,14 @@ rules: - get - list - watch +- apiGroups: + - networking.datumapis.com + resources: + - networkinterfaces/status + verbs: + - get + - patch + - update - apiGroups: - quota.miloapis.com resources: diff --git a/internal/controller/instance_controller.go b/internal/controller/instance_controller.go index 4dfb8358..1932aeb8 100644 --- a/internal/controller/instance_controller.go +++ b/internal/controller/instance_controller.go @@ -250,6 +250,7 @@ type InstanceReconciler struct { // +kubebuilder:rbac:groups=quota.miloapis.com,resources=resourceclaims,verbs=get;list;watch;create;delete // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networkinterfaceclaims,verbs=get;list;watch // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networkinterfaces,verbs=get;list;watch +// +kubebuilder:rbac:groups=networking.datumapis.com,resources=networkinterfaces/status,verbs=get;update;patch // +kubebuilder:rbac:groups="",resources=namespaces,verbs=get // +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch @@ -384,6 +385,10 @@ func (r *InstanceReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ return ctrl.Result{}, err } + if err := r.reconcileHolderAvailability(ctx, cl.GetClient(), &instance); err != nil { + return ctrl.Result{}, err + } + if err := r.writeBackToUpstream(ctx, &instance); err != nil { if quotaReq > 0 && apierrors.IsConflict(err) { logger.Info("upstream writeback conflicted while quota pending; requeuing instead of error-backoff", @@ -999,6 +1004,12 @@ func (r *InstanceReconciler) reconcileSuspendedState( } } + // A suspended instance must stop receiving traffic, so the interfaces it + // holds are told before the hub is. + if err := r.reconcileHolderAvailability(ctx, cl, instance); err != nil { + return err + } + // Write suspended status back to the federation hub so the management // plane aggregates the correct per-instance state. return r.writeBackToUpstream(ctx, instance) diff --git a/internal/controller/networkinterface_holder.go b/internal/controller/networkinterface_holder.go new file mode 100644 index 00000000..a8ca88cf --- /dev/null +++ b/internal/controller/networkinterface_holder.go @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controller + +import ( + "context" + "errors" + "fmt" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + computev1alpha "go.datum.net/compute/api/v1alpha" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +// networkInterfaceHolderAvailable is the condition compute publishes on every +// NetworkInterface an instance holds, reporting whether the holder considers +// itself able to serve. Networking selects members by it without ever learning +// that the holder is a workload. +// +// It is deliberately not NetworkInterfacePhaseAvailable, which reports that no +// claim holds the interface — the opposite of a healthy holder. +const networkInterfaceHolderAvailable = "HolderAvailable" + +const ( + // holderReasonNotReported covers an interface whose holder has published no + // availability state at all, which is not the same as a holder that reported + // itself unavailable. + holderReasonNotReported = "HolderNotReported" + + // holderReasonUnavailable is the fallback for a holder that reported itself + // unavailable without naming a reason. + holderReasonUnavailable = "HolderUnavailable" +) + +const msgHolderNotReported = "The instance holding this interface has not reported an availability state" + +// holderAvailableCondition translates an instance's Available condition into the +// condition networking reads on the interfaces it holds. +// +// Anything other than Available=True yields False, carrying the instance's own +// reason and message: Unknown means nobody has vouched for the instance, and a +// member that cannot be vouched for must drain rather than keep taking traffic. +func holderAvailableCondition(instance *computev1alpha.Instance) metav1.Condition { + condition := metav1.Condition{ + Type: networkInterfaceHolderAvailable, + Status: metav1.ConditionFalse, + Reason: holderReasonNotReported, + Message: msgHolderNotReported, + } + + available := apimeta.FindStatusCondition(instance.Status.Conditions, computev1alpha.InstanceAvailable) + if available == nil { + return condition + } + + if available.Status == metav1.ConditionTrue { + condition.Status = metav1.ConditionTrue + condition.Reason = computev1alpha.InstanceAvailableReasonAvailable + condition.Message = msgInstanceAvailable + return condition + } + + if available.Reason != "" { + condition.Reason = available.Reason + } else { + condition.Reason = holderReasonUnavailable + } + condition.Message = available.Message + if condition.Message == "" { + condition.Message = fmt.Sprintf("Instance %q is not available", instance.Name) + } + return condition +} + +// reconcileHolderAvailability projects the instance's availability onto every +// NetworkInterface it holds, so networking can decide membership health without +// depending on a fabric that may not be attached. +// +// It runs on every pass rather than only on a transition, so interfaces that +// predate the condition pick it up without their workload being redeployed. +// Writes only when the condition actually moves, so a steady instance produces +// no API traffic. +func (r *InstanceReconciler) reconcileHolderAvailability( + ctx context.Context, + clusterClient client.Client, + instance *computev1alpha.Instance, +) error { + if !r.NetworkingEnabled { + return nil + } + + condition := holderAvailableCondition(instance) + + var errs []error + for _, interfaceStatus := range instance.Status.NetworkInterfaces { + ref := interfaceStatus.NetworkInterfaceRef + if ref == nil || ref.Name == "" { + continue + } + + key := client.ObjectKey{Namespace: instance.Namespace, Name: ref.Name} + var networkInterface networkingv1alpha.NetworkInterface + if err := clusterClient.Get(ctx, key, &networkInterface); err != nil { + if apierrors.IsNotFound(err) { + continue + } + errs = append(errs, fmt.Errorf("get network interface %s: %w", key, err)) + continue + } + + projected := condition + projected.ObservedGeneration = networkInterface.Generation + if !apimeta.SetStatusCondition(&networkInterface.Status.Conditions, projected) { + continue + } + + if err := clusterClient.Status().Update(ctx, &networkInterface); err != nil { + errs = append(errs, fmt.Errorf("update network interface %s status: %w", key, err)) + } + } + + return errors.Join(errs...) +} diff --git a/internal/controller/networkinterface_holder_test.go b/internal/controller/networkinterface_holder_test.go new file mode 100644 index 00000000..43c2f374 --- /dev/null +++ b/internal/controller/networkinterface_holder_test.go @@ -0,0 +1,311 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package controller + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + computev1alpha "go.datum.net/compute/api/v1alpha" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" +) + +const holderTestInterface = "holder-test-eth0" + +// newHolderTestInstance builds an instance already publishing one bound +// interface, which is the shape every instance reaches once its claim binds. +func newHolderTestInstance(available *metav1.Condition) *computev1alpha.Instance { + instance := &computev1alpha.Instance{ + ObjectMeta: metav1.ObjectMeta{ + Name: "holder-test-instance", + Namespace: claimTestNamespace, + }, + Status: computev1alpha.InstanceStatus{ + NetworkInterfaces: []computev1alpha.InstanceNetworkInterfaceStatus{{ + Name: defaultInterfaceName, + NetworkInterfaceRef: &networkingv1alpha.LocalNetworkInterfaceRef{ + Name: holderTestInterface, + }, + }}, + }, + } + if available != nil { + apimeta.SetStatusCondition(&instance.Status.Conditions, *available) + } + return instance +} + +func newHolderTestInterface() *networkingv1alpha.NetworkInterface { + return &networkingv1alpha.NetworkInterface{ + ObjectMeta: metav1.ObjectMeta{ + Name: holderTestInterface, + Namespace: claimTestNamespace, + Generation: 4, + }, + } +} + +func holderTestClient(objects ...client.Object) client.Client { + return fake.NewClientBuilder(). + WithScheme(newClaimTestScheme()). + WithObjects(objects...). + WithStatusSubresource(&networkingv1alpha.NetworkInterface{}, &computev1alpha.Instance{}). + Build() +} + +func getHolderCondition(t *testing.T, cl client.Client) *metav1.Condition { + t.Helper() + var networkInterface networkingv1alpha.NetworkInterface + require.NoError(t, cl.Get(context.Background(), client.ObjectKey{ + Namespace: claimTestNamespace, + Name: holderTestInterface, + }, &networkInterface)) + return apimeta.FindStatusCondition(networkInterface.Status.Conditions, networkInterfaceHolderAvailable) +} + +// TestHolderAvailableCondition covers the translation from the instance's own +// Available condition, including the states that must not read as healthy. +func TestHolderAvailableCondition(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + available *metav1.Condition + wantStatus metav1.ConditionStatus + wantReason string + wantMessage string + }{ + { + name: "available instance vouches for its interfaces", + available: &metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + }, + wantStatus: metav1.ConditionTrue, + wantReason: computev1alpha.InstanceAvailableReasonAvailable, + wantMessage: msgInstanceAvailable, + }, + { + name: "stopped instance carries its own reason", + available: &metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionFalse, + Reason: computev1alpha.InstanceAvailableReasonStopped, + Message: "Instance is stopped", + }, + wantStatus: metav1.ConditionFalse, + wantReason: computev1alpha.InstanceAvailableReasonStopped, + wantMessage: "Instance is stopped", + }, + { + name: "unknown availability does not read as healthy", + available: &metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionUnknown, + Reason: computev1alpha.InstanceReadyReasonImageUnavailable, + Message: "Image could not be pulled", + }, + wantStatus: metav1.ConditionFalse, + wantReason: computev1alpha.InstanceReadyReasonImageUnavailable, + wantMessage: "Image could not be pulled", + }, + { + name: "instance that has reported nothing is distinguishable", + available: nil, + wantStatus: metav1.ConditionFalse, + wantReason: holderReasonNotReported, + wantMessage: msgHolderNotReported, + }, + { + name: "unavailable without a message still says something", + available: &metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionFalse, + Reason: computev1alpha.InstanceAvailableReasonStarting, + }, + wantStatus: metav1.ConditionFalse, + wantReason: computev1alpha.InstanceAvailableReasonStarting, + wantMessage: `Instance "holder-test-instance" is not available`, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + condition := holderAvailableCondition(newHolderTestInstance(tc.available)) + + assert.Equal(t, networkInterfaceHolderAvailable, condition.Type) + assert.Equal(t, tc.wantStatus, condition.Status) + assert.Equal(t, tc.wantReason, condition.Reason) + assert.Equal(t, tc.wantMessage, condition.Message) + }) + } +} + +// TestReconcileHolderAvailability_BecomesAvailable covers the transition an +// existing interface must pick up on an ordinary reconcile pass. +func TestReconcileHolderAvailability_BecomesAvailable(t *testing.T) { + t.Parallel() + + instance := newHolderTestInstance(&metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + }) + cl := holderTestClient(instance, newHolderTestInterface()) + + r := &InstanceReconciler{NetworkingEnabled: true} + require.NoError(t, r.reconcileHolderAvailability(context.Background(), cl, instance)) + + condition := getHolderCondition(t, cl) + require.NotNil(t, condition) + assert.Equal(t, metav1.ConditionTrue, condition.Status) + assert.Equal(t, computev1alpha.InstanceAvailableReasonAvailable, condition.Reason) + assert.Equal(t, int64(4), condition.ObservedGeneration, + "the interface's generation, not the instance's") +} + +// TestReconcileHolderAvailability_LeavesAvailable covers the drain path: an +// interface already vouched for must be told when the holder stops serving. +func TestReconcileHolderAvailability_LeavesAvailable(t *testing.T) { + t.Parallel() + + instance := newHolderTestInstance(&metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + }) + cl := holderTestClient(instance, newHolderTestInterface()) + + r := &InstanceReconciler{NetworkingEnabled: true} + require.NoError(t, r.reconcileHolderAvailability(context.Background(), cl, instance)) + require.Equal(t, metav1.ConditionTrue, getHolderCondition(t, cl).Status) + + apimeta.SetStatusCondition(&instance.Status.Conditions, metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionFalse, + Reason: computev1alpha.InstanceAvailableReasonStopping, + Message: "Instance is stopping", + }) + require.NoError(t, r.reconcileHolderAvailability(context.Background(), cl, instance)) + + condition := getHolderCondition(t, cl) + require.NotNil(t, condition) + assert.Equal(t, metav1.ConditionFalse, condition.Status) + assert.Equal(t, computev1alpha.InstanceAvailableReasonStopping, condition.Reason) + assert.Equal(t, "Instance is stopping", condition.Message) +} + +// TestReconcileHolderAvailability_Stable covers convergence: a pass that +// changes nothing must not write, so a steady fleet produces no API traffic and +// no transition timestamp churn. +func TestReconcileHolderAvailability_Stable(t *testing.T) { + t.Parallel() + + instance := newHolderTestInstance(&metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + }) + cl := holderTestClient(instance, newHolderTestInterface()) + + r := &InstanceReconciler{NetworkingEnabled: true} + require.NoError(t, r.reconcileHolderAvailability(context.Background(), cl, instance)) + + var first networkingv1alpha.NetworkInterface + require.NoError(t, cl.Get(context.Background(), client.ObjectKey{ + Namespace: claimTestNamespace, Name: holderTestInterface, + }, &first)) + + require.NoError(t, r.reconcileHolderAvailability(context.Background(), cl, instance)) + + var second networkingv1alpha.NetworkInterface + require.NoError(t, cl.Get(context.Background(), client.ObjectKey{ + Namespace: claimTestNamespace, Name: holderTestInterface, + }, &second)) + + assert.Equal(t, first.ResourceVersion, second.ResourceVersion, + "a no-op pass must not write the interface") + assert.Equal(t, first.Status.Conditions, second.Status.Conditions) +} + +// TestReconcileHolderAvailability_NoInterface covers the two ways there is +// nothing to write to, neither of which is an error. +func TestReconcileHolderAvailability_NoInterface(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + instance func(*computev1alpha.Instance) + objects []client.Object + }{ + { + name: "interface object does not exist", + }, + { + name: "status entry carries no interface reference", + instance: func(i *computev1alpha.Instance) { + i.Status.NetworkInterfaces[0].NetworkInterfaceRef = nil + }, + objects: []client.Object{newHolderTestInterface()}, + }, + { + name: "instance publishes no interfaces at all", + instance: func(i *computev1alpha.Instance) { + i.Status.NetworkInterfaces = nil + }, + objects: []client.Object{newHolderTestInterface()}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + instance := newHolderTestInstance(&metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + }) + if tc.instance != nil { + tc.instance(instance) + } + + cl := holderTestClient(append([]client.Object{instance}, tc.objects...)...) + + r := &InstanceReconciler{NetworkingEnabled: true} + assert.NoError(t, r.reconcileHolderAvailability(context.Background(), cl, instance)) + + if len(tc.objects) > 0 { + assert.Nil(t, getHolderCondition(t, cl)) + } + }) + } +} + +// TestReconcileHolderAvailability_NetworkingDisabled covers a cell without the +// networking CRDs, where the interface kind is not even served. +func TestReconcileHolderAvailability_NetworkingDisabled(t *testing.T) { + t.Parallel() + + instance := newHolderTestInstance(&metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + }) + cl := holderTestClient(instance, newHolderTestInterface()) + + r := &InstanceReconciler{NetworkingEnabled: false} + require.NoError(t, r.reconcileHolderAvailability(context.Background(), cl, instance)) + + assert.Nil(t, getHolderCondition(t, cl)) +} From 7a19ed177d1987e91b12a13239cf52ff24858ba9 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 26 Aug 2026 15:30:13 -0500 Subject: [PATCH 2/2] fix: drain interfaces when an instance terminates Deletion is the drain path that matters. Scale-down, rolling update, and redeploy all end an instance by deleting it, and the deletion reconcile returns before availability was ever projected. Nothing clears the instance's Available condition on the way out either, so HolderAvailable stayed True for the whole deletion window and the edge kept routing to a process shutting down. Deletion is now judged ahead of the instance's own Available condition, and the deletion reconcile drains the interfaces it held before releasing anything else. That write is best effort: an instance that cannot reach its interfaces must still finish deleting, or a transient API error strands it in Terminating forever. A retained interface outlives the instance that held it, so a True left behind could be inherited by the next instance in the slot before it is serving. Draining on delete removes it at the source, and the projection derives the condition wholly from the current holder rather than merging with what is on the interface, so a successor starts False regardless. The projection also moved above the quota and status-update returns in the main reconcile. Those returns skipped it, and an instance stuck waiting on quota is exactly one that should not be receiving traffic. Key changes: - Report HolderTerminating=False for an instance with a deletion timestamp - Drain from reconcileDeletion before quota cleanup and finalizer removal, logging rather than returning a drain failure - Project before the early returns that previously skipped it --- internal/controller/instance_controller.go | 18 +- .../controller/networkinterface_holder.go | 46 +++++- .../networkinterface_holder_test.go | 156 ++++++++++++++++++ 3 files changed, 215 insertions(+), 5 deletions(-) diff --git a/internal/controller/instance_controller.go b/internal/controller/instance_controller.go index 1932aeb8..e637ef44 100644 --- a/internal/controller/instance_controller.go +++ b/internal/controller/instance_controller.go @@ -347,6 +347,14 @@ func (r *InstanceReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ readyErr = fmt.Errorf("failed reconciling network interface status: %w", interfacesErr) } + // Publish availability to the interfaces the instance holds while every + // condition is settled in memory. This sits above the quota and status-update + // returns below, because an instance stuck on one of them is precisely an + // instance that should stop receiving traffic. + if err := r.reconcileHolderAvailability(ctx, cl.GetClient(), &instance); err != nil && readyErr == nil { + readyErr = err + } + if statusChanged || readyChanged { if err := cl.GetClient().Status().Update(ctx, &instance); err != nil { if quotaReq > 0 && apierrors.IsConflict(err) { @@ -385,10 +393,6 @@ func (r *InstanceReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ return ctrl.Result{}, err } - if err := r.reconcileHolderAvailability(ctx, cl.GetClient(), &instance); err != nil { - return ctrl.Result{}, err - } - if err := r.writeBackToUpstream(ctx, &instance); err != nil { if quotaReq > 0 && apierrors.IsConflict(err) { logger.Info("upstream writeback conflicted while quota pending; requeuing instead of error-backoff", @@ -1018,6 +1022,12 @@ func (r *InstanceReconciler) reconcileSuspendedState( // reconcileDeletion handles quota-claim cleanup when an Instance is being // deleted. It removes the quota finalizer once the ResourceClaim is gone. func (r *InstanceReconciler) reconcileDeletion(ctx context.Context, cl client.Client, clusterName multicluster.ClusterName, instance *computev1alpha.Instance) error { + // Stop traffic first. Deletion is the drain path that matters — every + // scale-down, rolling update, and redeploy goes through it — and the + // interfaces must be told before the instance's own cleanup runs, not after. + // Best effort: this must never be what keeps an instance in Terminating. + r.drainHolderAvailability(ctx, cl, instance) + if !controllerutil.ContainsFinalizer(instance, instanceQuotaFinalizer) { return nil } diff --git a/internal/controller/networkinterface_holder.go b/internal/controller/networkinterface_holder.go index a8ca88cf..d295c382 100644 --- a/internal/controller/networkinterface_holder.go +++ b/internal/controller/networkinterface_holder.go @@ -11,6 +11,7 @@ import ( apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" computev1alpha "go.datum.net/compute/api/v1alpha" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" @@ -34,9 +35,18 @@ const ( // holderReasonUnavailable is the fallback for a holder that reported itself // unavailable without naming a reason. holderReasonUnavailable = "HolderUnavailable" + + // holderReasonTerminating covers a holder being deleted. Its own Available + // condition can still read True for the whole deletion window, so deletion is + // judged before it. + holderReasonTerminating = "HolderTerminating" ) -const msgHolderNotReported = "The instance holding this interface has not reported an availability state" +const ( + msgHolderNotReported = "The instance holding this interface has not reported an availability state" + + msgHolderTerminating = "The instance holding this interface is terminating" +) // holderAvailableCondition translates an instance's Available condition into the // condition networking reads on the interfaces it holds. @@ -52,6 +62,16 @@ func holderAvailableCondition(instance *computev1alpha.Instance) metav1.Conditio Message: msgHolderNotReported, } + // A terminating instance is not serving, whatever it last said about itself: + // nothing clears Available on the way out, so it reads True for the whole + // deletion window. Deletion is decided first so a scale-down or a rolling + // update drains the member instead of routing to a process shutting down. + if !instance.DeletionTimestamp.IsZero() { + condition.Reason = holderReasonTerminating + condition.Message = msgHolderTerminating + return condition + } + available := apimeta.FindStatusCondition(instance.Status.Conditions, computev1alpha.InstanceAvailable) if available == nil { return condition @@ -84,6 +104,11 @@ func holderAvailableCondition(instance *computev1alpha.Instance) metav1.Conditio // predate the condition pick it up without their workload being redeployed. // Writes only when the condition actually moves, so a steady instance produces // no API traffic. +// +// The condition is derived wholly from the current holder, never merged with +// what is already on the interface. A reclaimPolicy Retain interface outlives +// the instance that held it, so a True left by a previous holder must not be +// inherited by the next one before it is serving. func (r *InstanceReconciler) reconcileHolderAvailability( ctx context.Context, clusterClient client.Client, @@ -125,3 +150,22 @@ func (r *InstanceReconciler) reconcileHolderAvailability( return errors.Join(errs...) } + +// drainHolderAvailability reports the holder as gone from the interfaces it +// held, without letting that write become a reason the caller cannot proceed. +// +// It is used on the deletion path, where the failure to reach an interface must +// not wedge finalizer removal: an instance that cannot drain must still be able +// to finish deleting, or a transient API error strands it in Terminating +// forever. The interface being gone already is the expected case under a +// reclaimPolicy of Delete and is not a failure at all. +func (r *InstanceReconciler) drainHolderAvailability( + ctx context.Context, + clusterClient client.Client, + instance *computev1alpha.Instance, +) { + if err := r.reconcileHolderAvailability(ctx, clusterClient, instance); err != nil { + log.FromContext(ctx).Error(err, "failed draining network interfaces; deletion continues", + "instance", instance.Name, "namespace", instance.Namespace) + } +} diff --git a/internal/controller/networkinterface_holder_test.go b/internal/controller/networkinterface_holder_test.go index 43c2f374..84437cb9 100644 --- a/internal/controller/networkinterface_holder_test.go +++ b/internal/controller/networkinterface_holder_test.go @@ -4,6 +4,7 @@ package controller import ( "context" + "errors" "testing" "github.com/stretchr/testify/assert" @@ -309,3 +310,158 @@ func TestReconcileHolderAvailability_NetworkingDisabled(t *testing.T) { assert.Nil(t, getHolderCondition(t, cl)) } + +// TestHolderAvailableCondition_Terminating covers the case the instance's own +// Available condition cannot express: nothing clears it on the way out, so a +// terminating instance still reports itself available. +func TestHolderAvailableCondition_Terminating(t *testing.T) { + t.Parallel() + + instance := newHolderTestInstance(&metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + }) + deleting := metav1.Now() + instance.DeletionTimestamp = &deleting + + condition := holderAvailableCondition(instance) + + assert.Equal(t, metav1.ConditionFalse, condition.Status) + assert.Equal(t, holderReasonTerminating, condition.Reason) + assert.Equal(t, msgHolderTerminating, condition.Message) +} + +// TestReconcileDeletion_Drains covers the drain path that carries every +// scale-down, rolling update, and redeploy: a terminating instance must stop +// being routed to before its cleanup runs. +func TestReconcileDeletion_Drains(t *testing.T) { + t.Parallel() + + instance := newHolderTestInstance(&metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + }) + + networkInterface := newHolderTestInterface() + apimeta.SetStatusCondition(&networkInterface.Status.Conditions, metav1.Condition{ + Type: networkInterfaceHolderAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + }) + + deleting := metav1.Now() + instance.DeletionTimestamp = &deleting + instance.Finalizers = []string{instanceQuotaFinalizer} + + cl := holderTestClient(instance, networkInterface) + + r := &InstanceReconciler{NetworkingEnabled: true} + require.NoError(t, r.reconcileDeletion(context.Background(), cl, "test-cluster", instance)) + + condition := getHolderCondition(t, cl) + require.NotNil(t, condition) + assert.Equal(t, metav1.ConditionFalse, condition.Status) + assert.Equal(t, holderReasonTerminating, condition.Reason) +} + +// TestReconcileDeletion_DrainFailureDoesNotWedge covers the ordering rule: an +// instance that cannot reach its interfaces must still finish deleting, or a +// transient API error strands it in Terminating forever. +func TestReconcileDeletion_DrainFailureDoesNotWedge(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + client func(client.Client) client.Client + objects []client.Object + }{ + { + name: "interface is already gone", + objects: nil, + }, + { + name: "the status write fails", + objects: []client.Object{newHolderTestInterface()}, + client: func(cl client.Client) client.Client { + return &holderFailingStatusClient{Client: cl} + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + instance := newHolderTestInstance(&metav1.Condition{ + Type: computev1alpha.InstanceAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + }) + deleting := metav1.Now() + instance.DeletionTimestamp = &deleting + instance.Finalizers = []string{instanceQuotaFinalizer} + + cl := holderTestClient(append([]client.Object{instance}, tc.objects...)...) + if tc.client != nil { + cl = tc.client(cl) + } + + r := &InstanceReconciler{NetworkingEnabled: true} + require.NoError(t, r.reconcileDeletion(context.Background(), cl, "test-cluster", instance), + "a drain failure must not block deletion") + + assert.NotContains(t, instance.Finalizers, instanceQuotaFinalizer, + "the finalizer must still be released") + }) + } +} + +// TestReconcileHolderAvailability_StaleTrueNotInherited covers a reclaimPolicy +// Retain interface outliving the instance that held it: the next instance in +// the slot must not begin life looking healthy on a predecessor's condition. +func TestReconcileHolderAvailability_StaleTrueNotInherited(t *testing.T) { + t.Parallel() + + retained := newHolderTestInterface() + apimeta.SetStatusCondition(&retained.Status.Conditions, metav1.Condition{ + Type: networkInterfaceHolderAvailable, + Status: metav1.ConditionTrue, + Reason: computev1alpha.InstanceAvailableReasonAvailable, + Message: msgInstanceAvailable, + }) + + // The successor has rebound the interface but has not started serving, which + // on a fresh instance means no Available condition at all. + successor := newHolderTestInstance(nil) + successor.Name = "holder-test-instance-successor" + + cl := holderTestClient(successor, retained) + + r := &InstanceReconciler{NetworkingEnabled: true} + require.NoError(t, r.reconcileHolderAvailability(context.Background(), cl, successor)) + + condition := getHolderCondition(t, cl) + require.NotNil(t, condition) + assert.Equal(t, metav1.ConditionFalse, condition.Status) + assert.Equal(t, holderReasonNotReported, condition.Reason) +} + +// holderFailingStatusClient fails every status write, standing in for an +// interface the API server will not let the instance update on its way out. +type holderFailingStatusClient struct { + client.Client +} + +func (c *holderFailingStatusClient) Status() client.SubResourceWriter { + return &holderFailingStatusWriter{SubResourceWriter: c.Client.Status()} +} + +type holderFailingStatusWriter struct { + client.SubResourceWriter +} + +func (w *holderFailingStatusWriter) Update(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { + return errors.New("status update refused") +}