Skip to content

feat: [FEP-0001] implement the placement policy controller - #820

Draft
Yetkin Timocin (ytimocin) wants to merge 13 commits into
mainfrom
feat/fep0001-placement-policy-controllers
Draft

feat: [FEP-0001] implement the placement policy controller#820
Yetkin Timocin (ytimocin) wants to merge 13 commits into
mainfrom
feat/fep0001-placement-policy-controllers

Conversation

@ytimocin

@ytimocin Yetkin Timocin (ytimocin) commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Description

Closes #786.

Implements the placement policy controller for FEP-0001: it reconciles PlacementPolicy and ClusterPlacementPolicy, resolves their cluster selectors against the member cluster inventory, reports scheduling status, and manages the ClusterClaim lifecycle for selectors it cannot fulfill.

The whole surface is behind a new --enable-placement-policy-apis flag (alpha, off by default), so nothing changes for existing users.

Note

Branched off #803 (the ClusterClaim rename) rather than main, since it touches those types throughout. Please merge #803 first; I will rebase.

What works

  • Selector evaluationmatchLabels, matchLabelExpressions (upstream label selector semantics, including absence handling), and matchClusterPropertyExpressions with both string and numeric (quantity) operators against reported properties and resource usage; count/minCount resolution including the All sentinel. Terms are ORed error-tolerantly: a term that cannot be evaluated does not veto a later term that matches on its own, and the error surfaces only when nothing matched.
  • Fulfillment counts only schedulable clusters — a cluster satisfies a selector only once it passes the scheduler's own eligibility gate (clustereligibilitychecker: member agent online, heartbeating, joined) and its taints are tolerated. A provisioner-created cluster that has registered but not joined does not count, and does not withdraw a claim. This is the join-window issue raised on [FEP-0001] Verify the cluster claim workflow #791.
  • Status — the Scheduled condition uses the API's existing reason constants (FoundAllRequiredClusters / FailedToFindSomeRequiredClusters), plus desiredClusters, scheduledClusters, and activeClusterClaims.
  • Cluster claims — added for unfulfilled selectors that opt into AddClusterClaim, withdrawn once the selector is satisfied, with the freshness marker stamped at creation and advanced as the fleet grows.

