CNF-22194 Core: ensure one copy of MachineConfigs in Core RDS - #825
CNF-22194 Core: ensure one copy of MachineConfigs in Core RDS#825kchennag wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kchennag 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @kchennag. Thanks for your PR. I'm waiting for a openshift-kni member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMachineConfig and MachineConfigPool manifests are moved into install-time extra manifests, packaged through Kustomize, validated by a new ChangesMachineConfig relocation and validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Makefile
participant compare_sh as compare.sh
participant install_manifests as install/extra-manifests
participant kube_compare as reference-crs-kube-compare
participant custom_manifests as reference-crs/custom-manifests
Makefile->>compare_sh: run --check-extra-manifests
compare_sh->>install_manifests: read install manifests
compare_sh->>kube_compare: compare reference manifests
compare_sh->>custom_manifests: compare MCP copies
compare_sh-->>Makefile: return validation status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
telco-core/configuration/compare.sh (1)
117-158: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMaintenance contract is implicit in hardcoded pairs array.
The
pairsarray (lines 122-130) acts as an explicit contract: all MachineConfig files underinstall/extra-manifeststhat require validation must be listed here and have corresponding entries inreference-crs-kube-compare/. If a new file is added toinstall/extra-manifests/, it must be:
- Added to
reference-crs-kube-compare/- Added to the
pairsarrayWithout this update, the new file won't be diff-validated (though
check_no_machineconfig_in_reference_crswill still catch strays inreference-crs). This is reasonable for a controlled migration, but document this maintenance contract in a comment or the README to avoid future confusion.For now, the validation logic itself is solid: lines 145–151 positively assert version 3.2.0 and
data:,sctp, while lines 153–155 negatively assert no 2.2.0 or legacyfilesystemfields.Add comment documenting the pairs array contract
local -a pairs=( + # Contract: All MachineConfig files in install/extra-manifests that require + # validation against kube-compare-reference must be listed here. + # New files added to install/extra-manifests/ must be added to this list, + # reference-crs-kube-compare/, and reference-crs/custom-manifests/ (MCPs only). "control-plane-load-kernel-modules.yaml:optional/other/control-plane-load-kernel-modules.yaml"🤖 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-core/configuration/compare.sh` around lines 117 - 158, The hardcoded pairs array in the compare_install_extra_manifests() function implements an implicit maintenance contract that is not documented. Add a comment above the pairs array declaration explaining that this array defines the validation contract: any new MachineConfig files added to install/extra-manifests/ must be explicitly added to the pairs array with their corresponding path in reference-crs-kube-compare/, otherwise they will not be diff-validated by this comparison function.
🤖 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.
Nitpick comments:
In `@telco-core/configuration/compare.sh`:
- Around line 117-158: The hardcoded pairs array in the
compare_install_extra_manifests() function implements an implicit maintenance
contract that is not documented. Add a comment above the pairs array declaration
explaining that this array defines the validation contract: any new
MachineConfig files added to install/extra-manifests/ must be explicitly added
to the pairs array with their corresponding path in reference-crs-kube-compare/,
otherwise they will not be diff-validated by this comparison function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: efa75f82-4a15-45cd-8a8f-ef965a66aa22
📒 Files selected for processing (28)
telco-core/configuration/Makefiletelco-core/configuration/compare.shtelco-core/configuration/core-overlay.yamltelco-core/configuration/reference-crs-kube-compare/compare_ignoretelco-core/configuration/reference-crs-kube-compare/metadata.yamltelco-core/configuration/reference-crs-kube-compare/optional/networking/multus/tap_cni/mc_rootless_pods_selinux.yamltelco-core/configuration/reference-crs-kube-compare/optional/other/control-plane-load-kernel-modules.yamltelco-core/configuration/reference-crs-kube-compare/optional/other/worker-load-kernel-modules.yamltelco-core/configuration/reference-crs/custom-manifests/README.mdtelco-core/configuration/reference-crs/custom-manifests/mcp-worker-1.yamltelco-core/configuration/reference-crs/custom-manifests/mcp-worker-2.yamltelco-core/configuration/reference-crs/custom-manifests/mcp-worker-3.yamltelco-core/configuration/reference-crs/optional/networking/multus/tap_cni/mc_rootless_pods_selinux.yamltelco-core/configuration/reference-crs/optional/other/control-plane-load-kernel-modules.yamltelco-core/configuration/reference-crs/optional/other/kdump-master.yamltelco-core/configuration/reference-crs/optional/other/kdump-worker.yamltelco-core/configuration/reference-crs/optional/other/mount_namespace_config_master.yamltelco-core/configuration/reference-crs/optional/other/mount_namespace_config_worker.yamltelco-core/configuration/reference-crs/optional/other/sctp_module_mc.yamltelco-core/configuration/reference-crs/optional/other/worker-load-kernel-modules.yamltelco-core/install/README.mdtelco-core/install/custom-manifests/README.mdtelco-core/install/example-standard-clusterinstance.yamltelco-core/install/extra-manifests/mcp-worker-1.yamltelco-core/install/extra-manifests/mcp-worker-2.yamltelco-core/install/extra-manifests/mcp-worker-3.yamltelco-core/install/extra-manifests/sctp_module_mc.yamltelco-core/install/kustomization.yaml
💤 Files with no reviewable changes (9)
- telco-core/configuration/reference-crs/optional/other/mount_namespace_config_master.yaml
- telco-core/install/example-standard-clusterinstance.yaml
- telco-core/configuration/reference-crs/optional/networking/multus/tap_cni/mc_rootless_pods_selinux.yaml
- telco-core/configuration/reference-crs/optional/other/kdump-master.yaml
- telco-core/configuration/reference-crs/optional/other/sctp_module_mc.yaml
- telco-core/configuration/reference-crs/optional/other/kdump-worker.yaml
- telco-core/configuration/reference-crs/optional/other/worker-load-kernel-modules.yaml
- telco-core/configuration/reference-crs/optional/other/mount_namespace_config_worker.yaml
- telco-core/configuration/reference-crs/optional/other/control-plane-load-kernel-modules.yaml
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-core/configuration/compare.sh`:
- Around line 142-147: The grep commands on lines checking for 'version: 3.2.0'
and 'path: /etc/modules-load.d/kernel-load.conf' are using regex semantics where
the dot (.) character acts as a wildcard matching any character, which allows
incorrect versions or paths to pass validation. Fix this by either using grep -F
flag for fixed string matching instead of regex, or by escaping the dot
characters in the patterns with backslashes (e.g., 3\.2\.0 and
/etc/modules-load\.d/kernel-load\.conf) to ensure they match literal dots only.
🪄 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: 2bd232a3-8d36-42a1-ace0-41a73699c58a
📒 Files selected for processing (1)
telco-core/configuration/compare.sh
| Reference MachineConfig CRs are validated from telco-core/install/extra-manifests | ||
| via compare.sh --check-extra-manifests, not from reference-crs. |
There was a problem hiding this comment.
At runtime I don't think this works. compare.sh would verify the repository contents, but kube-compare needs to include all the lines which are deleted in order to verify that these CRs exist and are correct on a deployed cluster.
There was a problem hiding this comment.
Hi @imiller0, i understand your concern about removing the MachineConfig CRs from metadata.yaml.
if kube-compare needs those entries to validate the deployed cluster state, should we keep them in metadata.yaml and instead just add the compare.sh --check-extra-manifests check as an additional repo-level validation?
happy to revert those metadata.yaml removals if that's the right approach
| paused: false | ||
| maxUnavailable: 1 | ||
| paused: true | ||
| maxUnavailable: 100% |
There was a problem hiding this comment.
Is there a reason that these are being changed? In general we don't want the user to keep the MCP paused for long durations of time
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
telco-core/configuration/compare.sh (1)
142-150: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftValidate the complete templated MachineConfig, not just two sentinel strings.
After these checks,
continueskipsdiff -u, so changes to any other field can pass validation. Render the kube-compare template with the expected values and compare normalized YAML, or validate the complete expected object.🤖 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-core/configuration/compare.sh` around lines 142 - 150, Update the validation loop around the install/${inst} checks so it verifies the complete rendered kube-compare MachineConfig rather than only the ignition version and kernel-load.conf sentinel strings. Render the template with the expected values and compare normalized YAML, or otherwise validate the full expected object, before continuing; retain failure reporting through fail.
🤖 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.
Outside diff comments:
In `@telco-core/configuration/compare.sh`:
- Around line 142-150: Update the validation loop around the install/${inst}
checks so it verifies the complete rendered kube-compare MachineConfig rather
than only the ignition version and kernel-load.conf sentinel strings. Render the
template with the expected values and compare normalized YAML, or otherwise
validate the full expected object, before continuing; retain failure reporting
through fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5371b935-8aab-4fdd-8d07-2dc70809c46f
📒 Files selected for processing (1)
telco-core/configuration/compare.sh
4 participants
Core of CNF-22194.