feat: add HCP scheduling PlacementController (phase 1) - #6651
feat: add HCP scheduling PlacementController (phase 1)#6651Chai-bot (redhat-chai-bot) wants to merge 4 commits into
Conversation
Introduce simple placement and provision-shard pinning for hosted control planes. A new scheduling controller resolves the initial management cluster for an HCP and records that intent on the ServiceProviderCluster spec, and cluster creation is gated on and pinned to that decision. - api: add ServiceProviderClusterSpec.ManagementClusterResourceID (scheduler intent; distinct from the observed Status field) and regenerate deepcopy. - placement: new PlacementController (single worker) that selects an eligible management cluster (SchedulingPolicy Schedulable AND Ready condition True) and bin-packs onto the fullest one via a pure, unit-tested selection function, writing Spec.ManagementClusterResourceID with conflict retry. - creation: ClusterPendingClusterServiceIDAssign waits until placement is resolved (Spec.ManagementClusterResourceID != nil) before assigning a pending Cluster Service ID. - creation: ClusterClusterServiceCreate looks up the placed management cluster and pins the Cluster Service provision shard by passing provision_shard_id (from Status.ClusterServiceProvisionShardID) to BuildCSCluster. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
…tions Addresses two review comments on the Phase 1 scheduling changes: - creation: ClusterClusterServiceCreate now resolves the placed management cluster via the fleet lister's Get(stampIdentifier) (derived from the management cluster resource ID's parent stamp segment, with a nil-parent guard) using cosmosstorageutils.IsNotFoundError for the not-found path, instead of listing all management clusters and scanning. Removes the now-unused lookupManagementCluster helper and its azcorearm/fleetapi imports. - placement: spell out abbreviated identifiers in the controller (serviceProviderClusterCRUD, cachedServiceProviderCluster, existingServiceProviderCluster, serviceProviderCluster, managementCluster). Test-local identifiers are left consistent with existing package tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces Phase 1 of explicit HCP scheduling in the backend by adding a new PlacementController that records scheduler intent on ServiceProviderCluster.Spec.ManagementClusterResourceID, then gating cluster creation on that placement and pinning Cluster Service provisioning to the selected management cluster’s provision shard.
Changes:
- Add
ServiceProviderCluster.Spec.ManagementClusterResourceIDas scheduler intent (distinct from observedStatus.ManagementClusterResourceID) and regenerate deepcopy. - Add
PlacementControllerto select an eligible management cluster and persist the placement decision with conflict retry. - Gate pending Cluster Service ID assignment on placement resolution, and pass the pinned
provision_shard_idas a required property when building the CS cluster.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ocm/client.go | Adds CSPropertyProvisionShardID constant for Cluster Service property key. |
| internal/api/coreapi/types_serviceprovider_cluster.go | Adds scheduler-intent Spec.ManagementClusterResourceID field with writer annotation. |
| internal/api/coreapi/zz_generated.deepcopy.go | Updates deepcopy generation for the new spec field. |
| backend/pkg/controllers/cluster/placement/placement_controller.go | New PlacementController implementation (eligibility filter + bin-pack selection + write with retries). |
| backend/pkg/controllers/cluster/placement/placement_controller_test.go | Unit tests for selection logic and SyncOnce. |
| backend/pkg/controllers/cluster/creation/cluster_pending_cluster_service_id_assign_controller.go | Gates Pending CS ID assignment on placement being resolved in SPC.Spec. |
| backend/pkg/controllers/cluster/creation/cluster_pending_cluster_service_id_assign_controller_test.go | Adds test cases covering the new placement precondition. |
| backend/pkg/controllers/cluster/creation/cluster_cluster_service_create_controller.go | Resolves pinned provision shard from placed management cluster and passes it to BuildCSCluster. |
| backend/pkg/controllers/cluster/creation/cluster_cluster_service_create_controller_test.go | Updates fixtures and asserts the pinned provision_shard_id is set on the built CS cluster. |
| backend/pkg/app/backend.go | Wires and starts the new placement controller (single worker) and passes MC lister into create controller. |
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/retest The single AI-generated. Review for accuracy. |
…transition; document new spec field Two follow-ups from the Copilot review on the Phase 1 scheduling changes: - placement: handle the transition to scheduler-owned placement. When a ServiceProviderCluster already has an observed placement on Status.ManagementClusterResourceID (written by ManagementClusterPlacementSync) but the new Spec intent is still nil, PlacementController now backfills Spec = Status instead of running a fresh selection, so an already-placed HCP is never moved. A fresh selection runs only when both Spec and Status are nil. selectManagementCluster also counts existing placements by Spec, falling back to Status when Spec is unset, so counts do not collapse to zero (and placement to the lexicographically-lowest MC) during the transition. Selection stays a pure function. Adds tabular tests for backfill, fresh-only-when-both-nil, and the count fallback. - docs: document ServiceProviderCluster.Spec.ManagementClusterResourceID in docs/cosmos-data-flow.md (writer: PlacementController; readers: ClusterPendingClusterServiceIDAssign gate and ClusterClusterServiceCreate shard pinning) and its relationship to the observed Status field; add a PlacementController controller section and the new reads to ClusterClusterServiceCreate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: redhat-chai-bot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test e2e-parallel Triage of the latest This is a Cluster Service / environment deletion-timeout flake — this PR (Phase 1) only changes the creation path (placement gating + provision-shard pinning) and does not touch deletion. Retesting once. If it recurs, this looks like a pre-existing CS deletion-timeout issue in the dev environment that a maintainer should weigh in on rather than something in this change. AI-generated. Review for accuracy. |
| CSPropertySingleReplica = "hosted_cluster_single_replica" | ||
| CSPropertySizeOverride = "hosted_cluster_size_override" | ||
| CSPropertyCPOImageOverride = "control_plane_operator_image" | ||
| CSPropertyProvisionShardID = "provision_shard_id" |
There was a problem hiding this comment.
lets not do this. #6496 introduced an ocm sdk updated that brings in ClusterBuilder.ProvisionShardID(string) so we dont have to pass provision shard IDs via the map mechanism.
There was a problem hiding this comment.
Done in 0eea67a6a. Switched to the SDK builder method — ClusterClusterServiceCreate now calls csClusterBuilder.ProvisionShardID(provisionShardID) (from the ocm-api-model update in #6496) before PostCluster, and BuildCSCluster is called with nil requiredProperties (as before this PR). Removed the CSPropertyProvisionShardID constant here in internal/ocm/client.go, updated the create-controller test to assert built.ProvisionShardID(), and adjusted the cosmos-data-flow.md wording. Build/tests/lint all green.
AI-generated. Review for accuracy.
…ID instead of properties map geoberle asked not to pass the provision shard through the requiredProperties map. PR Azure#6496 added ClusterBuilder.ProvisionShardID(string) to the vendored OCM SDK (arohcpv1alpha1, aliased from ocm-api-model/clientapi), so use it directly. - creation: ClusterClusterServiceCreate resolves the provision shard ID from the placed management cluster (unchanged MC lister lookup + nil-guards) and sets it on the *arohcpv1alpha1.ClusterBuilder via .ProvisionShardID(...) after BuildCSCluster (now called with nil requiredProperties, as before this PR), instead of building a {"provision_shard_id": ...} map. BuildCSCluster's signature is unchanged; the update-dispatch caller is untouched. - ocm: remove the now-unused CSPropertyProvisionShardID constant. - test: assert the pinned shard via built.ProvisionShardID() instead of Properties()["provision_shard_id"]. - docs: cosmos-data-flow.md now says the shard is pinned via ClusterBuilder.ProvisionShardID rather than a cluster property. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
backend/pkg/controllers/cluster/creation/cluster_pending_cluster_service_id_assign_controller.go:92
- The NotFound branch comment is misleading: PlacementController does not create ServiceProviderCluster documents (CreateServiceProviderCluster does). This can confuse future debugging of why PendingClusterServiceID assignment is waiting.
serviceProviderCluster, err := c.serviceProviderClusterLister.Get(ctx, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName)
if cosmosstorageutils.IsNotFoundError(err) {
// Placement has not produced a ServiceProviderCluster yet; wait.
return nil
|
/test e2e-parallel Triage of the 3 cluster-create tests — no-CNI/cilium (labeled This is a dev-environment control-plane bring-up problem, not a code issue. Dozens of other create tests passed (external-auth, authorized-CIDR, node-pool create/upgrade/labels-taints, KMS rotation, pull-secret, autoscaling), which proves the Phase 1 creation-gating and provision-shard pinning both work — a broken shard-pin or placement gate would have failed all creates, not three. 1 fleet test — Retesting once. If it recurs, this looks like dev-environment e2e instability (hosted-cluster availability + scheduling-doc population) that a maintainer should weigh in on rather than repeated retests. AI-generated. Review for accuracy. AI-generated. Review for accuracy. |
|
Chai-bot (@redhat-chai-bot): The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
The That's the CI harness/environment failing to run the suite, not a test-body failure in the placement/creation code. Combined with the prior run on this same commit (3 creates failing with HyperShift hosted-cluster I'm going to stop retesting to avoid churn. The PR is otherwise ready: all review threads are addressed and every non-e2e check is green ( AI-generated. Review for accuracy. AI-generated. Review for accuracy. |
Align the Spec/Status mismatch description with the actual behavior (log error + overwrite Spec with Status to self-heal) and replace the PR Azure#6651 reference with a self-contained implementation description.
Align the Spec/Status mismatch description with the actual behavior (log error + overwrite Spec with Status to self-heal) and replace the PR Azure#6651 reference with a self-contained implementation description.
Summary
Phase 1 of RP-backend-driven HCP scheduling, based on the design in #6649 (Phase 1: "Simple Placement + Provision Shard Pinning"). Introduces a
PlacementControllerthat records scheduler intent onServiceProviderCluster.Spec.ManagementClusterResourceID, gates cluster creation on that placement, and pins Cluster Service provisioning to the selected management cluster's provision shard.Phases 2–3 (capacity-aware scheduling,
HCPResourceRequirements,ManagementClusterScheduling,PendingAssignedClusters,ManagementClusterAssignmentSync) are out of scope for this PR.Changes
ManagementClusterResourceID *azcorearm.ResourceIDtoServiceProviderClusterSpecininternal/api/coreapi/types_serviceprovider_cluster.goas scheduler intent — distinct from the existing observedStatus.ManagementClusterResourceID. Regeneratedzz_generated.deepcopy.go.PlacementController(backend/pkg/controllers/cluster/placement/): single worker, cluster-keyed. Filters eligible management clusters (SchedulingPolicy == SchedulableandReady), bin-packs by current SPC count via a pure, unit-testedselectManagementClusterfunction, and writes the decision toSpec.ManagementClusterResourceIDwith conflict-retry.Spec == nilbutStatus.ManagementClusterResourceIDis already set (previously placed byManagementClusterPlacementSync), it backfillsSpec = Statusrather than re-scheduling. Fresh selection runs only when bothSpecandStatusare nil. Placement counts fall back toStatuswhenSpecis unset, so bin-pack counts don't collapse during migration.ClusterPendingClusterServiceIDAssign(backend/pkg/controllers/cluster/creation/): gated on placement being resolved (Spec.ManagementClusterResourceID != nil).ClusterClusterServiceCreate(backend/pkg/controllers/cluster/creation/): resolves the placed management cluster's provision shard (via the MC lister's.Get, stamp derived from the resource ID parent) and pins it on the Cluster Service builder using the OCM SDK methodClusterBuilder.ProvisionShardID(string)— the SDK update brought in by feat: update ocm sdk go v0.1.509 #6496.BuildCSClusteris called withnilrequired properties (no property-map /provision_shard_idkey mechanism).docs/cosmos-data-flow.mddocuments the newSpec.ManagementClusterResourceIDfield (writer: PlacementController; readers: the creation gate and shard pinning) and its relationship to the observedStatusfield, plus aPlacementControllersection.built.ProvisionShardID()).Validation
make verify-deepcopy,go build,make -C backend build,go test(internal/ocm, creation, placement, app, coreapi), andmake lint(0 issues) all pass locally.