added placement tolerations check into best practices policyset - #556
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR adds a hub-targeted Placement manifest and policy set, a Helm-based policy template that syncs unreachable and unavailable tolerations onto hub Placement resources, and updates policyGenerator.yaml to define the new policy, wire the new policy set, and apply policySets selectors to existing policies. ChangesHub policy set and toleration sync
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Go Template Utils Linter found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
`@policygenerator/policy-sets/community/ocp-best-practices/input-tolerations/placement-tolerations.yaml`:
- Around line 32-34: The toleration rendering in placement-tolerations.yaml
drops `tolerationSeconds: 0` because the template uses `empty`, which treats
zero as unset and changes the semantics. Update the toleration block that checks
`$t.tolerationSeconds` to use `hasKey` on the toleration map instead of `empty`,
so `tolerationSeconds` is emitted whenever it is explicitly present, including
zero. Keep the existing `tolerationSeconds` field rendering in the same template
section and ensure the zero value is preserved.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82f5ef4f-9bdb-4258-a921-3906245dad82
📒 Files selected for processing (4)
policygenerator/policy-sets/community/ocp-best-practices/input-tolerations/placement-tolerations.yamlpolicygenerator/policy-sets/community/ocp-best-practices/input/hub-placement.yamlpolicygenerator/policy-sets/community/ocp-best-practices/input/placement.yamlpolicygenerator/policy-sets/community/ocp-best-practices/policyGenerator.yaml
| {{- if not (empty $t.tolerationSeconds) }} | ||
| tolerationSeconds: {{ $t.tolerationSeconds }} | ||
| {{- end }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
tolerationSeconds: 0 is silently omitted, changing toleration semantics.
The empty function returns true for the integer zero value. If an existing toleration has tolerationSeconds: 0 (meaning "evict immediately"), it will be omitted from the rendered template, effectively changing the semantics to "tolerate forever."
🛡️ Proposed fix: use `hasKey` instead of `empty` for `tolerationSeconds`
Since lookup returns map representations, hasKey can distinguish between "not set" and "set to zero":
- {{- if not (empty $t.tolerationSeconds) }}
+ {{- if hasKey $t "tolerationSeconds" }}
tolerationSeconds: {{ $t.tolerationSeconds }}
{{- end }}📝 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.
| {{- if not (empty $t.tolerationSeconds) }} | |
| tolerationSeconds: {{ $t.tolerationSeconds }} | |
| {{- end }} | |
| {{- if hasKey $t "tolerationSeconds" }} | |
| tolerationSeconds: {{ $t.tolerationSeconds }} | |
| {{- end }} |
🤖 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
`@policygenerator/policy-sets/community/ocp-best-practices/input-tolerations/placement-tolerations.yaml`
around lines 32 - 34, The toleration rendering in placement-tolerations.yaml
drops `tolerationSeconds: 0` because the template uses `empty`, which treats
zero as unset and changes the semantics. Update the toleration block that checks
`$t.tolerationSeconds` to use `hasKey` on the toleration map instead of `empty`,
so `tolerationSeconds` is emitted whenever it is explicitly present, including
zero. Keep the existing `tolerationSeconds` field rendering in the same template
section and ensure the zero value is preserved.
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
`@policygenerator/policy-sets/community/ocp-best-practices/policyGenerator.yaml`:
- Around line 130-136: The policy entry for policy-placement-tolerations is
still inheriting policySets from policyDefaults, so it ends up in both
openshift-best-practices and openshift-best-practices-hub. Adjust the
PolicyGenerator configuration so the shared defaults no longer include
policySets for this case, or split the defaults and apply a hub-only set
specifically to policy-placement-tolerations. Use policyDefaults and the
per-policy policySets override to keep this policy scoped only to
openshift-best-practices-hub.
🪄 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: Pro
Run ID: cbd57ad6-ba96-47d6-9707-466510ff4b7f
📒 Files selected for processing (3)
policygenerator/policy-sets/community/ocp-best-practices/input-tolerations/placement-tolerations.yamlpolicygenerator/policy-sets/community/ocp-best-practices/input/hub-placement.yamlpolicygenerator/policy-sets/community/ocp-best-practices/policyGenerator.yaml
✅ Files skipped from review due to trivial changes (1)
- policygenerator/policy-sets/community/ocp-best-practices/input/hub-placement.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- policygenerator/policy-sets/community/ocp-best-practices/input-tolerations/placement-tolerations.yaml
| - name: policy-placement-tolerations | ||
| severity: critical | ||
| remediationAction: inform | ||
| manifests: | ||
| - path: input-tolerations/placement-tolerations.yaml | ||
| policySets: | ||
| - openshift-best-practices-hub |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check PolicyGenerator documentation or source for policySets override vs merge behavior.
# Search for PolicyGenerator documentation in the repo
fd -i 'readme' --type f | head -5
# Search for policySets handling in any PolicyGenerator docs
rg -n -C3 'policySets' --type yaml policygenerator/ | head -40Repository: open-cluster-management-io/policy-collection
Length of output: 4129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant PolicyGenerator config around the defaults and the new policy.
sed -n '220,280p' policygenerator/policy-sets/community/ocp-best-practices/policyGenerator.yaml
# Find any docs or examples that explain whether per-policy policySets override or merge with defaults.
rg -n -C3 'policyDefaults|policySets|override|merge' policygenerator README.md blogs 3rd-party community stable .github -g '*.md' -g '*.yaml' -g '*.yml' | head -200
# Look for the generator implementation or schema references in-repo.
fd -i 'policygenerator' .
fd -i 'policygenerator.*' .Repository: open-cluster-management-io/policy-collection
Length of output: 20965
🌐 Web query:
Policy Generator policySets merge override policyDefaults policySets Open Cluster Management
💡 Result:
In the Open Cluster Management (OCM) Policy Generator, the configuration follows a hierarchical structure where defaults can be overridden at more specific levels. PolicyDefaults vs. Overrides The policyDefaults section defines common settings that apply to all policies generated by the PolicyGenerator [1][2]. Most fields defined in policyDefaults can be overridden for individual policies by specifying them within the specific entry in the policies array [1][3]. The only field in policyDefaults that cannot be overridden in this manner is namespace [1][3]. PolicySets and Merging PolicySets are used to group related policies, allowing for unified placement management [1][4]. When a policy is included in a PolicySet, a placement binding is automatically generated for the set rather than for the individual policy [1]. Regarding the interaction between policySets defined in different locations: 1. Merging Behavior: If policySets are specified in multiple places (such as in policyDefaults.policySets and within individual policies[].policySets), the lists of policy sets are merged [1][3]. 2. Overriding: You can define default values for all policy sets using the policySetDefaults section [1]. Any values defined there can be overridden by specific entries within the policySets array [1]. 3. Placement Generation: By default, when a policy is part of a policy set, the generator does not create a separate placement for that policy. You can change this behavior by setting generatePlacementWhenInSet to true in either policyDefaults or at the specific policy level [1]. In summary, the OCM Policy Generator uses a cascading approach where policySetDefaults provide base configurations for all sets, and policyDefaults provides base configurations for all policies, both of which can be selectively overridden by more granular definitions in the policySets or policies arrays, respectively [1][3].
Citations:
- 1: https://docs.redhat.com/en/documentation/red_hat_advanced_cluster_management_for_kubernetes/2.10/html/governance/integrate-third-party-policy-controllers
- 2: https://www.redhat.com/en/blog/generating-governance-policies-using-kustomize-and-gitops
- 3: https://github.com/stolostron/policy-generator-plugin/blob/main/docs/policygenerator-reference.yaml
- 4: https://github.com/open-cluster-management-io/policy-collection/tree/main/policygenerator/policy-sets
policy-placement-tolerations still inherits openshift-best-practices PolicyGenerator merges policySets from policyDefaults with the per-policy entry, so this policy will land in both openshift-best-practices and openshift-best-practices-hub. Move policySets out of the defaults or split the defaults so this one stays hub-only.
🤖 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
`@policygenerator/policy-sets/community/ocp-best-practices/policyGenerator.yaml`
around lines 130 - 136, The policy entry for policy-placement-tolerations is
still inheriting policySets from policyDefaults, so it ends up in both
openshift-best-practices and openshift-best-practices-hub. Adjust the
PolicyGenerator configuration so the shared defaults no longer include
policySets for this case, or split the defaults and apply a hub-only set
specifically to policy-placement-tolerations. Use policyDefaults and the
per-policy policySets override to keep this policy scoped only to
openshift-best-practices-hub.
Source: Coding guidelines
Signed-off-by: Christian Stark <cstark@cstark-thinkpadx1carbongen11.muc.csb>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ch-stark, gparvin 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 |
b264336
into
open-cluster-management-io:main
3 participants
Summary by CodeRabbit