From 7c48a0bad0c3ec3d4f5004a086b49f12fce6345b Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Fri, 14 Aug 2026 07:30:24 -0500 Subject: [PATCH 1/2] feat: resolve a deployment's location from the cell's ServingLocation A cell could not tell users where their workload was running. Compute asked the cell for a Location matching the deployment's city, but Locations do not exist on cells, so the answer was always "no location" and every instance ran without one. Cells are now delivered a ServingLocation naming the place they sit in; read that instead. This also makes a misplaced deployment visible for the first time. A deployment that reaches a cell serving another city now says so, and is held out of service rather than quietly running in a city nobody asked for. A cell that has not been identified yet still runs workloads, it just reports what it is waiting for. Instances now carry their location from the moment they are created, and any instance that predates the cell learning where it is gets it backfilled. Co-Authored-By: Claude Opus 5 --- api/v1alpha/instance_types.go | 17 +- config/components/controller_rbac/role.yaml | 16 +- go.mod | 11 +- go.sum | 4 +- internal/controller/indexers.go | 18 +- internal/controller/workload_controller.go | 2 +- .../workloaddeployment_controller.go | 236 +++++++---- .../workloaddeployment_controller_test.go | 22 +- .../workloaddeployment_federator.go | 3 +- .../workloaddeployment_location_test.go | 376 ++++++++++++------ internal/webhook/v1alpha/workload_webhook.go | 4 +- 11 files changed, 461 insertions(+), 248 deletions(-) diff --git a/api/v1alpha/instance_types.go b/api/v1alpha/instance_types.go index ffcf582f..b8e14a7a 100644 --- a/api/v1alpha/instance_types.go +++ b/api/v1alpha/instance_types.go @@ -838,10 +838,23 @@ const ( WorkloadReasonNetworkNotFound = "NetworkNotFound" // WorkloadDeploymentReasonNoMatchingLocation is set on WorkloadDeployment.Available - // while no Location matches the deployment's city code. The message names the - // unresolved city; network provisioning cannot start until that Location exists. + // while the cell has not been told which location it serves, so the deployment + // cannot be given one. The value is kept for compatibility with clients that + // already match on it. WorkloadDeploymentReasonNoMatchingLocation = "NoMatchingLocation" + // WorkloadDeploymentReasonAmbiguousServingLocation is set on + // WorkloadDeployment.Available when more than one location has been delivered + // to the cell. The cell will not guess which one it serves, so the deployment + // waits until the platform resolves the conflict. + WorkloadDeploymentReasonAmbiguousServingLocation = "AmbiguousServingLocation" + + // WorkloadDeploymentReasonCityCodeMismatch is set on + // WorkloadDeployment.Available when the deployment asks for one city and the + // cell serves another. It means the deployment was placed on the wrong cell, + // which is a platform fault rather than anything the user can correct. + WorkloadDeploymentReasonCityCodeMismatch = "CityCodeMismatch" + // WorkloadDeploymentReasonNetworkProvisioning is set on WorkloadDeployment.Available // while the network binding or subnet is still being provisioned. // Replaces the previously-emitted inline literal "ProvisioningNetwork". diff --git a/config/components/controller_rbac/role.yaml b/config/components/controller_rbac/role.yaml index 236bff0b..935b4b8a 100644 --- a/config/components/controller_rbac/role.yaml +++ b/config/components/controller_rbac/role.yaml @@ -79,24 +79,24 @@ rules: - apiGroups: - networking.datumapis.com resources: - - locations - - networkinterfaces - - networks + - networkinterfaceclaims verbs: + - create + - delete - get - list + - patch + - update - watch - apiGroups: - networking.datumapis.com resources: - - networkinterfaceclaims + - networkinterfaces + - networks + - servinglocations verbs: - - create - - delete - get - list - - patch - - update - watch - apiGroups: - quota.miloapis.com diff --git a/go.mod b/go.mod index b9e749fd..b99706d2 100644 --- a/go.mod +++ b/go.mod @@ -10,9 +10,10 @@ require ( github.com/onsi/gomega v1.42.1 github.com/prometheus/client_golang v1.23.2 github.com/stretchr/testify v1.11.1 - // TODO: temporary pin to the NetworkInterfaceClaim branch (datum-cloud/network-services-operator#360). - // Re-pin to a tagged release before merging. - go.datum.net/network-services-operator v0.25.6-0.20260813185515-6a019e3fd9b8 + // TODO: temporary pin to network-services-operator main. The latest tag + // (v0.25.5) predates both NetworkInterfaceClaim (#360) and ServingLocation + // (#372); re-pin to a tagged release before compute cuts a release. + go.datum.net/network-services-operator v0.25.6-0.20260814031118-aba36815ac91 go.miloapis.com/milo v0.32.0 golang.org/x/crypto v0.54.0 golang.org/x/sync v0.22.0 @@ -112,7 +113,7 @@ require ( gopkg.in/evanphx/json-patch.v4 v4.13.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - k8s.io/apiextensions-apiserver v0.36.1 + k8s.io/apiextensions-apiserver v0.36.1 // indirect k8s.io/apiserver v0.36.1 // indirect k8s.io/klog/v2 v2.140.0 // indirect k8s.io/kube-openapi v0.0.0-20260330154417-16be699c7b31 // indirect @@ -120,5 +121,5 @@ require ( sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect sigs.k8s.io/randfill v1.0.0 // indirect sigs.k8s.io/structured-merge-diff/v6 v6.3.2 // indirect - sigs.k8s.io/yaml v1.6.0 + sigs.k8s.io/yaml v1.6.0 // indirect ) diff --git a/go.sum b/go.sum index 380e5aaf..47176e16 100644 --- a/go.sum +++ b/go.sum @@ -175,8 +175,8 @@ github.com/tidwall/sjson v1.2.5 h1:kLy8mja+1c9jlljvWTlSazM7cKDRfJuR/bOJhcY5NcY= github.com/tidwall/sjson v1.2.5/go.mod h1:Fvgq9kS/6ociJEDnK0Fk1cpYF4FIW6ZF7LAe+6jwd28= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= -go.datum.net/network-services-operator v0.25.6-0.20260813185515-6a019e3fd9b8 h1:R82aXI8iC+6JMm5WjLxpBJRgRTPUACLlvxOW4svZ03Y= -go.datum.net/network-services-operator v0.25.6-0.20260813185515-6a019e3fd9b8/go.mod h1:oz57/uTB5HAjLGrqs1pn1HTP/jW9vkGgMODPvCiUlvc= +go.datum.net/network-services-operator v0.25.6-0.20260814031118-aba36815ac91 h1:9jo8ZB4TMFjOWVu0XEdND46QR3hJRepYYzUaBRUkV5w= +go.datum.net/network-services-operator v0.25.6-0.20260814031118-aba36815ac91/go.mod h1:oz57/uTB5HAjLGrqs1pn1HTP/jW9vkGgMODPvCiUlvc= go.miloapis.com/milo v0.32.0 h1:TkNIQu/37d+SEquLJ5+GmdisSl+K2RT7eEC4idg6RIs= go.miloapis.com/milo v0.32.0/go.mod h1:GKK3afjCwshfZfvhjNe1wp/H45z4m7x5oG/8xbSgU1M= go.miloapis.com/service-catalog v0.4.0 h1:LvO1WCHMCoFokpS5igWMP8kyqly9gUFQmQj5IGhwuKs= diff --git a/internal/controller/indexers.go b/internal/controller/indexers.go index 65ea258c..09785dd5 100644 --- a/internal/controller/indexers.go +++ b/internal/controller/indexers.go @@ -16,10 +16,6 @@ import ( const ( deploymentWorkloadUIDIndex = "deploymentWorkloadUIDIndex" workloadNetworksIndex = "workloadNetworksIndex" - // deploymentCityCodeIndex indexes WorkloadDeployments by their Spec.CityCode - // so that the Location watch can efficiently find the deployments targeting - // the city a changed Location serves. - deploymentCityCodeIndex = "deploymentCityCodeIndex" deploymentLocationIndex = "deploymentLocationIndex" @@ -55,10 +51,6 @@ func addWorkloadDeploymentIndexers(ctx context.Context, mgr mcmanager.Manager) e return fmt.Errorf("failed to add workload deployment indexer %q: %w", deploymentWorkloadUIDIndex, err) } - if err := mgr.GetFieldIndexer().IndexField(ctx, &computev1alpha.WorkloadDeployment{}, deploymentCityCodeIndex, deploymentCityCodeIndexFunc); err != nil { - return fmt.Errorf("failed to add workload deployment indexer %q: %w", deploymentCityCodeIndex, err) - } - // Index workload deployments by location if err := mgr.GetFieldIndexer().IndexField(ctx, &computev1alpha.WorkloadDeployment{}, deploymentLocationIndex, deploymentLocationIndexFunc); err != nil { return fmt.Errorf("failed to add workload deployment indexer %q: %w", deploymentLocationIndex, err) @@ -79,7 +71,7 @@ func addWorkloadDeploymentIndexers(ctx context.Context, mgr mcmanager.Manager) e func deploymentWorkloadUIDIndexFunc(o client.Object) []string { // Skip deployments without a workload UID: indexing them under the empty // key would make them matchable by a GC query built from a corrupt (empty) - // UID, mirroring deploymentCityCodeIndexFunc. + // UID. uid := string(o.(*computev1alpha.WorkloadDeployment).Spec.WorkloadRef.UID) if uid == "" { return nil @@ -87,14 +79,6 @@ func deploymentWorkloadUIDIndexFunc(o client.Object) []string { return []string{uid} } -func deploymentCityCodeIndexFunc(o client.Object) []string { - deployment := o.(*computev1alpha.WorkloadDeployment) - if deployment.Spec.CityCode == "" { - return nil - } - return []string{deployment.Spec.CityCode} -} - func deploymentLocationIndexFunc(o client.Object) []string { deployment := o.(*computev1alpha.WorkloadDeployment) if deployment.Status.Location == nil { diff --git a/internal/controller/workload_controller.go b/internal/controller/workload_controller.go index 562df4a8..c9d8ac98 100644 --- a/internal/controller/workload_controller.go +++ b/internal/controller/workload_controller.go @@ -460,7 +460,7 @@ func (r *WorkloadReconciler) getDeploymentsForWorkload( for _, cityCode := range placement.CityCodes { foundLocation := false for _, location := range locations.Items { - locationCityCode, ok := location.Spec.Topology["topology.datum.net/city-code"] + locationCityCode, ok := location.Spec.Topology[networkingv1alpha.TopologyCityCodeKey] if ok && cityCode == locationCityCode { foundLocation = true break diff --git a/internal/controller/workloaddeployment_controller.go b/internal/controller/workloaddeployment_controller.go index b435c758..308b3700 100644 --- a/internal/controller/workloaddeployment_controller.go +++ b/internal/controller/workloaddeployment_controller.go @@ -6,6 +6,7 @@ import ( "context" "fmt" "slices" + "strings" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -79,7 +80,7 @@ func workloadDeploymentPodSelector(deployment *computev1alpha.WorkloadDeployment // +kubebuilder:rbac:groups=compute.datumapis.com,resources=workloaddeployments,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=compute.datumapis.com,resources=workloaddeployments/status,verbs=get;update;patch // +kubebuilder:rbac:groups=compute.datumapis.com,resources=workloaddeployments/finalizers,verbs=update -// +kubebuilder:rbac:groups=networking.datumapis.com,resources=locations,verbs=get;list;watch +// +kubebuilder:rbac:groups=networking.datumapis.com,resources=servinglocations,verbs=get;list;watch // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networkinterfaceclaims,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=networking.datumapis.com,resources=networkinterfaces,verbs=get;list;watch // The management-mode WorkloadReconciler watches Networks. Declare the grant as @@ -137,6 +138,25 @@ func (r *WorkloadDeploymentReconciler) Reconcile(ctx context.Context, req mcreco // Status().Update call when nothing changed (see loop-prevention comment below). existingStatus := *deployment.Status.DeepCopy() + // Resolve the cell's location before instances are built: the instance + // control strategy stamps Instance.Spec.Location from Status.Location as it + // creates them, so resolving afterwards left the first generation of + // instances permanently without one. + var location servingLocationResult + if r.NetworkingEnabled { + location, err = r.resolveLocation(ctx, cl.GetClient()) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed resolving location: %w", err) + } + location.evaluate(&deployment) + + // A location the cell contradicts is never written to status: an Instance + // carrying the wrong location is worse than one carrying none. + if location.reference != nil { + deployment.Status.Location = location.reference + } + } + // Collect all instances for this deployment listOpts := client.MatchingLabels{ computev1alpha.WorkloadDeploymentUIDLabel: string(deployment.GetUID()), @@ -178,27 +198,24 @@ func (r *WorkloadDeploymentReconciler) Reconcile(ctx context.Context, req mcreco // interface claims are created. This lets Instances reach the runtime on cells // where network-services-operator (VPC) is not yet available. networkReadyByInstance := make(map[string]bool, len(instances.Items)) - locationResolved := true - if !r.NetworkingEnabled { + switch { + case !r.NetworkingEnabled: for _, instance := range instances.Items { networkReadyByInstance[instance.Name] = true } - } else { - resolvedLocation, err := r.resolveLocation(ctx, cl.GetClient(), &deployment) - if err != nil { - return ctrl.Result{}, fmt.Errorf("failed resolving location: %w", err) - } - // Persist the resolved Location to status so downstream components (e.g. - // the stateful instance control strategy) can propagate it to Instances. - // When no matching Location exists, resolvedLocation is nil and - // Status.Location remains nil — instance creation is not blocked, and - // interface claims do not depend on it: a claim is served by the control - // plane it is created in, which is already location scoped. - locationResolved = resolvedLocation != nil - if resolvedLocation != nil { - deployment.Status.Location = resolvedLocation - } + case location.blocked: + // The cell has stated an identity that contradicts where this deployment + // was asked to run. Addresses are allocated out of the cell's own + // location, so claiming them here would place the workload in a city the + // user did not ask for. Leaving every instance's Network gate held keeps + // the deployment out of service until the placement fault is corrected, + // and releases it again with no further action once it is. The Available + // condition names the fault. + logger.Info("holding instances: cell location contradicts deployment placement", + "reason", location.reason, "message", location.message) + + default: networkReadyByInstance, err = r.reconcileNetworkInterfaceClaims(ctx, cl.GetClient(), &deployment, instances.Items) if err != nil { return ctrl.Result{}, fmt.Errorf("failed reconciling network interface claims: %w", err) @@ -279,7 +296,7 @@ func (r *WorkloadDeploymentReconciler) Reconcile(ctx context.Context, req mcreco ObservedGeneration: deployment.Generation, }) } else { - availCond := selectWDBlockingCondition(&deployment, networkReady, locationResolved, quotaBlockedReplicas, referencedDataBlockedReplicas, replicas, desiredReplicas) + availCond := selectWDBlockingCondition(&deployment, networkReady, location, quotaBlockedReplicas, referencedDataBlockedReplicas, replicas, desiredReplicas) apimeta.SetStatusCondition(&deployment.Status.Conditions, availCond) } @@ -308,6 +325,19 @@ func (r *WorkloadDeploymentReconciler) reconcileInstanceGates( ) (currentReplicas, updatedReplicas, readyReplicas, quotaBlockedReplicas, referencedDataBlockedReplicas int, err error) { templateHash := instancecontrol.ComputeHash(deployment.Spec.Template) for _, instance := range instances { + // Instances are stamped with the deployment's location as they are + // created, which leaves any instance that predates the cell learning its + // own location without one, and nothing else ever revisits it. Backfill + // it here. Best-effort by design: a failure is logged and the instance + // keeps running, because location has never gated scheduling. + if deployment.Status.Location != nil && instance.Spec.Location == nil { + base := instance.DeepCopy() + instance.Spec.Location = deployment.Status.Location + if patchErr := c.Patch(ctx, &instance, client.MergeFrom(base)); patchErr != nil { + log.FromContext(ctx).Error(patchErr, "failed backfilling instance location", "instance", instance.Name) + } + } + // Propagate suspension state from deployment to instance. if instance.Status.Suspended != deployment.Status.Suspended { base := instance.DeepCopy() @@ -460,7 +490,8 @@ func wdRefDataCondChanged(old, new *metav1.Condition) bool { // actionable one (e.g. missing referenced data). func selectWDBlockingCondition( deployment *computev1alpha.WorkloadDeployment, - networkReady, locationResolved bool, + networkReady bool, + location servingLocationResult, quotaBlockedReplicas, referencedDataBlockedReplicas, replicas int, desiredReplicas int32, ) metav1.Condition { @@ -480,13 +511,12 @@ func selectWDBlockingCondition( } } - // An unresolved city is the only user-visible signal that instances are - // running without the Location their placement was asked for, and it is - // considered first so it wins over the generic provisioning reason they - // share a priority with. - if !locationResolved { - consider(computev1alpha.WorkloadDeploymentReasonNoMatchingLocation, - fmt.Sprintf("No Location matches city code %q", deployment.Spec.CityCode)) + // An unusable cell location is the only user-visible signal that instances + // are running without the location their placement asked for, and it is + // considered first so it wins over the generic provisioning reason it shares + // a priority with. + if location.reason != "" { + consider(location.reason, location.message) } if !networkReady { @@ -567,6 +597,9 @@ func selectWDBlockingCondition( // 5 - SourceNotFound / SourceTooLarge / SourceUnauthorized (hard spec error) // 6 - NetworkNotFound (hard error; user action required) // 7 - NetworkFailedToCreate (hard infra error) +// 8 - CityCodeMismatch / AmbiguousServingLocation (the deployment is on a cell +// that cannot serve it; nothing the user does clears it, and no other +// blocker is worth reporting until it is fixed) func wdBlockingReasonPriority(reason string) int { switch reason { case computev1alpha.WorkloadDeploymentReasonInstancesProvisioning: @@ -589,42 +622,108 @@ func wdBlockingReasonPriority(reason string) int { return 6 case reasonNetworkFailedToCreate: return 7 + case computev1alpha.WorkloadDeploymentReasonCityCodeMismatch, + computev1alpha.WorkloadDeploymentReasonAmbiguousServingLocation: + return 8 default: return 0 } } -// resolveLocation returns the Location matching the deployment's city code, or -// nil when the city has no Location yet. It is reported to users on the -// Available condition and persisted to status so instances carry it, but -// nothing is gated on it: interface claims are served by the control plane they -// are created in, which is already location scoped. +// servingLocationResult is what the cell was able to say about the location it +// serves, expressed the way the deployment needs it. +type servingLocationResult struct { + // reference is the location to stamp on the deployment and its instances. It + // is nil whenever the cell's answer is missing or unusable. + reference *networkingv1alpha.LocationReference + + // servingLocation is the single ServingLocation the cell was delivered, or + // nil when it was delivered none or more than one. + servingLocation *networkingv1alpha.ServingLocation + + // reason and message are the Available condition the deployment should + // report while the location is unusable. Both are empty once it is usable. + reason string + message string + + // blocked distinguishes a cell that contradicts the deployment's placement + // from one that simply has not been identified yet. Only the former holds + // instances back; an unidentified cell must never stop a workload from + // running. + blocked bool +} + +// resolveLocation reads the location the cell serves. +// +// A cell cannot tell where it is on its own; the platform delivers it exactly +// one ServingLocation naming the place it sits in. Anything other than exactly +// one is reported rather than guessed at, per the ServingLocation contract. func (r *WorkloadDeploymentReconciler) resolveLocation( ctx context.Context, c client.Client, - deployment *computev1alpha.WorkloadDeployment, -) (*networkingv1alpha.LocationReference, error) { - // With Karmada propagation the WorkloadDeployment lands in the cluster that - // serves the requested city, so the Location object for that city must exist - // locally. - var locationList networkingv1alpha.LocationList - if err := c.List(ctx, &locationList); err != nil { - return nil, fmt.Errorf("failed to list locations: %w", err) - } - - for _, location := range locationList.Items { - if cityCode, ok := location.Spec.Topology["topology.datum.net/city-code"]; ok && cityCode == deployment.Spec.CityCode { - return &networkingv1alpha.LocationReference{ - Name: location.Name, - Namespace: location.Namespace, - }, nil +) (servingLocationResult, error) { + var servingLocations networkingv1alpha.ServingLocationList + if err := c.List(ctx, &servingLocations); err != nil { + return servingLocationResult{}, fmt.Errorf("failed to list serving locations: %w", err) + } + + if len(servingLocations.Items) > 1 { + names := make([]string, 0, len(servingLocations.Items)) + for _, servingLocation := range servingLocations.Items { + names = append(names, servingLocation.Name) } + slices.Sort(names) + + return servingLocationResult{ + reason: computev1alpha.WorkloadDeploymentReasonAmbiguousServingLocation, + message: fmt.Sprintf("This cell has been given %d locations to serve (%s) and will not guess between them", + len(names), strings.Join(names, ", ")), + blocked: true, + }, nil + } + + if len(servingLocations.Items) == 0 { + // Not an error: a cell that has not been identified yet still runs + // workloads, it just cannot tell them where they are. + log.FromContext(ctx).V(1).Info("cell has no serving location, waiting") + + return servingLocationResult{ + reason: computev1alpha.WorkloadDeploymentReasonNoMatchingLocation, + message: fmt.Sprintf("This cell has not been told which location it serves; it needs the %s cluster label, or its location has not reached it yet", + networkingv1alpha.ServingLocationTopologyLabel), + }, nil + } + + servingLocation := &servingLocations.Items[0] + + // A ServingLocation takes the name of the Location it was copied from, and + // Location is cluster scoped, so the reference carries a name and no + // namespace. + return servingLocationResult{ + reference: &networkingv1alpha.LocationReference{Name: servingLocation.Name}, + servingLocation: servingLocation, + }, nil +} + +// evaluate checks the resolved location against where the deployment asked to +// run. A deployment that reaches a cell serving another city was misplaced by +// the propagation layer, so it is reported as a fault rather than silently run +// in the wrong city. +func (s *servingLocationResult) evaluate(deployment *computev1alpha.WorkloadDeployment) { + if s.servingLocation == nil { + return + } + + cityCode := s.servingLocation.CityCode() + if cityCode == deployment.Spec.CityCode { + return } - // Surfaced to users via the Available condition (NoMatchingLocation); the log - // is debug-level detail only. - log.FromContext(ctx).V(1).Info("no location found for city code, waiting", "cityCode", deployment.Spec.CityCode) - return nil, nil + s.reference = nil + s.reason = computev1alpha.WorkloadDeploymentReasonCityCodeMismatch + s.message = fmt.Sprintf("Deployment asked for city %q but this cell serves %q; it was delivered to the wrong cell", + deployment.Spec.CityCode, cityCode) + s.blocked = true } // reconcileNetworkInterfaceClaims ensures one NetworkInterfaceClaim exists per @@ -784,14 +883,13 @@ func (r *WorkloadDeploymentReconciler) SetupWithManager(mgr mcmanager.Manager, o return enqueueWorkloadDeploymentForClaim(ctx, cl.GetClient(), clusterName, o) }) }). - // A deployment whose city has no Location yet waits without any other - // wake-up event, and the reconciler does not poll. Watching Locations - // re-reconciles the waiting deployments when their city's Location - // appears (or its topology changes) so Status.Location is filled in. - Watches(&networkingv1alpha.Location{}, func(clusterName multicluster.ClusterName, cl cluster.Cluster) handler.TypedEventHandler[client.Object, mcreconcile.Request] { - return handler.TypedEnqueueRequestsFromMapFunc(func(ctx context.Context, o client.Object) []mcreconcile.Request { - location := o.(*networkingv1alpha.Location) - return enqueueWorkloadDeploymentsForLocation(ctx, cl.GetClient(), clusterName, location) + // A deployment on a cell that does not yet know its own location waits + // without any other wake-up event, and the reconciler does not poll. + // Watching ServingLocations re-reconciles those deployments as soon as + // the cell learns where it is, so Status.Location is filled in. + Watches(&networkingv1alpha.ServingLocation{}, func(clusterName multicluster.ClusterName, cl cluster.Cluster) handler.TypedEventHandler[client.Object, mcreconcile.Request] { + return handler.TypedEnqueueRequestsFromMapFunc(func(ctx context.Context, _ client.Object) []mcreconcile.Request { + return enqueueWorkloadDeploymentsForServingLocation(ctx, cl.GetClient(), clusterName) }) }) } @@ -836,20 +934,16 @@ func enqueueWorkloadDeploymentForClaim(ctx context.Context, c client.Client, clu } } -// enqueueWorkloadDeploymentsForLocation maps a Location to the -// WorkloadDeployments that target its city, via the deploymentCityCodeIndex. -func enqueueWorkloadDeploymentsForLocation(ctx context.Context, c client.Client, clusterName multicluster.ClusterName, location *networkingv1alpha.Location) []mcreconcile.Request { +// enqueueWorkloadDeploymentsForServingLocation maps a ServingLocation change to +// every WorkloadDeployment on the cell. A cell's identity applies to all of +// them: it decides the location they are stamped with, and whether they are on +// the right cell at all. There is at most one ServingLocation per cell, so this +// fans out once per delivery, not per object. +func enqueueWorkloadDeploymentsForServingLocation(ctx context.Context, c client.Client, clusterName multicluster.ClusterName) []mcreconcile.Request { logger := log.FromContext(ctx) - cityCode, ok := location.Spec.Topology["topology.datum.net/city-code"] - if !ok { - return nil - } - var workloadDeployments computev1alpha.WorkloadDeploymentList - if err := c.List(ctx, &workloadDeployments, client.MatchingFields{ - deploymentCityCodeIndex: cityCode, - }); err != nil { + if err := c.List(ctx, &workloadDeployments); err != nil { logger.Error(err, "failed to list workload deployments") return nil } diff --git a/internal/controller/workloaddeployment_controller_test.go b/internal/controller/workloaddeployment_controller_test.go index d8944a3e..c19dee2b 100644 --- a/internal/controller/workloaddeployment_controller_test.go +++ b/internal/controller/workloaddeployment_controller_test.go @@ -900,7 +900,7 @@ func TestWDAvailableCondition_ReferencedDataSourceNotFound(t *testing.T) { deployment := makeWDForAvailTest(gen, metav1.ConditionFalse, computev1alpha.ReferencedDataReasonSourceNotFound, msg) - cond := selectWDBlockingCondition(deployment, true, true, 0, 1, replicas, desiredReplicas) + cond := selectWDBlockingCondition(deployment, true, resolvedTestLocation(), 0, 1, replicas, desiredReplicas) assert.Equal(t, computev1alpha.WorkloadDeploymentAvailable, cond.Type) assert.Equal(t, metav1.ConditionFalse, cond.Status) @@ -919,7 +919,7 @@ func TestWDAvailableCondition_QuotaNotGranted(t *testing.T) { ) deployment := makeWDForAvailTest(gen, metav1.ConditionTrue, computev1alpha.ReferencedDataReasonReady, "all present") - cond := selectWDBlockingCondition(deployment, true, true, 2, 0, replicas, desiredReplicas) + cond := selectWDBlockingCondition(deployment, true, resolvedTestLocation(), 2, 0, replicas, desiredReplicas) assert.Equal(t, metav1.ConditionFalse, cond.Status) assert.Equal(t, computev1alpha.WorkloadDeploymentReasonQuotaNotGranted, cond.Reason) @@ -940,7 +940,7 @@ func TestWDAvailableCondition_ReferencedDataWinsOverQuota(t *testing.T) { computev1alpha.ReferencedDataReasonSourceNotFound, `ConfigMap "X" not found in namespace "default"`) - cond := selectWDBlockingCondition(deployment, true, true, 1, 1, replicas, desiredReplicas) + cond := selectWDBlockingCondition(deployment, true, resolvedTestLocation(), 1, 1, replicas, desiredReplicas) assert.Equal(t, computev1alpha.WorkloadDeploymentReasonReferencedDataNotReady, cond.Reason, "ReferencedDataNotReady (priority 4) must beat QuotaNotGranted (priority 3)") @@ -961,7 +961,7 @@ func TestWDAvailableCondition_NetworkProvisioningVsReferencedData(t *testing.T) computev1alpha.ReferencedDataReasonSourceNotFound, `ConfigMap "X" not found`) - cond := selectWDBlockingCondition(deployment, false /* !networkReady */, true, 0, 1, replicas, desiredReplicas) + cond := selectWDBlockingCondition(deployment, false /* !networkReady */, resolvedTestLocation(), 0, 1, replicas, desiredReplicas) assert.Equal(t, computev1alpha.WorkloadDeploymentReasonReferencedDataNotReady, cond.Reason, "ReferencedDataNotReady (priority 4) must beat NetworkProvisioning (priority 2)") @@ -1004,7 +1004,7 @@ func TestWDAvailableCondition_ObservedGeneration(t *testing.T) { const gen = int64(42) deployment := makeWDForAvailTest(gen, "", "", "") - cond := selectWDBlockingCondition(deployment, true, true, 0, 0, 0, 1) + cond := selectWDBlockingCondition(deployment, true, resolvedTestLocation(), 0, 0, 0, 1) assert.Equal(t, gen, cond.ObservedGeneration, "ObservedGeneration must match deployment generation") // Verify the condition is also reachable via apimeta.FindStatusCondition (field @@ -1062,7 +1062,7 @@ func TestWDAvailableCondition_AnnotationSourceNotFound(t *testing.T) { ) deployment := makeWDWithAnnotation(gen, annot) - cond := selectWDBlockingCondition(deployment, true, true, 0, 1, replicas, desiredReplicas) + cond := selectWDBlockingCondition(deployment, true, resolvedTestLocation(), 0, 1, replicas, desiredReplicas) assert.Equal(t, computev1alpha.WorkloadDeploymentAvailable, cond.Type) assert.Equal(t, metav1.ConditionFalse, cond.Status) @@ -1100,7 +1100,7 @@ func TestWDAvailableCondition_AnnotationAndConditionBothPresent(t *testing.T) { }, } - cond := selectWDBlockingCondition(deployment, true, true, 0, 1, replicas, desiredReplicas) + cond := selectWDBlockingCondition(deployment, true, resolvedTestLocation(), 0, 1, replicas, desiredReplicas) assert.Equal(t, metav1.ConditionFalse, cond.Status) // Both paths arrive at the same terminal reason; the winner is stable regardless @@ -1125,7 +1125,7 @@ func TestWDAvailableCondition_AnnotationWinsOverQuota(t *testing.T) { deployment := makeWDWithAnnotation(gen, annot) // quotaBlockedReplicas=1 would normally surface QuotaNotGranted (priority 3). - cond := selectWDBlockingCondition(deployment, true, true, 1, 0, replicas, desiredReplicas) + cond := selectWDBlockingCondition(deployment, true, resolvedTestLocation(), 1, 0, replicas, desiredReplicas) assert.Equal(t, computev1alpha.ReferencedDataReasonSourceNotFound, cond.Reason, "SourceNotFound (priority 5) must beat QuotaNotGranted (priority 3)") @@ -1143,7 +1143,7 @@ func TestWDAvailableCondition_NoAnnotationPropagationLag(t *testing.T) { // No annotation, no ReferencedDataReady condition: companions still propagating. deployment := makeWDForAvailTest(gen, "", "", "") - cond := selectWDBlockingCondition(deployment, true, true, 0, 1, replicas, desiredReplicas) + cond := selectWDBlockingCondition(deployment, true, resolvedTestLocation(), 0, 1, replicas, desiredReplicas) assert.Equal(t, computev1alpha.WorkloadDeploymentReasonReferencedDataNotReady, cond.Reason, "propagation-lag path must still fire when annotation is absent") @@ -1160,7 +1160,7 @@ func TestWDAvailableCondition_AnnotationEmptyString(t *testing.T) { ) deployment := makeWDWithAnnotation(gen, "") - cond := selectWDBlockingCondition(deployment, true, true, 0, 0, replicas, desiredReplicas) + cond := selectWDBlockingCondition(deployment, true, resolvedTestLocation(), 0, 0, replicas, desiredReplicas) // No real blockers; falls through to InstancesProvisioning. assert.Equal(t, computev1alpha.WorkloadDeploymentReasonInstancesProvisioning, cond.Reason) @@ -1178,7 +1178,7 @@ func TestWDAvailableCondition_AnnotationMalformedJSON(t *testing.T) { deployment := makeWDWithAnnotation(gen, "not-valid-json{{") // Should not panic; malformed annotation is skipped. - cond := selectWDBlockingCondition(deployment, true, true, 0, 0, replicas, desiredReplicas) + cond := selectWDBlockingCondition(deployment, true, resolvedTestLocation(), 0, 0, replicas, desiredReplicas) assert.Equal(t, computev1alpha.WorkloadDeploymentReasonInstancesProvisioning, cond.Reason, "malformed annotation must be silently ignored; fallback to InstancesProvisioning") diff --git a/internal/controller/workloaddeployment_federator.go b/internal/controller/workloaddeployment_federator.go index 7e8b05d9..ac384497 100644 --- a/internal/controller/workloaddeployment_federator.go +++ b/internal/controller/workloaddeployment_federator.go @@ -30,6 +30,7 @@ import ( karmadapolicyv1alpha1 "github.com/karmada-io/api/policy/v1alpha1" computev1alpha "go.datum.net/compute/api/v1alpha" + networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" "go.miloapis.com/milo/pkg/downstreamclient" milosource "go.miloapis.com/milo/pkg/multicluster-runtime/source" ) @@ -45,7 +46,7 @@ const ( // and is used by PropagationPolicy selectors to route them to the correct // POP-cell clusters. Downstream Cluster objects are expected to carry this // label with their city-code value. - cityCodeLabel = "topology.datum.net/city-code" + cityCodeLabel = networkingv1alpha.TopologyCityCodeKey kindWorkloadDeployment = "WorkloadDeployment" ) diff --git a/internal/controller/workloaddeployment_location_test.go b/internal/controller/workloaddeployment_location_test.go index 5337824c..03a8c110 100644 --- a/internal/controller/workloaddeployment_location_test.go +++ b/internal/controller/workloaddeployment_location_test.go @@ -27,20 +27,13 @@ import ( const ( // locTestCityCode / locTestOtherCityCode: deployments under test target - // locTestCityCode; locTestOtherCityCode identifies a decoy Location that - // must never match. + // locTestCityCode; locTestOtherCityCode identifies the city a mis-delivered + // cell serves. locTestCityCode = "DFW" locTestOtherCityCode = "ORD" - // locTestNamespace mirrors where Location objects live in real clusters. - locTestNamespace = "networking-system" - // locTestWDNamespace is the namespace of the deployments under test. locTestWDNamespace = "default" - - // locTestTopologyKey is the production topology key that carries a - // Location's city code. - locTestTopologyKey = "topology.datum.net/city-code" ) // newNetworkingScheme returns a scheme with compute + networkingv1alpha types. @@ -51,89 +44,148 @@ func newNetworkingScheme() *runtime.Scheme { return s } -// newTestLocation builds a Location fixture shaped like production: the city -// code is carried in Spec.Topology under the topology.datum.net/city-code key. -func newTestLocation(name, cityCode string) *networkingv1alpha.Location { - return &networkingv1alpha.Location{ - ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: locTestNamespace}, - Spec: networkingv1alpha.LocationSpec{ - Topology: map[string]string{locTestTopologyKey: cityCode}, +// newTestServingLocation builds a ServingLocation fixture shaped like the one a +// cell is delivered: cluster scoped, and carrying its city under the +// topology.datum.net/city-code key. +func newTestServingLocation(name, cityCode string) *networkingv1alpha.ServingLocation { + return &networkingv1alpha.ServingLocation{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: networkingv1alpha.ServingLocationSpec{ + Topology: map[string]string{networkingv1alpha.TopologyCityCodeKey: cityCode}, }, } } -// TestResolveLocation_PersistsLocation_WhenLocationFound verifies that when a -// Location object matching the deployment's city code exists in the cluster, the -// resolved LocationReference is returned and can be persisted to -// deployment.Status.Location. -func TestResolveLocation_PersistsLocation_WhenLocationFound(t *testing.T) { - t.Parallel() - - const locationName = "loc-dfw-1" - - location := newTestLocation(locationName, locTestCityCode) - - s := newNetworkingScheme() - cl := fake.NewClientBuilder().WithScheme(s).WithObjects(location).Build() +// resolvedTestLocation is the result of a cell that knows where it is and +// serves the city the deployment asked for. +func resolvedTestLocation() servingLocationResult { + return servingLocationResult{ + reference: &networkingv1alpha.LocationReference{Name: "loc-dfw-1"}, + } +} - deployment := &computev1alpha.WorkloadDeployment{ - ObjectMeta: metav1.ObjectMeta{Name: "test-wd", Namespace: locTestWDNamespace}, +func newLocationTestDeployment(name string) *computev1alpha.WorkloadDeployment { + return &computev1alpha.WorkloadDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: locTestWDNamespace}, Spec: computev1alpha.WorkloadDeploymentSpec{ CityCode: locTestCityCode, }, } +} - r := &WorkloadDeploymentReconciler{} - resolvedLocation, err := r.resolveLocation(context.Background(), cl, deployment) +// TestResolveLocation_ExactlyOneServingLocation verifies the ordinary case: the +// single ServingLocation delivered to the cell resolves to a LocationReference +// naming it, with no namespace (Location is cluster scoped). +func TestResolveLocation_ExactlyOneServingLocation(t *testing.T) { + t.Parallel() + + const locationName = "loc-dfw-1" + cl := fake.NewClientBuilder(). + WithScheme(newNetworkingScheme()). + WithObjects(newTestServingLocation(locationName, locTestCityCode)). + Build() + + deployment := newLocationTestDeployment("test-wd") + + r := &WorkloadDeploymentReconciler{} + result, err := r.resolveLocation(context.Background(), cl) require.NoError(t, err) - require.NotNil(t, resolvedLocation, - "resolved location must be non-nil when a matching Location object exists") - assert.Equal(t, locationName, resolvedLocation.Name) - assert.Equal(t, locTestNamespace, resolvedLocation.Namespace) - - // Simulate what the Reconcile loop does: persist resolvedLocation to Status. - deployment.Status.Location = resolvedLocation - assert.Equal(t, locationName, deployment.Status.Location.Name, - "Status.Location.Name must match the resolved Location object name") + result.evaluate(deployment) + + require.NotNil(t, result.reference, + "the cell's single serving location must resolve") + assert.Equal(t, locationName, result.reference.Name) + assert.Empty(t, result.reference.Namespace, + "Location is cluster scoped, so the reference carries no namespace") + assert.Empty(t, result.reason, "a resolved location reports no blocking reason") + assert.False(t, result.blocked) } -// TestResolveLocation_ReturnsNilLocation_WhenNoLocationFound verifies that when -// no Location object in the cluster matches the deployment's city code, the -// resolver returns (nil, nil) — no error and no resolved location. The caller -// must treat a nil location as best-effort and must NOT block instance creation. -func TestResolveLocation_ReturnsNilLocation_WhenNoLocationFound(t *testing.T) { +// TestResolveLocation_NoServingLocation_IsNonGating verifies that a cell which +// has not been told where it is does not block the deployment: no location is +// resolved, nothing is marked blocked, and the reason names what is missing. +func TestResolveLocation_NoServingLocation_IsNonGating(t *testing.T) { t.Parallel() - s := newNetworkingScheme() - // Cluster has a Location for a DIFFERENT city code. - otherLocation := newTestLocation("loc-ord-1", locTestOtherCityCode) - cl := fake.NewClientBuilder().WithScheme(s).WithObjects(otherLocation).Build() + cl := fake.NewClientBuilder().WithScheme(newNetworkingScheme()).Build() - deployment := &computev1alpha.WorkloadDeployment{ - ObjectMeta: metav1.ObjectMeta{Name: "test-wd", Namespace: locTestWDNamespace}, - Spec: computev1alpha.WorkloadDeploymentSpec{ - CityCode: locTestCityCode, // no matching Location - }, - } + deployment := newLocationTestDeployment("test-wd") r := &WorkloadDeploymentReconciler{} - resolvedLocation, err := r.resolveLocation(context.Background(), cl, deployment) + result, err := r.resolveLocation(context.Background(), cl) + require.NoError(t, err, "an unidentified cell must not surface as an error") + result.evaluate(deployment) + + assert.Nil(t, result.reference) + assert.False(t, result.blocked, + "a cell that has not been identified yet must never hold instances back") + assert.Equal(t, computev1alpha.WorkloadDeploymentReasonNoMatchingLocation, result.reason) + assert.Contains(t, result.message, networkingv1alpha.ServingLocationTopologyLabel, + "the message must name the cluster label that fixes it") + assert.Nil(t, deployment.Status.Location, + "Status.Location must be left alone when nothing resolved") +} - require.NoError(t, err, "missing location must not cause an error") - assert.Nil(t, resolvedLocation, - "resolved location must be nil when no matching Location object exists") +// TestResolveLocation_MultipleServingLocations_RefusesToGuess verifies the +// ServingLocation contract: two or more delivered locations means the cell +// cannot tell which one it serves, so it picks neither and blocks. +func TestResolveLocation_MultipleServingLocations_RefusesToGuess(t *testing.T) { + t.Parallel() - // Status.Location remains nil — callers must not update it in this case. - // Confirm the deployment's Status.Location is unaffected (nil → nil). - assert.Nil(t, deployment.Status.Location, - "Status.Location must remain nil when no Location matches the city code") + cl := fake.NewClientBuilder(). + WithScheme(newNetworkingScheme()). + WithObjects( + newTestServingLocation("loc-dfw-1", locTestCityCode), + newTestServingLocation("loc-ord-1", locTestOtherCityCode), + ). + Build() + + deployment := newLocationTestDeployment("test-wd") + + r := &WorkloadDeploymentReconciler{} + result, err := r.resolveLocation(context.Background(), cl) + require.NoError(t, err) + result.evaluate(deployment) + + assert.Nil(t, result.reference, + "an ambiguous cell must not resolve to either candidate") + assert.True(t, result.blocked) + assert.Equal(t, computev1alpha.WorkloadDeploymentReasonAmbiguousServingLocation, result.reason) + assert.Contains(t, result.message, "loc-dfw-1") + assert.Contains(t, result.message, "loc-ord-1") +} + +// TestResolveLocation_CityCodeMismatch verifies that a deployment which reaches +// a cell serving another city is reported as a placement fault rather than +// quietly stamped with the wrong location. +func TestResolveLocation_CityCodeMismatch(t *testing.T) { + t.Parallel() + + cl := fake.NewClientBuilder(). + WithScheme(newNetworkingScheme()). + WithObjects(newTestServingLocation("loc-ord-1", locTestOtherCityCode)). + Build() + + deployment := newLocationTestDeployment("test-wd") + + r := &WorkloadDeploymentReconciler{} + result, err := r.resolveLocation(context.Background(), cl) + require.NoError(t, err) + result.evaluate(deployment) + + assert.Nil(t, result.reference, + "the wrong cell's location must never be stamped on the deployment") + assert.True(t, result.blocked, "a misplaced deployment must not proceed silently") + assert.Equal(t, computev1alpha.WorkloadDeploymentReasonCityCodeMismatch, result.reason) + assert.Contains(t, result.message, locTestCityCode) + assert.Contains(t, result.message, locTestOtherCityCode) } // newLocationTestWDReconciler builds a WorkloadDeploymentReconciler with // networking enabled, wired to a fake cluster, with the controller finalizer // pre-registered the same way SetupWithManager does. Networking must be enabled -// so Reconcile exercises Location resolution. +// so Reconcile exercises location resolution. func newLocationTestWDReconciler(cl client.Client) *WorkloadDeploymentReconciler { r := &WorkloadDeploymentReconciler{ mgr: newFakeMCManager(testCluster, newFakeCluster(cl)), @@ -147,19 +199,15 @@ func newLocationTestWDReconciler(cl client.Client) *WorkloadDeploymentReconciler return r } -// TestWorkloadDeploymentReconcile_NoMatchingLocation_SetsCondition verifies the -// user-visible surface while a deployment waits for its city's Location: the -// Available condition must name the unresolved city (reason NoMatchingLocation), -// and once a matching Location appears the next reconcile must replace that -// reason — the unresolved-city signal must not outlive its cause. -func TestWorkloadDeploymentReconcile_NoMatchingLocation_SetsCondition(t *testing.T) { - t.Parallel() - - deployment := &computev1alpha.WorkloadDeployment{ +// newLocationTestReconcilableWD builds a deployment shaped the way Reconcile +// expects to find one already running: finalized, with replicas and the +// defaulted management policy. +func newLocationTestReconcilableWD(name string) *computev1alpha.WorkloadDeployment { + return &computev1alpha.WorkloadDeployment{ ObjectMeta: metav1.ObjectMeta{ - Name: "location-test-wd", + Name: name, Namespace: locTestWDNamespace, - UID: "location-test-wd-uid", + UID: types.UID(name + "-uid"), // Pre-set the finalizer so Reconcile proceeds past the finalizer-add // branch. Finalizers: []string{workloadControllerFinalizer}, @@ -176,13 +224,14 @@ func TestWorkloadDeploymentReconcile_NoMatchingLocation_SetsCondition(t *testing }, }, } +} - // An instance shaped the way the instance-control strategy creates it: - // ordinal name, controller labels, and the scheduling gates stamped at - // creation. Pre-seeding it (with a CreationTimestamp, which the fake client - // does not stamp on Create) keeps the strategy in its wait path so the test - // exercises only the condition transitions. - instance := &computev1alpha.Instance{ +// newLocationTestInstance builds an instance shaped the way the instance-control +// strategy creates it: ordinal name, deployment UID label, and the scheduling +// gates stamped at creation. The CreationTimestamp (which the fake client does +// not stamp on Create) keeps the strategy in its wait path. +func newLocationTestInstance(deployment *computev1alpha.WorkloadDeployment) *computev1alpha.Instance { + return &computev1alpha.Instance{ ObjectMeta: metav1.ObjectMeta{ Name: deployment.Name + "-0", Namespace: deployment.Namespace, @@ -200,23 +249,35 @@ func TestWorkloadDeploymentReconcile_NoMatchingLocation_SetsCondition(t *testing }, }, } +} + +func locationTestRequest(deployment *computev1alpha.WorkloadDeployment) mcreconcile.Request { + return mcreconcile.Request{ + ClusterName: testCluster, + Request: ctrl.Request{ + NamespacedName: types.NamespacedName{Name: deployment.Name, Namespace: deployment.Namespace}, + }, + } +} + +// TestWorkloadDeploymentReconcile_UnidentifiedCell_SetsCondition verifies the +// user-visible surface while a cell has not learned where it is: the Available +// condition explains what is missing, and once the cell is told, the next +// reconcile resolves the location and replaces the reason — the waiting signal +// must not outlive its cause. +func TestWorkloadDeploymentReconcile_UnidentifiedCell_SetsCondition(t *testing.T) { + t.Parallel() - // The only Location in the cluster serves a different city. - otherLocation := newTestLocation("loc-ord-1", locTestOtherCityCode) + deployment := newLocationTestReconcilableWD("location-test-wd") + instance := newLocationTestInstance(deployment) cl := fake.NewClientBuilder(). WithScheme(newNetworkingScheme()). - WithObjects(deployment, instance, otherLocation). + WithObjects(deployment, instance). WithStatusSubresource(deployment). Build() r := newLocationTestWDReconciler(cl) - - req := mcreconcile.Request{ - ClusterName: testCluster, - Request: ctrl.Request{ - NamespacedName: types.NamespacedName{Name: deployment.Name, Namespace: deployment.Namespace}, - }, - } + req := locationTestRequest(deployment) _, err := r.Reconcile(context.Background(), req) require.NoError(t, err) @@ -225,17 +286,17 @@ func TestWorkloadDeploymentReconcile_NoMatchingLocation_SetsCondition(t *testing require.NoError(t, cl.Get(context.Background(), req.NamespacedName, &updated)) cond := apimeta.FindStatusCondition(updated.Status.Conditions, computev1alpha.WorkloadDeploymentAvailable) - require.NotNil(t, cond, "Available must be set while the city has no Location") + require.NotNil(t, cond, "Available must be set while the cell has no location") assert.Equal(t, metav1.ConditionFalse, cond.Status) assert.Equal(t, computev1alpha.WorkloadDeploymentReasonNoMatchingLocation, cond.Reason) - assert.Contains(t, cond.Message, locTestCityCode, - "the condition message must name the unresolved city code") + assert.Contains(t, cond.Message, networkingv1alpha.ServingLocationTopologyLabel, + "the condition message must name the label that identifies the cell") assert.Nil(t, updated.Status.Location) - // Provision the city's Location; the next reconcile resolves it and must - // replace the NoMatchingLocation reason. - matchingLocation := newTestLocation("loc-dfw-2", locTestCityCode) - require.NoError(t, cl.Create(context.Background(), matchingLocation)) + // Deliver the cell's location; the next reconcile resolves it and must + // replace the waiting reason. + servingLocation := newTestServingLocation("loc-dfw-2", locTestCityCode) + require.NoError(t, cl.Create(context.Background(), servingLocation)) _, err = r.Reconcile(context.Background(), req) require.NoError(t, err) @@ -244,46 +305,105 @@ func TestWorkloadDeploymentReconcile_NoMatchingLocation_SetsCondition(t *testing cond = apimeta.FindStatusCondition(updated.Status.Conditions, computev1alpha.WorkloadDeploymentAvailable) require.NotNil(t, cond) assert.Equal(t, computev1alpha.WorkloadDeploymentReasonInstancesProvisioning, cond.Reason, - "the unresolved-city reason must give way once the Location resolves") + "the waiting reason must give way once the cell knows where it is") require.NotNil(t, updated.Status.Location) - assert.Equal(t, matchingLocation.Name, updated.Status.Location.Name) + assert.Equal(t, servingLocation.Name, updated.Status.Location.Name) } -// TestEnqueueWorkloadDeploymentsForLocation verifies the Location watch mapping: -// a Location event must enqueue exactly the WorkloadDeployments whose CityCode -// matches the Location's topology (via deploymentCityCodeIndex), and a Location -// without a city code in its topology must map to nothing. -func TestEnqueueWorkloadDeploymentsForLocation(t *testing.T) { +// TestWorkloadDeploymentReconcile_CityCodeMismatch_HoldsInstances verifies that a +// deployment delivered to the wrong cell reports the fault and is kept out of +// service: its instances keep the Network scheduling gate rather than booting in +// a city the user did not ask for. +func TestWorkloadDeploymentReconcile_CityCodeMismatch_HoldsInstances(t *testing.T) { t.Parallel() - wdDFW := &computev1alpha.WorkloadDeployment{ - ObjectMeta: metav1.ObjectMeta{Name: "wd-dfw", Namespace: locTestWDNamespace}, - Spec: computev1alpha.WorkloadDeploymentSpec{CityCode: locTestCityCode}, - } - wdORD := &computev1alpha.WorkloadDeployment{ - ObjectMeta: metav1.ObjectMeta{Name: "wd-ord", Namespace: locTestWDNamespace}, - Spec: computev1alpha.WorkloadDeploymentSpec{CityCode: locTestOtherCityCode}, - } + deployment := newLocationTestReconcilableWD("mismatch-test-wd") + instance := newLocationTestInstance(deployment) + + cl := fake.NewClientBuilder(). + WithScheme(newNetworkingScheme()). + WithObjects(deployment, instance, newTestServingLocation("loc-ord-1", locTestOtherCityCode)). + WithStatusSubresource(deployment). + Build() + r := newLocationTestWDReconciler(cl) + req := locationTestRequest(deployment) + + _, err := r.Reconcile(context.Background(), req) + require.NoError(t, err) + + var updated computev1alpha.WorkloadDeployment + require.NoError(t, cl.Get(context.Background(), req.NamespacedName, &updated)) + + cond := apimeta.FindStatusCondition(updated.Status.Conditions, computev1alpha.WorkloadDeploymentAvailable) + require.NotNil(t, cond) + assert.Equal(t, computev1alpha.WorkloadDeploymentReasonCityCodeMismatch, cond.Reason, + "a misplaced deployment must report the placement fault over any other blocker") + assert.Nil(t, updated.Status.Location, + "the wrong cell's location must never be written to status") + + var updatedInstance computev1alpha.Instance + require.NoError(t, cl.Get(context.Background(), types.NamespacedName{ + Name: instance.Name, Namespace: instance.Namespace, + }, &updatedInstance)) + require.NotNil(t, updatedInstance.Spec.Controller) + assert.Contains(t, updatedInstance.Spec.Controller.SchedulingGates, + computev1alpha.SchedulingGate{Name: instancecontrol.NetworkSchedulingGate.String()}, + "the Network gate must stay held while the deployment is on the wrong cell") +} + +// TestWorkloadDeploymentReconcile_BackfillsInstanceLocation verifies that an +// instance created before the cell knew its location does not stay without one: +// the reconcile that resolves the location also stamps it on the existing +// instance. +func TestWorkloadDeploymentReconcile_BackfillsInstanceLocation(t *testing.T) { + t.Parallel() + + deployment := newLocationTestReconcilableWD("backfill-test-wd") + instance := newLocationTestInstance(deployment) + require.Nil(t, instance.Spec.Location, "the fixture must start without a location") + + servingLocation := newTestServingLocation("loc-dfw-1", locTestCityCode) + + cl := fake.NewClientBuilder(). + WithScheme(newNetworkingScheme()). + WithObjects(deployment, instance, servingLocation). + WithStatusSubresource(deployment). + Build() + r := newLocationTestWDReconciler(cl) + + _, err := r.Reconcile(context.Background(), locationTestRequest(deployment)) + require.NoError(t, err) + + var updatedInstance computev1alpha.Instance + require.NoError(t, cl.Get(context.Background(), types.NamespacedName{ + Name: instance.Name, Namespace: instance.Namespace, + }, &updatedInstance)) + + require.NotNil(t, updatedInstance.Spec.Location, + "an instance predating the cell's location must be backfilled") + assert.Equal(t, servingLocation.Name, updatedInstance.Spec.Location.Name) +} + +// TestEnqueueWorkloadDeploymentsForServingLocation verifies the ServingLocation +// watch mapping: the cell's identity applies to every deployment on it, so all +// of them are enqueued regardless of the city they target. +func TestEnqueueWorkloadDeploymentsForServingLocation(t *testing.T) { + t.Parallel() + + wdDFW := newLocationTestDeployment("wd-dfw") + wdORD := newLocationTestDeployment("wd-ord") + wdORD.Spec.CityCode = locTestOtherCityCode cl := fake.NewClientBuilder(). WithScheme(newNetworkingScheme()). - WithIndex(&computev1alpha.WorkloadDeployment{}, deploymentCityCodeIndex, deploymentCityCodeIndexFunc). WithObjects(wdDFW, wdORD). Build() - location := newTestLocation("loc-dfw-1", locTestCityCode) + requests := enqueueWorkloadDeploymentsForServingLocation(context.Background(), cl, testCluster) + require.Len(t, requests, 2) - requests := enqueueWorkloadDeploymentsForLocation(context.Background(), cl, testCluster, location) - require.Len(t, requests, 1, "only deployments whose CityCode matches the Location must be enqueued") - assert.Equal(t, wdDFW.Name, requests[0].Name) + names := []string{requests[0].Name, requests[1].Name} + assert.ElementsMatch(t, []string{wdDFW.Name, wdORD.Name}, names) assert.Equal(t, locTestWDNamespace, requests[0].Namespace) assert.Equal(t, multicluster.ClusterName(testCluster), requests[0].ClusterName) - - // A Location without a city code in its topology identifies no city, so no - // deployment can match it. - noCityLocation := &networkingv1alpha.Location{ - ObjectMeta: metav1.ObjectMeta{Name: "loc-no-city", Namespace: locTestNamespace}, - Spec: networkingv1alpha.LocationSpec{Topology: map[string]string{}}, - } - assert.Empty(t, enqueueWorkloadDeploymentsForLocation(context.Background(), cl, testCluster, noCityLocation)) } diff --git a/internal/webhook/v1alpha/workload_webhook.go b/internal/webhook/v1alpha/workload_webhook.go index b8cffecb..e8926b60 100644 --- a/internal/webhook/v1alpha/workload_webhook.go +++ b/internal/webhook/v1alpha/workload_webhook.go @@ -96,7 +96,7 @@ func (r *workloadWebhook) ValidateCreate(ctx context.Context, workload *computev validCityCodes := sets.Set[string]{} for _, location := range locations.Items { - cityCode, ok := location.Spec.Topology["topology.datum.net/city-code"] + cityCode, ok := location.Spec.Topology[networkingv1alpha.TopologyCityCodeKey] if ok { validCityCodes.Insert(cityCode) } @@ -141,7 +141,7 @@ func (r *workloadWebhook) ValidateUpdate(ctx context.Context, _ *computev1alpha. validCityCodes := sets.Set[string]{} for _, location := range locations.Items { - cityCode, ok := location.Spec.Topology["topology.datum.net/city-code"] + cityCode, ok := location.Spec.Topology[networkingv1alpha.TopologyCityCodeKey] if ok { validCityCodes.Insert(cityCode) } From 27ffa6a2521d01c6f6b4294e540452c60016c417 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Fri, 14 Aug 2026 08:35:10 -0500 Subject: [PATCH 2/2] chore: re-pin network-services-operator to pick up per-location networks NSO #373 gives a network a presence in every location it is used, and adds the claim rejection reasons that come with it. Compute passes those reasons through verbatim, so picking them up needs no code change. The same change drops LocationReference.Namespace: Locations are cluster scoped, so the field was always empty. Dropping it from the index key and the CRDs removes a namespace nobody could ever have set. Co-Authored-By: Claude Opus 5 --- .../base/crd/bases/compute.datumapis.com_instances.yaml | 4 ---- .../bases/compute.datumapis.com_workloaddeployments.yaml | 8 -------- .../base/crd/bases/compute.datumapis.com_workloads.yaml | 4 ---- go.mod | 7 ++++--- go.sum | 4 ++-- internal/controller/indexers.go | 8 ++------ .../instancecontrol/stateful/stateful_control_test.go | 4 +--- internal/controller/workloaddeployment_location_test.go | 2 -- 8 files changed, 9 insertions(+), 32 deletions(-) diff --git a/config/base/crd/bases/compute.datumapis.com_instances.yaml b/config/base/crd/bases/compute.datumapis.com_instances.yaml index 5d3a8daa..5e050e61 100644 --- a/config/base/crd/bases/compute.datumapis.com_instances.yaml +++ b/config/base/crd/bases/compute.datumapis.com_instances.yaml @@ -97,12 +97,8 @@ spec: name: description: Name of a datum location type: string - namespace: - description: Namespace for the datum location - type: string required: - name - - namespace type: object networkInterfaces: description: |- diff --git a/config/base/crd/bases/compute.datumapis.com_workloaddeployments.yaml b/config/base/crd/bases/compute.datumapis.com_workloaddeployments.yaml index 391987d1..48fa5d2e 100644 --- a/config/base/crd/bases/compute.datumapis.com_workloaddeployments.yaml +++ b/config/base/crd/bases/compute.datumapis.com_workloaddeployments.yaml @@ -210,12 +210,8 @@ spec: name: description: Name of a datum location type: string - namespace: - description: Namespace for the datum location - type: string required: - name - - namespace type: object networkInterfaces: description: |- @@ -1281,12 +1277,8 @@ spec: name: description: Name of a datum location type: string - namespace: - description: Namespace for the datum location - type: string required: - name - - namespace type: object observedGeneration: description: |- diff --git a/config/base/crd/bases/compute.datumapis.com_workloads.yaml b/config/base/crd/bases/compute.datumapis.com_workloads.yaml index 2b8e3dfd..41dfd418 100644 --- a/config/base/crd/bases/compute.datumapis.com_workloads.yaml +++ b/config/base/crd/bases/compute.datumapis.com_workloads.yaml @@ -214,12 +214,8 @@ spec: name: description: Name of a datum location type: string - namespace: - description: Namespace for the datum location - type: string required: - name - - namespace type: object networkInterfaces: description: |- diff --git a/go.mod b/go.mod index b99706d2..146ed055 100644 --- a/go.mod +++ b/go.mod @@ -11,9 +11,10 @@ require ( github.com/prometheus/client_golang v1.23.2 github.com/stretchr/testify v1.11.1 // TODO: temporary pin to network-services-operator main. The latest tag - // (v0.25.5) predates both NetworkInterfaceClaim (#360) and ServingLocation - // (#372); re-pin to a tagged release before compute cuts a release. - go.datum.net/network-services-operator v0.25.6-0.20260814031118-aba36815ac91 + // (v0.25.5) predates NetworkInterfaceClaim (#360), ServingLocation (#372), + // and per-location networks (#373); re-pin to a tagged release before + // compute cuts a release. + go.datum.net/network-services-operator v0.25.6-0.20260814125050-f3a972d9da33 go.miloapis.com/milo v0.32.0 golang.org/x/crypto v0.54.0 golang.org/x/sync v0.22.0 diff --git a/go.sum b/go.sum index 47176e16..30417465 100644 --- a/go.sum +++ b/go.sum @@ -175,8 +175,8 @@ github.com/tidwall/sjson v1.2.5 h1:kLy8mja+1c9jlljvWTlSazM7cKDRfJuR/bOJhcY5NcY= github.com/tidwall/sjson v1.2.5/go.mod h1:Fvgq9kS/6ociJEDnK0Fk1cpYF4FIW6ZF7LAe+6jwd28= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= -go.datum.net/network-services-operator v0.25.6-0.20260814031118-aba36815ac91 h1:9jo8ZB4TMFjOWVu0XEdND46QR3hJRepYYzUaBRUkV5w= -go.datum.net/network-services-operator v0.25.6-0.20260814031118-aba36815ac91/go.mod h1:oz57/uTB5HAjLGrqs1pn1HTP/jW9vkGgMODPvCiUlvc= +go.datum.net/network-services-operator v0.25.6-0.20260814125050-f3a972d9da33 h1:oAbsJRV6zNrwO7XymY2WJ7+uAa+PBx9mNeXbo48CQgk= +go.datum.net/network-services-operator v0.25.6-0.20260814125050-f3a972d9da33/go.mod h1:oz57/uTB5HAjLGrqs1pn1HTP/jW9vkGgMODPvCiUlvc= go.miloapis.com/milo v0.32.0 h1:TkNIQu/37d+SEquLJ5+GmdisSl+K2RT7eEC4idg6RIs= go.miloapis.com/milo v0.32.0/go.mod h1:GKK3afjCwshfZfvhjNe1wp/H45z4m7x5oG/8xbSgU1M= go.miloapis.com/service-catalog v0.4.0 h1:LvO1WCHMCoFokpS5igWMP8kyqly9gUFQmQj5IGhwuKs= diff --git a/internal/controller/indexers.go b/internal/controller/indexers.go index 09785dd5..9816273b 100644 --- a/internal/controller/indexers.go +++ b/internal/controller/indexers.go @@ -85,12 +85,8 @@ func deploymentLocationIndexFunc(o client.Object) []string { return nil } - return []string{ - types.NamespacedName{ - Namespace: deployment.Status.Location.Namespace, - Name: deployment.Status.Location.Name, - }.String(), - } + // Locations are cluster-scoped, so the name alone identifies one. + return []string{deployment.Status.Location.Name} } func addWorkloadIndexers(ctx context.Context, mgr mcmanager.Manager) error { diff --git a/internal/controller/instancecontrol/stateful/stateful_control_test.go b/internal/controller/instancecontrol/stateful/stateful_control_test.go index dc0557f2..393ab437 100644 --- a/internal/controller/instancecontrol/stateful/stateful_control_test.go +++ b/internal/controller/instancecontrol/stateful/stateful_control_test.go @@ -338,8 +338,7 @@ func TestInstanceLocation_SetWhenDeploymentStatusLocationPresent(t *testing.T) { deployment := getWorkloadDeployment("test-location-set", 1) deployment.Status.Location = &networkingv1alpha.LocationReference{ - Name: "loc-dfw-1", - Namespace: "networking-system", + Name: "loc-dfw-1", } var currentInstances []v1alpha.Instance @@ -353,7 +352,6 @@ func TestInstanceLocation_SetWhenDeploymentStatusLocationPresent(t *testing.T) { assert.NotNil(t, instance.Spec.Location, "Spec.Location must be set when deployment.Status.Location is non-nil") assert.Equal(t, "loc-dfw-1", instance.Spec.Location.Name) - assert.Equal(t, "networking-system", instance.Spec.Location.Namespace) } // TestInstanceLocation_NilWhenDeploymentStatusLocationAbsent verifies that when diff --git a/internal/controller/workloaddeployment_location_test.go b/internal/controller/workloaddeployment_location_test.go index 03a8c110..20e5739f 100644 --- a/internal/controller/workloaddeployment_location_test.go +++ b/internal/controller/workloaddeployment_location_test.go @@ -96,8 +96,6 @@ func TestResolveLocation_ExactlyOneServingLocation(t *testing.T) { require.NotNil(t, result.reference, "the cell's single serving location must resolve") assert.Equal(t, locationName, result.reference.Name) - assert.Empty(t, result.reference.Namespace, - "Location is cluster scoped, so the reference carries no namespace") assert.Empty(t, result.reason, "a resolved location reports no blocking reason") assert.False(t, result.blocked) }