hypershift: support Minimal control plane availability-zone scheduling - #3150
hypershift: support Minimal control plane availability-zone scheduling#3150stevekuznetsov wants to merge 1 commit into
Conversation
When the HostedControlPlane opts into the Minimal control plane availability-zone scheduling policy (spec.controlPlaneAvailabilityZoneScheduling.policy=Minimal), transform the network control-plane operands accordingly: - network-node-identity and multus-admission-controller (blocking-webhook backends) stay spread across availability zones as two-replica pairs on the zonal node pools (network-node-identity drops from three replicas to two). - ovnkube-control-plane (a leader-elected controller) floats onto the non-zonal overflow node pools. For each, the zone podAntiAffinity is replaced with topologySpreadConstraints (hard zone spread for zone-critical, best-effort for float; hard host spread for all), the pod is steered onto the correct node pool via the well-known hypershift.openshift.io/control-plane-node-role label (required for zone-critical and hard float placement, preferred for soft float placement), zone-critical pods tolerate the zonal taint, and colocation is scoped per scheduling tier. Parsed from the HostedControlPlane CR; a no-op when the policy is not set. Signed-off-by: Steve Kuznetsov <stekuznetsov@microsoft.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. Summary by CodeRabbit
WalkthroughHyperShift parsing now exposes availability-zone scheduling settings. Network rendering applies Minimal scheduling to eligible control-plane Deployments, including placement, affinity, toleration, replica, and topology changes. Tests cover disabled, zonal, overflow, and required non-zonal modes. ChangesHyperShift zonal scheduling
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change adds opt-in Minimal availability-zone scheduling for HyperShift network control-plane workloads while preserving existing behavior when the policy is unset. The covered placement, spreading, taint, and replica behaviors present no remaining merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant Render
participant applyMinimalZonalScheduling
participant Deployment
Render->>applyMinimalZonalScheduling: Apply HostedControlPlane scheduling policy
applyMinimalZonalScheduling->>Deployment: Convert and mutate eligible operands
applyMinimalZonalScheduling-->>Render: Return updated resources or error
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 3 warnings)
✅ Passed checks (17 passed)
Full details: Pr QualityExplanation The PR is non-trivial and fails the description requirements. The authored description explains what changes, but it has no explicit Why section or motivation, no How to verify/Testing section, and no automated CNO CI jobs or target platforms. It describes the user-facing scheduling behavior, but it does not state upgrade or rollback considerations. The size criterion passes: the diff is 464 lines across five related files, well below 7,000 lines. The bug-fix criterion is not applicable because this is a feature. Resolution Update the PR description with explicit Why, What, and How to verify/Testing sections. In Testing, name the automated CNO CI jobs and the platforms that run them. Add the feature's upgrade and rollback considerations, or explicitly state why none apply. Full details: Commit Message QualityExplanation The PR contains one non-merge commit, Resolution Amend the commit body to state the operational motivation before the implementation summary. For example: explain that zonal HyperShift control planes need network control-plane operands to remain available across availability zones while leader-elected control-plane components use overflow capacity, and that the Minimal policy provides this placement behavior. Retain a shorter high-level summary of the resulting changes. Full details: Unit Tests For Go ChangesExplanation PASS. The PR modifies production Go files under pkg/: pkg/hypershift/hypershift.go, pkg/network/render.go, and adds pkg/network/zonal_scheduling.go. It also modifies or adds corresponding Go test files: pkg/hypershift/hypershift_test.go and pkg/network/zonal_scheduling_test.go. No bindata templates changed. Full details: E2e Tests For Feature ChangesExplanation The pull request adds user-facing HyperShift behavior. The diff changes Resolution Add or modify appropriate files under Full details: Rbac Least PrivilegeExplanation No RBAC rules changed. The pull-request commit changes only five Go files, and it changes no YAML or YML files under bindata/ or manifests/. Therefore, the wildcard and mutation-access conditions do not apply. Full details: Docs For Feature And Behavior ChangesExplanation The pull request adds a new CNO-specific HyperShift feature. Resolution Add a new Markdown file under Full details: Stale Project Docs And ConfigExplanation No stale project documentation or configuration was introduced. The PR only adds HyperShift parsing and scheduling logic in existing Full details: Go And Test Code QualityExplanation The pull request adds an unwrapped error return in Full details: Ai-Generated Code SmellExplanation The changed code does not show any of the four stated smell conditions. The added comments explain scheduling semantics, placement rationale, or public API fields; they do not merely restate simple statements. The 202 added test lines cover the new parser and scheduling implementation, so they are proportional and related. A search found no AI-tool, prompt, or generation references. The implementation uses intermediate values and checks for distinct Kubernetes structures and conversion/error paths, not redundant copy-paste logic. Full details: Stable And Deterministic Test NamesExplanation PASS: The pull request adds no Ginkgo tests or Ginkgo title declarations. The new Go Full details: Test Structure And QualityExplanation The new Resolution Add a meaningful diagnostic message to every newly added Gomega assertion in Full details: Microshift Test CompatibilityExplanation PASS: The pull request adds only standard Go unit tests ( Full details: Single Node Openshift (Sno) Test CompatibilityExplanation PASS: The pull request adds only Go unit tests using Full details: Topology-Aware Scheduling CompatibilityExplanation PASS. The new constraints are opt-in and HyperShift-specific. Full details: Ote Binary Stdout ContractExplanation PASS. The pull request adds no Full details: Ipv6 And Disconnected Network Test CompatibilityExplanation The pull request adds only Go unit tests: Full details: No-Weak-CryptoExplanation No weak cryptography or custom cryptographic implementation was introduced. The changed files add scheduling and parsing logic only. The existing Full details: Container-PrivilegesExplanation PASS: The pull request adds Go parsing and scheduling logic plus tests. It does not add or change container/Kubernetes security settings. The changed additions contain no Full details: No-Sensitive-Data-In-LogsExplanation The pull request adds no logging calls and does not add any log or diagnostic output containing passwords, tokens, API keys, PII, session IDs, hostnames, or customer data. The new errors contain only fixed field names, conversion context, and the matched Deployment name (
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v1.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.40.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17689 characters] ... e0dc3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.36.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.36.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/streaming@v0.36.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.4.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stevekuznetsov 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 |
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
@stevekuznetsov: The following tests 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. |
When the HostedControlPlane opts into the Minimal control plane availability-zone scheduling policy (spec.controlPlaneAvailabilityZoneScheduling.policy=Minimal), transform the network control-plane operands accordingly:
For each, the zone podAntiAffinity is replaced with topologySpreadConstraints (hard zone spread for zone-critical, best-effort for float; hard host spread for all), the pod is steered onto the correct node pool via the well-known hypershift.openshift.io/control-plane-node-role label (required for zone-critical and hard float placement, preferred for soft float placement), zone-critical pods tolerate the zonal taint, and colocation is scoped per scheduling tier. Parsed from the HostedControlPlane CR; a no-op when the policy is not set.