feat(scheduling): backend-driven HCP placement - #6657
feat(scheduling): backend-driven HCP placement#6657Chai-bot (redhat-chai-bot) wants to merge 2 commits into
Conversation
…o management cluster scheduling doc
Phase 0 of HCP scheduling: pure data plumbing, no scheduling logic and no new
controllers.
- fleetapi: add ManagementClusterScheduling.Status.ReadyResourceIDs and
NotReadyResourceIDs ([]string), mirrored verbatim from the CapacityReport CR's
Status.HostedControlPlanes.{ReadyResourceIDs,NotReadyResourceIDs}. Regenerated
deepcopy.
- fleet: CapacityReportingController now mirrors those two lists onto the
ManagementClusterScheduling document using the same read-modify-write path that
already mirrors capacity/usage/requests into ObservedResources.
- test: assert the controller mirrors ReadyResourceIDs/NotReadyResourceIDs from
the CapacityReport source onto Status.
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 |
There was a problem hiding this comment.
Pull request overview
Implements backend-driven initial HCP placement by introducing scheduler intent on ServiceProviderCluster.Spec, mirroring observed readiness sets into the fleet scheduling document, and pinning Cluster Service provisioning to the scheduler-selected management cluster.
Changes:
- Extend fleet scheduling status to include ready/not-ready HCP resource ID sets plus pending placement reservations.
- Add backend placement + pending-reservation cleanup controllers, and make placement sync self-heal Spec to match Cluster Service observed reality.
- Gate Cluster Service ID assignment on placement intent and pin Cluster Service provision shard during cluster creation.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/api/fleetapi/zz_generated.deepcopy.go | Regenerated deepcopy for new scheduling status fields. |
| internal/api/fleetapi/types_management_cluster_scheduling.go | Adds Ready/NotReady resource ID sets and pending assignment reservations to scheduling status. |
| internal/api/coreapi/zz_generated.deepcopy.go | Regenerated deepcopy for new SPC spec field. |
| internal/api/coreapi/types_serviceprovider_cluster.go | Adds Spec.ManagementClusterResourceID (scheduler intent) to SPC. |
| fleet/pkg/controllers/capacityreporting/controller_test.go | Adds unit tests for mirroring ready/not-ready IDs and dropping observed pending reservations. |
| fleet/pkg/controllers/capacityreporting/capacity_reporting_controller.go | Mirrors ready/not-ready IDs into scheduling doc and cleans up observed pending reservations. |
| backend/pkg/controllers/cluster/placement/placement_controller.go | New controller to select an eligible MC by swift-NIC capacity and write placement intent + reserve capacity. |
| backend/pkg/controllers/cluster/placement/placement_controller_test.go | Unit tests for capacity math, candidate selection, and placement/backfill behavior. |
| backend/pkg/controllers/cluster/placement/pending_cleanup_controller.go | New controller to periodically remove stale pending placement reservations. |
| backend/pkg/controllers/cluster/placement/pending_cleanup_controller_test.go | Unit tests for stale pending reservation cleanup behavior. |
| backend/pkg/controllers/cluster/placement/management_cluster_placement_sync.go | Always reconciles observed placement from Cluster Service and self-heals Spec on drift. |
| backend/pkg/controllers/cluster/placement/management_cluster_placement_sync_test.go | Updates tests for always-reconcile + spec self-heal semantics. |
| backend/pkg/controllers/cluster/creation/cluster_pending_cluster_service_id_assign_controller.go | Gates PendingClusterServiceID assignment on placement intent being resolved in SPC.Spec. |
| backend/pkg/controllers/cluster/creation/cluster_pending_cluster_service_id_assign_controller_test.go | Tests new placement gating behavior for PendingClusterServiceID assignment. |
| backend/pkg/controllers/cluster/creation/cluster_cluster_service_create_controller.go | Resolves and pins provision shard based on scheduler-selected management cluster. |
| backend/pkg/controllers/cluster/creation/cluster_cluster_service_create_controller_test.go | Tests provision shard pinning derived from placed management cluster. |
| backend/pkg/app/backend.go | Wires up PlacementController and PendingCleanupController and passes MC lister into CS create controller. |
Files not reviewed (2)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- internal/api/fleetapi/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.
704f949 to
8574c9a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
8574c9a to
6515b5a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 18 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
6515b5a to
7df3771
Compare
7df3771 to
66438d6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 20 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
docs/cosmos-data-flow.md:998
- The PendingCleanupController docs table lists only
Spec.ManagementClusterResourceIDas read from ServiceProviderCluster, but the controller also readsStatus.ManagementClusterResourceIDand prefers it when set. The documented read set should include both fields to match the actual Cosmos contract.
| Read | `ServiceProviderCluster` (per pending entry) | <ul><li>`Spec.ManagementClusterResourceID`</li></ul> |
66438d6 to
716e07d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
716e07d to
cbac8ad
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
cbac8ad to
1b73f10
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 3 comments.
Files not reviewed (2)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
Suppressed comments (1)
backend/pkg/controllers/cluster/placement/placement_controller.go:293
- The PR description says placement “bin-packs by lowest available (fill MCs before spreading)”, but the implemented selection logic here is the opposite: it chooses the candidate with the highest available swift-NIC capacity (spread), with deterministic tie-breaking by lowest resource ID. Please align the PR description (or the code) so operators/reviewers don’t get the wrong placement behavior from the write-up.
// A candidate is a fit only when its available swift-NIC capacity is at least
// swiftNICsPerHCP. Among fitting candidates it returns the one with the HIGHEST
// available capacity (spread: place each new HCP on the emptiest management
// cluster so load is distributed evenly rather than concentrated). Ties are
// broken deterministically by the lowest resource ID string so the selection is
// stable and unit-testable.
|
/retest |
1b73f10 to
8b8c62c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 4 comments.
Files not reviewed (2)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
Gerd Oberlechner (geoberle)
changed the title
8b8c62c to
b7448d3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
| // Reset before re-emitting so the gauge never carries a stale series if the | ||
| // emitted label set ever changes; the counts below then set the current view. | ||
| c.placementState.Reset() | ||
| c.placementState.WithLabelValues(placementStateLabelPlaced).Set(float64(counts.placed)) | ||
| c.placementState.WithLabelValues(placementStateLabelUnplaced).Set(float64(counts.unplaced)) | ||
| c.placementState.WithLabelValues(placementStateLabelMismatch).Set(float64(counts.mismatch)) |
There was a problem hiding this comment.
Resolved by the refactor in 52e11dc: the standalone PlacementMetricsController (and its Reset()-every-tick GaugeVec) has been removed entirely. Placement state is now a placement label on the existing backend_cluster_info metric, which uses per-cluster DeletePartialMatch(resource_id) + re-Set rather than a global Reset(), so there's no window where all series vanish mid-scrape.
AI-generated. Review for accuracy.
AI-generated. Review for accuracy.
David Eads (deads2k)
left a comment
David Eads (deads2k)
left a comment
There was a problem hiding this comment.
needs heavy changes.
| go placementController.Run(ctx, 1) // single worker: capacity selection reads/reserves across MCs and must not race itself | ||
| go pendingCleanupController.Run(ctx, 1) // single worker: sweeps pending reservations per management cluster |
There was a problem hiding this comment.
design problem if this needed. Since we have optimistic write enforcement AND we write the pending and actual assignments to the same place as estimated usage AND we write estimated usage, we should be able to run in parallel without an issue and assignments to differnet management clusters should be non-conflicting.
| @@ -235,10 +239,18 @@ func (c *clusterClusterServiceCreateSyncer) csClustersMatchingClusterByAzureInfo | |||
| func (c *clusterClusterServiceCreateSyncer) createClusterServiceCluster(ctx context.Context, cluster *coreapi.HCPOpenShiftCluster, serviceProviderCluster *coreapi.ServiceProviderCluster, tenantID string) (*arohcpv1alpha1.Cluster, error) { | |||
There was a problem hiding this comment.
Before calling createClusterServiceCluster, ensure that serviceProviderCluster.Spec.ManagementClusterResourceID is non-nil and print a message if it is nil and then return nil.
| len(cluster.ServiceProviderProperties.ClusterServiceID.String()) == 0) | ||
| len(cluster.ServiceProviderProperties.ClusterServiceID.String()) == 0) && | ||
| serviceProviderCluster != nil && | ||
| serviceProviderCluster.Spec.ManagementClusterResourceID != nil |
There was a problem hiding this comment.
This is not required here. We can determine the future ID and unleash all sorts of Azure changes before we assign a management cluster.
There was a problem hiding this comment.
this is me being pesimistic. why would we unleash all sorts of azure changes before we know we get a slot to schedule
| } | ||
| if !c.needsWork(cachedSPC) { | ||
| logger.V(1).Info("ServiceProviderCluster already has ManagementClusterResourceID, skipping") | ||
| if cachedSPC.Status.ManagementClusterResourceID != nil { |
There was a problem hiding this comment.
this change looks strictly worse. Put needswork back.
| cachedCluster, err := c.clusterLister.Get(ctx, key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName) | ||
| // Status not yet observed in the cache: a write may follow, so fall back to a | ||
| // live Cosmos read to act on fresh data. | ||
| spcCRUD := c.cosmosClient.ServiceProviderClusters(key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName) |
There was a problem hiding this comment.
No, don't. Use a lister like we had it before.
There was a problem hiding this comment.
All the changes to this file look confused. What are you trying to do. You shouldn't have to touch this at all.
| } | ||
|
|
||
| // Live read to confirm work is still needed and to decide backfill vs. fresh. | ||
| spcCRUD := c.cosmosClient.ServiceProviderClusters(key.SubscriptionID, key.ResourceGroupName, key.HCPClusterName) |
There was a problem hiding this comment.
no live reads. Always use the cache
| return nil | ||
| } | ||
|
|
||
| // Rollout backfill: when the HCP was already placed by |
There was a problem hiding this comment.
every old record will reach here. Old records need ot have this filled in based on the cluster-service cluster.
| // ManagementClusterPlacementSync (Status set) but the new Spec intent is | ||
| // nil, adopt the observed placement rather than re-scheduling it. | ||
| if liveServiceProviderCluster.Status.ManagementClusterResourceID != nil { | ||
| if err := c.setSpecPlacement(ctx, key, liveServiceProviderCluster.Status.ManagementClusterResourceID); err != nil { |
There was a problem hiding this comment.
this is non-sensical
| // Record how long it took to place this HCP. This is a fresh placement (the | ||
| // rollout-backfill branch above returns early), so it reflects real | ||
| // scheduling latency, not adoption of an already-observed placement. | ||
| c.observePlacementDelay(ctx, key) |
There was a problem hiding this comment.
This smells wrong. Why wouldn't we do it like kube-state-metrics and scheduling pods, where the time is computed based on the time it takes for the NodeName to be specified?
…servations, provision shard pinning
Builds on phase 0 (ready/not-ready mirroring). Adds capacity-aware placement of
HostedControlPlanes onto management clusters based on SWIFT-NIC capacity, plus
provision-shard pinning at Cluster Service creation.
Per eligible management cluster, available swift NICs are computed as:
ScaleCeiling.Capacity[swift-nic]
- ObservedResources.Usage[swift-nic]
- (non-empty NotReadyResourceIDs)*3
- (non-nil PendingAssignedClusters)*3
and an HCP fits when available >= 3 (swiftNICsPerHCP; a conservative flat
per-HCP cost that never overbooks). Nil/empty list entries do not correspond to
a real HCP and never reserve capacity.
API:
- coreapi: add ServiceProviderCluster.Spec.ManagementClusterResourceID
(scheduler intent; drift from the observed Status placement is logged for
investigation but NOT auto-corrected), regenerate deepcopy.
- fleetapi: add ManagementClusterScheduling.Status.PendingAssignedClusters
(transient reservations), regenerate deepcopy.
Controllers:
- placement: new PlacementController (cluster-keyed, single worker). Backfills
Spec from an already-observed Status placement during rollout; otherwise runs
a pure, capacity-aware selection (fit filter + lowest-available bin-pack
tie-break), reserves capacity in the chosen MC's PendingAssignedClusters, then
records Spec. Transient write/conflict failures return an error so the
workqueue retries with backoff.
- placement: new PendingCleanupController (MC-keyed periodic sweep) removes stale
pending reservations. Effective placement favors the SPC's observed Status over
Spec; an entry is kept when it points here or is still resolving (nil) and
removed when it points elsewhere or the SPC is gone. SPCs are read via the
shared lister/cache; transient failures return an error for workqueue backoff.
- placement: ManagementClusterPlacementSync reconciles Status from Cluster
Service only while the observed placement is still unknown; once Status is set
it skips the CS lookup. Spec/Status drift is logged, never auto-corrected.
- fleet CapacityReportingController: in the same read-modify-write, drops
pending reservations that are now observed (Ready or NotReady).
- creation: ClusterPendingClusterServiceIDAssign gates on
Spec.ManagementClusterResourceID != nil; ClusterClusterServiceCreate pins the
provision shard via ClusterBuilder.ProvisionShardID resolved from the placed
management cluster.
- wire PlacementController and PendingCleanupController into backend (single
worker each; FleetDBClient + listers).
Tests: pure selection combos + lowest-available tie-break; capacity formula
edges incl. nil/empty entries; capacity-report pending cleanup; pending-cleanup
stale-entry matrix (Status-favored); rollout backfill; placement-sync
skip-when-Status-set and log-only drift; gated needsWork; provision shard
pinning.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b7448d3 to
52e11dc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated 2 comments.
Files not reviewed (2)
- internal/api/coreapi/zz_generated.deepcopy.go: Generated file
- internal/api/fleetapi/zz_generated.deepcopy.go: Generated file
| - alert: SchedulerPlacementMismatch | ||
| expr: | | ||
| max without (prometheus_replica) (backend_cluster_info{placement="mismatch", namespace="aro-hcp"}) > 0 | ||
| for: 15m | ||
| labels: |
| "resource_id": resourceID, | ||
| "subscription_id": subscriptionID, | ||
| "management_cluster_resource_id": managementClusterResourceID, | ||
| "placement": placementState(serviceProviderCluster), | ||
| }).Set(1.0) |
|
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. |
Implements the RP-backend-driven HCP scheduling design from #6649, delivered as one commit per phase.
Commit 1 — Phase 0: Scheduling Data Foundation (
scheduling-phase0-data-foundation.md)Pure data plumbing, no scheduling logic or new controllers.
ReadyResourceIDs []stringandNotReadyResourceIDs []stringtoManagementClusterScheduling.Status(fleetapi) + regenerated deepcopy.CapacityReportingControllernow mirrors both lists fromCapacityReport.Status.HostedControlPlanesinto the scheduling doc, in the same read-modify-write path that already mirrors capacity/usage/requests.Commit 2 — Phase 1: Swift-NIC Scheduling + Provision Shard Pinning (
scheduling-phase1-swift-nic-scheduling.md)Moves HCP placement into the backend using a swift-NIC capacity strategy (each HCP consumes 3 swift-NICs; conservative, never overbooks).
Capacity formula (per eligible MC):
(nil/empty not-ready and pending entries reserve no capacity)
ServiceProviderCluster.Spec.ManagementClusterResourceID(scheduler intent; distinct from CS-confirmedStatus) andManagementClusterScheduling.Status.PendingAssignedClusters+ regenerated deepcopy.PlacementController(cluster-keyed, single worker): eligible-MC filter (Schedulable+Ready), computes available swift-NICs per MC, rejects< 3, and spreads — selects the eligible MC with the highest available capacity (distribute load), tie-break by lowest resource ID. Pure, unit-tested selection. Rollout backfill:Specnil +Statusset → backfillSpec = Status(no reschedule). Two-step conflict-retry write: reserve inPendingAssignedClusters, then setSpec. Transient failures return an error and let the workqueue retry with backoff.CapacityReportingController: observation-based pending cleanup — dropsPendingAssignedClustersentries that now appear inReadyResourceIDs ∪ NotReadyResourceIDs, in the same write.PendingCleanupController(MC-keyed periodic sweep): removes stale pending entries (SPC points elsewhere / SPC deleted); keeps entries still pointing here or with placement in progress. Uses the SPC lister/cache (not direct DB gets); favorsStatusoverSpecfor the effective MC whenStatusis set. Transient failures return an error for workqueue backoff.ManagementClusterPlacementSync: resolvesStatusfrom CS only until a shard is observed — onceStatus.ManagementClusterResourceIDis set, the CS lookup is skipped. OnSpec != Status(both set) it logs the drift only and does not mutateSpec(no self-heal); reconcilingStatusoverSpecis left toPendingCleanupController. (Drift would only occur if CS ignored the pin or changed placement post-commit — surfaced via logs/metric, not silently corrected.)ClusterPendingClusterServiceIDAssigngated onSpec.ManagementClusterResourceID != nil;ClusterClusterServiceCreatepins the placed MC's provision shard viaClusterBuilder.ProvisionShardID(...)(OCM SDK method, not a property map).Observability
This PR adds backend metrics, an alert, and CI visualization panels:
backend_cluster_placement_state{state="placed|unplaced|mismatch"}(renamed frombackend_serviceprovidercluster_placement_state), recomputed each tick from the ServiceProviderCluster cache.mismatch=Specset ANDStatusset ANDSpec != Status— the drift signal that replaces self-heal.backend_cluster_placement_delay_secondsmeasures creation→placement latency (time.Since(cluster.SystemData.CreatedAt)), observed once per fresh placement.SchedulerPlacementMismatch(observability/alerts/scheduler-placement-prometheusRule.yaml, with a promtool test; registered inalerts-rp-services.yamland the generated bicep regenerated) fires whenbackend_cluster_placement_state{state="mismatch"} > 0for 15m (warning).test/cmd/aro-hcp-tests/gather-observability/queries.yaml— a placement-delay percentile chart (p50/p90/p99 overbackend_cluster_placement_delay_seconds_bucket) and a stacked placement-state chart bystate.These observability additions are code/config within this PR (Prometheus rule + gather-observability query panels); there are no Grafana dashboard changes and no dashboard screenshots to attach.
Validation
make verify-deepcopy(coreapi + fleetapi) clean ·go buildinternal + backend + fleet ·go testfor all touched packages (incl. metrics, placement, prometheus-rules, gather-observability) ·make lint→ 0 issues · promtool test on the alert rule.Design reference: #6649. Phase 2 (multi-resource
HCPResourceRequirementsscheduling) is intentionally out of scope.AI-generated PR. Please review for accuracy.