Design decisions worth reviewing

  • Claim naming: deterministic and namespace-qualified (selector index + hash of the policy's namespaced name). Claims are cluster-scoped while policy names are only unique per namespace, so a naive scheme collides; determinism also makes creation get-or-create, so a restarted reconciler converges instead of duplicating claims.
  • Claim cleanup: a namespaced policy cannot own a cluster-scoped claim — garbage collection rejects cross-scope owners, though admission accepts them, which makes ownerReferences an inviting trap here. Ownership therefore uses labels (now exported API constants, since provisioners and kubectl users depend on them) and cleanup uses a policy finalizer, added before the first claim exists and released only after an uncached read confirms no claims remain — a read that goes by each claim's immutable spec.placementPolicyRef rather than the labels, since a label-stripped claim vanishing from a label-selected list would be orphaned permanently.
  • Terminating claims: a claim held by a provisioner finalizer keeps its budget slot and its name, so a slow teardown can starve the slot but can never cause double-provisioning. A claim withdrawn within a reconcile round also keeps its slot until it is observed gone, so a differently-named claim for another selector cannot stand beside it; and a claim round that fails partway does not publish its partial count.
  • Watch volume: member cluster events pass through a projection predicate that ignores heartbeat and observation timestamps; time-driven eligibility changes are covered by periodic requeues instead.

pkg/controllers/placementpolicy/IMPLEMENTATION-NOTES.md records these decisions, the deliberate divergences from the FEP text (all in the direction of the #791 findings), and the API gaps found while implementing — including two pinned as KNOWN GAP specs:

  1. count: 0 and negative integers bypass CRD validation (the XIntOrString pattern only constrains the string form), so the controller rejects them at evaluation time. feat: [FEP-0001] support annotation-based placement #829 now adds exactly that CEL rule — whichever PR merges second flips the KNOWN GAP spec pinning this. (Same hand-off for the startup flags: annotation-based placement in feat: [FEP-0001] support annotation-based placement #829 is useless without --enable-placement-policy-apis here, so the second PR to merge should also add the cross-flag startup guard.)
  2. Numeric operators (Gt, Lt, …) are accepted in matchLabelExpressions at admission; the API defers misuse to the scheduling phase.

Not in scope (deferred, and noted in the implementation notes): the FEP's fleet-wide concurrency limit and the eligible-keys allowlist, both prose-only config surfaces today; per-selector cluster ranking, which belongs with #788.

Found by manual testing on a live cluster

Two ways a policy could be silently starved of its claim, both from one root cause — a name legal for a Kubernetes object is not always legal where the controller put it:

  • A policy name longer than 63 bytes overflowed the ownership label value, so claim creation failed API validation and the reconciler hot-looped while the policy reported no claims. Long names now contribute a prefix plus a hash to the label; the authoritative identity is read from spec.placementPolicyRef (required, immutable, unbounded), which the watch mappers now use.
  • Truncating a long name for the generated claim name could land on a separator and produce a name the API server rejects. Both truncation points now trim trailing separators.

The name hash is 64 bits of SHA-256 rather than a 32-bit FNV sum, since a collision would let one policy select — and therefore withdraw — another's claim. Both cases have regression tests at the truncation boundaries.

Also added from review: metrics for scheduling status and outstanding claim count (dropped when the policy is deleted), and documentation that disabling the feature flag while claims are outstanding leaves those policies undeletable until it is re-enabled.

Boy Scout fixes riding along

  • pkg/scheduler/framework/plugins/clusteraffinity: resource property names whose resource part contains a dash (e.g. allocatable-ephemeral-storage) were rejected by a two-segment split; now parsed with strings.Cut, with a regression test.
  • The hub agent chart's placement.kubefleet.dev CRDs are now symlinks into config/crd/bases like every sibling CRD (they were copies that would silently diverge on the next make manifests), and crd-verify no longer excludes them now that the implementation ships.

How has this code been tested

  • make reviewable (fmt, vet, lint, staticcheck, crd-verify, go mod tidy).
  • Unit tests: selector matching, count resolution, structural validation, aggregation, condition mapping, toleration matching, claim naming and selection.
  • Integration (envtest): scheduling status across the eligibility join window, taints and tolerations, minCount, count: All, the cluster-scoped API, and the full claim lifecycle — creation, budget capping, KeepSearching, freshness refresh, policy-deletion cleanup, selector-change replacement, and both provisioner-finalizer paths.
  • E2E: added test/e2e/placement_policy_test.go for both the namespaced and cluster-scoped APIs; test/e2e/setup.sh enables the flag. These have not been executed anywhere yet — the local run did not get past building the agent images, so CI will be their first real run.
  • Manual testing against a kind hub with the flag on, driving the controller directly: the join window (a matching cluster that has not joined does not satisfy a selector or withdraw a claim), claim add/withdraw, provisioner-finalizer teardown, both known API validation gaps, malformed cluster property data isolated to the offending cluster, out-of-band claim deletion, finalizer stripping, heartbeat staleness caught by the periodic requeue, and claim naming across namespaces and scopes. This is where the two naming bugs above came from.

Special notes for your reviewer

This grew out of #791 (verifying the claim workflow) and the spike in #811. Where the FEP text and the verification findings disagree, the implementation follows the findings and documents why:

FEP as written Implemented Why
Withdraw when a matching cluster joins Withdraw when a schedulable cluster matches Otherwise the claim disappears minutes before the cluster is usable, and the placement is left unscheduled with nothing explaining why
Withdraw regardless of claim status, no finalizer contract Provisioner finalizers honored; Terminating claims keep their slot Hard-deleting mid-provision has no defined owner for the resulting cluster
No naming scheme Deterministic, namespace-qualified Cluster-scoped claims from same-named policies in different namespaces would collide
No cleanup contract Policy finalizer + ownership labels Cross-scope owner references cannot work; policy deletion would orphan claims forever

Happy to split any of this out if it is easier to review in pieces — the commits are self-contained.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds the alpha, feature-gated FEP-0001 placement policy controller, including selector evaluation, scheduling status, and ClusterClaim lifecycle management.

Changes:

  • Implements namespaced and cluster-scoped policy reconciliation with claims and metrics.
  • Adds unit, integration, and E2E coverage.
  • Wires APIs, CRDs, RBAC, Helm configuration, and resource-property parsing fixes.

Reviewed changes

Copilot reviewed 38 out of 39 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/e2e/setup.sh Enables the feature in E2E.
test/e2e/setup_test.go Registers policy APIs.
test/e2e/placement_policy_test.go Adds policy E2E scenarios.
pkg/scheduler/framework/plugins/clusteraffinity/types.go Supports dashed resource names.
pkg/scheduler/framework/plugins/clusteraffinity/types_test.go Tests dashed resource names.
pkg/metrics/hub/metrics.go Defines policy metrics.
pkg/controllers/placementpolicy/watch.go Implements watch predicates and mappings.
pkg/controllers/placementpolicy/watch_test.go Tests claim watch mapping.
pkg/controllers/placementpolicy/suite_test.go Configures the integration suite.
pkg/controllers/placementpolicy/selector.go Implements selector evaluation.
pkg/controllers/placementpolicy/selector_test.go Tests selector semantics.
pkg/controllers/placementpolicy/scheduling.go Computes scheduling outcomes.
pkg/controllers/placementpolicy/scheduling_test.go Tests scheduling behavior.
pkg/controllers/placementpolicy/policy.go Adapts both policy scopes.
pkg/controllers/placementpolicy/metrics.go Publishes and removes metrics.
pkg/controllers/placementpolicy/IMPLEMENTATION-NOTES.md Documents design decisions and gaps.
pkg/controllers/placementpolicy/controller.go Implements reconciliation and setup.
pkg/controllers/placementpolicy/controller_integration_test.go Tests controller scheduling.
pkg/controllers/placementpolicy/claims.go Manages ClusterClaim lifecycle.
pkg/controllers/placementpolicy/claims_test.go Tests claim naming and selection.
pkg/controllers/placementpolicy/claims_integration_test.go Tests claim lifecycle behavior.
Makefile Includes new CRDs in verification.
config/crd/bases/placement.kubefleet.dev_placementpolicies.yaml Updates policy claim schema.
config/crd/bases/placement.kubefleet.dev_clusterplacementpolicies.yaml Updates cluster-policy claim schema.
config/crd/bases/placement.kubefleet.dev_clusterclaims.yaml Defines renamed ClusterClaim CRD.
charts/hub-agent/templates/crds/placement.kubefleet.dev_placementpolicies.yaml Links the generated policy CRD.
charts/hub-agent/templates/crds/placement.kubefleet.dev_clusterplacementpolicies.yaml Links the generated cluster-policy CRD.
charts/hub-agent/templates/crds/placement.kubefleet.dev_clusterclaims.yaml Links the generated claim CRD.
cmd/hubagent/workload/setup.go Registers feature-gated controllers.
cmd/hubagent/options/featureflags.go Adds the feature flag.
cmd/hubagent/main.go Registers policy API types.
charts/hub-agent/values.yaml Adds the disabled-by-default value.
charts/hub-agent/templates/rbac.yaml Grants controller API permissions.
charts/hub-agent/templates/deployment.yaml Passes the feature flag.
charts/hub-agent/README.md Documents Helm configuration.
apis/kubefleet.dev/placement/v1alpha1/zz_generated.deepcopy.go Regenerates deepcopy methods.
apis/kubefleet.dev/placement/v1alpha1/placementpolicy_types.go Updates policy claim fields.
apis/kubefleet.dev/placement/v1alpha1/gvk_info.go Exports API kind constants.
apis/kubefleet.dev/placement/v1alpha1/clusterclaim_types.go Defines ClusterClaim types and labels.
Files not reviewed (1)
  • apis/kubefleet.dev/placement/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (2)

pkg/controllers/placementpolicy/claims.go:322

  • The ownership labels are not authoritative or immutable, so a claim can carry this policy's labels while its immutable placementPolicyRef names another policy. Returning it here lets this reconciler refresh or withdraw a foreign claim. Filter label-selected candidates by the exact policy reference before acting on them.
	claims := &kfplacementv1alpha1.ClusterClaimList{}
	if err := r.List(ctx, claims, claimOwnershipLabels(policy)); err != nil {
		klog.ErrorS(err, "Failed to list cluster claims for the policy", "placementPolicy", klog.KObj(policy.Unwrap()))
		return nil, err
	}
	return claims.Items, nil

pkg/controllers/placementpolicy/claims.go:276

  • This deletion path also trusts the selectable labels without checking the immutable reference. A foreign claim with copied or stale labels will be deleted when this policy is deleted. Verify placementPolicyRef before counting or withdrawing each candidate.
	for i := range claims.Items {
		claim := &claims.Items[i]

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkg/controllers/placementpolicy/selector.go Outdated
Comment thread pkg/controllers/placementpolicy/claims.go
Comment thread pkg/controllers/placementpolicy/controller.go Outdated
Comment thread pkg/controllers/placementpolicy/claims.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 38 out of 39 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • apis/kubefleet.dev/placement/v1alpha1/zz_generated.deepcopy.go: Generated file
Suppressed comments (4)

pkg/controllers/placementpolicy/claims.go:203

  • The creation loop does not account for outstanding, so a terminating claim with a different deterministic name does not actually consume the one-claim budget. For example, if selector 0's claim is held by a provisioner finalizer and selector 1 becomes the desired claim, this loop creates selector 1's claim while selector 0 is still terminating, allowing two provisioning operations despite maxConcurrentClaimsPerPolicy == 1. Count successfully issued deletions as outstanding for this round and stop creating once the budget is occupied.
	for _, w := range wanted {
		if _, still := wantedByName[w.name]; !still {
			continue
		}

pkg/controllers/placementpolicy/claims.go:40

  • This finalizer is only removed by cleanupClaims, which is called after deletion has already started. Once a policy has ever created a claim, normal reconciliation leaves the finalizer behind even after the last claim is gone. If the feature is then disabled, deleting that policy hangs despite having zero outstanding claims, which is broader than the flag and chart documentation warn about. Release the finalizer after an uncached read confirms no claims remain, or update the documented lifecycle to cover every policy that has ever held a claim.
	// claimCleanupFinalizer marks policies with outstanding cluster claims; deleting such a
	// policy first withdraws its claims.
	claimCleanupFinalizer = "placement.kubefleet.dev/claim-cleanup"

pkg/controllers/placementpolicy/metrics.go:43

  • Each generation/status/reason combination creates a distinct gauge series, but old series are retained until the policy is deleted. After policy updates, stale statuses continue to be exported and metric cardinality grows with every generation or reason change. Delete the existing series for this namespace/name before publishing the current status.
	hubmetrics.FleetPlacementPolicyStatusLastTimestampSeconds.
		WithLabelValues(
			namespace,
			name,
			strconv.FormatInt(policy.GetGeneration(), 10),
			scheduledCond.Type,
			string(scheduledCond.Status),
			scheduledCond.Reason,
		).SetToCurrentTime()

pkg/controllers/placementpolicy/selector.go:95

  • matchLabels is not validated here, and the CRD schema accepts arbitrary string keys and values. labels.SelectorFromSet does not return validation errors, so a policy containing an invalid label key/value is reported as merely unfulfilled and can emit a permanently unsatisfiable cluster claim instead of InvalidClusterSelectors. Validate each entry with Kubernetes label-key/value validation before evaluating clusters, as is already done for expression requirements.
	for i := range terms {
		term := &terms[i]
		for j := range term.MatchLabelExpressions {
			expr := &term.MatchLabelExpressions[j]

@ytimocin

Copy link
Copy Markdown
Collaborator Author

Live: policy self-heals when a satisfying cluster is removed

On a live cluster running the FEP-0001 stack, with a count: 2 ClusterPlacementPolicy satisfied by two eligible member clusters (scheduled=2/2, no claim):

  • Deleting one satisfying MemberCluster → the controller re-evaluates on the delete event → scheduled=1/2, and an AddClusterClaim selector re-issues a claim for the shortfall; a KeepSearching selector drops to 1/2 with no claim. The policy spec is untouched (only status moves).
  • The delete goes through a cleanup finalizer, so the cluster is Terminating briefly and is correctly excluded from the eligible count the moment deletionTimestamp is set.

Note for the claim-fulfillment contract (#831): the re-issued claim is named per (policy, selector-index) and carries no reference to the removed cluster, so a fulfiller can't distinguish "first shortfall" from "re-issued after a delete" — the re-issue-vs-remediate question. (Tested on the integrated #820 + #829 build.)

@ytimocin
Yetkin Timocin (ytimocin) force-pushed the feat/fep0001-placement-policy-controllers branch from 8be9151 to 659eb7a Compare August 19, 2026 21:10
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Adds the enable-placement-policy-apis feature flag (alpha, off by
default), scheme registration for the placement.kubefleet.dev/v1alpha1
API group, a reconciler skeleton for PlacementPolicy and
ClusterPlacementPolicy, CRD preflight checks in the hub agent setup,
and the hub-agent chart plumbing (flag value, deployment arg, RBAC
rules, CRD manifests).

Both reviewer-raised issues from the pre-commit pass are addressed:
the chart now ships the three placement.kubefleet.dev CRDs so enabling
the flag via helm cannot crash-loop the hub agent on the CRD preflight,
and fetchPolicy returns raw errors per the FetchPlacementFromNamespacedName
convention with not-found handling at the call site.

Part of #786.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
retrieveResourceUsageFrom split resource property names on every dash
and required exactly two segments, so any resource whose name itself
contains a dash (e.g. allocatable-ephemeral-storage) was rejected as
malformed. Only the first dash separates the capacity type; the rest is
the resource name. Adds a regression test for the dashed case.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
… scheduling status

Implements the first functional slice of the placement policy
controller (#786): cluster selector evaluation and scheduling status
for the PlacementPolicy and ClusterPlacementPolicy APIs.

- Selector engine: matchLabels/matchLabelExpressions via upstream label
  selector semantics, matchClusterPropertyExpressions with string and
  numeric (quantity) operators against reported cluster properties and
  resource usage; count/minCount resolution incl. the All sentinel;
  structural validation so invalid selectors surface even in an empty
  fleet, keeping spec errors separate from per-cluster data errors (a
  cluster reporting unevaluable data is skipped and logged, never fails
  the policy).
- Fulfillment counts only schedulable clusters: the scheduler's
  eligibility gate (clustereligibilitychecker) plus taint/toleration
  filtering, so a registered-but-not-joined cluster does not satisfy a
  selector.
- Status: Scheduled condition per the API's binary reason contract
  (FoundAllRequiredClusters/FailedToFindSomeRequiredClusters, plus a
  local InvalidClusterSelectors reason for unevaluable specs),
  desiredClusters/scheduledClusters as per-selector sums per the FEP's
  overlapping-selectors note, with count:All desired flooring at
  minCount so unfulfilled All selectors show a gap.
- Watch: member cluster events map to policies through a projection
  predicate that drops heartbeat/observation-timestamp-only updates;
  periodic requeues backstop time-driven eligibility transitions.
- policyObject adapter unifies both policy kinds behind one reconcile
  path; the interface exposes metadata only, with Unwrap() as the
  bridge for client calls.
- Tests: table-driven unit coverage for the engine and aggregation, an
  envtest suite for the reconcile flow (eligibility join-window, taints,
  minCount, count:All, cluster-scoped variant), and pinned KNOWN GAP
  specs for two API validation holes recorded in IMPLEMENTATION-NOTES.md
  (integer count:0 bypasses the IntOrString pattern; numeric operators
  in matchLabelExpressions pass admission).

Part of #786.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…policy controller

Adds cluster claim management to the placement policy controller (#786):
when a cluster selector cannot find its desired number of schedulable
clusters and opts into AddClusterClaim, the controller adds a
ClusterClaim carrying the selector terms for an external provisioner to
fulfill, and withdraws it once the selector is satisfied.

Design points worth calling out:

- Withdrawal is eligibility-gated: a provisioned cluster that has
  registered but whose member agent has not joined does not withdraw the
  claim, so the claim stays visible through the join window.
- Claim names are deterministic and namespace-qualified (selector index
  plus a hash of the policy's namespaced name), so cluster-scoped claims
  cannot collide across namespaces and a restarted reconciler converges
  via get-or-create instead of duplicating claims.
- A claim held in Terminating by a provisioner finalizer keeps its budget
  slot and its name, so a slow teardown can starve the slot but can never
  cause double-provisioning.
- Claims cannot be owner-referenced by a namespaced policy (garbage
  collection rejects cross-scope owners, yet admission accepts them), so
  ownership uses labels and cleanup uses a policy finalizer, added before
  the first claim exists and released only after an uncached read
  confirms no claims remain.
- The freshness marker is stamped at creation and advanced as the fleet
  grows; conflicts are treated as no-ops since provisioners co-write the
  claim status.

Covered by unit tests for naming and claim selection plus envtest specs
for creation, withdrawal, budget capping, KeepSearching, freshness
refresh, policy-deletion cleanup, selector-change replacement, and both
provisioner-finalizer paths. IMPLEMENTATION-NOTES.md records the
deliberate divergences from the FEP text and the API gaps found.

Part of #786.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
Exercises the alpha placement experience against the E2E fleet: a
selector the fleet satisfies reports fulfilled scheduling with the
expected cluster counts, an unfulfillable selector adds a cluster claim
carrying the selector terms and policy reference, and deleting the
policy withdraws the claim. The cluster-scoped ClusterPlacementPolicy
gets its own specs, since it has separate watch wiring and a distinct
claim reference path.

The hub agent chart's placement.kubefleet.dev CRDs become symlinks into
config/crd/bases, matching every sibling CRD, so regenerating manifests
cannot silently leave the chart copies behind; crd-verify no longer
excludes those CRDs now that the implementation ships them. The claim
ownership label keys move to the API package as exported constants:
they are the only association between a claim and its policy, so
provisioners and users querying claims depend on them.

Part of #786.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
Manual testing against a live cluster surfaced two ways a placement
policy could be silently starved of its cluster claim, both from the
same root cause: a name that is legal for a Kubernetes object is not
always legal where the controller put it.

- A policy name longer than 63 bytes overflowed the ownership label
  value, so creating the claim failed API validation and the reconciler
  retried forever while the policy reported no claims. Long names now
  contribute a prefix plus a hash of the full name to the label value,
  and the policy's authoritative identity is read from the claim's
  spec.placementPolicyRef, which the watch mappers now use instead of
  the labels.
- Truncating a long name for the generated claim name could land on a
  separator and produce a name the API server rejects. Both truncation
  points now trim trailing separators.

The name hash is 64 bits of SHA-256 rather than a 32-bit FNV sum: a
collision would let one policy select, and therefore withdraw, another
policy's claim.

Also adds placement policy metrics (scheduling status and outstanding
claim count, dropped when the policy is deleted), and documents that
disabling the feature flag while claims are outstanding leaves those
policies undeletable until it is enabled again.

Part of #786.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…troller

Four fixes, each with a test that fails on the previous behavior:

A selector term that cannot be evaluated no longer vetoes the whole
disjunction: a later term matching on its own proves the OR true, and
the error surfaces only when no term matched, since only then does the
broken term's answer matter.

A claim withdrawn within a reconcile round still occupies its budget
slot. A provisioner finalizer can hold the withdrawn claim past the
round, and a differently named claim created in the same pass would
have stood beside it, exceeding the concurrency budget that exists to
prevent double provisioning. Creations now wait for a freed slot.

A claim round that fails partway no longer publishes its partial count:
a failed round reports whatever it had counted when it stopped, which
would misstate the claims the rest of the round never reached. The last
completed round's count stands until a round completes.

Releasing the claim cleanup finalizer no longer trusts the ownership
labels, which are mutable: a label-stripped claim would vanish from a
label-selected list and be orphaned permanently the moment the
finalizer released. Every claim is listed uncached and ownership is
decided by the claim's immutable back-reference instead; the comparison
deliberately ignores the API version, which changes across a promotion
while kind, namespace, and name identify the same object throughout.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
The crd-verify check compares config/crd/bases against the CRDs the charts
install, and the charts install only the placement.kubefleet.dev CRDs whose
controllers exist: the policy and claim CRDs. The binding, resource-snapshot,
and work CRDs of the same group are API definitions with no controller yet, so
they are deliberately not packaged; verifying them made crd-verify -- which the
code-lint workflow runs -- fail. Exclude exactly those five until their
controllers ship, while still verifying the ones the charts do install.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
The count field is an int-or-string whose CRD pattern admits "All" and the
digit strings 1 through 999, and whose minCount CEL rule reads a digit string
as a number. A manifest with count: "3" therefore passes admission, but the
selector rejected every string other than "All", failing a placement the API
had accepted. Parse digit strings as their integer value instead.

The pattern constrains only the string form of the field, so an integer count
bypassed the intended limit: values such as 1000 or the maximum int32 were
accepted, and two maximum-valued selectors overflowed the aggregated desired
count to a negative number. Bound both forms to 999, matching the pattern.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
… the finalizer when none remain

Two review findings on the cluster claim lifecycle:

- The live reconcile listed a policy's claims by their ownership labels, which
  are mutable: a provisioner or user that removed a label made the claim vanish
  from the reconcile while spec.placementPolicyRef still named the policy, so it
  was never withdrawn once its selector was fulfilled, changed, or switched to
  KeepSearching, and a provisioner could keep acting on it. List by the
  immutable reference instead, as the cleanup path already does.

- A policy whose only claim was fulfilled and deleted kept the cleanup
  finalizer forever, since the reconcile returned as soon as nothing was wanted
  without revisiting it. Disabling the feature and then deleting such a
  claim-free policy would hang its deletion with no controller left to clear
  the finalizer, outside the documented outstanding-claims caveat. Release the
  finalizer once an authoritative, uncached claim count confirms none remain.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
…s count is met

A cluster claim asks for a single cluster: its spec carries no count and its
status names one provisioned cluster. A selector wanting several clusters was
served by keeping that one claim outstanding, so once a provisioner completed
it -- one cluster -- nothing asked for the next, and a count of three from an
empty fleet stalled at one.

Serve such a selector one claim at a time instead. When a claim's provisioner
has completed it and the cluster it provisioned is eligible while the selector
still has a deficit, the completed claim is withdrawn and a fresh one is issued
for the next cluster, until the count is met. The rotation is gated on the
provisioned cluster being eligible, so the next claim is never issued before
the current cluster is confirmed and a provisioner is never asked to provision
two clusters at once for one selector. This matches the enhancement proposal:
one claim outstanding per selector, withdrawn once its cluster is counted,
reissued while a deficit remains.

Document on the ProvisionedClusterName field that, because a completed claim is
now routinely replaced under the same deterministic name, a provisioner must
key a conflicting status-write retry on the claim's identity rather than its
name.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
… labels

Reject cluster selector terms whose matchLabels keys or values are not
valid Kubernetes labels: labels.SelectorFromSet performs no validation,
so such a term matches no cluster and the policy would misread it as an
ordinarily unfulfilled selector and answer by triggering an unnecessary
cluster claim. matchLabels keys are validated in sorted order so a term
with more than one invalid entry always surfaces the same error, keeping
the Scheduled condition message stable across reconciles.

Validate non-resource cluster property keys with the same slash-segmented
grammar the property provider uses (mirroring validateName), rather than
treating a present-but-malformed key as an always-absent property: a
DoesNotExist expression on an empty or malformed key would otherwise match
every cluster and could withdraw a still-needed claim. Multi-segment
provider names such as the Azure per-SKU capacity key remain accepted.

Restore the ownership labels on a still-wanted claim when a provisioner or
user strips or rewrites them. The controller tracks claims by the immutable
spec.placementPolicyRef, so its own reconcile is unaffected, but external
consumers that watch a policy's claims by label would otherwise lose sight
of the claim; the labels self-heal for the same reason the cleanup
finalizer is re-asserted while claims exist.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
… keys

A resource cluster property is a numeric quantity, but In and NotIn compare
the property's value as a string, and a quantity's canonical string form is
not unique -- 8000m and 8 are equal. A membership test against a resource
property therefore silently mismatches (In: ["8000m"] never matches a cluster
reporting "8"), which the policy reads as an unfulfilled selector and answers
with a spurious cluster claim. The API reserves In and NotIn for labels and
string-based properties, so validateTerms now rejects them on a resource key
instead of evaluating a comparison that cannot be trusted. Exists and
DoesNotExist stay valid on resource keys: a presence check is a map-key lookup,
immune to the canonicalization that breaks membership tests, and the evaluation
path already supports it.

Behavior change: a live PlacementPolicy using In or NotIn on a resource
property -- which happened to work when the value's string form matched exactly
-- now reports InvalidClusterSelectors, since validateTerms runs each reconcile.
This is a deliberate, blanket rejection rather than a value-by-value one.

Also validate the resource-name portion of a resource property key as a
qualified name: a key such as resources.kubernetes-fleet.io/allocatable-not a
resource previously passed the non-empty check yet names a resource no cluster
can report, so Exists would raise an unfulfillable claim and DoesNotExist would
match every cluster. Only the resource name is validated, not the whole key, so
a domain-qualified extended resource such as nvidia.com/gpu is still admitted --
a deliberate divergence from the whole-key check in validateName.

The v1alpha1 API type doc describes a two-way operator/property split while the
enforced (and correct) rule is three-way; reconciling that wording is left as a
separate change to avoid a shared-API / CRD edit in this focused fix.

Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
@ytimocin
Yetkin Timocin (ytimocin) force-pushed the feat/fep0001-placement-policy-controllers branch from e556aa9 to 8f0f19d Compare August 27, 2026 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEP-0001] Implement the Placement Policy controllers

2 participants