Enforce workload partitioning in cluster compare#804
Open
imiller0 wants to merge 1 commit into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: imiller0 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a kube-compare reference OpenShift Infrastructure manifest with status.cpuPartitioning=AllNodes, wires it into kube-compare metadata as a required-platform check, updates compare_ignore, and aligns telco-core infrastructure spec/status to use AllNodes. ChangesInfrastructure Platform Reference Configuration
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
telco-ran/configuration/kube-compare-reference/metadata.yaml (1)
27-35: ⚡ Quick winAdd description field for consistency and maintainability.
The new
required-platformpart lacks adescriptionfield, breaking the pattern established throughout this file where every other part includes a description (e.g., lines 4-6, 16-18, 37-39, 53-55). Adding a description would clarify that this validates workload partitioning configuration and improve maintainability.📝 Suggested addition
- name: required-platform + description: |- + Validates that workload partitioning is enabled (cpuPartitioning: AllNodes) + for telco workload isolation requirements. components: - name: platform🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@telco-ran/configuration/kube-compare-reference/metadata.yaml` around lines 27 - 35, Add a missing description for the new "required-platform" part to match the file's pattern: locate the required-platform entry (component name "platform" referencing "platform/infrastructure.yaml" with config and fieldsToOmitRefs including allowStatusCheck) and add a short descriptive string explaining that this section validates workload partitioning configuration so it follows the same structure and improves maintainability and clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@telco-hub/configuration/reference-crs-kube-compare/optional/backup-recovery/policy-backup-mcsb.yaml`:
- Around line 7-8: Add the ArgoCD skip-dry-run annotation to each ACM resource
so ArgoCD won't fail if CRDs are missing: for every ManagedClusterSetBinding,
Placement, and PlacementBinding manifest (the six backup-recovery ACM manifests
in both kube-compare and reference-crs sets) add metadata.annotations with key
"argocd.argoproj.io/sync-options" and value "SkipDryRunOnMissingResource=true"
(follow the same placement and formatting as in addPluginsMCSB.yaml /
addPluginsPolicyPlacement.yaml / addPluginsPolicyPlacementBinding.yaml).
In
`@telco-hub/configuration/reference-crs/required/gitops/ztp-policies/extra-manifests-clusterrolebinding.yaml`:
- Line 7: The ClusterRoleBinding manifest sets argocd.argoproj.io/sync-wave:
"-30" which conflicts with the documented RBAC ordering; change the sync-wave
annotation in the ClusterRoleBinding resource to "-40" so RBAC resources
(ClusterRole, ClusterRoleBinding) deploy at the correct ordering per
SYNC-WAVES.md — locate the ClusterRoleBinding manifest and update the
argocd.argoproj.io/sync-wave value from "-30" to "-40".
---
Nitpick comments:
In `@telco-ran/configuration/kube-compare-reference/metadata.yaml`:
- Around line 27-35: Add a missing description for the new "required-platform"
part to match the file's pattern: locate the required-platform entry (component
name "platform" referencing "platform/infrastructure.yaml" with config and
fieldsToOmitRefs including allowStatusCheck) and add a short descriptive string
explaining that this section validates workload partitioning configuration so it
follows the same structure and improves maintainability and clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9d0a70e3-2cab-47d0-8c32-7a3675404036
📒 Files selected for processing (51)
Makefilehack/test-hub-kustomization-coverage.shhack/test-hub-single-cr-per-file.shhack/test-hub-sync-wave-annotation.shtelco-core/configuration/reference-crs-kube-compare/required/platform/infrastructure.yamltelco-hub/configuration/Makefiletelco-hub/configuration/reference-crs-kube-compare/.gitignoretelco-hub/configuration/reference-crs-kube-compare/compare_ignoretelco-hub/configuration/reference-crs-kube-compare/metadata.yamltelco-hub/configuration/reference-crs-kube-compare/optional/backup-recovery/policy-backup-mcsb.yamltelco-hub/configuration/reference-crs-kube-compare/optional/backup-recovery/policy-backup-placement.yamltelco-hub/configuration/reference-crs-kube-compare/optional/backup-recovery/policy-backup-placementbinding.yamltelco-hub/configuration/reference-crs-kube-compare/optional/cert-manager/apiServerCertificate.yamltelco-hub/configuration/reference-crs-kube-compare/optional/cert-manager/apiServerConfig.yamltelco-hub/configuration/reference-crs-kube-compare/optional/cert-manager/certManagerCertificatePolicy.yamltelco-hub/configuration/reference-crs-kube-compare/optional/cert-manager/certManagerCertificatePolicyPlacement.yamltelco-hub/configuration/reference-crs-kube-compare/optional/cert-manager/certManagerCertificatePolicyPlacementBinding.yamltelco-hub/configuration/reference-crs-kube-compare/optional/cert-manager/ingressControllerConfig.yamltelco-hub/configuration/reference-crs-kube-compare/required/gitops/ztp-installation/clusters-app-namespace.yamltelco-hub/configuration/reference-crs-kube-compare/required/gitops/ztp-installation/policies-app-namespace.yamltelco-hub/configuration/reference-crs/optional/backup-recovery/kustomization.yamltelco-hub/configuration/reference-crs/optional/backup-recovery/policy-backup-mcsb.yamltelco-hub/configuration/reference-crs/optional/backup-recovery/policy-backup-placement.yamltelco-hub/configuration/reference-crs/optional/backup-recovery/policy-backup-placementbinding.yamltelco-hub/configuration/reference-crs/optional/backup-recovery/policy-backup.yamltelco-hub/configuration/reference-crs/optional/cert-manager/apiServerCertificate.yamltelco-hub/configuration/reference-crs/optional/cert-manager/apiServerConfig.yamltelco-hub/configuration/reference-crs/optional/cert-manager/certManagerCertificatePolicy.yamltelco-hub/configuration/reference-crs/optional/cert-manager/certManagerCertificatePolicyPlacement.yamltelco-hub/configuration/reference-crs/optional/cert-manager/certManagerCertificatePolicyPlacementBinding.yamltelco-hub/configuration/reference-crs/optional/cert-manager/ingressControllerConfig.yamltelco-hub/configuration/reference-crs/optional/odf-internal/kustomization.yamltelco-hub/configuration/reference-crs/optional/odf-internal/odfReady.yamltelco-hub/configuration/reference-crs/optional/odf-internal/odfReadyPlacement.yamltelco-hub/configuration/reference-crs/optional/odf-internal/odfReadyPlacementBinding.yamltelco-hub/configuration/reference-crs/required/gitops/kustomization.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-installation/clusters-app-namespace.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-installation/clusters-app.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-installation/kustomization.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-installation/policies-app-namespace.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-installation/policies-app.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-policies/extra-manifests-clusterrole.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-policies/extra-manifests-clusterrolebinding.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-policies/extra-manifests-mcsb.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-policies/extra-manifests-namespace.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-policies/extra-manifests-placement.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-policies/extra-manifests-placementbinding.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-policies/extra-manifests-policy.yamltelco-hub/configuration/reference-crs/required/gitops/ztp-policies/extra-manifests-serviceaccount.yamltelco-ran/configuration/kube-compare-reference/metadata.yamltelco-ran/configuration/kube-compare-reference/platform/infrastructure.yaml
💤 Files with no reviewable changes (4)
- telco-hub/configuration/reference-crs/required/gitops/ztp-installation/policies-app.yaml
- telco-hub/configuration/reference-crs/optional/backup-recovery/policy-backup.yaml
- telco-hub/configuration/reference-crs/required/gitops/ztp-installation/clusters-app.yaml
- telco-hub/configuration/reference-crs/optional/odf-internal/odfReady.yaml
Comment on lines
+7
to
+8
| annotations: | ||
| argocd.argoproj.io/sync-wave: "-30" |
There was a problem hiding this comment.
All ACM resources missing SkipDryRunOnMissingResource annotation. The six backup-recovery ACM manifests (ManagedClusterSetBinding, Placement, and PlacementBinding in both kube-compare and reference-crs directories) all lack argocd.argoproj.io/sync-options: SkipDryRunOnMissingResource=true. This annotation is an established pattern for ACM resources in this repository (see addPluginsMCSB.yaml, addPluginsPolicyPlacement.yaml, addPluginsPolicyPlacementBinding.yaml) and is critical for resilient deployment when CRDs may not be present at sync time. Without this annotation, ArgoCD will fail to sync these resources if the ACM operator hasn't yet created the required CRDs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@telco-hub/configuration/reference-crs-kube-compare/optional/backup-recovery/policy-backup-mcsb.yaml`
around lines 7 - 8, Add the ArgoCD skip-dry-run annotation to each ACM resource
so ArgoCD won't fail if CRDs are missing: for every ManagedClusterSetBinding,
Placement, and PlacementBinding manifest (the six backup-recovery ACM manifests
in both kube-compare and reference-crs sets) add metadata.annotations with key
"argocd.argoproj.io/sync-options" and value "SkipDryRunOnMissingResource=true"
(follow the same placement and formatting as in addPluginsMCSB.yaml /
addPluginsPolicyPlacement.yaml / addPluginsPolicyPlacementBinding.yaml).
| metadata: | ||
| name: ztp-template-watcher-binding | ||
| annotations: | ||
| argocd.argoproj.io/sync-wave: "-30" |
There was a problem hiding this comment.
Sync-wave should be -40 per documented RBAC ordering.
The ClusterRoleBinding has sync-wave: "-30", but the repository's SYNC-WAVES.md documentation places all RBAC resources (ClusterRole, ClusterRoleBinding) at sync-wave -40. This violates the documented deployment ordering contract.
📋 Proposed fix
annotations:
- argocd.argoproj.io/sync-wave: "-30"
+ argocd.argoproj.io/sync-wave: "-40"As per coding guidelines, RBAC resources including ClusterRoleBinding should be deployed at sync-wave -40 as documented in SYNC-WAVES.md.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| argocd.argoproj.io/sync-wave: "-30" | |
| annotations: | |
| argocd.argoproj.io/sync-wave: "-40" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@telco-hub/configuration/reference-crs/required/gitops/ztp-policies/extra-manifests-clusterrolebinding.yaml`
at line 7, The ClusterRoleBinding manifest sets argocd.argoproj.io/sync-wave:
"-30" which conflicts with the documented RBAC ordering; change the sync-wave
annotation in the ClusterRoleBinding resource to "-40" so RBAC resources
(ClusterRole, ClusterRoleBinding) deploy at the correct ordering per
SYNC-WAVES.md — locate the ClusterRoleBinding manifest and update the
argocd.argoproj.io/sync-wave value from "-30" to "-40".
Update the cluster compare templates to require workload partitioning on all topologies for Core and RAN clusters Signed-off-by: Ian Miller <imiller@redhat.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add check to cluster compare templates for Core and RAN clusters (all topologies) that workload partitioning is enabled.