From 58ff0b56ac52d9d601d97f323c7e77a8416ccdfd Mon Sep 17 00:00:00 2001 From: Nick Marden Date: Wed, 29 Jul 2026 14:06:19 -0400 Subject: [PATCH 1/2] Resolve policy per pod and make it part of profile identity (#87) policy.Resolver.Resolve served two callers with incompatible contracts. The metrics collector and resource adjuster both called it with only a WorkloadProfile's identity tuple, which produced two silent failures from one cause. Over-reach: an Input with no namespace listed ResourcePolicies with client.InNamespace(""), which means *all namespaces*, and Resolve ranks namespace-scoped above cluster-scoped before comparing priority. One ResourcePolicy with a loose selector therefore governed recommendations and resizes for every matching profile in the cluster, outranking policies that explicitly set a higher priority. Split resolution: a policy selecting on kinds, annotations, or namespaces.include matched at admission (which has the pod) but could never match in the controllers (which have no namespace, owner kind, or annotations). Pods were admitted with one policy's values and resized toward another's, with nothing logging a conflict because each path resolved successfully on its own terms. Policy is now resolved once per pod by the workloadwatcher, the only component holding a pod and therefore the only one that can evaluate a selector correctly. The result is recorded on workloadprofile.status.policyRef; the collector and adjuster load it instead of resolving again, so all three paths agree by construction. An Input with no namespace can no longer match a ResourcePolicy at all. The governing policy is now part of profile identity, because a profile holds one set of recommendations per container and the policy chooses the sources, cadence, tracked resources, aggregation, and headroom that produce them. Profile names carry a policy token, and each profile owns a Redis key namespace via status.measurementHash, so sibling profiles sharing a tuple never write into one sample series and the finalizer purges only its own keys. The policy is deliberately kept out of tupleLabels: that map also produces status.selectorLabels, the server-side query that finds a profile's pods, and a policy name is not a pod label. Both policy kinds are now watched, so a policy applied to a running cluster takes effect without pod churn. Only selector and priority changes re-scope identity; every other spec field is read live on the next cycle, so tuning a policy does not re-key measurement history. Upgrade impact: every existing WorkloadProfile is replaced by a newly-named one and the fleet accrues from zero for one readiness.minTimeSpan before resizes resume. Old profiles orphan, age out over orphanTTL, and their history is purged by the finalizer. --- CHANGELOG.md | 19 ++ README.md | 11 +- api/v1/clusterresourcepolicy_types.go | 44 ++- api/v1/resourcepolicy_types.go | 5 +- api/v1/workloadprofile_types.go | 58 ++++ api/v1/workloadprofile_types_test.go | 56 +++ api/v1/zz_generated.deepcopy.go | 37 +- charts/ballast/templates/clusterrole.yaml | 8 + cmd/ballastd/main.go | 10 + ...esoftware.com_clusterresourcepolicies.yaml | 33 +- ...ightlinesoftware.com_resourcepolicies.yaml | 33 +- ...ightlinesoftware.com_workloadprofiles.yaml | 47 +++ docs/convergence.md | 114 +++++-- .../controller/metricscollector/controller.go | 59 +++- .../metricscollector/controller_test.go | 134 +++++++- .../controller/policystatus/controller.go | 106 ++++++ .../policystatus/controller_test.go | 196 +++++++++++ .../controller/resourceadjuster/controller.go | 21 +- .../resourceadjuster/controller_test.go | 63 ++++ .../controller/workloadwatcher/controller.go | 295 +++++++++++----- .../workloadwatcher/controller_test.go | 70 ++-- .../workloadwatcher/policy_identity_test.go | 323 ++++++++++++++++++ .../workloadwatcher/watch_internal_test.go | 122 ++++++- internal/naming/naming.go | 122 +++++++ internal/naming/naming_test.go | 160 +++++++++ internal/policy/input.go | 31 ++ internal/policy/resolver.go | 128 ++++++- internal/policy/scope_test.go | 276 +++++++++++++++ internal/store/keys.go | 44 ++- internal/store/keys_test.go | 68 ++++ internal/webhook/pod_mutator.go | 73 ++-- internal/webhook/pod_mutator_test.go | 68 ++-- 32 files changed, 2553 insertions(+), 281 deletions(-) create mode 100644 api/v1/workloadprofile_types_test.go create mode 100644 internal/controller/policystatus/controller.go create mode 100644 internal/controller/policystatus/controller_test.go create mode 100644 internal/controller/workloadwatcher/policy_identity_test.go create mode 100644 internal/naming/naming.go create mode 100644 internal/naming/naming_test.go create mode 100644 internal/policy/input.go create mode 100644 internal/policy/scope_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 50bfec5..4ff4dea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **BREAKING (data): a `ResourcePolicy` in any namespace no longer outranks every `ClusterResourcePolicy` fleet-wide, and policies are no longer resolved differently at admission than during measurement and resize** ([#87](https://github.com/Tight-Line/ballast/issues/87)). Both were the same defect. `WorkloadProfile` is cluster-scoped and carries only its identity tuple, so the metrics collector and resource adjuster were resolving policy from an input with no namespace, no owner kind, and no annotations. `client.InNamespace("")` means *all namespaces*, and namespace-scoped policies rank above cluster-scoped ones regardless of priority, so one `ResourcePolicy` with a loose selector governed recommendations and resizes for every matching profile in the cluster, outranking policies that explicitly set a higher priority. In the same breath, any policy selecting on `kinds`, `annotations`, or `namespaces.include` matched at admission but could never match in the controllers, so pods were admitted with one policy's values and then resized toward another's, with nothing logging a conflict because each path resolved successfully on its own terms. + + Policy is now resolved exactly once per pod, by the workloadwatcher, which is the only component holding a pod and therefore the only one that can evaluate a selector correctly. The result is recorded on `workloadprofile.status.policyRef`; the metrics collector and resource adjuster read it instead of resolving again, so all three paths agree by construction. A `Resolver` `Input` with no namespace can no longer match a `ResourcePolicy` at all. + + **The governing policy is now part of a profile's identity.** A profile holds one set of recommendations per container, and the policy chooses the metrics sources, poll cadence, tracked resources, aggregation, and headroom that produce them, so pods resolving to different policies now belong to different profiles. Profile names gain a policy token (`checkout--server--fleet-a1b2c3d4`), and each profile owns its own Redis key namespace via `status.measurementHash`, so sibling profiles never write into one sample series and the profile finalizer purges only its own keys. + + **Upgrade impact: every existing `WorkloadProfile` is replaced by a newly-named one, and the fleet accrues from zero for one `readiness.minTimeSpan` (24h by default) before resizes resume.** No resizes are issued during that window; admission continues to apply nothing until each new profile is ready, so pods keep whatever requests they were created with. The old profiles orphan, age out over `orphanTTL` (168h), and their Redis history is purged by the finalizer as they go. Nothing needs to be done by hand. + +### Added + +- **Policy changes now take effect on a running cluster without waiting for pod churn.** The workloadwatcher watches both policy kinds and re-evaluates every enrolled pod when a policy is created, deleted, or has its selector or priority changed. Edits to the rest of a policy's spec (aggregation, headroom, thresholds, sources, cadence) are deliberately *not* treated as identity changes: they are read live on the next collection or resize cycle, so they take effect without re-keying measurement history or forcing a fresh accrual. +- **`status.profileDiscriminator` on `ClusterResourcePolicy` and `ResourcePolicy`**, shown as a `DISCRIMINATOR` column in `kubectl get`. It is the token that policy contributes to the names of the profiles it governs, so they can be listed without computing a hash by hand: + `kubectl get workloadprofiles | grep "$(kubectl get crp fleet -o jsonpath='{.status.profileDiscriminator}')"`. Derived from the policy's kind, namespace, and name, so two same-named `ResourcePolicies` in different namespaces stay distinct. +- **`POLICY` column on `kubectl get workloadprofiles`**, showing the governing policy from `status.policyRef`. +- The `policy-ref` pod annotation is now refreshed by the workloadwatcher when a policy change moves a pod. Previously it kept advertising whatever was resolved at admission. +- Documented on the `PolicySelector` CRD type which selector fields can scope measurement and which cannot: `namespaces` and `labelSelector` are expressible as server-side pod queries, `annotations` is not, so annotation selectors govern admission but cannot narrow a profile to an annotated subset of its tuple. + ## [0.5.0] - 2026-07-28 ### Changed diff --git a/README.md b/README.md index 73fc6a0..7711d6f 100644 --- a/README.md +++ b/README.md @@ -419,7 +419,16 @@ This catch-all policy applies to every opted-in pod in the cluster. Key design d - **250 samples over 24 hours before acting.** At the 5-minute poll interval a single long-running pod accrues ~288 samples in 24h, so the 24h window — not the sample count — is the binding constraint. A high coefficient of variation (CV > 1.5) also blocks action — it means the workload is too spiky to size reliably. The CV check is skipped when mean usage sits below a tiny per-resource floor (`cvMeanFloor`, defaults: 25m CPU, 25Mi memory, 2Mi ephemeral-storage): CV divides by the mean, so near-idle workloads produce huge CVs from quantization noise and rare startup spikes alone, and without the floor a single near-idle resource would pin the whole profile at `Accruing` forever — blocking recommendations for every other resource. Usage below the floor is too small for a mis-sized recommendation to matter. - **10% drift threshold.** A resize only fires when the current resource value deviates from the recommendation by more than 10%. In-place resize is cheap and safe (a request/limit patch on a running pod, no restart), so the band is deliberately tight: a recommendation that has moved more than 10% reflects a real shift in observed usage worth acting on, not noise. - **50% max change per cycle.** Each resize moves at most half the remaining gap between the current value and the recommendation, giving workloads time to stabilize between adjustments. The first step makes most of the correction; once a step would land within the drift threshold, the recommendation is applied exactly, so convergence completes instead of stalling just inside the threshold. -- **Priority 0.** This is the lowest possible priority. Any `ClusterResourcePolicy` or `ResourcePolicy` with `priority > 0` wins for matched workloads, so you can override specific namespaces or workload kinds without touching this default. +- **Priority 0.** This is the lowest possible priority. Any `ClusterResourcePolicy` or `ResourcePolicy` with `priority > 0` wins for matched workloads, so you can override specific namespaces or workload kinds without touching this default. A `ResourcePolicy` also beats a `ClusterResourcePolicy` for pods in its own namespace regardless of priority, on the principle that the namespace owner's policy is the more specific match; it has no effect on pods in any other namespace. + +Each workload is governed by exactly one policy, and that policy is part of the workload's `WorkloadProfile` identity: pods that resolve to different policies get different profiles, because a profile holds one set of recommendations and the policy is what produces them. Profile names therefore carry a token identifying the policy (`checkout--server--default-a1b2c3d4`), and `kubectl get workloadprofiles` shows the governing policy in its own column. To list the profiles a policy governs: + +```sh +kubectl get workloadprofiles | grep "$(kubectl get clusterresourcepolicy default \ + -o jsonpath='{.status.profileDiscriminator}')" +``` + +Applying, editing, or deleting a policy takes effect on a running cluster within seconds; no pod restart is needed. Changing a policy's **selector or priority** re-scopes which workloads it governs, so affected pods move to a different profile and begin accruing history there. Changing anything else (aggregation, headroom, thresholds, sources, cadence) takes effect on the next collection or resize cycle with no loss of history. ### Policy presets diff --git a/api/v1/clusterresourcepolicy_types.go b/api/v1/clusterresourcepolicy_types.go index 6f17a74..5f81aca 100644 --- a/api/v1/clusterresourcepolicy_types.go +++ b/api/v1/clusterresourcepolicy_types.go @@ -37,6 +37,19 @@ type ClusterResourcePolicySpec struct { } // PolicySelector filters which workloads a policy applies to. +// +// Every field is evaluated against a real pod, at admission and again whenever the +// workloadwatcher reconciles that pod. Both use the same inputs, so all four fields +// are honored consistently and a pod is measured and resized under the same policy +// it was admitted with; the resolved policy is recorded on the WorkloadProfile's +// status.policyRef, and the metrics collector and resource adjuster read it there +// rather than re-deriving it from the profile. +// +// One asymmetry is inherent rather than incidental: a policy decides which pods it +// governs, but the pods a WorkloadProfile *measures* are fetched with a +// server-side label selector. Distinctions that Kubernetes can express in such a +// query (namespace, labels) can therefore split measurement; annotations cannot. +// See the note on Annotations below. type PolicySelector struct { // Kinds lists the owner kinds this policy applies to (e.g. Deployment, StatefulSet). // Empty means all kinds. @@ -48,6 +61,13 @@ type PolicySelector struct { Namespaces NamespaceSelector `json:"namespaces,omitempty"` // Annotations maps annotation keys to regex patterns that must match on the pod. + // + // Annotation selectors decide which policy governs a pod, but they cannot + // scope *measurement*. A WorkloadProfile gathers its pods with a label + // selector served by the API server, and there is no equivalent query for + // annotations, so a profile cannot be narrowed to "the annotated subset" of + // its identity tuple. Where that distinction matters, use a label instead: + // labelSelector and namespaces are both expressible server-side. // +optional Annotations map[string]string `json:"annotations,omitempty"` @@ -200,11 +220,33 @@ type ResizeConfig struct { } // ClusterResourcePolicyStatus defines the observed state of ClusterResourcePolicy. -type ClusterResourcePolicyStatus struct{} +type ClusterResourcePolicyStatus struct { + // ProfileDiscriminator is the token this policy contributes to the name of + // every WorkloadProfile it governs, in the form "-". + // + // A WorkloadProfile's identity includes the policy governing it, because the + // policy decides which metrics sources are polled, how samples are + // aggregated, and how much headroom is added; pods resolving to different + // policies therefore cannot share one set of recommendations. This field + // makes the resulting profile names traceable back to their policy without + // recomputing a hash by hand: + // + // kubectl get workloadprofiles | grep "$(kubectl get crp fleet \ + // -o jsonpath='{.status.profileDiscriminator}')" + // + // The token is derived from the policy's kind, namespace, and name, so it is + // stable for the life of the object and identical across every profile that + // resolves to it. Renaming a policy produces a different token, and hence + // new profiles. + // +optional + ProfileDiscriminator string `json:"profileDiscriminator,omitempty"` +} // +kubebuilder:object:root=true // +kubebuilder:resource:scope=Cluster +// +kubebuilder:subresource:status // +kubebuilder:printcolumn:name="Priority",type="integer",JSONPath=".spec.priority" +// +kubebuilder:printcolumn:name="Discriminator",type="string",JSONPath=".status.profileDiscriminator" // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" // ClusterResourcePolicy is the Schema for the clusterresourcepolicies API diff --git a/api/v1/resourcepolicy_types.go b/api/v1/resourcepolicy_types.go index 7a7c3ca..5b04cc7 100644 --- a/api/v1/resourcepolicy_types.go +++ b/api/v1/resourcepolicy_types.go @@ -16,11 +16,14 @@ import ( type ResourcePolicySpec = ClusterResourcePolicySpec // ResourcePolicyStatus defines the observed state of ResourcePolicy. -type ResourcePolicyStatus struct{} +// It uses the same shape as ClusterResourcePolicyStatus. +type ResourcePolicyStatus = ClusterResourcePolicyStatus // +kubebuilder:object:root=true // +kubebuilder:resource:scope=Namespaced +// +kubebuilder:subresource:status // +kubebuilder:printcolumn:name="Priority",type="integer",JSONPath=".spec.priority" +// +kubebuilder:printcolumn:name="Discriminator",type="string",JSONPath=".status.profileDiscriminator" // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" // ResourcePolicy is the Schema for the resourcepolicies API diff --git a/api/v1/workloadprofile_types.go b/api/v1/workloadprofile_types.go index 9fef058..5a41b34 100644 --- a/api/v1/workloadprofile_types.go +++ b/api/v1/workloadprofile_types.go @@ -18,6 +18,7 @@ import ( // +kubebuilder:resource:scope=Cluster // +kubebuilder:subresource:status // +kubebuilder:printcolumn:name="ActiveWorkloads",type="integer",JSONPath=".status.activeWorkloads" +// +kubebuilder:printcolumn:name="Policy",type="string",JSONPath=".status.policyRef.name" // +kubebuilder:printcolumn:name="State",type="string",JSONPath=".status.state" // +kubebuilder:printcolumn:name="Orphaned",type="string",JSONPath=".status.conditions[?(@.type=='Orphaned')].status" // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" @@ -45,12 +46,69 @@ const ( WorkloadProfileStateSufficient WorkloadProfileState = "Sufficient" ) +// Policy object kinds, as recorded in PolicyReference.Kind. +const ( + // KindClusterResourcePolicy is the cluster-scoped policy kind. + KindClusterResourcePolicy = "ClusterResourcePolicy" + // KindResourcePolicy is the namespace-scoped policy kind. + KindResourcePolicy = "ResourcePolicy" +) + +// PolicyReference identifies the policy that governs a WorkloadProfile. +type PolicyReference struct { + // Kind is the referenced policy's kind. + // +kubebuilder:validation:Enum=ClusterResourcePolicy;ResourcePolicy + Kind string `json:"kind"` + + // Namespace is the policy's namespace, empty for a ClusterResourcePolicy. + // +optional + Namespace string `json:"namespace,omitempty"` + + // Name is the policy object's name. + Name string `json:"name"` +} + +// Key returns the canonical identity of the referenced policy as +// "kind/namespace/name". Two ResourcePolicies in different namespaces may share +// a name, so namespace and kind both participate: the key is what distinguishes +// genuinely different policies wherever a policy identity is hashed or compared. +func (r PolicyReference) Key() string { + return r.Kind + "/" + r.Namespace + "/" + r.Name +} + // WorkloadProfileStatus holds the observed state of a WorkloadProfile. type WorkloadProfileStatus struct { // TupleLabels are the identity labels that define this profile. // +optional TupleLabels map[string]string `json:"tupleLabels,omitempty"` + // PolicyRef identifies the policy governing this profile, or is unset when no + // policy currently matches (the metrics collector then has nothing to + // measure with and skips the profile). + // + // The governing policy is part of a profile's identity, not merely an + // observation about it: the policy chooses the metrics sources, the poll + // cadence, the tracked resources, the aggregation, and the headroom behind + // one set of recommendations. Pods that resolve to different policies + // therefore belong to different profiles. The workloadwatcher resolves policy + // per pod (the only place with a pod's namespace, labels, annotations, and + // owner kind) and records the result here; the metrics collector and resource + // adjuster read it rather than resolving again, so all three agree by + // construction. + // +optional + PolicyRef *PolicyReference `json:"policyRef,omitempty"` + + // MeasurementHash identifies the Redis key namespace this profile owns. It + // covers both TupleLabels and PolicyRef, so profiles that share a tuple but + // resolve to different policies keep separate sample series and the profile + // finalizer can purge its own keys without reference counting. + // + // Recorded here rather than recomputed on demand so that the collector's + // writes and the finalizer's purge can never disagree about which keys belong + // to this profile. + // +optional + MeasurementHash string `json:"measurementHash,omitempty"` + // SelectorLabels are used to query pods from the metrics API. // Keys absent from the originating pod carry the sentinel value "--missing--", // which the metrics plugin translates to a Kubernetes "!key" (does-not-exist) diff --git a/api/v1/workloadprofile_types_test.go b/api/v1/workloadprofile_types_test.go new file mode 100644 index 0000000..55e8630 --- /dev/null +++ b/api/v1/workloadprofile_types_test.go @@ -0,0 +1,56 @@ +/* +Copyright 2026 Tight Line LLC. + +Licensed under the MIT License. See LICENSE for the full text. +*/ + +package v1_test + +import ( + "testing" + + ballastv1 "github.com/tight-line/ballast/api/v1" +) + +// Key is what distinguishes genuinely different policies wherever a policy +// identity is hashed or compared, so kind and namespace must both participate: +// two ResourcePolicies in different namespaces may share a name. +func TestPolicyReference_Key(t *testing.T) { + tests := []struct { + name string + ref ballastv1.PolicyReference + want string + }{ + { + name: "cluster-scoped policy has no namespace segment", + ref: ballastv1.PolicyReference{Kind: ballastv1.KindClusterResourcePolicy, Name: "fleet"}, + want: "ClusterResourcePolicy//fleet", + }, + { + name: "namespaced policy carries its namespace", + ref: ballastv1.PolicyReference{ + Kind: ballastv1.KindResourcePolicy, + Namespace: "team-a", + Name: "local", + }, + want: "ResourcePolicy/team-a/local", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := tc.ref.Key(); got != tc.want { + t.Errorf("Key() = %q, want %q", got, tc.want) + } + }) + } +} + +// Same name, different namespaces: different policies, so different keys. +func TestPolicyReference_KeyDistinguishesNamespace(t *testing.T) { + a := ballastv1.PolicyReference{Kind: ballastv1.KindResourcePolicy, Namespace: "team-a", Name: "defaults"} + b := ballastv1.PolicyReference{Kind: ballastv1.KindResourcePolicy, Namespace: "team-b", Name: "defaults"} + if a.Key() == b.Key() { + t.Errorf("same key %q for policies in different namespaces", a.Key()) + } +} diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index dcf2aab..58eab01 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -416,6 +416,21 @@ func (in *NamespaceSelector) DeepCopy() *NamespaceSelector { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PolicyReference) DeepCopyInto(out *PolicyReference) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PolicyReference. +func (in *PolicyReference) DeepCopy() *PolicyReference { + if in == nil { + return nil + } + out := new(PolicyReference) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PolicySelector) DeepCopyInto(out *PolicySelector) { *out = *in @@ -529,7 +544,7 @@ func (in *ResourcePolicy) DeepCopyInto(out *ResourcePolicy) { out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) in.Spec.DeepCopyInto(&out.Spec) - out.Status = in.Status + in.Status.DeepCopyInto(&out.Status) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ResourcePolicy. @@ -582,21 +597,6 @@ func (in *ResourcePolicyList) DeepCopyObject() runtime.Object { return nil } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ResourcePolicyStatus) DeepCopyInto(out *ResourcePolicyStatus) { - *out = *in -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ResourcePolicyStatus. -func (in *ResourcePolicyStatus) DeepCopy() *ResourcePolicyStatus { - if in == nil { - return nil - } - out := new(ResourcePolicyStatus) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ResourceRecommendation) DeepCopyInto(out *ResourceRecommendation) { *out = *in @@ -696,6 +696,11 @@ func (in *WorkloadProfileStatus) DeepCopyInto(out *WorkloadProfileStatus) { (*out)[key] = val } } + if in.PolicyRef != nil { + in, out := &in.PolicyRef, &out.PolicyRef + *out = new(PolicyReference) + **out = **in + } if in.SelectorLabels != nil { in, out := &in.SelectorLabels, &out.SelectorLabels *out = make(map[string]string, len(*in)) diff --git a/charts/ballast/templates/clusterrole.yaml b/charts/ballast/templates/clusterrole.yaml index 28a348d..f63d287 100644 --- a/charts/ballast/templates/clusterrole.yaml +++ b/charts/ballast/templates/clusterrole.yaml @@ -44,3 +44,11 @@ rules: - apiGroups: ["ballast.tightlinesoftware.com"] resources: ["workloadprofiles/status"] verbs: ["patch"] +# Policy status carries only the operator-derived profileDiscriminator, which the +# policystatus controllers publish so the WorkloadProfiles a policy governs can be +# found from the policy. The policies themselves stay read-only above. +- apiGroups: ["ballast.tightlinesoftware.com"] + resources: + - clusterresourcepolicies/status + - resourcepolicies/status + verbs: ["patch"] diff --git a/cmd/ballastd/main.go b/cmd/ballastd/main.go index ed0c2c2..ae98dbc 100644 --- a/cmd/ballastd/main.go +++ b/cmd/ballastd/main.go @@ -40,6 +40,7 @@ import ( ballastv1 "github.com/tight-line/ballast/api/v1" crdmanifests "github.com/tight-line/ballast/config/crd/bases" "github.com/tight-line/ballast/internal/controller/metricscollector" + "github.com/tight-line/ballast/internal/controller/policystatus" "github.com/tight-line/ballast/internal/controller/resourceadjuster" "github.com/tight-line/ballast/internal/controller/workloadwatcher" "github.com/tight-line/ballast/internal/crdapply" @@ -398,6 +399,15 @@ func registerComponents( return fmt.Errorf("set up metricscollector controller: %w", err) } + // Publishes each policy's profile discriminator on its status, so the profiles + // a policy governs can be found from the policy object. + if err := policystatus.NewCluster(mgr.GetClient()).SetupWithManager(mgr); err != nil { + return fmt.Errorf("set up clusterresourcepolicy status controller: %w", err) + } + if err := policystatus.NewNamespaced(mgr.GetClient()).SetupWithManager(mgr); err != nil { + return fmt.Errorf("set up resourcepolicy status controller: %w", err) + } + ballastwebhook.NewPodMutator(mgr.GetClient(), ks, dryRunApply, rec).SetupWithManager(mgr) if err := resourceadjuster.Setup(mgr, ks, dryRunResize, rec); err != nil { diff --git a/config/crd/bases/ballast.tightlinesoftware.com_clusterresourcepolicies.yaml b/config/crd/bases/ballast.tightlinesoftware.com_clusterresourcepolicies.yaml index 0c16fc4..95e2f77 100644 --- a/config/crd/bases/ballast.tightlinesoftware.com_clusterresourcepolicies.yaml +++ b/config/crd/bases/ballast.tightlinesoftware.com_clusterresourcepolicies.yaml @@ -18,6 +18,9 @@ spec: - jsonPath: .spec.priority name: Priority type: integer + - jsonPath: .status.profileDiscriminator + name: Discriminator + type: string - jsonPath: .metadata.creationTimestamp name: Age type: date @@ -206,8 +209,15 @@ spec: annotations: additionalProperties: type: string - description: Annotations maps annotation keys to regex patterns - that must match on the pod. + description: |- + Annotations maps annotation keys to regex patterns that must match on the pod. + + Annotation selectors decide which policy governs a pod, but they cannot + scope *measurement*. A WorkloadProfile gathers its pods with a label + selector served by the API server, and there is no equivalent query for + annotations, so a profile cannot be narrowed to "the annotated subset" of + its identity tuple. Where that distinction matters, use a label instead: + labelSelector and namespaces are both expressible server-side. type: object kinds: description: |- @@ -287,10 +297,27 @@ spec: type: object status: description: status defines the observed state of ClusterResourcePolicy + properties: + profileDiscriminator: + description: "ProfileDiscriminator is the token this policy contributes + to the name of\nevery WorkloadProfile it governs, in the form \"-\".\n\nA + WorkloadProfile's identity includes the policy governing it, because + the\npolicy decides which metrics sources are polled, how samples + are\naggregated, and how much headroom is added; pods resolving + to different\npolicies therefore cannot share one set of recommendations. + This field\nmakes the resulting profile names traceable back to + their policy without\nrecomputing a hash by hand:\n\n\tkubectl get + workloadprofiles | grep \"$(kubectl get crp fleet \\\n\t -o jsonpath='{.status.profileDiscriminator}')\"\n\nThe + token is derived from the policy's kind, namespace, and name, so + it is\nstable for the life of the object and identical across every + profile that\nresolves to it. Renaming a policy produces a different + token, and hence\nnew profiles." + type: string type: object required: - spec type: object served: true storage: true - subresources: {} + subresources: + status: {} diff --git a/config/crd/bases/ballast.tightlinesoftware.com_resourcepolicies.yaml b/config/crd/bases/ballast.tightlinesoftware.com_resourcepolicies.yaml index 678d245..24694ae 100644 --- a/config/crd/bases/ballast.tightlinesoftware.com_resourcepolicies.yaml +++ b/config/crd/bases/ballast.tightlinesoftware.com_resourcepolicies.yaml @@ -18,6 +18,9 @@ spec: - jsonPath: .spec.priority name: Priority type: integer + - jsonPath: .status.profileDiscriminator + name: Discriminator + type: string - jsonPath: .metadata.creationTimestamp name: Age type: date @@ -205,8 +208,15 @@ spec: annotations: additionalProperties: type: string - description: Annotations maps annotation keys to regex patterns - that must match on the pod. + description: |- + Annotations maps annotation keys to regex patterns that must match on the pod. + + Annotation selectors decide which policy governs a pod, but they cannot + scope *measurement*. A WorkloadProfile gathers its pods with a label + selector served by the API server, and there is no equivalent query for + annotations, so a profile cannot be narrowed to "the annotated subset" of + its identity tuple. Where that distinction matters, use a label instead: + labelSelector and namespaces are both expressible server-side. type: object kinds: description: |- @@ -286,10 +296,27 @@ spec: type: object status: description: status defines the observed state of ResourcePolicy + properties: + profileDiscriminator: + description: "ProfileDiscriminator is the token this policy contributes + to the name of\nevery WorkloadProfile it governs, in the form \"-\".\n\nA + WorkloadProfile's identity includes the policy governing it, because + the\npolicy decides which metrics sources are polled, how samples + are\naggregated, and how much headroom is added; pods resolving + to different\npolicies therefore cannot share one set of recommendations. + This field\nmakes the resulting profile names traceable back to + their policy without\nrecomputing a hash by hand:\n\n\tkubectl get + workloadprofiles | grep \"$(kubectl get crp fleet \\\n\t -o jsonpath='{.status.profileDiscriminator}')\"\n\nThe + token is derived from the policy's kind, namespace, and name, so + it is\nstable for the life of the object and identical across every + profile that\nresolves to it. Renaming a policy produces a different + token, and hence\nnew profiles." + type: string type: object required: - spec type: object served: true storage: true - subresources: {} + subresources: + status: {} diff --git a/config/crd/bases/ballast.tightlinesoftware.com_workloadprofiles.yaml b/config/crd/bases/ballast.tightlinesoftware.com_workloadprofiles.yaml index 9efd703..6f134ba 100644 --- a/config/crd/bases/ballast.tightlinesoftware.com_workloadprofiles.yaml +++ b/config/crd/bases/ballast.tightlinesoftware.com_workloadprofiles.yaml @@ -18,6 +18,9 @@ spec: - jsonPath: .status.activeWorkloads name: ActiveWorkloads type: integer + - jsonPath: .status.policyRef.name + name: Policy + type: string - jsonPath: .status.state name: State type: string @@ -206,10 +209,54 @@ spec: - name type: object type: array + measurementHash: + description: |- + MeasurementHash identifies the Redis key namespace this profile owns. It + covers both TupleLabels and PolicyRef, so profiles that share a tuple but + resolve to different policies keep separate sample series and the profile + finalizer can purge its own keys without reference counting. + + Recorded here rather than recomputed on demand so that the collector's + writes and the finalizer's purge can never disagree about which keys belong + to this profile. + type: string meetsThreshold: description: MeetsThreshold is true when the profile has sufficient history to act on. type: boolean + policyRef: + description: |- + PolicyRef identifies the policy governing this profile, or is unset when no + policy currently matches (the metrics collector then has nothing to + measure with and skips the profile). + + The governing policy is part of a profile's identity, not merely an + observation about it: the policy chooses the metrics sources, the poll + cadence, the tracked resources, the aggregation, and the headroom behind + one set of recommendations. Pods that resolve to different policies + therefore belong to different profiles. The workloadwatcher resolves policy + per pod (the only place with a pod's namespace, labels, annotations, and + owner kind) and records the result here; the metrics collector and resource + adjuster read it rather than resolving again, so all three agree by + construction. + properties: + kind: + description: Kind is the referenced policy's kind. + enum: + - ClusterResourcePolicy + - ResourcePolicy + type: string + name: + description: Name is the policy object's name. + type: string + namespace: + description: Namespace is the policy's namespace, empty for a + ClusterResourcePolicy. + type: string + required: + - kind + - name + type: object selectorLabels: additionalProperties: type: string diff --git a/docs/convergence.md b/docs/convergence.md index a08aef0..693110e 100644 --- a/docs/convergence.md +++ b/docs/convergence.md @@ -17,22 +17,47 @@ These are the load-bearing principles. Every scenario below is a consequence of them; do not add behavior that violates one without revisiting this document. 1. **`profile-ref` is a deterministic function of identity, not an identity itself.** - A profile's name is `ProfileName(tupleLabels, identityLabels)`. Any pod with the - same identity computes the same name, so re-association after a delete/recreate is - automatic and requires no UID/ownerReference bookkeeping. Never model this - relationship with `metav1.OwnerReference` (wrong cardinality and wrong cascade - direction). - -2. **Annotations and stamps are hints, not the source of truth.** Every pod + A profile's name is `naming.ProfileName(tupleLabels, identityLabels, discriminator)`, + where the discriminator is a token derived from the policy governing the pod (or + `nopolicy` when none matches). Any pod with the same identity computes the same + name, so re-association after a delete/recreate is automatic and requires no + UID/ownerReference bookkeeping. Never model this relationship with + `metav1.OwnerReference` (wrong cardinality and wrong cascade direction). + + **The governing policy is part of identity.** A profile carries one set of + recommendations per container, and the policy decides the metrics sources, poll + cadence, tracked resources, aggregation, and headroom behind them; pods resolving + to different policies have no shared answer and so cannot share a profile. Policy + is resolved *per pod*, here, because a pod reconcile is the only place holding the + namespace, full labels, annotations, and owner kind that selectors are written + against. The result is recorded on `status.policyRef` and read by the metrics + collector and resource adjuster; they never re-resolve, which is what keeps + admission, measurement, and resize on one policy. + +2. **A profile's measurement history is keyed separately from its name.** + `status.measurementHash` covers the identity tuple *and* the policy, and is what + Redis keys derive from. Two consequences are load-bearing: sibling profiles + sharing a tuple never write into one sample series (samples carry no per-sample + timestamps, so interleaved writes would inflate counts and distort the + distribution), and the profile finalizer can purge its own keys with no reference + counting. Never fold the policy into `tupleLabels` to achieve the same split: + that map is also the source of `status.selectorLabels`, the server-side label + query that finds the profile's pods, and a policy name is not a pod label. + +3. **Annotations and stamps are hints, not the source of truth.** Every pod CREATE/UPDATE reconcile recomputes the desired profile from the pod's *current* labels and the *current* `identityLabels`, then reconciles toward it. The stamp is a cache used only on the DELETE path (where the pod is leaving and there is - nothing to recompute). The same applies to the profile's own status: each - reconcile converges `status.tupleLabels` / `status.selectorLabels` to the - recomputed values, so a lost initial status write (conflict, crash, older - operator version) heals on the next reconcile of any member pod. - -3. **Counts are level-triggered, never incremental.** `setActiveWorkloads` derives + nothing to recompute). Policy is re-resolved on every reconcile for the same + reason, so a policy created, edited, or deleted while the pod runs moves it to + the right profile. The `policy-ref` annotation is likewise refreshed rather than + left as the webhook wrote it at admission. The same applies to the profile's own + status: each reconcile converges `status.tupleLabels`, `status.selectorLabels`, + `status.policyRef`, and `status.measurementHash` to the recomputed values, so a + lost initial status write (conflict, crash, older operator version) heals on the + next reconcile of any member pod. + +4. **Counts are level-triggered, never incremental.** `setActiveWorkloads` derives the count by listing the profile's member pods and counting live ones, so any missed or duplicated event self-heals on the next reconcile. It never does `count++/count--`. Both reconcilers enforce this. The pod reconciler recounts @@ -58,22 +83,34 @@ them; do not add behavior that violates one without revisiting this document. `old == new`, so any field-diff predicate would also drop the resync events the backstop depends on. -4. **Cleanup lives in the finalizer, and only there.** Redis history is purged by the +5. **Cleanup lives in the finalizer, and only there.** Redis history is purged by the WorkloadProfile cleanup finalizer, so every deletion path (orphan-TTL sweep or manual `kubectl delete`) clears history exactly once. The finalizer never reaches out to mutate sibling Pods — each controller repairs its own object. -5. **Watches are for promptness; the informer resync is the correctness backstop.** - The pod controller watches WorkloadProfile deletions and `identityLabels` changes - so convergence is prompt (seconds). Even if a watch event is missed, the ~10h - resync re-reconciles every pod and converges. Watch predicates are deliberately - narrow (delete-only; identityLabels-only and filtered to the canonical - BallastConfig name) to avoid enqueue amplification, since profile status is - written on every count change. BallastConfig *creation* is also admitted: pods - reconciled while the config was absent were skipped, and a delete + re-apply - never fires the update predicate. - -6. **The kill switch defers work; it must not lose it.** Enrollment reconciles +6. **Watches are for promptness; the informer resync is the correctness backstop.** + The pod controller watches WorkloadProfile deletions, `identityLabels` changes, + and both policy kinds, so convergence is prompt (seconds). Even if a watch event + is missed, the ~10h resync re-reconciles every pod and converges. Watch + predicates are deliberately narrow (delete-only; identityLabels-only and filtered + to the canonical BallastConfig name) to avoid enqueue amplification, since profile + status is written on every count change. BallastConfig *creation* is also + admitted: pods reconciled while the config was absent were skipped, and a delete + + re-apply never fires the update predicate. + + The policy watches admit creation, deletion, and updates that change the selector + or the priority — the only fields that decide *which* policy wins. Every other + spec field is read live by the metrics collector and resource adjuster on their + next cycle, so admitting those edits would re-key measurement history and force a + fresh accrual for no gain. Because resolution depends on the whole policy set, + the handler enqueues every enrolled pod rather than trying to compute the affected + subset: on a delete the matching spec no longer exists, on a narrowed selector the + affected pods are the ones that stopped matching, and a new high-priority policy + flips pods that previously matched nothing. Each of those reconciles is a cache + read plus a resolve and writes nothing unless the pod's identity actually + changed. + +7. **The kill switch defers work; it must not lose it.** Enrollment reconciles skipped while the kill switch is active requeue every minute, so releasing the switch converges promptly (including a one-shot `identityLabels` fan-out that fired mid-outage) instead of waiting for resync. The DELETE path is never @@ -83,8 +120,8 @@ them; do not add behavior that violates one without revisiting this document. - **W** — the workload / kubelet / user (`kubectl`, GitOps) - **API** — the Kubernetes API server and the controllers' shared informer cache -- **PodR** — `PodReconciler` (watches Pods; also WorkloadProfile deletes and - BallastConfig `identityLabels` changes) +- **PodR** — `PodReconciler` (watches Pods; also WorkloadProfile deletes, + BallastConfig `identityLabels` changes, and both policy kinds) - **ProfR** — `ProfileReconciler` (watches WorkloadProfile) - **Redis** — the metric-history store @@ -215,11 +252,21 @@ sequenceDiagram ## 6. Identity change — migration -Changing `identityLabels` (cluster-wide, via `BallastConfig`) or a pod's own -identity-label values changes the pod's computed profile name. The pod migrates and -the profile it leaves is recounted so it can orphan and age out. A `BallastConfig` -`identityLabels` edit is made prompt by the config watch; a pod-label change is -delivered as an ordinary pod update. +Three inputs change a pod's computed profile name: `identityLabels` (cluster-wide, +via `BallastConfig`), the pod's own identity-label values, and the policy that +governs the pod. In every case the pod migrates and the profile it leaves is +recounted so it can orphan and age out. A `BallastConfig` `identityLabels` edit is +made prompt by the config watch, a policy change by the policy watches, and a +pod-label change is delivered as an ordinary pod update. + +A policy-driven migration is the one that also moves measurement history, because +`status.measurementHash` covers the policy: the new profile starts from zero samples +and accrues for `readiness.minTimeSpan` before it can be acted on, while the old +profile keeps its history until it ages out and its finalizer purges it. This is the +accepted cost of making the policy part of identity — a profile holds one answer, and +a different policy is a different answer. It is deliberately *not* triggered by edits +to a policy's aggregation, headroom, thresholds, sources, or cadence; those are read +live on the next cycle (see invariant 6). ```mermaid sequenceDiagram @@ -304,12 +351,13 @@ within one cycle. | Profile deleted (manual or TTL) | WorkloadProfile delete watch (`podsForProfile`) | resync of referencing pods | | Stale count / lost trailing recount | WorkloadProfile events (`recountActiveWorkloads`) | resync of the profile | | Profile status labels lost | Next reconcile of any member pod | resync | +| Policy created / deleted / re-scoped | Policy watches (`allManagedPods` fan-out) | resync | | Kill switch released | 1-minute requeue of skipped pods | resync | | Redis history on any profile delete | Cleanup finalizer | — (single chokepoint) | ## Known limitations -- **Profile-name collisions.** `sanitizeName` can map two distinct identity tuples to +- **Profile-name collisions.** `naming.SanitizeSegment` can map two distinct identity tuples to the same profile name (`Web` vs `web`, `a.b` vs `a-b`). Colliding workloads share one profile, and its status labels converge to whichever pod reconciled most recently (visible as the tuple labels flapping between the two identities). Avoid diff --git a/internal/controller/metricscollector/controller.go b/internal/controller/metricscollector/controller.go index 01e8f51..6fcc484 100644 --- a/internal/controller/metricscollector/controller.go +++ b/internal/controller/metricscollector/controller.go @@ -129,12 +129,35 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{RequeueAfter: 5 * time.Second}, nil } - resolved, err := r.resolver.Resolve(ctx, policy.Input{Labels: profile.Status.TupleLabels}) - if err != nil { // coverage:ignore - transient API error + // Same race, and the same treatment: measurementHash names the Redis key + // namespace this profile owns, and writing samples before it is set would put + // them under a key belonging to no profile, where nothing would ever read or + // purge them. A profile inherited from a release that predates the field is + // back-filled by the workloadwatcher on its next pod reconcile. + if profile.Status.MeasurementHash == "" { + log.Info("measurementHash not yet set, requeueing", "profile", profile.Name) + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + + // The governing policy is read from the profile, not re-resolved from it. A + // WorkloadProfile is cluster-scoped and carries only its identity tuple, so it + // supplies neither a namespace nor the pod labels outside that tuple; + // re-resolving here would reach a different policy than admission did and + // silently measure against one policy while pods were admitted under another. + // The workloadwatcher resolves per pod and records the answer in policyRef. + if profile.Status.PolicyRef == nil { + log.Info("no policy matches profile, skipping", "profile", profile.Name) + return ctrl.Result{RequeueAfter: defaultPollInterval}, nil + } + resolved, err := r.resolver.Load(ctx, *profile.Status.PolicyRef) + if err != nil { return ctrl.Result{}, err } if resolved == nil { - log.Info("no policy matches profile, skipping", "profile", profile.Name) + // The policy was deleted. The workloadwatcher's policy watch is already + // migrating these pods to a new profile, which orphans this one. + log.Info("policy referenced by profile no longer exists, skipping", + "profile", profile.Name, "policy", profile.Status.PolicyRef.Key()) return ctrl.Result{RequeueAfter: defaultPollInterval}, nil } @@ -147,7 +170,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } now := time.Now() - tupleHash := store.TupleHash(profile.Status.TupleLabels) + measurementHash := profile.Status.MeasurementHash pid := metrics.ProfileID{Name: profile.Name, Labels: profile.Status.TupleLabels} excluded := r.excludedContainerNames(ctx, profile.Status.SelectorLabels) @@ -159,7 +182,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // the manager's pod cache, which is already scoped to enrolled pods. measureSelector := enrolledSelector(profile.Status.SelectorLabels) - observed, err := r.collectAllSamples(ctx, tupleHash, pid, measureSelector, now, sources, excluded) + observed, err := r.collectAllSamples(ctx, measurementHash, pid, measureSelector, now, sources, excluded) if err != nil { // coverage:ignore - Redis error return ctrl.Result{}, err } @@ -167,7 +190,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu resourcesInPolicy := policyResourceMap(resolved.Spec.Metrics) containers := mergeContainerSets(observed, profile.Status.Containers, resourcesInPolicy) containerProfiles, allReady := r.buildContainerProfiles( - ctx, tupleHash, containers, resourcesInPolicy, resolved.Spec, now.UnixMilli()) + ctx, measurementHash, containers, resourcesInPolicy, resolved.Spec, now.UnixMilli()) if r.dryRunMeasure { log.Info("dry-run: would update WorkloadProfile status", @@ -216,7 +239,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // Returns the union of (container, resource) pairs seen across all sources. func (r *Reconciler) collectAllSamples( ctx context.Context, - tupleHash string, + measurementHash string, pid metrics.ProfileID, selectorLabels map[string]string, now time.Time, @@ -234,7 +257,7 @@ func (r *Reconciler) collectAllSamples( continue } - additional, err := r.collectFromSource(ctx, tupleHash, pid, selectorLabels, now, sourceName, ms, p, excluded) + additional, err := r.collectFromSource(ctx, measurementHash, pid, selectorLabels, now, sourceName, ms, p, excluded) if err != nil { // coverage:ignore - Redis error return nil, err } @@ -257,7 +280,7 @@ func (r *Reconciler) collectAllSamples( // Returns the (container, resource) pairs observed from this source's FetchStats results. func (r *Reconciler) collectFromSource( ctx context.Context, - tupleHash string, + measurementHash string, pid metrics.ProfileID, selectorLabels map[string]string, now time.Time, @@ -294,7 +317,7 @@ func (r *Reconciler) collectFromSource( continue } - if err := r.writeSample(ctx, tupleHash, ms, s); err != nil { // coverage:ignore - Redis error + if err := r.writeSample(ctx, measurementHash, ms, s); err != nil { // coverage:ignore - Redis error return nil, err } r.rec.SampleCollected(ctx, sourceName, s.Resource, s.ContainerName, pid) @@ -306,11 +329,11 @@ func (r *Reconciler) collectFromSource( // writeSample persists a single ContainerStats entry to Redis. func (r *Reconciler) writeSample( ctx context.Context, - tupleHash string, + measurementHash string, ms *ballastv1.MetricsSource, s plugin.ContainerStats, ) error { - key := store.MetricKey(tupleHash, s.ContainerName, s.Resource) + key := store.MetricKey(measurementHash, s.ContainerName, s.Resource) valueStr := quantityToStoreValue(s.Resource, s.Value) if err := store.AddSample(ctx, r.storeClient, key, s.Timestamp.UnixMilli(), valueStr, ms.Spec.Config.ReservoirSize); err != nil { // coverage:ignore - Redis error return fmt.Errorf("adding sample for %s: %w", key, err) @@ -436,7 +459,7 @@ func (r *Reconciler) tryLoadSource(ctx context.Context, name string) *ballastv1. // at least one container was processed and all are ready). func (r *Reconciler) buildContainerProfiles( ctx context.Context, - tupleHash string, + measurementHash string, containers map[string][]string, resourcesInPolicy map[string][]ballastv1.MetricConfig, policySpec ballastv1.ClusterResourcePolicySpec, @@ -450,7 +473,7 @@ func (r *Reconciler) buildContainerProfiles( var containerProfiles []ballastv1.ContainerProfile for _, containerName := range sortedKeys(containers) { - cp, ready := r.buildContainerProfile(ctx, tupleHash, containerName, + cp, ready := r.buildContainerProfile(ctx, measurementHash, containerName, containers[containerName], resourcesInPolicy, policySpec, nowMs) if !ready { allReady = false @@ -474,7 +497,7 @@ func (r *Reconciler) buildContainerProfiles( // 2. Return the assembled profile and whether all tracked resources were ready. func (r *Reconciler) buildContainerProfile( ctx context.Context, - tupleHash, containerName string, + measurementHash, containerName string, resources []string, resourcesInPolicy map[string][]ballastv1.MetricConfig, policySpec ballastv1.ClusterResourcePolicySpec, @@ -494,7 +517,7 @@ func (r *Reconciler) buildContainerProfile( } usageStats, recs, ready, err := r.processResourceStats( - ctx, tupleHash, containerName, resourceName, metricsForResource, policySpec, nowMs) + ctx, measurementHash, containerName, resourceName, metricsForResource, policySpec, nowMs) if err != nil { // coverage:ignore - Redis error log.Error(err, "processResourceStats failed", "container", containerName, "resource", resourceName) @@ -534,12 +557,12 @@ func (r *Reconciler) buildContainerProfile( // 8. If ready: computeAllRecommendations for all metric entries for this resource. func (r *Reconciler) processResourceStats( ctx context.Context, - tupleHash, containerName, resourceName string, + measurementHash, containerName, resourceName string, metricsForResource []ballastv1.MetricConfig, policySpec ballastv1.ClusterResourcePolicySpec, nowMs int64, ) (containerStats ballastv1.ContainerUsageStats, resourceRecs map[string]ballastv1.ResourceRecommendation, meetsReadiness bool, err error) { - key := store.MetricKey(tupleHash, containerName, resourceName) + key := store.MetricKey(measurementHash, containerName, resourceName) vals, err := store.QueryAll(ctx, r.storeClient, key) if err != nil { // coverage:ignore - Redis error diff --git a/internal/controller/metricscollector/controller_test.go b/internal/controller/metricscollector/controller_test.go index a67b05f..11e7b02 100644 --- a/internal/controller/metricscollector/controller_test.go +++ b/internal/controller/metricscollector/controller_test.go @@ -153,12 +153,30 @@ func defaultProfile(tupleLabels map[string]string) *ballastv1.WorkloadProfile { return &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: "web"}, Status: ballastv1.WorkloadProfileStatus{ - TupleLabels: tupleLabels, - SelectorLabels: tupleLabels, + TupleLabels: tupleLabels, + SelectorLabels: tupleLabels, + PolicyRef: defaultPolicyRef(), + MeasurementHash: profileHash(tupleLabels), }, } } +// defaultPolicyRef references the policy defaultPolicy builds. The collector reads +// its governing policy from status.policyRef rather than resolving one, so every +// profile fixture must record it the way the workloadwatcher would. +func defaultPolicyRef() *ballastv1.PolicyReference { + return &ballastv1.PolicyReference{ + Kind: ballastv1.KindClusterResourcePolicy, + Name: "platform-defaults", + } +} + +// profileHash is the Redis key namespace a profile owns, mirroring what the +// workloadwatcher records in status.measurementHash. +func profileHash(tupleLabels map[string]string) string { + return store.MeasurementHash(tupleLabels, defaultPolicyRef().Key()) +} + func cpuSample(container string, milliCores int64, ts time.Time) plugin.ContainerStats { return plugin.ContainerStats{ ContainerName: container, @@ -248,8 +266,8 @@ func TestReconcile_KillSwitchActive(t *testing.T) { } // No samples should have been written to Redis. - tupleHash := store.TupleHash(map[string]string{"app": "web"}) - key := store.MetricKey(tupleHash, "app", "cpu") + measurementHash := profileHash(map[string]string{"app": "web"}) + key := store.MetricKey(measurementHash, "app", "cpu") count, _ := store.SampleCount(ctx, sc, key) if count != 0 { t.Errorf("expected 0 Redis samples when kill switch active, got %d", count) @@ -276,8 +294,8 @@ func TestReconcile_DryRun(t *testing.T) { } // No samples written. - tupleHash := store.TupleHash(map[string]string{"app": "web"}) - key := store.MetricKey(tupleHash, "app", "cpu") + measurementHash := profileHash(map[string]string{"app": "web"}) + key := store.MetricKey(measurementHash, "app", "cpu") count, _ := store.SampleCount(ctx, sc, key) if count != 0 { t.Errorf("expected 0 Redis samples in dry-run, got %d", count) @@ -349,8 +367,8 @@ func TestReconcile_CollectAndUpdate(t *testing.T) { } // Samples should be written to Redis. - tupleHash := store.TupleHash(tupleLabels) - key := store.MetricKey(tupleHash, "app", "cpu") + measurementHash := profileHash(tupleLabels) + key := store.MetricKey(measurementHash, "app", "cpu") count, err := store.SampleCount(ctx, sc, key) if err != nil { t.Fatalf("SampleCount: %v", err) @@ -461,9 +479,9 @@ func TestReconcile_ExcludesInitAndEphemeralContainers(t *testing.T) { t.Fatalf("Reconcile: %v", err) } - tupleHash := store.TupleHash(tupleLabels) + measurementHash := profileHash(tupleLabels) for _, name := range []string{"app", "sidecar"} { - key := store.MetricKey(tupleHash, name, "cpu") + key := store.MetricKey(measurementHash, name, "cpu") if count, err := store.SampleCount(ctx, sc, key); err != nil { t.Fatalf("SampleCount(%s): %v", name, err) } else if count != 3 { @@ -472,7 +490,7 @@ func TestReconcile_ExcludesInitAndEphemeralContainers(t *testing.T) { } for _, name := range []string{"init-db", "debugger"} { - key := store.MetricKey(tupleHash, name, "cpu") + key := store.MetricKey(measurementHash, name, "cpu") count, err := store.SampleCount(ctx, sc, key) if err != nil { t.Fatalf("SampleCount(%s): %v", name, err) @@ -503,8 +521,10 @@ func TestReconcile_ExclusionScopedBySelector(t *testing.T) { profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: "web"}, Status: ballastv1.WorkloadProfileStatus{ - TupleLabels: map[string]string{"app": "web"}, - SelectorLabels: map[string]string{"app": "web", "role": plugin.LabelAbsent}, + TupleLabels: map[string]string{"app": "web"}, + SelectorLabels: map[string]string{"app": "web", "role": plugin.LabelAbsent}, + PolicyRef: defaultPolicyRef(), + MeasurementHash: profileHash(map[string]string{"app": "web"}), }, } matchingPod := &corev1.Pod{ @@ -534,13 +554,13 @@ func TestReconcile_ExclusionScopedBySelector(t *testing.T) { t.Fatalf("Reconcile: %v", err) } - tupleHash := store.TupleHash(profile.Status.TupleLabels) - if count, err := store.SampleCount(ctx, sc, store.MetricKey(tupleHash, "web-init", "cpu")); err != nil { + measurementHash := profileHash(profile.Status.TupleLabels) + if count, err := store.SampleCount(ctx, sc, store.MetricKey(measurementHash, "web-init", "cpu")); err != nil { t.Fatalf("SampleCount(web-init): %v", err) } else if count != 0 { t.Errorf("web-init: got %d samples, want 0 (init container of a matching pod is excluded)", count) } - if count, err := store.SampleCount(ctx, sc, store.MetricKey(tupleHash, "batch-init", "cpu")); err != nil { + if count, err := store.SampleCount(ctx, sc, store.MetricKey(measurementHash, "batch-init", "cpu")); err != nil { t.Fatalf("SampleCount(batch-init): %v", err) } else if count != 1 { t.Errorf("batch-init: got %d samples, want 1 (pod not matched by selector, so not an exclusion)", count) @@ -1134,3 +1154,85 @@ func waitForProfileExists(t *testing.T, ctx context.Context, c client.Client, na } t.Errorf("timed out waiting for WorkloadProfile %q", name) } + +// A profile whose pods match no policy has nothing to measure with: the policy is +// what names the metrics sources, so the collector skips rather than guessing. +func TestReconcile_NoPolicyRef_Skipped(t *testing.T) { + profile := defaultProfile(map[string]string{"app": "web"}) + profile.Status.PolicyRef = nil + fc := newFakeClient(profile, defaultMetricsSource(), defaultPolicy()) + _, sc := newMiniredisClient(t) + r := newReconcilerWithPlugin(t, fc, sc, inactiveKS(t), false, nil) + + result, err := reconcileProfile(t, r, "web") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.RequeueAfter == 0 { + t.Error("expected a requeue so the profile is revisited once a policy matches") + } + + var got ballastv1.WorkloadProfile + if err := fc.Get(context.Background(), types.NamespacedName{Name: "web"}, &got); err != nil { + t.Fatalf("Get profile: %v", err) + } + if len(got.Status.Containers) != 0 { + t.Error("no containers should be recorded without a policy") + } +} + +// The referenced policy can be deleted between the workloadwatcher recording it and +// this reconcile. Skipping is correct: the policy watch is already migrating those +// pods to a profile under whatever policy now governs them. +func TestReconcile_PolicyRefDangling_Skipped(t *testing.T) { + profile := defaultProfile(map[string]string{"app": "web"}) + profile.Status.PolicyRef = &ballastv1.PolicyReference{ + Kind: ballastv1.KindClusterResourcePolicy, + Name: "deleted-policy", + } + fc := newFakeClient(profile, defaultMetricsSource()) + _, sc := newMiniredisClient(t) + r := newReconcilerWithPlugin(t, fc, sc, inactiveKS(t), false, nil) + + result, err := reconcileProfile(t, r, "web") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.RequeueAfter == 0 { + t.Error("expected a requeue while the profile awaits migration") + } +} + +// measurementHash names the Redis key namespace the profile owns. Writing samples +// before it is set would file them under a key belonging to no profile, where +// nothing would ever read or purge them. +func TestReconcile_NoMeasurementHash_Requeues(t *testing.T) { + profile := defaultProfile(map[string]string{"app": "web"}) + profile.Status.MeasurementHash = "" + fc := newFakeClient(profile, defaultMetricsSource(), defaultPolicy()) + _, sc := newMiniredisClient(t) + r := newReconcilerWithPlugin(t, fc, sc, inactiveKS(t), false, nil) + + result, err := reconcileProfile(t, r, "web") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.RequeueAfter == 0 { + t.Error("expected a requeue until the workloadwatcher back-fills the hash") + } +} + +// A policyRef the resolver cannot interpret is surfaced as an error rather than +// treated as "no policy": silently skipping would leave the profile accruing +// nothing with no signal as to why. +func TestReconcile_PolicyRefUnknownKind_Errors(t *testing.T) { + profile := defaultProfile(map[string]string{"app": "web"}) + profile.Status.PolicyRef = &ballastv1.PolicyReference{Kind: "Nonsense", Name: "x"} + fc := newFakeClient(profile, defaultMetricsSource()) + _, sc := newMiniredisClient(t) + r := newReconcilerWithPlugin(t, fc, sc, inactiveKS(t), false, nil) + + if _, err := reconcileProfile(t, r, "web"); err == nil { + t.Fatal("expected an error for an uninterpretable policyRef") + } +} diff --git a/internal/controller/policystatus/controller.go b/internal/controller/policystatus/controller.go new file mode 100644 index 0000000..c20f624 --- /dev/null +++ b/internal/controller/policystatus/controller.go @@ -0,0 +1,106 @@ +// Package policystatus publishes each policy's profile discriminator on the +// policy's own status. +// +// A WorkloadProfile's identity includes the policy governing it, so profile names +// carry a token derived from the policy's kind, namespace, and name. That token is +// a hash, and nobody should have to compute one by hand to answer "which profiles +// does this policy govern?". Recording it on the policy closes the loop: +// +// kubectl get workloadprofiles | grep "$(kubectl get crp fleet \ +// -o jsonpath='{.status.profileDiscriminator}')" +// +// It is written to status rather than to a label because it is operator-derived +// data about a user-owned object, and the operator has no business mutating the +// spec or metadata of objects its users wrote. +package policystatus + +import ( + "context" + "fmt" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + ballastv1 "github.com/tight-line/ballast/api/v1" + "github.com/tight-line/ballast/internal/logger" + "github.com/tight-line/ballast/internal/naming" +) + +// Reconciler maintains status.profileDiscriminator on one policy kind. Both kinds +// share this implementation: ResourcePolicyStatus is a type alias for +// ClusterResourcePolicyStatus, so one pointer serves either object. +type Reconciler struct { + client client.Client + kind string +} + +// NewCluster creates a Reconciler for ClusterResourcePolicy objects. +func NewCluster(c client.Client) *Reconciler { + return &Reconciler{client: c, kind: ballastv1.KindClusterResourcePolicy} +} + +// NewNamespaced creates a Reconciler for ResourcePolicy objects. +func NewNamespaced(c client.Client) *Reconciler { + return &Reconciler{client: c, kind: ballastv1.KindResourcePolicy} +} + +// Reconcile writes the policy's discriminator to its status, and is a no-op once +// the stored value agrees. The token is a pure function of the object's identity, +// so it changes only if the object is replaced under a different name. +func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + obj, status, err := r.fetch(ctx, req) + if err != nil { + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } + return ctrl.Result{}, err // coverage:ignore - transient API error + } + + want := naming.PolicyDiscriminator(r.kind, req.Namespace, req.Name) + if status.ProfileDiscriminator == want { + return ctrl.Result{}, nil + } + + base := obj.DeepCopyObject().(client.Object) //nolint:errcheck,forcetypeassert // DeepCopyObject of a client.Object is one + status.ProfileDiscriminator = want + return ctrl.Result{}, r.client.Status().Patch(ctx, obj, client.MergeFrom(base)) +} + +// fetch loads the policy named by req and returns it alongside a pointer to its +// status, so the caller can read and write the status without caring which kind +// it holds. +func (r *Reconciler) fetch(ctx context.Context, req ctrl.Request) (client.Object, *ballastv1.ClusterResourcePolicyStatus, error) { + if r.kind == ballastv1.KindResourcePolicy { + var rp ballastv1.ResourcePolicy + if err := r.client.Get(ctx, req.NamespacedName, &rp); err != nil { + return nil, nil, err + } + return &rp, &rp.Status, nil + } + + var crp ballastv1.ClusterResourcePolicy + if err := r.client.Get(ctx, req.NamespacedName, &crp); err != nil { + return nil, nil, err + } + return &crp, &crp.Status, nil +} + +// SetupWithManager registers the Reconciler for its policy kind. +func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { + name := "policystatus-cluster" + var obj client.Object = &ballastv1.ClusterResourcePolicy{} + if r.kind == ballastv1.KindResourcePolicy { + name = "policystatus-namespaced" + obj = &ballastv1.ResourcePolicy{} + } + + if err := ctrl.NewControllerManagedBy(mgr). + Named(name). + WithLogConstructor(logger.ControllerLogConstructor(mgr.GetLogger(), name)). + For(obj). + Complete(r); err != nil { // coverage:ignore - requires a malformed manager + return fmt.Errorf("registering %s controller: %w", name, err) + } + return nil +} diff --git a/internal/controller/policystatus/controller_test.go b/internal/controller/policystatus/controller_test.go new file mode 100644 index 0000000..6f1cd7c --- /dev/null +++ b/internal/controller/policystatus/controller_test.go @@ -0,0 +1,196 @@ +package policystatus_test + +import ( + "context" + "path/filepath" + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/envtest" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + ballastv1 "github.com/tight-line/ballast/api/v1" + "github.com/tight-line/ballast/internal/controller/policystatus" + "github.com/tight-line/ballast/internal/naming" +) + +func newFakeClient(objs ...client.Object) client.Client { + s := runtime.NewScheme() + _ = clientgoscheme.AddToScheme(s) + _ = ballastv1.AddToScheme(s) + return fake.NewClientBuilder(). + WithScheme(s). + WithStatusSubresource(&ballastv1.ClusterResourcePolicy{}, &ballastv1.ResourcePolicy{}). + WithObjects(objs...). + Build() +} + +func TestReconcile_ClusterPolicy_PublishesDiscriminator(t *testing.T) { + ctx := context.Background() + crp := &ballastv1.ClusterResourcePolicy{ObjectMeta: metav1.ObjectMeta{Name: "fleet"}} + fc := newFakeClient(crp) + + if _, err := policystatus.NewCluster(fc).Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{Name: "fleet"}, + }); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + var got ballastv1.ClusterResourcePolicy + if err := fc.Get(ctx, types.NamespacedName{Name: "fleet"}, &got); err != nil { + t.Fatalf("Get: %v", err) + } + want := naming.PolicyDiscriminator(ballastv1.KindClusterResourcePolicy, "", "fleet") + if got.Status.ProfileDiscriminator != want { + t.Errorf("profileDiscriminator = %q, want %q", got.Status.ProfileDiscriminator, want) + } +} + +// The token must distinguish the kind, so a same-named ResourcePolicy publishes a +// different value than the ClusterResourcePolicy above. +func TestReconcile_ResourcePolicy_PublishesDiscriminator(t *testing.T) { + ctx := context.Background() + rp := &ballastv1.ResourcePolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "team-a", Name: "fleet"}, + } + fc := newFakeClient(rp) + + if _, err := policystatus.NewNamespaced(fc).Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{Namespace: "team-a", Name: "fleet"}, + }); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + var got ballastv1.ResourcePolicy + if err := fc.Get(ctx, types.NamespacedName{Namespace: "team-a", Name: "fleet"}, &got); err != nil { + t.Fatalf("Get: %v", err) + } + want := naming.PolicyDiscriminator(ballastv1.KindResourcePolicy, "team-a", "fleet") + if got.Status.ProfileDiscriminator != want { + t.Errorf("profileDiscriminator = %q, want %q", got.Status.ProfileDiscriminator, want) + } + clusterToken := naming.PolicyDiscriminator(ballastv1.KindClusterResourcePolicy, "", "fleet") + if got.Status.ProfileDiscriminator == clusterToken { + t.Error("a namespaced policy must not share a token with a same-named cluster policy") + } +} + +// The token is a pure function of the object's identity, so a second reconcile has +// nothing to write. Without this guard every policy event would generate a status +// write, and each write another event. +func TestReconcile_AlreadyPublished_NoWrite(t *testing.T) { + ctx := context.Background() + crp := &ballastv1.ClusterResourcePolicy{ObjectMeta: metav1.ObjectMeta{Name: "fleet"}} + fc := newFakeClient(crp) + r := policystatus.NewCluster(fc) + req := reconcile.Request{NamespacedName: types.NamespacedName{Name: "fleet"}} + + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("first Reconcile: %v", err) + } + var first ballastv1.ClusterResourcePolicy + if err := fc.Get(ctx, req.NamespacedName, &first); err != nil { + t.Fatalf("Get: %v", err) + } + + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("second Reconcile: %v", err) + } + var second ballastv1.ClusterResourcePolicy + if err := fc.Get(ctx, req.NamespacedName, &second); err != nil { + t.Fatalf("Get: %v", err) + } + + if first.ResourceVersion != second.ResourceVersion { + t.Errorf("status rewritten on a no-op reconcile: %s -> %s", + first.ResourceVersion, second.ResourceVersion) + } +} + +func TestReconcile_NotFound(t *testing.T) { + fc := newFakeClient() + + for name, r := range map[string]*policystatus.Reconciler{ + "cluster": policystatus.NewCluster(fc), + "namespaced": policystatus.NewNamespaced(fc), + } { + result, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{Namespace: "team-a", Name: "gone"}, + }) + if err != nil { + t.Errorf("%s: Reconcile of a deleted policy should not error: %v", name, err) + } + if result.RequeueAfter != 0 { + t.Errorf("%s: unexpected requeue %v", name, result.RequeueAfter) + } + } +} + +// TestSetupWithManager registers both reconcilers against a real API server and +// asserts the discriminator is published end to end. +func TestSetupWithManager(t *testing.T) { + testEnv := &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "config", "crd", "bases")}, + } + cfg, err := testEnv.Start() + if err != nil { + t.Fatalf("start envtest: %v", err) + } + t.Cleanup(func() { _ = testEnv.Stop() }) + + s := runtime.NewScheme() + _ = clientgoscheme.AddToScheme(s) + _ = ballastv1.AddToScheme(s) + + mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: s, + Metrics: metricsserver.Options{BindAddress: "0"}, + HealthProbeBindAddress: "0", + }) + if err != nil { + t.Fatalf("new manager: %v", err) + } + + if err := policystatus.NewCluster(mgr.GetClient()).SetupWithManager(mgr); err != nil { + t.Fatalf("cluster SetupWithManager: %v", err) + } + if err := policystatus.NewNamespaced(mgr.GetClient()).SetupWithManager(mgr); err != nil { + t.Fatalf("namespaced SetupWithManager: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + go func() { _ = mgr.Start(ctx) }() + + if !mgr.GetCache().WaitForCacheSync(ctx) { + t.Fatal("cache did not sync") + } + c := mgr.GetClient() + + crp := &ballastv1.ClusterResourcePolicy{ObjectMeta: metav1.ObjectMeta{Name: "fleet"}} + if err := c.Create(ctx, crp); err != nil { + t.Fatalf("create ClusterResourcePolicy: %v", err) + } + + want := naming.PolicyDiscriminator(ballastv1.KindClusterResourcePolicy, "", "fleet") + deadline := time.Now().Add(20 * time.Second) + for { + var got ballastv1.ClusterResourcePolicy + if err := c.Get(ctx, types.NamespacedName{Name: "fleet"}, &got); err == nil && + got.Status.ProfileDiscriminator == want { + return + } + if time.Now().After(deadline) { + t.Fatalf("timed out waiting for profileDiscriminator %q", want) + } + time.Sleep(100 * time.Millisecond) + } +} diff --git a/internal/controller/resourceadjuster/controller.go b/internal/controller/resourceadjuster/controller.go index 19a9a74..4d66f2b 100644 --- a/internal/controller/resourceadjuster/controller.go +++ b/internal/controller/resourceadjuster/controller.go @@ -152,12 +152,27 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{RequeueAfter: ballastv1.DefaultResizeIntervalDuration}, nil } - resolved, err := r.resolver.Resolve(ctx, policy.Input{Labels: profile.Status.TupleLabels}) - if err != nil { // coverage:ignore - transient API error + // Read the governing policy from the profile rather than re-resolving it here. + // A profile carries no namespace and only its identity tuple, so resolving from + // it would let any ResourcePolicy in the cluster outrank every + // ClusterResourcePolicy, and would miss policies selecting on kinds or + // annotations that admission matched. The workloadwatcher resolves per pod and + // records the result, so this path resizes toward the same policy the pod was + // admitted under. + if profile.Status.PolicyRef == nil { + log.Info("no policy matches profile, skipping resize", "profile", profile.Name) + r.rec.ResizeSkipped(ctx, "no_policy", pid, "", "") + return ctrl.Result{RequeueAfter: ballastv1.DefaultResizeIntervalDuration}, nil + } + resolved, err := r.resolver.Load(ctx, *profile.Status.PolicyRef) + if err != nil { return ctrl.Result{}, err } if resolved == nil { - log.Info("no policy matches profile, skipping resize", "profile", profile.Name) + // The policy was deleted; the workloadwatcher is migrating these pods to a + // profile under whichever policy now governs them. + log.Info("policy referenced by profile no longer exists, skipping resize", + "profile", profile.Name, "policy", profile.Status.PolicyRef.Key()) r.rec.ResizeSkipped(ctx, "no_policy", pid, "", "") return ctrl.Result{RequeueAfter: ballastv1.DefaultResizeIntervalDuration}, nil } diff --git a/internal/controller/resourceadjuster/controller_test.go b/internal/controller/resourceadjuster/controller_test.go index 19e1901..16fbe32 100644 --- a/internal/controller/resourceadjuster/controller_test.go +++ b/internal/controller/resourceadjuster/controller_test.go @@ -136,12 +136,23 @@ func noResizePolicy() *ballastv1.ClusterResourcePolicy { } } +// testPolicyRef references the policy the fixtures above build. The adjuster reads +// its governing policy from status.policyRef rather than resolving one from the +// profile, so profile fixtures must record it the way the workloadwatcher would. +func testPolicyRef() *ballastv1.PolicyReference { + return &ballastv1.PolicyReference{ + Kind: ballastv1.KindClusterResourcePolicy, + Name: "test-policy", + } +} + // readyProfile returns a WorkloadProfile with MeetsThreshold=true and one container recommendation. func readyProfile(cpuRequest, cpuLimit string) *ballastv1.WorkloadProfile { return &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: "prod"}, Status: ballastv1.WorkloadProfileStatus{ TupleLabels: map[string]string{"app": "app", "env": "prod"}, + PolicyRef: testPolicyRef(), MeetsThreshold: true, Containers: []ballastv1.ContainerProfile{ { @@ -570,6 +581,7 @@ func readyProfileWithRecs(recs map[string]ballastv1.ResourceRecommendation) *bal ObjectMeta: metav1.ObjectMeta{Name: "prod"}, Status: ballastv1.WorkloadProfileStatus{ TupleLabels: map[string]string{"app": "app", "env": "prod"}, + PolicyRef: testPolicyRef(), MeetsThreshold: true, Containers: []ballastv1.ContainerProfile{ {Name: "app", Recommendations: recs}, @@ -1431,3 +1443,54 @@ func TestResolveMaxChangePercent(t *testing.T) { t.Errorf("explicit 25%%: got %v, want 25", got) } } + +// Without a policy there is no resize threshold or cadence to apply, so the +// adjuster skips instead of falling back to a guess. +func TestReconcile_NoPolicyRef_Skipped(t *testing.T) { + profile := readyProfile("200m", "400m") + profile.Status.PolicyRef = nil + fc := newFakeClient(profile, noResizePolicy(), resizePod("100m", "200m")) + r := resourceadjuster.New(fc, inactiveKS(t), false, nil) + + result, err := doReconcile(t, r, "prod") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.RequeueAfter == 0 { + t.Error("expected a requeue so the profile is revisited once a policy matches") + } +} + +// The referenced policy may be deleted before this reconcile runs; the pods are +// already being migrated to a profile under whichever policy now governs them. +func TestReconcile_PolicyRefDangling_Skipped(t *testing.T) { + profile := readyProfile("200m", "400m") + profile.Status.PolicyRef = &ballastv1.PolicyReference{ + Kind: ballastv1.KindClusterResourcePolicy, + Name: "deleted-policy", + } + fc := newFakeClient(profile, resizePod("100m", "200m")) + r := resourceadjuster.New(fc, inactiveKS(t), false, nil) + + result, err := doReconcile(t, r, "prod") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result.RequeueAfter == 0 { + t.Error("expected a requeue while the profile awaits migration") + } +} + +// A policyRef the resolver cannot interpret is surfaced as an error rather than +// treated as "no policy", so the failure is visible instead of looking like a +// profile that simply has no policy. +func TestReconcile_PolicyRefUnknownKind_Errors(t *testing.T) { + profile := readyProfile("200m", "400m") + profile.Status.PolicyRef = &ballastv1.PolicyReference{Kind: "Nonsense", Name: "x"} + fc := newFakeClient(profile, resizePod("100m", "200m")) + r := resourceadjuster.New(fc, inactiveKS(t), false, nil) + + if _, err := doReconcile(t, r, "prod"); err == nil { + t.Fatal("expected an error for an uninterpretable policyRef") + } +} diff --git a/internal/controller/workloadwatcher/controller.go b/internal/controller/workloadwatcher/controller.go index e8ba00d..3ed7ad3 100644 --- a/internal/controller/workloadwatcher/controller.go +++ b/internal/controller/workloadwatcher/controller.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "maps" + "reflect" "slices" "strings" "time" @@ -27,7 +28,9 @@ import ( "github.com/tight-line/ballast/internal/killswitch" "github.com/tight-line/ballast/internal/logger" "github.com/tight-line/ballast/internal/metrics" + "github.com/tight-line/ballast/internal/naming" "github.com/tight-line/ballast/internal/plugin" + "github.com/tight-line/ballast/internal/policy" "github.com/tight-line/ballast/internal/store" "github.com/tight-line/ballast/internal/validation" ) @@ -81,7 +84,12 @@ type Controller struct { // New creates a Controller. func New(c client.Client, ks *killswitch.KillSwitch, storeClient store.Client, rec *metrics.Recorder) *Controller { return &Controller{ - Pod: &PodReconciler{client: c, ks: ks, rec: rec}, + Pod: &PodReconciler{ + client: c, + ks: ks, + rec: rec, + resolver: policy.NewResolver(c, ctrl.Log.WithName("workloadwatcher-pod")), + }, Profile: &ProfileReconciler{client: c, storeClient: storeClient, rec: rec}, } } @@ -111,10 +119,18 @@ func (c *Controller) SetupWithManager(mgr ctrl.Manager) error { // PodReconciler watches pods carrying Ballast behavior annotations and maintains // WorkloadProfile objects and their activeWorkloads counters. +// +// It is also the only component that resolves policy for a pod outside admission, +// and the only one positioned to: a pod reconcile has the namespace, the full +// label set, the annotations, and the owner kind that policy selectors are +// written against, none of which survive into the cluster-scoped WorkloadProfile +// the other controllers work from. The resolved policy is recorded on the +// profile's status.policyRef for those controllers to read. type PodReconciler struct { - client client.Client - ks *killswitch.KillSwitch - rec *metrics.Recorder + client client.Client + ks *killswitch.KillSwitch + rec *metrics.Recorder + resolver *policy.Resolver } // Reconcile handles pod CREATE/UPDATE (stamp and increment) and DELETE (decrement). @@ -164,16 +180,19 @@ func (r *PodReconciler) handleCreateUpdate(ctx context.Context, pod *corev1.Pod) return ctrl.Result{}, err // coverage:ignore - transient API error } - // The desired profile name is recomputed from the pod's current identity every - // reconcile, so a change to the pod's labels or to identityLabels migrates the - // pod to the correct profile instead of trusting a possibly-stale stamp. - tupleLabels := ExtractTupleLabels(pod.Labels, cfg.Spec.IdentityLabels) - selectorLabels := ExtractSelectorLabels(pod.Labels, cfg.Spec.IdentityLabels) - profName := ProfileName(tupleLabels, cfg.Spec.IdentityLabels) + // The desired identity is recomputed from the pod's current state every + // reconcile, so a change to the pod's labels, to identityLabels, or to the + // policy set migrates the pod to the correct profile instead of trusting a + // possibly-stale stamp. + id, err := r.identityFor(ctx, pod, cfg.Spec.IdentityLabels) + if err != nil { // coverage:ignore - transient API error listing policy objects + return ctrl.Result{}, err + } + profName := id.name // Ensure the target profile exists; recreates it if it was deleted while pods // still reference it. - if err := r.ensureProfile(ctx, profName, tupleLabels, selectorLabels); err != nil { + if err := r.ensureProfile(ctx, id); err != nil { if errors.Is(err, errProfileTerminating) { // The profile is being purged; wait for it to finish, then a later // reconcile recreates it fresh and rebinds this pod. @@ -182,7 +201,7 @@ func (r *PodReconciler) handleCreateUpdate(ctx context.Context, pod *corev1.Pod) return ctrl.Result{}, err // coverage:ignore - transient API error } - pid := metrics.ProfileID{Name: profName, Labels: tupleLabels} + pid := metrics.ProfileID{Name: profName, Labels: id.tupleLabels} firstEnroll := currentRef == "" migrating := currentRef != "" && currentRef != profName @@ -196,10 +215,8 @@ func (r *PodReconciler) handleCreateUpdate(ctx context.Context, pod *corev1.Pod) } } - if currentRef != profName { - if err := r.stampProfileRef(ctx, pod, profName); err != nil { // coverage:ignore - transient API error - return ctrl.Result{}, err - } + if err := r.stampRefs(ctx, pod, id); err != nil { // coverage:ignore - transient API error + return ctrl.Result{}, err } if firstEnroll { @@ -271,9 +288,57 @@ func (r *PodReconciler) handleDelete(ctx context.Context, pod *corev1.Pod) (ctrl return ctrl.Result{}, r.client.Patch(ctx, pod, client.MergeFrom(base)) } -func (r *PodReconciler) ensureProfile(ctx context.Context, profName string, tupleLabels, selectorLabels map[string]string) error { +// profileIdentity is everything that distinguishes one WorkloadProfile: the pods +// it pools, the policy governing them, and the Redis namespace holding their +// samples. +type profileIdentity struct { + name string + tupleLabels map[string]string + selectorLabels map[string]string + policyRef *ballastv1.PolicyReference + measurementHash string +} + +// identityFor derives the WorkloadProfile identity a pod belongs to: its label +// tuple plus the policy governing it. +// +// The policy belongs in identity because a profile holds exactly one set of +// recommendations per container, and the policy is what decides the metrics +// sources, poll cadence, tracked resources, aggregation, and headroom that +// produce them. Pods resolving to different policies cannot share one answer, so +// they cannot share one profile. +func (r *PodReconciler) identityFor(ctx context.Context, pod *corev1.Pod, identityLabels []string) (id profileIdentity, err error) { + resolved, err := r.resolver.Resolve(ctx, policy.InputForPod(pod)) + if err != nil { // coverage:ignore - transient API error listing policy objects + return profileIdentity{}, err + } + + id = profileIdentity{ + tupleLabels: ExtractTupleLabels(pod.Labels, identityLabels), + selectorLabels: ExtractSelectorLabels(pod.Labels, identityLabels), + } + + // A pod matching no policy still gets a profile, so it stays counted and + // visible; the metrics collector has no policy to measure with and skips it. + // That profile's identity carries the NoPolicy token, so as soon as a policy + // does match, the pod migrates to a new profile and the empty one orphans. + discriminator := naming.NoPolicy + var policyKey string + if resolved != nil { + ref := resolved.Ref + id.policyRef = &ref + discriminator = naming.PolicyDiscriminator(ref.Kind, ref.Namespace, ref.Name) + policyKey = ref.Key() + } + + id.name = naming.ProfileName(id.tupleLabels, identityLabels, discriminator) + id.measurementHash = store.MeasurementHash(id.tupleLabels, policyKey) + return id, nil +} + +func (r *PodReconciler) ensureProfile(ctx context.Context, id profileIdentity) error { var existing ballastv1.WorkloadProfile - err := r.client.Get(ctx, types.NamespacedName{Name: profName}, &existing) + err := r.client.Get(ctx, types.NamespacedName{Name: id.name}, &existing) if err == nil { // A profile mid-deletion is having its Redis history purged by the // finalizer. Binding a live pod to it now would race the purge and lose @@ -281,14 +346,14 @@ func (r *PodReconciler) ensureProfile(ctx context.Context, profName string, tupl if !existing.DeletionTimestamp.IsZero() { return errProfileTerminating } - return r.ensureProfileStatus(ctx, &existing, tupleLabels, selectorLabels) + return r.ensureProfileStatus(ctx, &existing, id) } if !apierrors.IsNotFound(err) { // coverage:ignore - transient API error return err } profile := &ballastv1.WorkloadProfile{ - ObjectMeta: metav1.ObjectMeta{Name: profName}, + ObjectMeta: metav1.ObjectMeta{Name: id.name}, } if err := r.client.Create(ctx, profile); err != nil { if apierrors.IsAlreadyExists(err) { @@ -301,30 +366,44 @@ func (r *PodReconciler) ensureProfile(ctx context.Context, profName string, tupl } return err // coverage:ignore - transient non-AlreadyExists error } - r.rec.WorkloadProfileCreated(ctx, metrics.ProfileID{Name: profName, Labels: tupleLabels}) + r.rec.WorkloadProfileCreated(ctx, metrics.ProfileID{Name: id.name, Labels: id.tupleLabels}) // Status is a subresource; it can only be written after creation. - return r.ensureProfileStatus(ctx, profile, tupleLabels, selectorLabels) + return r.ensureProfileStatus(ctx, profile, id) } -// ensureProfileStatus level-triggers the profile's identity labels: whenever the -// stored status does not match the desired tuple/selector labels, patch it. -// Converging on every reconcile (not only at creation) heals a profile whose -// initial status write was lost — a conflict with the profile reconciler's -// concurrent finalizer back-fill, a crash between create and status write, or a -// profile inherited from an older operator version. A Patch (not Update) is used +// ensureProfileStatus level-triggers the profile's identity: whenever the stored +// status does not match the desired tuple labels, selector labels, policy +// reference, or measurement hash, patch it. Converging on every reconcile (not +// only at creation) heals a profile whose initial status write was lost — a +// conflict with the profile reconciler's concurrent finalizer back-fill, a crash +// between create and status write, or a profile inherited from an older operator +// version that recorded no policy reference at all. A Patch (not Update) is used // so the write cannot 409 against that finalizer back-fill. -func (r *PodReconciler) ensureProfileStatus(ctx context.Context, profile *ballastv1.WorkloadProfile, tupleLabels, selectorLabels map[string]string) error { - if maps.Equal(profile.Status.TupleLabels, tupleLabels) && - maps.Equal(profile.Status.SelectorLabels, selectorLabels) { +func (r *PodReconciler) ensureProfileStatus(ctx context.Context, profile *ballastv1.WorkloadProfile, id profileIdentity) error { + if maps.Equal(profile.Status.TupleLabels, id.tupleLabels) && + maps.Equal(profile.Status.SelectorLabels, id.selectorLabels) && + samePolicyRef(profile.Status.PolicyRef, id.policyRef) && + profile.Status.MeasurementHash == id.measurementHash { return nil } base := profile.DeepCopy() - profile.Status.TupleLabels = tupleLabels - profile.Status.SelectorLabels = selectorLabels + profile.Status.TupleLabels = id.tupleLabels + profile.Status.SelectorLabels = id.selectorLabels + profile.Status.PolicyRef = id.policyRef + profile.Status.MeasurementHash = id.measurementHash return r.client.Status().Patch(ctx, profile, client.MergeFrom(base)) } +// samePolicyRef compares two policy references, treating "no policy matched" +// (nil) as distinct from every reference. +func samePolicyRef(a, b *ballastv1.PolicyReference) bool { + if a == nil || b == nil { + return a == b + } + return *a == *b +} + // podEnrollment overrides the reconciled pod's enrollment when recomputing a // profile's active-workload count, so the count reflects the state just written // even if the informer cache has not yet caught up (read-after-write lag). A ref @@ -417,12 +496,33 @@ func (r *PodReconciler) setActiveWorkloads(ctx context.Context, profName string, return r.client.Status().Patch(ctx, &profile, client.MergeFrom(base)) } -func (r *PodReconciler) stampProfileRef(ctx context.Context, pod *corev1.Pod, profName string) error { +// stampRefs writes the profile-ref and policy-ref annotations in a single patch, +// and is a no-op when both already hold their desired values. +// +// policy-ref is refreshed here rather than left as the webhook wrote it. The +// webhook stamps the policy it resolved at admission; after a policy is created, +// edited, or deleted, that annotation would otherwise keep advertising a policy +// that no longer governs the pod, which is a trap for anyone debugging from it. +func (r *PodReconciler) stampRefs(ctx context.Context, pod *corev1.Pod, id profileIdentity) error { + var policyRef string + if id.policyRef != nil { + policyRef = policy.PodAnnotationValue(*id.policyRef) + } + if pod.Annotations[AnnotationProfileRef] == id.name && + pod.Annotations[validation.AnnotationPolicyRef] == policyRef { + return nil + } + base := pod.DeepCopy() if pod.Annotations == nil { pod.Annotations = make(map[string]string) } - pod.Annotations[AnnotationProfileRef] = profName + pod.Annotations[AnnotationProfileRef] = id.name + if policyRef == "" { + delete(pod.Annotations, validation.AnnotationPolicyRef) + } else { + pod.Annotations[validation.AnnotationPolicyRef] = policyRef + } return r.client.Patch(ctx, pod, client.MergeFrom(base)) } @@ -465,9 +565,22 @@ func (r *PodReconciler) podsForProfile(ctx context.Context, obj client.Object) [ return reqs } -// podsForConfig maps a BallastConfig change to reconcile requests for every managed -// pod, so an identityLabels change promptly migrates each pod to its new profile. -func (r *PodReconciler) podsForConfig(ctx context.Context, _ client.Object) []ctrl.Request { +// allManagedPods maps a cluster-wide configuration change to reconcile requests +// for every managed pod, so the change promptly migrates each pod to its correct +// profile. It backs both the BallastConfig watch (identityLabels changes rename +// every profile) and the policy watches (a policy change can move any pod to a +// different policy, and therefore a different profile). +// +// The fan-out is deliberately indiscriminate. The set of pods a policy event +// affects is not "the pods this policy matches": deleting a policy affects the +// pods that matched the spec that no longer exists, narrowing a selector affects +// the pods that stopped matching, and because precedence is cluster-wide, a new +// high-priority policy can flip pods that never matched anything before. +// Computing that set exactly would need both the old and new spec plus a +// re-evaluation against every other policy. Enqueueing everything instead costs +// one cache read and one resolve per pod, writes nothing unless a pod's identity +// actually changed, and only happens on human-initiated policy edits. +func (r *PodReconciler) allManagedPods(ctx context.Context, _ client.Object) []ctrl.Request { var podList corev1.PodList if err := r.client.List(ctx, &podList); err != nil { // coverage:ignore - transient API error return nil @@ -518,12 +631,56 @@ func identityLabelsChanged() predicate.Predicate { } } +// policyResolutionChanged admits only the policy events that can change which +// policy governs a pod: creation, deletion, and updates that touch the selector +// or the priority. +// +// Every other spec field (metrics sources, aggregation, headroom, thresholds, +// cadence) is read live from the policy object by the metrics collector and the +// resource adjuster on their next cycle, so those edits take effect without any +// profile churn. Treating them as identity changes would re-key measurement +// history and force a fresh accrual for nothing — the sample already recorded +// does not change meaning because the headroom applied to it did. +func policyResolutionChanged() predicate.Predicate { + return predicate.Funcs{ + CreateFunc: func(event.CreateEvent) bool { return true }, + DeleteFunc: func(event.DeleteEvent) bool { return true }, + GenericFunc: func(event.GenericEvent) bool { return false }, + UpdateFunc: func(e event.UpdateEvent) bool { + oldSpec, ok1 := policySpecOf(e.ObjectOld) + newSpec, ok2 := policySpecOf(e.ObjectNew) + if !ok1 || !ok2 { + // Not a policy object we recognize; admit it rather than silently + // dropping an event that might matter. + return true + } + return oldSpec.Priority != newSpec.Priority || + !reflect.DeepEqual(oldSpec.Selector, newSpec.Selector) + }, + } +} + +// policySpecOf extracts the shared policy spec from either policy kind. +// ResourcePolicySpec is a type alias for ClusterResourcePolicySpec, so one +// pointer type serves both. +func policySpecOf(obj client.Object) (*ballastv1.ClusterResourcePolicySpec, bool) { + switch p := obj.(type) { + case *ballastv1.ClusterResourcePolicy: + return &p.Spec, true + case *ballastv1.ResourcePolicy: + return &p.Spec, true + default: + return nil, false + } +} + // SetupWithManager registers the PodReconciler with the manager. Beyond watching // pods, it watches WorkloadProfile deletions (to promptly recreate profiles still -// referenced by live pods) and BallastConfig identityLabels changes (to promptly -// migrate pods to their new profiles). It also registers the profile-ref pod -// index on the manager's shared cache, which serves both this reconciler's and -// the ProfileReconciler's count lookups. +// referenced by live pods), BallastConfig identityLabels changes (to promptly +// migrate pods to their new profiles), and both policy kinds (so a policy applied +// to a running cluster takes effect without waiting for pod churn). It also +// registers the profile-ref pod index on the manager's shared cache, which serves +// both this reconciler's and the ProfileReconciler's count lookups. func (r *PodReconciler) SetupWithManager(mgr ctrl.Manager) error { if err := mgr.GetFieldIndexer().IndexField( context.Background(), &corev1.Pod{}, PodProfileRefField, PodProfileRefIndexer, @@ -538,8 +695,14 @@ func (r *PodReconciler) SetupWithManager(mgr ctrl.Manager) error { handler.EnqueueRequestsFromMapFunc(r.podsForProfile), builder.WithPredicates(profileDeleted())). Watches(&ballastv1.BallastConfig{}, - handler.EnqueueRequestsFromMapFunc(r.podsForConfig), + handler.EnqueueRequestsFromMapFunc(r.allManagedPods), builder.WithPredicates(identityLabelsChanged())). + Watches(&ballastv1.ClusterResourcePolicy{}, + handler.EnqueueRequestsFromMapFunc(r.allManagedPods), + builder.WithPredicates(policyResolutionChanged())). + Watches(&ballastv1.ResourcePolicy{}, + handler.EnqueueRequestsFromMapFunc(r.allManagedPods), + builder.WithPredicates(policyResolutionChanged())). Complete(r) } @@ -657,8 +820,19 @@ func (r *ProfileReconciler) finalize(ctx context.Context, profile *ballastv1.Wor return ctrl.Result{}, nil } - tupleHash := store.TupleHash(profile.Status.TupleLabels) - keys, err := store.AllKeysForHash(ctx, r.storeClient, tupleHash) + // The profile's own measurement hash, not a hash of its tuple: profiles that + // share a tuple but resolve to different policies each own a separate key + // namespace, and purging by tuple would delete a live sibling's history. + hash := profile.Status.MeasurementHash + if hash == "" { + // A profile from a release that predates measurement hashes (or one whose + // status write was lost) holds its samples under the bare tuple hash. + // Falling back keeps those keys from being stranded in Redis when the + // profile ages out, which is how every profile inherited across the + // upgrade is cleaned up. + hash = store.TupleHash(profile.Status.TupleLabels) + } + keys, err := store.AllKeysForHash(ctx, r.storeClient, hash) if err != nil { // coverage:ignore - requires a broken Redis instance return ctrl.Result{}, err } @@ -738,34 +912,3 @@ func missingLabelPlaceholder(key string) string { }, seg) return "no" + clean } - -// ProfileName derives a deterministic Kubernetes-safe name from a label tuple. -// Values are joined with "--" in identityLabels order. Each value is sanitized -// to lowercase alphanumeric-and-dash. -func ProfileName(tupleLabels map[string]string, identityLabels []string) string { - var parts []string - for _, k := range identityLabels { - if v, ok := tupleLabels[k]; ok { - parts = append(parts, sanitizeName(v)) - } - } - name := strings.Join(parts, "--") - if len(name) > 253 { // coverage:ignore - triggered only with extremely long label values - name = name[:253] - } - return name -} - -// sanitizeName converts a string to a lowercase DNS-label-safe segment. -func sanitizeName(s string) string { - s = strings.ToLower(s) - var b strings.Builder - for _, r := range s { - if unicode.IsLetter(r) || unicode.IsDigit(r) || r == '-' { - b.WriteRune(r) - } else { - b.WriteRune('-') - } - } - return strings.Trim(b.String(), "-") -} diff --git a/internal/controller/workloadwatcher/controller_test.go b/internal/controller/workloadwatcher/controller_test.go index 62c1eda..2ce4c42 100644 --- a/internal/controller/workloadwatcher/controller_test.go +++ b/internal/controller/workloadwatcher/controller_test.go @@ -28,11 +28,20 @@ import ( ballastv1 "github.com/tight-line/ballast/api/v1" "github.com/tight-line/ballast/internal/controller/workloadwatcher" "github.com/tight-line/ballast/internal/killswitch" + "github.com/tight-line/ballast/internal/naming" "github.com/tight-line/ballast/internal/plugin" "github.com/tight-line/ballast/internal/store" "github.com/tight-line/ballast/internal/validation" ) +// noPolicyProfile is the profile name the reconciler derives for a label tuple +// when no policy matches it. A profile's identity includes its governing policy, +// and the fixtures in this file install no policies unless a test says otherwise, +// so their profile names carry the NoPolicy token. +func noPolicyProfile(tuple string) string { + return tuple + "--" + naming.NoPolicy +} + // -- scheme & client helpers -- func newScheme() *runtime.Scheme { @@ -133,7 +142,7 @@ func TestPodReconciler_NewPod(t *testing.T) { // WorkloadProfile should be created. var profile ballastv1.WorkloadProfile - profName := "web" + profName := noPolicyProfile("web") if err := fc.Get(ctx, types.NamespacedName{Name: profName}, &profile); err != nil { t.Fatalf("Get WorkloadProfile %q: %v", profName, err) } @@ -178,7 +187,7 @@ func TestPodReconciler_NotFound(t *testing.T) { func TestPodReconciler_AlreadyProcessed(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: profName}, } @@ -237,8 +246,8 @@ func TestPodReconciler_AbsentIdentityLabelUsesPlaceholder(t *testing.T) { if len(list.Items) != 1 { t.Fatalf("expected 1 WorkloadProfile, got %d", len(list.Items)) } - if got := list.Items[0].Name; got != "noapp" { - t.Errorf("profile name = %q, want %q", got, "noapp") + if want := noPolicyProfile("noapp"); list.Items[0].Name != want { + t.Errorf("profile name = %q, want %q", list.Items[0].Name, want) } } @@ -280,7 +289,7 @@ func TestPodReconciler_KillSwitchSuppresses(t *testing.T) { func TestPodReconciler_DeleteDecrement(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: profName}, } @@ -359,7 +368,7 @@ func TestPodReconciler_DeleteDecrement(t *testing.T) { func TestPodReconciler_RolloutRestart(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ObjectMeta: metav1.ObjectMeta{Name: profName}} // Initial state: 2 old pods fully processed. @@ -443,7 +452,7 @@ func TestPodReconciler_RolloutRestart(t *testing.T) { func TestPodReconciler_DeleteOrphanTransition(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: profName}, } @@ -491,7 +500,7 @@ func TestPodReconciler_DeleteOrphanTransition(t *testing.T) { func TestPodReconciler_DeleteKillSwitchAllowsDecrement(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: profName}, } @@ -533,7 +542,7 @@ func TestPodReconciler_DeleteKillSwitchAllowsDecrement(t *testing.T) { func TestPodReconciler_NewPodClearsOrphan(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: profName}, } @@ -602,7 +611,7 @@ func TestPodReconciler_BallastConfigNotFound(t *testing.T) { func TestPodReconciler_RecoveryAddFinalizer(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ObjectMeta: metav1.ObjectMeta{Name: profName}} // Pod already has profile-ref (was previously processed) but our finalizer is missing. pod := &corev1.Pod{ @@ -652,7 +661,7 @@ func TestPodReconciler_RecoveryAddFinalizer(t *testing.T) { func TestPodReconciler_DeleteNoFinalizer(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ObjectMeta: metav1.ObjectMeta{Name: profName}} // Pod is being deleted (held by a foreign finalizer) but lacks our finalizer. // Our finalizer is the "we've counted this pod" marker — without it, we skip @@ -717,7 +726,7 @@ func TestPodReconciler_SpecialLabelChars(t *testing.T) { reconcilePod(t, c, "default", "web-abc") // "my_app.v2" sanitizes to "my-app-v2" → profile name "my-app-v2". - expectedName := "my-app-v2" + expectedName := noPolicyProfile("my-app-v2") var profile ballastv1.WorkloadProfile if err := fc.Get(ctx, types.NamespacedName{Name: expectedName}, &profile); err != nil { t.Fatalf("Get WorkloadProfile %q: %v (sanitization of special chars may be wrong)", expectedName, err) @@ -753,7 +762,7 @@ func TestPodReconciler_BarePodIgnored(t *testing.T) { // decremented, profile orphaned once its last workload leaves. func TestPodReconciler_UnenrollOnAnnotationRemoval(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ObjectMeta: metav1.ObjectMeta{Name: profName}} pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -832,7 +841,7 @@ func TestPodReconciler_UnenrollNoProfileRef(t *testing.T) { // (created, count 1) and the old profile recounted to 0 and orphaned. func TestPodReconciler_MigrateOnLabelChange(t *testing.T) { ctx := context.Background() - oldName, newName := "stale", "web" + oldName, newName := "stale", noPolicyProfile("web") oldProfile := &ballastv1.WorkloadProfile{ObjectMeta: metav1.ObjectMeta{Name: oldName}} pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -910,11 +919,12 @@ func TestPodReconciler_MigrateOnIdentityLabelsChange(t *testing.T) { if err := fc.Get(ctx, types.NamespacedName{Namespace: "default", Name: "web-abc"}, &gotPod); err != nil { t.Fatalf("Get pod: %v", err) } - if ref := gotPod.Annotations[workloadwatcher.AnnotationProfileRef]; ref != "web--api" { - t.Errorf("profile-ref after identityLabels change: got %q, want %q", ref, "web--api") + if want := noPolicyProfile("web--api"); gotPod.Annotations[workloadwatcher.AnnotationProfileRef] != want { + t.Errorf("profile-ref after identityLabels change: got %q, want %q", + gotPod.Annotations[workloadwatcher.AnnotationProfileRef], want) } var newProf ballastv1.WorkloadProfile - if err := fc.Get(ctx, types.NamespacedName{Name: "web--api"}, &newProf); err != nil { + if err := fc.Get(ctx, types.NamespacedName{Name: noPolicyProfile("web--api")}, &newProf); err != nil { t.Fatalf("Get migrated profile: %v", err) } if newProf.Status.ActiveWorkloads != 1 { @@ -929,7 +939,7 @@ func TestPodReconciler_MigrateOnIdentityLabelsChange(t *testing.T) { // repaired on the next reconcile of any member pod. func TestPodReconciler_HealsMissingStatusLabels(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") // Profile exists but its status labels were never written. profile := &ballastv1.WorkloadProfile{ObjectMeta: metav1.ObjectMeta{Name: profName}} pod := &corev1.Pod{ @@ -1119,7 +1129,7 @@ func TestExtractSelectorLabels(t *testing.T) { func TestProfileReconciler_NotOrphaned(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: profName}, } @@ -1205,7 +1215,7 @@ func TestProfileReconciler_RecountHealsStaleCount(t *testing.T) { func TestProfileReconciler_OrphanTTLNotExpired(t *testing.T) { orphanedAt := metav1.Now() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: profName}, } @@ -1249,7 +1259,7 @@ func TestProfileReconciler_OrphanTTLNotExpired(t *testing.T) { } func TestProfileReconciler_InvalidOrphanTTL(t *testing.T) { - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ObjectMeta: metav1.ObjectMeta{Name: profName}} cfg := &ballastv1.BallastConfig{ ObjectMeta: metav1.ObjectMeta{Name: "ballast"}, @@ -1283,7 +1293,7 @@ func TestProfileReconciler_InvalidOrphanTTL(t *testing.T) { func TestProfileReconciler_OrphanTTLExpired(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") tupleLabels := map[string]string{"app": "web"} profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{Name: profName}, @@ -1414,7 +1424,7 @@ func TestProfileName_LongLabel(t *testing.T) { } func TestProfileReconciler_RedisFailure(t *testing.T) { - profName := "web" + profName := noPolicyProfile("web") tupleLabels := map[string]string{"app": "web"} profile := &ballastv1.WorkloadProfile{ObjectMeta: metav1.ObjectMeta{Name: profName}} cfg := &ballastv1.BallastConfig{ @@ -1453,7 +1463,7 @@ func TestProfileReconciler_RedisFailure(t *testing.T) { func TestProfileReconciler_AddsFinalizer(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ObjectMeta: metav1.ObjectMeta{Name: profName}} fc := newFakeClient(defaultBallastConfig(), profile) @@ -1484,7 +1494,7 @@ func TestProfileReconciler_AddsFinalizer(t *testing.T) { // driven — the whole point of moving cleanup into the finalizer. func TestProfileReconciler_ManualDeletePurgesRedis(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") tupleLabels := map[string]string{"app": "web"} profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{ @@ -1530,7 +1540,7 @@ func TestProfileReconciler_ManualDeletePurgesRedis(t *testing.T) { // foreign one) must be left untouched. func TestProfileReconciler_FinalizeWithoutFinalizer(t *testing.T) { ctx := context.Background() - profName := "web" + profName := noPolicyProfile("web") profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{ Name: profName, @@ -1563,7 +1573,7 @@ func TestPodReconciler_ProfileTerminatingRequeues(t *testing.T) { ctx := context.Background() profile := &ballastv1.WorkloadProfile{ ObjectMeta: metav1.ObjectMeta{ - Name: "web", + Name: noPolicyProfile("web"), Finalizers: []string{workloadwatcher.ProfileFinalizerName}, }, } @@ -1674,17 +1684,17 @@ func TestController_SetupWithManager(t *testing.T) { } // Wait for WorkloadProfile to appear. - waitForProfile(t, ctx, c, "web") + waitForProfile(t, ctx, c, noPolicyProfile("web")) // Wait for activeWorkloads=1: the pod reconciler's recount and the profile // reconciler's backstop recount converge on it, but may interleave briefly. - waitForActiveWorkloads(t, ctx, c, "web", 1) + waitForActiveWorkloads(t, ctx, c, noPolicyProfile("web"), 1) // Delete the pod and wait for the Orphaned condition to be set. if err := c.Delete(ctx, pod); err != nil { t.Fatalf("delete pod: %v", err) } - waitForOrphaned(t, ctx, c, "web") + waitForOrphaned(t, ctx, c, noPolicyProfile("web")) } func waitForProfile(t *testing.T, ctx context.Context, c client.Client, name string) { diff --git a/internal/controller/workloadwatcher/policy_identity_test.go b/internal/controller/workloadwatcher/policy_identity_test.go new file mode 100644 index 0000000..d03eb5b --- /dev/null +++ b/internal/controller/workloadwatcher/policy_identity_test.go @@ -0,0 +1,323 @@ +package workloadwatcher_test + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + ballastv1 "github.com/tight-line/ballast/api/v1" + "github.com/tight-line/ballast/internal/controller/workloadwatcher" + "github.com/tight-line/ballast/internal/naming" + "github.com/tight-line/ballast/internal/store" + "github.com/tight-line/ballast/internal/validation" +) + +// -- fixtures -- + +func enrolledPod(namespace, name, app string) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + Labels: map[string]string{"app": app, validation.LabelMode: validation.ModeMeasure}, + }, + } +} + +func fleetPolicy(name string, priority int32) *ballastv1.ClusterResourcePolicy { + return &ballastv1.ClusterResourcePolicy{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: ballastv1.ClusterResourcePolicySpec{Priority: priority}, + } +} + +func teamPolicy(namespace, name string) *ballastv1.ResourcePolicy { + return &ballastv1.ResourcePolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}, + Spec: ballastv1.ResourcePolicySpec{}, + } +} + +func profileNameFor(tuple string, ref ballastv1.PolicyReference) string { + return tuple + "--" + naming.PolicyDiscriminator(ref.Kind, ref.Namespace, ref.Name) +} + +func clusterRef(name string) ballastv1.PolicyReference { + return ballastv1.PolicyReference{Kind: ballastv1.KindClusterResourcePolicy, Name: name} +} + +func namespacedRef(namespace, name string) ballastv1.PolicyReference { + return ballastv1.PolicyReference{ + Kind: ballastv1.KindResourcePolicy, + Namespace: namespace, + Name: name, + } +} + +func getProfile(t *testing.T, c client.Client, name string) *ballastv1.WorkloadProfile { + t.Helper() + var p ballastv1.WorkloadProfile + if err := c.Get(context.Background(), types.NamespacedName{Name: name}, &p); err != nil { + t.Fatalf("Get WorkloadProfile %q: %v", name, err) + } + return &p +} + +// -- identity -- + +// The workloadwatcher is the only component with a pod in hand, so it is where +// policy is resolved. The resolved policy becomes part of the profile's identity. +func TestPodReconciler_ProfileNameIncludesPolicy(t *testing.T) { + ref := clusterRef("fleet") + fc := newFakeClient(defaultBallastConfig(), fleetPolicy("fleet", 0), enrolledPod("default", "web-abc", "web")) + _, rc := newMiniredisClient(t) + c := workloadwatcher.New(fc, inactiveKS(t), rc, nil) + + reconcilePod(t, c, "default", "web-abc") + + want := profileNameFor("web", ref) + profile := getProfile(t, fc, want) + + if profile.Status.PolicyRef == nil { + t.Fatal("status.policyRef must record the governing policy") + } + if *profile.Status.PolicyRef != ref { + t.Errorf("policyRef = %+v, want %+v", *profile.Status.PolicyRef, ref) + } + if got, want := profile.Status.MeasurementHash, store.MeasurementHash(map[string]string{"app": "web"}, ref.Key()); got != want { + t.Errorf("measurementHash = %q, want %q", got, want) + } + + var pod corev1.Pod + if err := fc.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "web-abc"}, &pod); err != nil { + t.Fatalf("Get pod: %v", err) + } + if got := pod.Annotations[workloadwatcher.AnnotationProfileRef]; got != want { + t.Errorf("profile-ref = %q, want %q", got, want) + } + if got := pod.Annotations[validation.AnnotationPolicyRef]; got != "fleet" { + t.Errorf("policy-ref = %q, want %q", got, "fleet") + } +} + +// Two pods sharing a label tuple but governed by different policies cannot share +// one profile: a profile holds one set of recommendations, and the policy decides +// how they are produced. Here the distinguishing dimension is the namespace, which +// is expressible in a pod query, so the split is real. +func TestPodReconciler_SameTupleDifferentPolicies_SplitsProfiles(t *testing.T) { + fc := newFakeClient( + defaultBallastConfig(), + fleetPolicy("fleet", 0), + teamPolicy("team-a", "local"), + enrolledPod("team-a", "web-a", "web"), + enrolledPod("team-b", "web-b", "web"), + ) + _, rc := newMiniredisClient(t) + c := workloadwatcher.New(fc, inactiveKS(t), rc, nil) + + reconcilePod(t, c, "team-a", "web-a") + reconcilePod(t, c, "team-b", "web-b") + + // team-a has its own ResourcePolicy, which outranks the fleet default for pods + // in that namespace only. + teamProfile := getProfile(t, fc, profileNameFor("web", namespacedRef("team-a", "local"))) + fleetProfile := getProfile(t, fc, profileNameFor("web", clusterRef("fleet"))) + + if teamProfile.Name == fleetProfile.Name { + t.Fatal("expected two distinct profiles") + } + if teamProfile.Status.MeasurementHash == fleetProfile.Status.MeasurementHash { + t.Error("sibling profiles must own separate Redis key namespaces") + } + if teamProfile.Status.ActiveWorkloads != 1 || fleetProfile.Status.ActiveWorkloads != 1 { + t.Errorf("activeWorkloads = %d and %d, want 1 each", + teamProfile.Status.ActiveWorkloads, fleetProfile.Status.ActiveWorkloads) + } + // The tuple itself is unchanged: it stays a map of real pod labels, which is + // what keeps pod selection working. + if teamProfile.Status.TupleLabels["app"] != "web" { + t.Errorf("tupleLabels = %v, want app=web", teamProfile.Status.TupleLabels) + } +} + +// Applying a policy to a running cluster must migrate pods without waiting for pod +// churn. This is the reconcile the policy watch triggers. +func TestPodReconciler_PolicyAppliedAtRuntime_MigratesProfile(t *testing.T) { + pod := enrolledPod("default", "web-abc", "web") + fc := newFakeClient(defaultBallastConfig(), pod) + _, rc := newMiniredisClient(t) + c := workloadwatcher.New(fc, inactiveKS(t), rc, nil) + + // No policy yet: the pod is tracked under a NoPolicy profile. + reconcilePod(t, c, "default", "web-abc") + before := getProfile(t, fc, noPolicyProfile("web")) + if before.Status.PolicyRef != nil { + t.Error("expected no policyRef before any policy exists") + } + + // A policy is applied at runtime. + if err := fc.Create(context.Background(), fleetPolicy("fleet", 0)); err != nil { + t.Fatalf("create policy: %v", err) + } + reconcilePod(t, c, "default", "web-abc") + + after := getProfile(t, fc, profileNameFor("web", clusterRef("fleet"))) + if after.Status.PolicyRef == nil || after.Status.PolicyRef.Name != "fleet" { + t.Fatalf("policyRef = %+v, want fleet", after.Status.PolicyRef) + } + if after.Status.ActiveWorkloads != 1 { + t.Errorf("new profile activeWorkloads = %d, want 1", after.Status.ActiveWorkloads) + } + + // The profile the pod left drops to zero and orphans, so it ages out. + old := getProfile(t, fc, noPolicyProfile("web")) + if old.Status.ActiveWorkloads != 0 { + t.Errorf("old profile activeWorkloads = %d, want 0", old.Status.ActiveWorkloads) + } + + // The pod's policy-ref must not keep advertising the stale answer. + var got corev1.Pod + if err := fc.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "web-abc"}, &got); err != nil { + t.Fatalf("Get pod: %v", err) + } + if ref := got.Annotations[validation.AnnotationPolicyRef]; ref != "fleet" { + t.Errorf("policy-ref = %q, want fleet", ref) + } +} + +// Removing the last matching policy moves the pod back to a NoPolicy profile and +// clears the policy-ref annotation rather than leaving a dangling reference. +func TestPodReconciler_PolicyRemovedAtRuntime_ClearsPolicyRef(t *testing.T) { + policy := fleetPolicy("fleet", 0) + fc := newFakeClient(defaultBallastConfig(), policy, enrolledPod("default", "web-abc", "web")) + _, rc := newMiniredisClient(t) + c := workloadwatcher.New(fc, inactiveKS(t), rc, nil) + + reconcilePod(t, c, "default", "web-abc") + if err := fc.Delete(context.Background(), policy); err != nil { + t.Fatalf("delete policy: %v", err) + } + reconcilePod(t, c, "default", "web-abc") + + profile := getProfile(t, fc, noPolicyProfile("web")) + if profile.Status.PolicyRef != nil { + t.Errorf("policyRef = %+v, want nil", profile.Status.PolicyRef) + } + + var pod corev1.Pod + if err := fc.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "web-abc"}, &pod); err != nil { + t.Fatalf("Get pod: %v", err) + } + if ref, ok := pod.Annotations[validation.AnnotationPolicyRef]; ok { + t.Errorf("policy-ref = %q, want the annotation removed", ref) + } +} + +// Re-reconciling an unchanged pod must not rewrite anything, or every policy event +// would churn the whole fleet's annotations. +func TestPodReconciler_StableIdentityDoesNotRewriteAnnotations(t *testing.T) { + fc := newFakeClient(defaultBallastConfig(), fleetPolicy("fleet", 0), enrolledPod("default", "web-abc", "web")) + _, rc := newMiniredisClient(t) + c := workloadwatcher.New(fc, inactiveKS(t), rc, nil) + + reconcilePod(t, c, "default", "web-abc") + var first corev1.Pod + if err := fc.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "web-abc"}, &first); err != nil { + t.Fatalf("Get pod: %v", err) + } + + reconcilePod(t, c, "default", "web-abc") + var second corev1.Pod + if err := fc.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "web-abc"}, &second); err != nil { + t.Fatalf("Get pod: %v", err) + } + + if first.ResourceVersion != second.ResourceVersion { + t.Errorf("pod was rewritten on a no-op reconcile: %s -> %s", + first.ResourceVersion, second.ResourceVersion) + } +} + +// -- finalizer purge -- + +// Each profile owns its own Redis key namespace, which is what lets the finalizer +// purge without reference counting. Deleting one profile must leave its sibling's +// samples intact even though both cover the same label tuple. +func TestProfileReconciler_PurgeLeavesSiblingHistoryIntact(t *testing.T) { + ctx := context.Background() + tuple := map[string]string{"app": "web"} + doomedHash := store.MeasurementHash(tuple, namespacedRef("team-a", "local").Key()) + siblingHash := store.MeasurementHash(tuple, clusterRef("fleet").Key()) + + doomed := &ballastv1.WorkloadProfile{ + ObjectMeta: metav1.ObjectMeta{ + Name: profileNameFor("web", namespacedRef("team-a", "local")), + Finalizers: []string{workloadwatcher.ProfileFinalizerName}, + }, + Status: ballastv1.WorkloadProfileStatus{TupleLabels: tuple, MeasurementHash: doomedHash}, + } + fc := newFakeClient(defaultBallastConfig(), doomed) + _, rc := newMiniredisClient(t) + c := workloadwatcher.New(fc, inactiveKS(t), rc, nil) + + doomedKey := store.MetricKey(doomedHash, "app", "cpu") + siblingKey := store.MetricKey(siblingHash, "app", "cpu") + for _, key := range []string{doomedKey, siblingKey} { + if err := store.AddSample(ctx, rc, key, 1, "100", 100); err != nil { + t.Fatalf("AddSample %s: %v", key, err) + } + } + + if err := fc.Delete(ctx, doomed); err != nil { + t.Fatalf("delete profile: %v", err) + } + if _, err := reconcileProfile(t, c, doomed.Name); err != nil { + t.Fatalf("Profile.Reconcile: %v", err) + } + + if n, err := store.SampleCount(ctx, rc, doomedKey); err != nil || n != 0 { + t.Errorf("deleted profile's samples: got %d (err %v), want 0", n, err) + } + if n, err := store.SampleCount(ctx, rc, siblingKey); err != nil || n != 1 { + t.Errorf("sibling profile's samples: got %d (err %v), want 1 (must survive)", n, err) + } +} + +// A profile inherited from a release that predates measurement hashes holds its +// samples under the bare tuple hash. Its keys must still be purged when it ages +// out, which is how every profile carried across the upgrade is cleaned up. +func TestProfileReconciler_PurgeFallsBackToTupleHash(t *testing.T) { + ctx := context.Background() + tuple := map[string]string{"app": "web"} + + legacy := &ballastv1.WorkloadProfile{ + ObjectMeta: metav1.ObjectMeta{ + Name: "web", + Finalizers: []string{workloadwatcher.ProfileFinalizerName}, + }, + Status: ballastv1.WorkloadProfileStatus{TupleLabels: tuple}, // no MeasurementHash + } + fc := newFakeClient(defaultBallastConfig(), legacy) + _, rc := newMiniredisClient(t) + c := workloadwatcher.New(fc, inactiveKS(t), rc, nil) + + key := store.MetricKey(store.TupleHash(tuple), "app", "cpu") + if err := store.AddSample(ctx, rc, key, 1, "100", 100); err != nil { + t.Fatalf("AddSample: %v", err) + } + + if err := fc.Delete(ctx, legacy); err != nil { + t.Fatalf("delete profile: %v", err) + } + if _, err := reconcileProfile(t, c, "web"); err != nil { + t.Fatalf("Profile.Reconcile: %v", err) + } + + if n, err := store.SampleCount(ctx, rc, key); err != nil || n != 0 { + t.Errorf("legacy samples: got %d (err %v), want 0", n, err) + } +} diff --git a/internal/controller/workloadwatcher/watch_internal_test.go b/internal/controller/workloadwatcher/watch_internal_test.go index 8c140ac..1782b3d 100644 --- a/internal/controller/workloadwatcher/watch_internal_test.go +++ b/internal/controller/workloadwatcher/watch_internal_test.go @@ -123,7 +123,7 @@ func TestPodsForConfig(t *testing.T) { WithObjects(managed, enrolled, unmanaged).Build() r := &PodReconciler{client: fc} - reqs := r.podsForConfig(context.Background(), &ballastv1.BallastConfig{}) + reqs := r.allManagedPods(context.Background(), &ballastv1.BallastConfig{}) if len(reqs) != 2 { t.Errorf("podsForConfig: got %d requests, want 2 (managed + enrolled)", len(reqs)) } @@ -215,3 +215,123 @@ func TestCountActiveWorkloads(t *testing.T) { }) } } + +func TestPolicyResolutionChangedPredicate(t *testing.T) { + p := policyResolutionChanged() + + if !p.Create(event.CreateEvent{}) { + t.Error("create must be admitted: a new policy can change which policy governs a pod") + } + if !p.Delete(event.DeleteEvent{}) { + t.Error("delete must be admitted: pods governed by the policy must move elsewhere") + } + if p.Generic(event.GenericEvent{}) { + t.Error("generic should not be admitted") + } +} + +// Only selector and priority decide which policy wins. Everything else in the spec +// is read live by the collector and adjuster on their next cycle, so treating it as +// an identity change would re-key measurement history for nothing. +func TestPolicyResolutionChangedPredicate_Update(t *testing.T) { + withSelector := func(sel ballastv1.PolicySelector, priority int32) *ballastv1.ClusterResourcePolicy { + return &ballastv1.ClusterResourcePolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "fleet"}, + Spec: ballastv1.ClusterResourcePolicySpec{ + Priority: priority, + Selector: sel, + Behaviors: ballastv1.BehaviorConfig{ + Thresholds: ballastv1.ThresholdConfig{Default: "20%"}, + }, + }, + } + } + + tests := []struct { + name string + old, new *ballastv1.ClusterResourcePolicy + want bool + }{ + { + name: "selector unchanged", + old: withSelector(ballastv1.PolicySelector{Kinds: []string{"Deployment"}}, 0), + new: withSelector(ballastv1.PolicySelector{Kinds: []string{"Deployment"}}, 0), + want: false, + }, + { + name: "selector changed", + old: withSelector(ballastv1.PolicySelector{Kinds: []string{"Deployment"}}, 0), + new: withSelector(ballastv1.PolicySelector{Kinds: []string{"StatefulSet"}}, 0), + want: true, + }, + { + name: "priority changed", + old: withSelector(ballastv1.PolicySelector{}, 0), + new: withSelector(ballastv1.PolicySelector{}, 100), + want: true, + }, + } + + p := policyResolutionChanged() + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := p.Update(event.UpdateEvent{ObjectOld: tc.old, ObjectNew: tc.new}) + if got != tc.want { + t.Errorf("Update admitted = %v, want %v", got, tc.want) + } + }) + } +} + +// A headroom edit must not churn profile identity: the sample already recorded does +// not change meaning because the headroom applied to it did. +func TestPolicyResolutionChangedPredicate_BehaviorEditIgnored(t *testing.T) { + base := &ballastv1.ClusterResourcePolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "fleet"}, + Spec: ballastv1.ClusterResourcePolicySpec{ + Metrics: []ballastv1.MetricConfig{{Resource: "cpu", Headroom: "1.2"}}, + }, + } + edited := base.DeepCopy() + edited.Spec.Metrics[0].Headroom = "1.5" + + if policyResolutionChanged().Update(event.UpdateEvent{ObjectOld: base, ObjectNew: edited}) { + t.Error("a headroom-only edit must not be treated as a resolution change") + } +} + +// Namespace-scoped policies share the predicate; ResourcePolicySpec is an alias of +// ClusterResourcePolicySpec, so one extractor serves both kinds. +func TestPolicyResolutionChangedPredicate_ResourcePolicy(t *testing.T) { + old := &ballastv1.ResourcePolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "team-a", Name: "local"}, + Spec: ballastv1.ResourcePolicySpec{Priority: 0}, + } + updated := old.DeepCopy() + updated.Spec.Priority = 10 + + if !policyResolutionChanged().Update(event.UpdateEvent{ObjectOld: old, ObjectNew: updated}) { + t.Error("a ResourcePolicy priority change must be admitted") + } +} + +// An event carrying an object that is not a policy is admitted rather than silently +// dropped, since guessing wrong would strand pods on a stale policy. +func TestPolicyResolutionChangedPredicate_UnknownObject(t *testing.T) { + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "p"}} + if !policyResolutionChanged().Update(event.UpdateEvent{ObjectOld: pod, ObjectNew: pod}) { + t.Error("an unrecognized object should be admitted, not dropped") + } +} + +func TestPolicySpecOf(t *testing.T) { + if _, ok := policySpecOf(&ballastv1.ClusterResourcePolicy{}); !ok { + t.Error("ClusterResourcePolicy should be recognized") + } + if _, ok := policySpecOf(&ballastv1.ResourcePolicy{}); !ok { + t.Error("ResourcePolicy should be recognized") + } + if _, ok := policySpecOf(&corev1.Pod{}); ok { + t.Error("a Pod is not a policy") + } +} diff --git a/internal/naming/naming.go b/internal/naming/naming.go new file mode 100644 index 0000000..082209b --- /dev/null +++ b/internal/naming/naming.go @@ -0,0 +1,122 @@ +// Package naming derives the deterministic Kubernetes object names Ballast uses +// for WorkloadProfiles, along with the policy tokens those names embed. It is a +// leaf package so that the webhook and the workloadwatcher, which must agree on +// every profile name exactly, can share one implementation. +package naming + +import ( + "crypto/sha256" + "encoding/hex" + "strings" + "unicode" +) + +const ( + // maxNameLength is the Kubernetes object-name limit. A WorkloadProfile name + // is only ever stored in object names and annotations, never in a label + // value, so the full 253 characters are available rather than the + // 63-character ceiling that would apply to a label. + maxNameLength = 253 + + // shortHashLength is the width, in hex characters, of the hash suffixes + // appended to a name segment. Eight characters (32 bits) is ample for + // disambiguating the handful of policies and tuples in one cluster while + // staying short enough to read in kubectl output. + shortHashLength = 8 + + // maxPolicySegment bounds the readable part of a policy discriminator so a + // verbose policy name cannot crowd the identity tuple out of the name. + maxPolicySegment = 24 + + // NoPolicy is the discriminator for a profile whose pods currently match no + // policy. Such a profile still accrues nothing and is skipped by the metrics + // collector, but it exists so the pods are tracked and so the profile's + // identity changes (and it orphans) the moment a policy starts matching. + NoPolicy = "nopolicy" + + // segmentSeparator joins the identity-tuple values to each other and the + // tuple to the policy discriminator. + segmentSeparator = "--" +) + +// PolicyDiscriminator returns the stable token identifying one policy within a +// WorkloadProfile name, in the form "-". +// +// The hash covers kind, namespace, and name rather than the name alone, because +// two ResourcePolicies in different namespaces may share a name while being +// entirely different policies. Hashing the name alone would give them the same +// token, and two distinct profiles would then contend for one object name. +// +// The token is a pure function of the policy's identity and never of the +// workload's, so it is identical across every profile that resolves to this +// policy. That is what makes it meaningful to publish on the policy's own status +// as status.profileDiscriminator. +func PolicyDiscriminator(kind, namespace, name string) string { + readable := SanitizeSegment(name) + if len(readable) > maxPolicySegment { + readable = strings.Trim(readable[:maxPolicySegment], "-") + } + if readable == "" { + // Sanitization can empty a segment (a name of only non-alphanumerics). + // The hash still disambiguates; this only keeps the token from starting + // with a dash. + readable = "policy" + } + return readable + "-" + shortHash(kind+"/"+namespace+"/"+name) +} + +// ProfileName derives a deterministic, DNS-safe WorkloadProfile name from an +// identity tuple and the discriminator of the policy governing that tuple. +// Tuple values are joined in identityLabels order and the discriminator is +// appended last, so a name reads as "---". +// +// An empty discriminator produces a tuple-only name; callers that resolve policy +// pass NoPolicy rather than "" when nothing matched, so a bare tuple name means +// "policy was not considered", not "no policy matched". +func ProfileName(tupleLabels map[string]string, identityLabels []string, discriminator string) string { + parts := make([]string, 0, len(identityLabels)) + for _, k := range identityLabels { + if v, ok := tupleLabels[k]; ok { + parts = append(parts, SanitizeSegment(v)) + } + } + tuple := strings.Join(parts, segmentSeparator) + + var suffix string + if discriminator != "" { + suffix = segmentSeparator + discriminator + } + + if budget := maxNameLength - len(suffix); len(tuple) > budget { + // Truncating the readable tuple can map two distinct tuples onto one + // name, which would silently merge two workloads into one profile. The + // hash of the full tuple is appended so the truncated form stays unique; + // the discriminator's own hash covers only the policy and cannot serve + // here. Only pathological label values reach this path. + suffix = "-" + shortHash(tuple) + suffix + tuple = strings.Trim(tuple[:maxNameLength-len(suffix)], "-") + } + return tuple + suffix +} + +// SanitizeSegment converts s to a lowercase DNS-label-safe segment: letters, +// digits, and dashes survive, everything else becomes a dash, and leading and +// trailing dashes are trimmed. +func SanitizeSegment(s string) string { + s = strings.ToLower(s) + var b strings.Builder + for _, r := range s { + if unicode.IsLetter(r) || unicode.IsDigit(r) || r == '-' { + b.WriteRune(r) + } else { + b.WriteRune('-') + } + } + return strings.Trim(b.String(), "-") +} + +// shortHash returns the first shortHashLength hex characters of the SHA-256 of s. +func shortHash(s string) string { + sum := sha256.Sum256([]byte(s)) + return hex.EncodeToString(sum[:])[:shortHashLength] +} diff --git a/internal/naming/naming_test.go b/internal/naming/naming_test.go new file mode 100644 index 0000000..4eee3dd --- /dev/null +++ b/internal/naming/naming_test.go @@ -0,0 +1,160 @@ +package naming_test + +import ( + "strings" + "testing" + + "github.com/tight-line/ballast/internal/naming" +) + +func TestPolicyDiscriminator_Deterministic(t *testing.T) { + a := naming.PolicyDiscriminator("ClusterResourcePolicy", "", "fleet") + b := naming.PolicyDiscriminator("ClusterResourcePolicy", "", "fleet") + if a != b { + t.Fatalf("not deterministic: %q != %q", a, b) + } +} + +func TestPolicyDiscriminator_ReadablePrefix(t *testing.T) { + got := naming.PolicyDiscriminator("ClusterResourcePolicy", "", "fleet-defaults") + if !strings.HasPrefix(got, "fleet-defaults-") { + t.Errorf("discriminator %q should start with the policy name", got) + } + // name + dash + 8 hex characters. + if len(got) != len("fleet-defaults")+1+8 { + t.Errorf("discriminator %q has unexpected length %d", got, len(got)) + } +} + +// Two ResourcePolicies in different namespaces may share a name while being +// entirely different policies. If the token were derived from the name alone they +// would collide, and two distinct profiles would contend for one object name. +func TestPolicyDiscriminator_DistinguishesNamespace(t *testing.T) { + a := naming.PolicyDiscriminator("ResourcePolicy", "team-a", "defaults") + b := naming.PolicyDiscriminator("ResourcePolicy", "team-b", "defaults") + if a == b { + t.Fatalf("same token %q for policies in different namespaces", a) + } + if !strings.HasPrefix(a, "defaults-") || !strings.HasPrefix(b, "defaults-") { + t.Errorf("both tokens should stay readable: %q, %q", a, b) + } +} + +// A cluster-scoped and a namespace-scoped policy of the same name are also +// different policies. +func TestPolicyDiscriminator_DistinguishesKind(t *testing.T) { + a := naming.PolicyDiscriminator("ClusterResourcePolicy", "", "defaults") + b := naming.PolicyDiscriminator("ResourcePolicy", "", "defaults") + if a == b { + t.Fatalf("same token %q for different kinds", a) + } +} + +func TestPolicyDiscriminator_LongNameTruncated(t *testing.T) { + long := strings.Repeat("a", 80) + got := naming.PolicyDiscriminator("ClusterResourcePolicy", "", long) + if len(got) != 24+1+8 { + t.Errorf("discriminator %q length = %d, want readable part capped at 24", got, len(got)) + } + // Truncation must not cost uniqueness: the hash covers the full name. + other := naming.PolicyDiscriminator("ClusterResourcePolicy", "", long+"-different") + if got == other { + t.Error("two long names sharing a prefix produced the same token") + } +} + +func TestPolicyDiscriminator_UnsanitizableName(t *testing.T) { + got := naming.PolicyDiscriminator("ClusterResourcePolicy", "", "...") + if !strings.HasPrefix(got, "policy-") { + t.Errorf("discriminator %q should fall back to a readable prefix", got) + } +} + +func TestProfileName_JoinsTupleInIdentityOrder(t *testing.T) { + tuple := map[string]string{"app": "checkout", "component": "server"} + got := naming.ProfileName(tuple, []string{"app", "component"}, "fleet-abc12345") + if want := "checkout--server--fleet-abc12345"; got != want { + t.Errorf("ProfileName = %q, want %q", got, want) + } + + // identityLabels order drives the name, not map iteration order. + got = naming.ProfileName(tuple, []string{"component", "app"}, "fleet-abc12345") + if want := "server--checkout--fleet-abc12345"; got != want { + t.Errorf("ProfileName = %q, want %q", got, want) + } +} + +func TestProfileName_SkipsAbsentIdentityLabels(t *testing.T) { + got := naming.ProfileName(map[string]string{"app": "checkout"}, []string{"app", "missing"}, "x-1") + if want := "checkout--x-1"; got != want { + t.Errorf("ProfileName = %q, want %q", got, want) + } +} + +func TestProfileName_EmptyDiscriminator(t *testing.T) { + got := naming.ProfileName(map[string]string{"app": "checkout"}, []string{"app"}, "") + if want := "checkout"; got != want { + t.Errorf("ProfileName = %q, want %q", got, want) + } +} + +func TestProfileName_SanitizesValues(t *testing.T) { + got := naming.ProfileName(map[string]string{"app": "My_App.v2"}, []string{"app"}, "") + if want := "my-app-v2"; got != want { + t.Errorf("ProfileName = %q, want %q", got, want) + } +} + +// Truncation must stay inside the Kubernetes name limit and must not merge two +// distinct tuples into one profile. The discriminator's hash covers only the +// policy, so a tuple hash is appended when the readable part is cut. +func TestProfileName_TruncatesWithinLimitAndStaysUnique(t *testing.T) { + long := strings.Repeat("a", 200) + other := strings.Repeat("a", 199) + "b" + labels := []string{"one", "two"} + + first := naming.ProfileName(map[string]string{"one": long, "two": long}, labels, "fleet-abc12345") + second := naming.ProfileName(map[string]string{"one": long, "two": other}, labels, "fleet-abc12345") + + for _, name := range []string{first, second} { + if len(name) > 253 { + t.Errorf("name length %d exceeds 253: %q", len(name), name) + } + } + if first == second { + t.Error("two different long tuples produced the same profile name") + } + if !strings.HasSuffix(first, "--fleet-abc12345") { + t.Errorf("discriminator must survive truncation, got %q", first) + } +} + +func TestProfileName_TruncatesWithoutDiscriminator(t *testing.T) { + long := strings.Repeat("a", 300) + got := naming.ProfileName(map[string]string{"one": long}, []string{"one"}, "") + if len(got) > 253 { + t.Errorf("name length %d exceeds 253", len(got)) + } + other := naming.ProfileName(map[string]string{"one": long + "b"}, []string{"one"}, "") + if got == other { + t.Error("two different long tuples produced the same profile name") + } +} + +func TestSanitizeSegment(t *testing.T) { + tests := []struct { + in string + want string + }{ + {"Web", "web"}, + {"my_app.v2", "my-app-v2"}, + {"-leading-and-trailing-", "leading-and-trailing"}, + {"...", ""}, + {"a1-b2", "a1-b2"}, + } + for _, tc := range tests { + if got := naming.SanitizeSegment(tc.in); got != tc.want { + t.Errorf("SanitizeSegment(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} diff --git a/internal/policy/input.go b/internal/policy/input.go new file mode 100644 index 0000000..09c2874 --- /dev/null +++ b/internal/policy/input.go @@ -0,0 +1,31 @@ +package policy + +import corev1 "k8s.io/api/core/v1" + +// InputForPod builds the resolver Input describing one pod. +// +// Both the admission webhook and the workloadwatcher resolve policy, and they +// must reach the same answer for the same pod: if they diverge, a pod is admitted +// with one policy's recommendations and then measured and resized under another, +// with nothing logging a conflict because each path resolved successfully on its +// own terms. Sharing this constructor is what keeps them from drifting apart. +func InputForPod(pod *corev1.Pod) Input { + return Input{ + Namespace: pod.Namespace, + OwnerKind: directOwnerKind(pod), + Labels: pod.Labels, + Annotations: pod.Annotations, + } +} + +// directOwnerKind returns the kind of the pod's controlling ownerReference, or "" +// when the pod has no controller (a standalone pod), in which case only policies +// with no Kinds selector match. +func directOwnerKind(pod *corev1.Pod) string { + for _, ref := range pod.OwnerReferences { + if ref.Controller != nil && *ref.Controller { + return ref.Kind + } + } + return "" +} diff --git a/internal/policy/resolver.go b/internal/policy/resolver.go index 749b1f8..215e167 100644 --- a/internal/policy/resolver.go +++ b/internal/policy/resolver.go @@ -8,16 +8,28 @@ import ( "sort" "github.com/go-logr/logr" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ballastv1 "github.com/tight-line/ballast/api/v1" ) // Input holds the pod attributes used to evaluate policy selectors. +// +// Every field must describe a single real pod. Resolution is a per-pod question, +// and answering it from a partially-filled Input silently changes the answer: an +// empty Namespace excludes every ResourcePolicy, an empty OwnerKind matches only +// policies with no Kinds, and absent Annotations match only policies with no +// annotation selector. Callers with a pod should build the Input with InputForPod +// rather than assembling it field by field. type Input struct { - // Namespace is the pod's namespace. + // Namespace is the pod's namespace. Empty means "no namespace is known", and + // no ResourcePolicy can match, since a namespace-scoped policy cannot be + // established as the more specific match for a workload whose namespace is + // unknown. Namespace string // OwnerKind is the pre-resolved top-level owner kind (e.g. "Deployment", "StatefulSet"). // Callers walk ownerReferences to resolve this before calling Resolve. @@ -38,11 +50,25 @@ type ResolvedPolicy struct { Name string // Namespaced is true for a ResourcePolicy, false for a ClusterResourcePolicy. Namespaced bool + // Ref identifies the policy object, for recording on a WorkloadProfile's + // status.policyRef and for loading the same policy again via Load. + Ref ballastv1.PolicyReference +} + +// PodAnnotationValue returns the value stamped into a pod's policy-ref +// annotation: "namespace/name" for a ResourcePolicy, and the bare name for a +// ClusterResourcePolicy, which has no namespace. +func PodAnnotationValue(ref ballastv1.PolicyReference) string { + if ref.Namespace != "" { + return ref.Namespace + "/" + ref.Name + } + return ref.Name } // policyCandidate is an intermediate match collected during policy resolution. type policyCandidate struct { spec ballastv1.ClusterResourcePolicySpec + ref ballastv1.PolicyReference name string priority int32 namespaced bool @@ -65,6 +91,11 @@ func NewResolver(c client.Client, log logr.Logger) *Resolver { // - ResourcePolicy (namespace-scoped) beats ClusterResourcePolicy regardless of priority. // - Within the same class, higher Priority wins. // - Equal priority ties break alphabetically by policy name. +// +// The first rule holds only because in.Namespace names the pod's own namespace: +// "namespaced" is shorthand for "more specific to this workload". An Input with +// no namespace has no such candidates to rank, because collectMatches admits no +// ResourcePolicy in that case. func (r *Resolver) Resolve(ctx context.Context, in Input) (*ResolvedPolicy, error) { matches, err := r.collectMatches(ctx, in) if err != nil { @@ -106,29 +137,90 @@ func (r *Resolver) Resolve(ctx context.Context, in Input) (*ResolvedPolicy, erro Spec: best.spec, Name: best.name, Namespaced: best.namespaced, + Ref: best.ref, + }, nil +} + +// Load returns the policy named by ref with defaults applied, or nil when the +// object no longer exists. +// +// Callers holding a WorkloadProfile use this instead of Resolve: the profile's +// status.policyRef was written by the workloadwatcher from a full per-pod Input, +// whereas a profile on its own supplies neither a namespace nor the pod labels +// outside its identity tuple, so re-resolving from it would reach a different +// answer than admission did. +func (r *Resolver) Load(ctx context.Context, ref ballastv1.PolicyReference) (*ResolvedPolicy, error) { + var spec ballastv1.ClusterResourcePolicySpec + + switch ref.Kind { + case ballastv1.KindResourcePolicy: + var rp ballastv1.ResourcePolicy + if err := r.client.Get(ctx, types.NamespacedName{Namespace: ref.Namespace, Name: ref.Name}, &rp); err != nil { + if apierrors.IsNotFound(err) { + return nil, nil + } + return nil, fmt.Errorf("getting ResourcePolicy %s/%s: %w", ref.Namespace, ref.Name, err) // coverage:ignore - transient API error + } + spec = rp.Spec + case ballastv1.KindClusterResourcePolicy: + var crp ballastv1.ClusterResourcePolicy + if err := r.client.Get(ctx, types.NamespacedName{Name: ref.Name}, &crp); err != nil { + if apierrors.IsNotFound(err) { + return nil, nil + } + return nil, fmt.Errorf("getting ClusterResourcePolicy %s: %w", ref.Name, err) // coverage:ignore - transient API error + } + spec = crp.Spec + default: + return nil, fmt.Errorf("unknown policy kind %q in reference %s", ref.Kind, ref.Key()) + } + + // Defaulting happens here for the same reason it happens in Resolve: sparse + // policies should track the running release's defaults rather than whatever + // was current when the object was written. + spec.ApplyDefaults() + + return &ResolvedPolicy{ + Spec: spec, + Name: ref.Name, + Namespaced: ref.Kind == ballastv1.KindResourcePolicy, + Ref: ref, }, nil } // collectMatches lists all ResourcePolicies and ClusterResourcePolicies that match in. +// +// ResourcePolicies are listed only when in.Namespace is set. client.InNamespace("") +// means *all namespaces*, so listing unconditionally would make every +// ResourcePolicy in the cluster a candidate for an Input with no namespace, and +// the scope-before-priority rule in Resolve would then hand any one of them +// precedence over every ClusterResourcePolicy. func (r *Resolver) collectMatches(ctx context.Context, in Input) ([]policyCandidate, error) { var matches []policyCandidate - var rpList ballastv1.ResourcePolicyList - if err := r.client.List(ctx, &rpList, client.InNamespace(in.Namespace)); err != nil { // coverage:ignore - client List failure requires envtest - return nil, fmt.Errorf("listing ResourcePolicies in %s: %w", in.Namespace, err) - } - for _, rp := range rpList.Items { - ok, err := r.matchesSelector(in, rp.Spec.Selector) - if err != nil { - return nil, fmt.Errorf("evaluating ResourcePolicy %s/%s: %w", in.Namespace, rp.Name, err) + if in.Namespace != "" { + var rpList ballastv1.ResourcePolicyList + if err := r.client.List(ctx, &rpList, client.InNamespace(in.Namespace)); err != nil { // coverage:ignore - client List failure requires envtest + return nil, fmt.Errorf("listing ResourcePolicies in %s: %w", in.Namespace, err) } - if ok { - matches = append(matches, policyCandidate{ - spec: rp.Spec, - name: rp.Name, - priority: rp.Spec.Priority, - namespaced: true, - }) + for _, rp := range rpList.Items { + ok, err := r.matchesSelector(in, rp.Spec.Selector) + if err != nil { + return nil, fmt.Errorf("evaluating ResourcePolicy %s/%s: %w", in.Namespace, rp.Name, err) + } + if ok { + matches = append(matches, policyCandidate{ + spec: rp.Spec, + name: rp.Name, + priority: rp.Spec.Priority, + namespaced: true, + ref: ballastv1.PolicyReference{ + Kind: ballastv1.KindResourcePolicy, + Namespace: in.Namespace, + Name: rp.Name, + }, + }) + } } } @@ -147,6 +239,10 @@ func (r *Resolver) collectMatches(ctx context.Context, in Input) ([]policyCandid name: crp.Name, priority: crp.Spec.Priority, namespaced: false, + ref: ballastv1.PolicyReference{ + Kind: ballastv1.KindClusterResourcePolicy, + Name: crp.Name, + }, }) } } diff --git a/internal/policy/scope_test.go b/internal/policy/scope_test.go new file mode 100644 index 0000000..08d6411 --- /dev/null +++ b/internal/policy/scope_test.go @@ -0,0 +1,276 @@ +package policy_test + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + ballastv1 "github.com/tight-line/ballast/api/v1" + "github.com/tight-line/ballast/internal/policy" +) + +// TestResolve_NoNamespace_ExcludesResourcePolicies reproduces the defect from +// issue #87. client.InNamespace("") means *all namespaces*, so an Input with no +// namespace used to make every ResourcePolicy in the cluster a candidate, and the +// scope-before-priority rule then handed one of them precedence over every +// ClusterResourcePolicy. A namespace-scoped policy can only be "the more specific +// match" for a workload whose namespace is known, so with none it must not match. +func TestResolve_NoNamespace_ExcludesResourcePolicies(t *testing.T) { + tuple := map[string]string{"app.kubernetes.io/name": "checkout"} + + tests := []struct { + name string + rp *ballastv1.ResourcePolicy + }{ + { + name: "RP whose labelSelector matches the tuple", + rp: namespacedPolicy("team-a", "rp-teama", 0, ballastv1.PolicySelector{ + LabelSelector: &metav1.LabelSelector{MatchLabels: tuple}, + }), + }, + { + name: "RP with an empty selector, which matches everything", + rp: namespacedPolicy("team-a", "rp-teama", 0, ballastv1.PolicySelector{}), + }, + { + name: "RP with a higher priority than the cluster policy", + rp: namespacedPolicy("team-a", "rp-teama", 500, ballastv1.PolicySelector{}), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + crp := clusterPolicy("crp-fleet", 100, ballastv1.PolicySelector{}) + r := policy.NewResolver(newClient(t, tc.rp, crp), logr.Discard()) + + got, err := r.Resolve(context.Background(), policy.Input{Labels: tuple}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got == nil { + t.Fatal("expected crp-fleet, got nil") + } + if got.Name != "crp-fleet" { + t.Errorf("got policy %q, want %q", got.Name, "crp-fleet") + } + }) + } +} + +// The scope-before-priority rule is still correct when the namespace is known: a +// namespace owner's policy beats a cluster-wide default. +func TestResolve_WithNamespace_ResourcePolicyStillWins(t *testing.T) { + rp := namespacedPolicy("team-a", "rp-teama", 0, ballastv1.PolicySelector{}) + crp := clusterPolicy("crp-fleet", 100, ballastv1.PolicySelector{}) + r := policy.NewResolver(newClient(t, rp, crp), logr.Discard()) + + got, err := r.Resolve(context.Background(), policy.Input{Namespace: "team-a"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got == nil || got.Name != "rp-teama" { + t.Fatalf("got %v, want rp-teama", got) + } + if !got.Namespaced { + t.Error("expected Namespaced=true for a ResourcePolicy") + } +} + +// A ResourcePolicy in another namespace must not reach this pod. +func TestResolve_ResourcePolicyDoesNotReachOtherNamespaces(t *testing.T) { + rp := namespacedPolicy("team-a", "rp-teama", 500, ballastv1.PolicySelector{}) + crp := clusterPolicy("crp-fleet", 0, ballastv1.PolicySelector{}) + r := policy.NewResolver(newClient(t, rp, crp), logr.Discard()) + + got, err := r.Resolve(context.Background(), policy.Input{Namespace: "team-b"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got == nil || got.Name != "crp-fleet" { + t.Fatalf("got %v, want crp-fleet", got) + } +} + +func TestResolve_PopulatesRef(t *testing.T) { + rp := namespacedPolicy("team-a", "rp-teama", 0, ballastv1.PolicySelector{}) + r := policy.NewResolver(newClient(t, rp), logr.Discard()) + + got, err := r.Resolve(context.Background(), policy.Input{Namespace: "team-a"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := ballastv1.PolicyReference{ + Kind: ballastv1.KindResourcePolicy, + Namespace: "team-a", + Name: "rp-teama", + } + if got.Ref != want { + t.Errorf("Ref = %+v, want %+v", got.Ref, want) + } +} + +func TestResolve_PopulatesClusterRef(t *testing.T) { + crp := clusterPolicy("crp-fleet", 0, ballastv1.PolicySelector{}) + r := policy.NewResolver(newClient(t, crp), logr.Discard()) + + got, err := r.Resolve(context.Background(), policy.Input{Namespace: "team-a"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := ballastv1.PolicyReference{Kind: ballastv1.KindClusterResourcePolicy, Name: "crp-fleet"} + if got.Ref != want { + t.Errorf("Ref = %+v, want %+v", got.Ref, want) + } +} + +// -- Load -- + +func TestLoad_ClusterPolicy(t *testing.T) { + crp := clusterPolicy("crp-fleet", 7, ballastv1.PolicySelector{}) + r := policy.NewResolver(newClient(t, crp), logr.Discard()) + + got, err := r.Load(context.Background(), ballastv1.PolicyReference{ + Kind: ballastv1.KindClusterResourcePolicy, + Name: "crp-fleet", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got == nil || got.Name != "crp-fleet" { + t.Fatalf("got %v, want crp-fleet", got) + } + if got.Namespaced { + t.Error("expected Namespaced=false for a ClusterResourcePolicy") + } + // Defaults are applied at load time, exactly as at resolve time, so a sparse + // policy tracks the running release rather than whatever was current when it + // was written. + if got.Spec.Readiness.MinTimeSpan != ballastv1.DefaultMinTimeSpan { + t.Errorf("MinTimeSpan = %q, want the release default %q", + got.Spec.Readiness.MinTimeSpan, ballastv1.DefaultMinTimeSpan) + } +} + +func TestLoad_ResourcePolicy(t *testing.T) { + rp := namespacedPolicy("team-a", "rp-teama", 0, ballastv1.PolicySelector{}) + r := policy.NewResolver(newClient(t, rp), logr.Discard()) + + got, err := r.Load(context.Background(), ballastv1.PolicyReference{ + Kind: ballastv1.KindResourcePolicy, + Namespace: "team-a", + Name: "rp-teama", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got == nil || got.Name != "rp-teama" || !got.Namespaced { + t.Fatalf("got %v, want namespaced rp-teama", got) + } +} + +// A deleted policy resolves to nil rather than an error: the workloadwatcher's +// policy watch is already migrating those pods to a different profile. +func TestLoad_MissingReturnsNil(t *testing.T) { + r := policy.NewResolver(newClient(t), logr.Discard()) + + for _, ref := range []ballastv1.PolicyReference{ + {Kind: ballastv1.KindClusterResourcePolicy, Name: "gone"}, + {Kind: ballastv1.KindResourcePolicy, Namespace: "team-a", Name: "gone"}, + } { + got, err := r.Load(context.Background(), ref) + if err != nil { + t.Fatalf("%s: unexpected error: %v", ref.Key(), err) + } + if got != nil { + t.Errorf("%s: got %v, want nil", ref.Key(), got) + } + } +} + +func TestLoad_UnknownKind(t *testing.T) { + r := policy.NewResolver(newClient(t), logr.Discard()) + + _, err := r.Load(context.Background(), ballastv1.PolicyReference{Kind: "Nonsense", Name: "x"}) + if err == nil { + t.Fatal("expected an error for an unknown policy kind") + } +} + +// -- helpers shared with the controllers -- + +func TestPolicyReferenceKey(t *testing.T) { + tests := []struct { + ref ballastv1.PolicyReference + want string + }{ + {ballastv1.PolicyReference{Kind: "ClusterResourcePolicy", Name: "fleet"}, "ClusterResourcePolicy//fleet"}, + {ballastv1.PolicyReference{Kind: "ResourcePolicy", Namespace: "team-a", Name: "local"}, "ResourcePolicy/team-a/local"}, + } + for _, tc := range tests { + if got := tc.ref.Key(); got != tc.want { + t.Errorf("Key() = %q, want %q", got, tc.want) + } + } +} + +func TestPodAnnotationValue(t *testing.T) { + cluster := ballastv1.PolicyReference{Kind: ballastv1.KindClusterResourcePolicy, Name: "fleet"} + if got := policy.PodAnnotationValue(cluster); got != "fleet" { + t.Errorf("cluster policy annotation = %q, want %q", got, "fleet") + } + + namespaced := ballastv1.PolicyReference{ + Kind: ballastv1.KindResourcePolicy, + Namespace: "team-a", + Name: "local", + } + if got := policy.PodAnnotationValue(namespaced); got != "team-a/local" { + t.Errorf("namespaced policy annotation = %q, want %q", got, "team-a/local") + } +} + +func TestInputForPod(t *testing.T) { + controller := true + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "team-a", + Name: "checkout-abc", + Labels: map[string]string{"app": "checkout"}, + Annotations: map[string]string{"note": "hi"}, + OwnerReferences: []metav1.OwnerReference{ + {Kind: "ReplicaSet", Name: "checkout-1", Controller: &controller}, + }, + }, + } + + got := policy.InputForPod(pod) + if got.Namespace != "team-a" { + t.Errorf("Namespace = %q, want team-a", got.Namespace) + } + if got.OwnerKind != "ReplicaSet" { + t.Errorf("OwnerKind = %q, want ReplicaSet", got.OwnerKind) + } + if got.Labels["app"] != "checkout" { + t.Errorf("Labels = %v", got.Labels) + } + if got.Annotations["note"] != "hi" { + t.Errorf("Annotations = %v", got.Annotations) + } +} + +// A pod with only non-controller owner references has no owner kind, so it matches +// only policies that set no Kinds selector. +func TestInputForPod_NoController(t *testing.T) { + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "team-a", + OwnerReferences: []metav1.OwnerReference{{Kind: "ReplicaSet", Name: "rs"}}, + }, + } + if got := policy.InputForPod(pod).OwnerKind; got != "" { + t.Errorf("OwnerKind = %q, want empty", got) + } +} diff --git a/internal/store/keys.go b/internal/store/keys.go index aec2791..7b29b06 100644 --- a/internal/store/keys.go +++ b/internal/store/keys.go @@ -8,10 +8,45 @@ import ( "strings" ) +// policyHashPrefix separates the governing policy from the label pairs in a +// measurement hash. The NUL byte is illegal in a Kubernetes label key, so no +// real label can produce the same canonical line and alias a different policy. +const policyHashPrefix = "\x00policy=" + // TupleHash returns a 16-character lowercase hex string derived from the // SHA-256 of the sorted key=value pairs in labels. The result is // deterministic and independent of map iteration order. +// +// This identifies a workload identity tuple, and is used for bookkeeping that is +// genuinely per-tuple (such as metrics-plugin backoff). Redis measurement keys +// use MeasurementHash instead, because the samples they hold belong to one +// profile rather than to the tuple as a whole. func TupleHash(labels map[string]string) string { + sum := sha256.Sum256([]byte(canonicalLabels(labels))) + return fmt.Sprintf("%x", sum[:8]) +} + +// MeasurementHash returns the 16-character hex hash identifying the Redis key +// namespace a single WorkloadProfile owns. It covers both the identity tuple and +// the governing policy, so two profiles that share a tuple but resolve to +// different policies never write into one series. +// +// Sharing a series across policies is not merely wasteful: samples carry no +// per-sample timestamps, so two profiles appending to one key would inflate the +// count, distort the distribution, and halve the effective retention window. A +// per-profile namespace also lets the profile finalizer purge its own keys +// without reference counting, since no sibling can be reading them. +// +// policyKey is the canonical identity of the governing policy, as produced by +// the profile's status.policyRef; an empty policyKey means no policy matched. +func MeasurementHash(labels map[string]string, policyKey string) string { + sum := sha256.Sum256([]byte(canonicalLabels(labels) + policyHashPrefix + policyKey + "\n")) + return fmt.Sprintf("%x", sum[:8]) +} + +// canonicalLabels serializes labels as sorted "key=value\n" lines, giving a +// representation that is independent of map iteration order. +func canonicalLabels(labels map[string]string) string { keys := make([]string, 0, len(labels)) for k := range labels { keys = append(keys, k) @@ -25,14 +60,13 @@ func TupleHash(labels map[string]string) string { b.WriteString(labels[k]) b.WriteByte('\n') } - - sum := sha256.Sum256([]byte(b.String())) - return fmt.Sprintf("%x", sum[:8]) + return b.String() } // MetricKey returns the Redis sorted-set key for a container/resource timeseries. -func MetricKey(tupleHash, container, resource string) string { - return "ballast:metrics:" + tupleHash + ":" + container + ":" + resource +// hash is the owning profile's MeasurementHash. +func MetricKey(hash, container, resource string) string { + return "ballast:metrics:" + hash + ":" + container + ":" + resource } // AllKeysForHash scans Redis for all metric keys that belong to tupleHash. diff --git a/internal/store/keys_test.go b/internal/store/keys_test.go index 67e78e5..f0f6c62 100644 --- a/internal/store/keys_test.go +++ b/internal/store/keys_test.go @@ -111,3 +111,71 @@ func TestAllKeysForHash_Empty(t *testing.T) { t.Fatalf("expected empty slice, got %v", keys) } } + +func TestMeasurementHash_Deterministic(t *testing.T) { + labels := map[string]string{"app": "api", "component": "server"} + h1 := store.MeasurementHash(labels, "ClusterResourcePolicy//fleet") + h2 := store.MeasurementHash(labels, "ClusterResourcePolicy//fleet") + if h1 != h2 { + t.Fatalf("MeasurementHash not deterministic: %q != %q", h1, h2) + } +} + +func TestMeasurementHash_OrderIndependent(t *testing.T) { + h1 := store.MeasurementHash(map[string]string{"a": "1", "b": "2"}, "p") + h2 := store.MeasurementHash(map[string]string{"b": "2", "a": "1"}, "p") + if h1 != h2 { + t.Fatalf("MeasurementHash is order-dependent: %q != %q", h1, h2) + } +} + +// Two profiles sharing a tuple but governed by different policies must not share a +// key namespace: samples carry no timestamps, so both appending to one series +// would inflate the count and distort the distribution. +func TestMeasurementHash_DiffersByPolicy(t *testing.T) { + labels := map[string]string{"app": "api"} + h1 := store.MeasurementHash(labels, "ClusterResourcePolicy//fleet") + h2 := store.MeasurementHash(labels, "ResourcePolicy/team-a/local") + if h1 == h2 { + t.Fatal("same measurement hash for different policies") + } +} + +func TestMeasurementHash_DiffersByLabels(t *testing.T) { + h1 := store.MeasurementHash(map[string]string{"app": "api"}, "p") + h2 := store.MeasurementHash(map[string]string{"app": "web"}, "p") + if h1 == h2 { + t.Fatal("same measurement hash for different tuples") + } +} + +// A label key cannot contain a NUL byte, so no real label can forge the policy +// line and make one policy's series alias another's. +func TestMeasurementHash_PolicyNotForgeableByLabel(t *testing.T) { + h1 := store.MeasurementHash(map[string]string{"policy": "b"}, "") + h2 := store.MeasurementHash(nil, "b") + if h1 == h2 { + t.Fatal("a label named 'policy' aliased the governing policy") + } +} + +// The empty policy (no policy matched) is still a distinct namespace from the bare +// tuple hash used by pre-upgrade profiles. +func TestMeasurementHash_DiffersFromTupleHash(t *testing.T) { + labels := map[string]string{"app": "api"} + if store.MeasurementHash(labels, "") == store.TupleHash(labels) { + t.Fatal("measurement hash collides with the tuple hash") + } +} + +func TestMeasurementHash_Format(t *testing.T) { + h := store.MeasurementHash(map[string]string{"k": "v"}, "p") + if len(h) != 16 { + t.Errorf("MeasurementHash length = %d, want 16", len(h)) + } + for _, r := range h { + if !strings.ContainsRune("0123456789abcdef", r) { + t.Errorf("MeasurementHash %q contains non-hex character %q", h, r) + } + } +} diff --git a/internal/webhook/pod_mutator.go b/internal/webhook/pod_mutator.go index faed21c..7573908 100644 --- a/internal/webhook/pod_mutator.go +++ b/internal/webhook/pod_mutator.go @@ -24,6 +24,7 @@ import ( "github.com/tight-line/ballast/internal/killswitch" "github.com/tight-line/ballast/internal/kube" "github.com/tight-line/ballast/internal/metrics" + "github.com/tight-line/ballast/internal/naming" "github.com/tight-line/ballast/internal/policy" "github.com/tight-line/ballast/internal/validation" ) @@ -80,7 +81,18 @@ func (m *PodMutator) Handle(ctx context.Context, req admission.Request) admissio return admission.Allowed("apply not requested") } - profile, err := m.lookupProfile(ctx, &pod) + // Resolve policy before looking up the profile: a profile's identity includes + // the policy governing it, so the policy is part of the profile's name. This + // is also the value stamped as policy-ref below, so admission resolves exactly + // once and cannot stamp one policy while applying another's recommendations. + resolved, err := m.resolver.Resolve(ctx, policy.InputForPod(&pod)) + if err != nil { // coverage:ignore - transient API error listing policy objects + log.V(1).Info("policy resolution error, allowing without mutation", "err", err) + m.rec.WebhookMutation(ctx, "not_available", req.Namespace, metrics.ProfileID{}) + return admission.Allowed("policy not available") + } + + profile, err := m.lookupProfile(ctx, &pod, resolved) if err != nil { log.V(1).Info("profile resolution error, allowing without mutation", "err", err) m.rec.WebhookMutation(ctx, "not_available", req.Namespace, metrics.ProfileID{}) @@ -98,11 +110,11 @@ func (m *PodMutator) Handle(ctx context.Context, req admission.Request) admissio return admission.Allowed("profile not ready") } - return m.mutate(ctx, &pod, profile) + return m.mutate(ctx, &pod, profile, resolved) } // mutate builds the patched pod and returns a JSON-patch admission response. -func (m *PodMutator) mutate(ctx context.Context, pod *corev1.Pod, profile *ballastv1.WorkloadProfile) admission.Response { +func (m *PodMutator) mutate(ctx context.Context, pod *corev1.Pod, profile *ballastv1.WorkloadProfile, resolved *policy.ResolvedPolicy) admission.Response { log := ctrl.Log.WithName("webhook") modifiedPod := pod.DeepCopy() @@ -112,7 +124,7 @@ func (m *PodMutator) mutate(ctx context.Context, pod *corev1.Pod, profile *balla if modifiedPod.Annotations == nil { modifiedPod.Annotations = make(map[string]string) } - policyRef := m.stampPolicyRef(ctx, pod, modifiedPod) + policyRef := stampPolicyRef(modifiedPod, resolved) applied := applyRecommendations(modifiedPod, profile) log.Info("applying resource recommendations", "dry_run", m.dryRunApply, "containers", applied) @@ -140,34 +152,26 @@ func (m *PodMutator) mutate(ctx context.Context, pod *corev1.Pod, profile *balla return patchResponse(pod, modifiedPod) } -// stampPolicyRef resolves the active policy and stamps its name onto modifiedPod, -// returning the stamped ref ("" when no policy resolved). Policy resolution -// failures are non-fatal — the admission proceeds without a policy-ref. -func (m *PodMutator) stampPolicyRef(ctx context.Context, pod, modifiedPod *corev1.Pod) string { - resolved, err := m.resolver.Resolve(ctx, policy.Input{ - Namespace: pod.Namespace, - OwnerKind: directOwnerKind(pod), - Labels: pod.Labels, - Annotations: pod.Annotations, - }) - if err != nil { // coverage:ignore - transient API error listing policy objects - ctrl.Log.WithName("webhook").V(1).Info("policy resolution error, skipping policy-ref", "err", err) - return "" - } +// stampPolicyRef records the governing policy on modifiedPod and returns the +// stamped value ("" when no policy matched). +// +// This is admission-time resolution; the workloadwatcher refreshes the annotation +// afterwards, because the policy set can change while the pod runs. +func stampPolicyRef(modifiedPod *corev1.Pod, resolved *policy.ResolvedPolicy) string { if resolved == nil { return "" } - ref := resolved.Name - if resolved.Namespaced { - ref = pod.Namespace + "/" + resolved.Name - } + ref := policy.PodAnnotationValue(resolved.Ref) modifiedPod.Annotations[validation.AnnotationPolicyRef] = ref return ref } -// lookupProfile resolves the WorkloadProfile for the given pod. -// Returns (nil, nil) when no profile exists yet — normal for new workloads. -func (m *PodMutator) lookupProfile(ctx context.Context, pod *corev1.Pod) (*ballastv1.WorkloadProfile, error) { +// lookupProfile resolves the WorkloadProfile holding this pod's recommendations, +// which is identified by the pod's label tuple together with the policy governing +// it. Returns (nil, nil) when no such profile exists yet — normal for new +// workloads, and also the case immediately after a policy change, until the +// workloadwatcher creates the profile for the new policy. +func (m *PodMutator) lookupProfile(ctx context.Context, pod *corev1.Pod, resolved *policy.ResolvedPolicy) (*ballastv1.WorkloadProfile, error) { var cfg ballastv1.BallastConfig if err := m.client.Get(ctx, types.NamespacedName{Name: killswitch.BallastConfigName}, &cfg); err != nil { return nil, fmt.Errorf("getting BallastConfig: %w", err) @@ -175,8 +179,14 @@ func (m *PodMutator) lookupProfile(ctx context.Context, pod *corev1.Pod) (*balla tupleLabels := workloadwatcher.ExtractTupleLabels(pod.Labels, cfg.Spec.IdentityLabels) + discriminator := naming.NoPolicy + if resolved != nil { + discriminator = naming.PolicyDiscriminator(resolved.Ref.Kind, resolved.Ref.Namespace, resolved.Ref.Name) + } + var wp ballastv1.WorkloadProfile - if err := m.client.Get(ctx, types.NamespacedName{Name: workloadwatcher.ProfileName(tupleLabels, cfg.Spec.IdentityLabels)}, &wp); err != nil { + name := naming.ProfileName(tupleLabels, cfg.Spec.IdentityLabels, discriminator) + if err := m.client.Get(ctx, types.NamespacedName{Name: name}, &wp); err != nil { return nil, nil //nolint:nilerr // not-found is expected for new workloads } @@ -254,14 +264,3 @@ func patchResponse(original, modified *corev1.Pod) admission.Response { } return admission.PatchResponseFromRaw(originalJSON, modifiedJSON) } - -// directOwnerKind returns the Kind of the first controller ownerReference on the pod, -// or empty string if none is set. -func directOwnerKind(pod *corev1.Pod) string { - for _, ref := range pod.OwnerReferences { - if ref.Controller != nil && *ref.Controller { - return ref.Kind - } - } - return "" -} diff --git a/internal/webhook/pod_mutator_test.go b/internal/webhook/pod_mutator_test.go index bcdd342..8396bdc 100644 --- a/internal/webhook/pod_mutator_test.go +++ b/internal/webhook/pod_mutator_test.go @@ -32,6 +32,7 @@ import ( ballastv1 "github.com/tight-line/ballast/api/v1" "github.com/tight-line/ballast/internal/killswitch" "github.com/tight-line/ballast/internal/metrics" + "github.com/tight-line/ballast/internal/naming" "github.com/tight-line/ballast/internal/validation" "github.com/tight-line/ballast/internal/webhook" ) @@ -83,11 +84,35 @@ func defaultBallastConfig() *ballastv1.BallastConfig { } } -// readyProfile returns a WorkloadProfile named "web" (matching pod label app=web) -// with cpu+memory recommendations and meetsThreshold=true. -func readyProfile() *ballastv1.WorkloadProfile { +// webProfileName is the profile name the webhook derives for a pod labeled +// app=web under the given policy, or under no policy when ref is nil. A profile's +// identity includes its governing policy, so a fixture must carry the name the +// webhook will actually look up: naming it "web" would simply not be found. +func webProfileName(ref *ballastv1.PolicyReference) string { + discriminator := naming.NoPolicy + if ref != nil { + discriminator = naming.PolicyDiscriminator(ref.Kind, ref.Namespace, ref.Name) + } + return naming.ProfileName(map[string]string{"app": "web"}, []string{"app"}, discriminator) +} + +func clusterPolicyRef(name string) *ballastv1.PolicyReference { + return &ballastv1.PolicyReference{Kind: ballastv1.KindClusterResourcePolicy, Name: name} +} + +func namespacedPolicyRef(namespace, name string) *ballastv1.PolicyReference { + return &ballastv1.PolicyReference{ + Kind: ballastv1.KindResourcePolicy, + Namespace: namespace, + Name: name, + } +} + +// readyProfile returns a WorkloadProfile for pod label app=web under policy ref +// (nil for no policy), with cpu+memory recommendations and meetsThreshold=true. +func readyProfile(ref *ballastv1.PolicyReference) *ballastv1.WorkloadProfile { return &ballastv1.WorkloadProfile{ - ObjectMeta: metav1.ObjectMeta{Name: "web"}, + ObjectMeta: metav1.ObjectMeta{Name: webProfileName(ref)}, Status: ballastv1.WorkloadProfileStatus{ MeetsThreshold: true, Containers: []ballastv1.ContainerProfile{ @@ -105,7 +130,7 @@ func readyProfile() *ballastv1.WorkloadProfile { func notReadyProfile() *ballastv1.WorkloadProfile { return &ballastv1.WorkloadProfile{ - ObjectMeta: metav1.ObjectMeta{Name: "web"}, + ObjectMeta: metav1.ObjectMeta{Name: webProfileName(nil)}, Status: ballastv1.WorkloadProfileStatus{MeetsThreshold: false}, } } @@ -220,7 +245,7 @@ func policyRefValue(resp admission.Response) string { // -- unit tests (fake client) -- func TestPodMutator_KillSwitch(t *testing.T) { - fc := newFakeClient(defaultBallastConfig(), readyProfile()) + fc := newFakeClient(defaultBallastConfig(), readyProfile(nil)) m := webhook.NewPodMutator(fc, activeKS(t), false, nil) resp := m.Handle(context.Background(), makeRequest(testPod("p", validation.ModeApply))) @@ -259,7 +284,7 @@ func TestPodMutator_NoApplyMode(t *testing.T) { } func TestPodMutator_DryRunApply(t *testing.T) { - fc := newFakeClient(defaultBallastConfig(), readyProfile()) + fc := newFakeClient(defaultBallastConfig(), readyProfile(nil)) m := webhook.NewPodMutator(fc, inactiveKS(t), true /* dryRunApply */, nil) resp := m.Handle(context.Background(), makeRequest(testPod("p", validation.ModeApply))) @@ -273,7 +298,7 @@ func TestPodMutator_DryRunApply(t *testing.T) { } func TestPodMutator_SuccessfulPatch(t *testing.T) { - fc := newFakeClient(defaultBallastConfig(), readyProfile()) + fc := newFakeClient(defaultBallastConfig(), readyProfile(nil)) m := webhook.NewPodMutator(fc, inactiveKS(t), false, nil) resp := m.Handle(context.Background(), makeRequest(testPod("p", validation.ModeApply))) @@ -329,7 +354,7 @@ func TestPodMutator_Autoresize_BelowThreshold(t *testing.T) { } func TestPodMutator_Autoresize_AboveThreshold(t *testing.T) { - fc := newFakeClient(defaultBallastConfig(), readyProfile()) + fc := newFakeClient(defaultBallastConfig(), readyProfile(nil)) m := webhook.NewPodMutator(fc, inactiveKS(t), false, nil) resp := m.Handle(context.Background(), makeRequest(testPod("p", validation.ModeResize))) @@ -398,7 +423,7 @@ func TestPodMutator_PolicyRefStamped(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "default-policy"}, Spec: ballastv1.ClusterResourcePolicySpec{}, } - fc := newFakeClient(defaultBallastConfig(), readyProfile(), policy) + fc := newFakeClient(defaultBallastConfig(), readyProfile(clusterPolicyRef("default-policy")), policy) m := webhook.NewPodMutator(fc, inactiveKS(t), false, nil) resp := m.Handle(context.Background(), makeRequest(testPod("p", validation.ModeApply))) @@ -418,7 +443,7 @@ func TestPodMutator_PolicyRefNamespaced(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "team-policy", Namespace: "default"}, Spec: ballastv1.ResourcePolicySpec{}, } - fc := newFakeClient(defaultBallastConfig(), readyProfile(), policy) + fc := newFakeClient(defaultBallastConfig(), readyProfile(namespacedPolicyRef("default", "team-policy")), policy) m := webhook.NewPodMutator(fc, inactiveKS(t), false, nil) resp := m.Handle(context.Background(), makeRequest(testPod("p", validation.ModeApply))) @@ -432,7 +457,7 @@ func TestPodMutator_PolicyRefNamespaced(t *testing.T) { } func TestPodMutator_UnmatchedContainer(t *testing.T) { - fc := newFakeClient(defaultBallastConfig(), readyProfile()) + fc := newFakeClient(defaultBallastConfig(), readyProfile(nil)) m := webhook.NewPodMutator(fc, inactiveKS(t), false, nil) // pod has an extra "sidecar" container not in the profile @@ -467,7 +492,7 @@ func TestPodMutator_ApplyAppliedMetric(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "default-policy"}, Spec: ballastv1.ClusterResourcePolicySpec{}, } - fc := newFakeClient(defaultBallastConfig(), readyProfile(), policy) + fc := newFakeClient(defaultBallastConfig(), readyProfile(clusterPolicyRef("default-policy")), policy) rec, reg := newMetricsRecorder(t) m := webhook.NewPodMutator(fc, inactiveKS(t), false, rec) @@ -480,7 +505,8 @@ func TestPodMutator_ApplyAppliedMetric(t *testing.T) { if got != 1 { t.Fatalf("ballast_apply_applied_total = %v, want 1", got) } - if labels["profile"] != "web" || labels["policy"] != "default-policy" || labels["namespace"] != "default" { + if labels["profile"] != webProfileName(clusterPolicyRef("default-policy")) || + labels["policy"] != "default-policy" || labels["namespace"] != "default" { t.Errorf("profile/policy/namespace attrs = %q/%q/%q", labels["profile"], labels["policy"], labels["namespace"]) } @@ -493,7 +519,7 @@ func TestPodMutator_ApplyAppliedMetric(t *testing.T) { func TestPodMutator_AppliesRestartableInitSidecar(t *testing.T) { restartAlways := corev1.ContainerRestartPolicyAlways profile := &ballastv1.WorkloadProfile{ - ObjectMeta: metav1.ObjectMeta{Name: "web"}, + ObjectMeta: metav1.ObjectMeta{Name: webProfileName(clusterPolicyRef("default-policy"))}, Status: ballastv1.WorkloadProfileStatus{ MeetsThreshold: true, Containers: []ballastv1.ContainerProfile{{ @@ -532,7 +558,7 @@ func TestPodMutator_AppliesRestartableInitSidecar(t *testing.T) { // patch touches no container resources (no container matches the profile) reports // result=mutated but does not record ballast.apply.applied. func TestPodMutator_ApplyAppliedMetric_AnnotationOnlyMutation(t *testing.T) { - fc := newFakeClient(defaultBallastConfig(), readyProfile()) + fc := newFakeClient(defaultBallastConfig(), readyProfile(nil)) rec, reg := newMetricsRecorder(t) m := webhook.NewPodMutator(fc, inactiveKS(t), false, rec) @@ -580,7 +606,7 @@ func TestPodMutator_ApplySkippedMetric_NotReady(t *testing.T) { t.Fatalf("ballast_apply_skipped_total = %v (reason=%q), want 1 with reason=not_ready", got, labels["reason"]) } - if labels["profile"] != "web" || labels["namespace"] != "default" { + if labels["profile"] != webProfileName(nil) || labels["namespace"] != "default" { t.Errorf("profile/namespace attrs = %q/%q", labels["profile"], labels["namespace"]) } } @@ -611,7 +637,7 @@ func TestPodMutator_ApplySkippedMetric_NoProfile(t *testing.T) { // TestPodMutator_ApplySkippedMetric_DryRun asserts a suppressed apply that would // have changed resources records reason=dry_run and no apply.applied. func TestPodMutator_ApplySkippedMetric_DryRun(t *testing.T) { - fc := newFakeClient(defaultBallastConfig(), readyProfile()) + fc := newFakeClient(defaultBallastConfig(), readyProfile(nil)) rec, reg := newMetricsRecorder(t) m := webhook.NewPodMutator(fc, inactiveKS(t), true /* dryRunApply */, rec) @@ -632,7 +658,7 @@ func TestPodMutator_ApplySkippedMetric_DryRun(t *testing.T) { func TestPodMutator_EmptyRecommendationField(t *testing.T) { profile := &ballastv1.WorkloadProfile{ - ObjectMeta: metav1.ObjectMeta{Name: "web"}, + ObjectMeta: metav1.ObjectMeta{Name: webProfileName(nil)}, Status: ballastv1.WorkloadProfileStatus{ MeetsThreshold: true, Containers: []ballastv1.ContainerProfile{ @@ -660,7 +686,7 @@ func TestPodMutator_EmptyRecommendationField(t *testing.T) { func TestPodMutator_InvalidQuantity(t *testing.T) { profile := &ballastv1.WorkloadProfile{ - ObjectMeta: metav1.ObjectMeta{Name: "web"}, + ObjectMeta: metav1.ObjectMeta{Name: webProfileName(nil)}, Status: ballastv1.WorkloadProfileStatus{ MeetsThreshold: true, Containers: []ballastv1.ContainerProfile{ @@ -685,7 +711,7 @@ func TestPodMutator_InvalidQuantity(t *testing.T) { } func TestPodMutator_OwnerReference(t *testing.T) { - fc := newFakeClient(defaultBallastConfig(), readyProfile()) + fc := newFakeClient(defaultBallastConfig(), readyProfile(nil)) m := webhook.NewPodMutator(fc, inactiveKS(t), false, nil) isController := true From f2464d75e7715599a4d623504b6c8ebc9b2abd10 Mon Sep 17 00:00:00 2001 From: Nick Marden Date: Wed, 29 Jul 2026 14:42:42 -0400 Subject: [PATCH 2/2] README: correct the profile-name examples for policy-driven identity The verification snippet named a profile 'billing--api--prod', which no longer resolves now that a profile's name carries a token for the policy governing it; following it verbatim gets a NotFound. Show listing the profiles first instead of hardcoding a name, so the snippet survives the next naming change too. Also qualified the cluster-scoped pooling claim: sharing an identity tuple is no longer sufficient to share a profile, since pods resolving to different policies are measured separately. --- README.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 7711d6f..3cdbd5c 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,8 @@ Ballast groups pods into `WorkloadProfile` objects by matching a configurable se **WorkloadProfiles are cluster-scoped.** Every pod in every namespace that shares the same label values for the identity keys feeds measurements into the same profile. This is intentional: forty dev namespaces all running the same billing app produce one well-sampled `WorkloadProfile`, not forty thin ones. +The one thing that splits an identity tuple is the policy governing it. A profile holds a single set of recommendations, and the policy is what decides how they are measured and sized, so pods that resolve to different policies get different profiles (see [policy precedence](#default-metricssource-and-clusterresourcepolicy)). With only the default cluster-wide policy in place, nothing splits and the tuple is the whole story. + ### Default: `name` + `component` ```yaml @@ -232,13 +234,15 @@ Two flags make rolling out enrollment across a large cluster quick, in two stage ## Verifying a WorkloadProfile -Once a pod carrying the `ballast.tightlinesoftware.com/mode` label is running, Ballast creates a `WorkloadProfile` for its identity tuple. Check it with: +Once a pod carrying the `ballast.tightlinesoftware.com/mode` label is running, Ballast creates a `WorkloadProfile` for its identity tuple and the policy governing it. Check it with: ```bash kubectl get workloadprofiles -kubectl describe workloadprofile billing--api--prod +kubectl describe workloadprofile ``` +A profile's name is its identity-tuple values joined with `--`, followed by a token identifying its policy (`billing--api--prod--default-a1b2c3d4`), so list the profiles first rather than assuming a name. The `POLICY` column shows which policy governs each one. + The profile status shows accumulated usage statistics and recommendations once the readiness threshold is met (default: 250 samples collected over 24 hours). CPU, memory, and ephemeral storage are all tracked and sized: ```yaml