Skip to content

mcp: selinux: extend annotation to stop reconciling#3989

Open
ffromani wants to merge 1 commit into
mainfrom
selinux-upgrade-gradual
Open

mcp: selinux: extend annotation to stop reconciling#3989
ffromani wants to merge 1 commit into
mainfrom
selinux-upgrade-gradual

Conversation

@ffromani

Copy link
Copy Markdown
Member

Add another value to the custom selinux policy annotation: if this value is set, the operand (RTE DaemonSet) will use the builtin policy and SCC v2, but the relevant MachineConfig will stop being reconciled.

Operators can remove the annotation anytime to trigger the reconciliation, causing a MC deletion and therefore a reboot.

This way, we decouple the SELinux upgrade from the MachineConfig removal, which helps (or should help) in the upgrade flow.

Add another value to the custom selinux policy annotation:
if this value is set, the operand (RTE DaemonSet) will
use the builtin policy and SCC v2, but the relevant MachineConfig
will *stop being reconciled*.

Operators can remove the annotation anytime to trigger the
reconciliation, causing a MC deletion and therefore a reboot.

This way, we decouple the SELinux upgrade from the MachineConfig
removal, which helps (or should help) in the upgrade flow.

Signed-off-by: Francesco Romani <fromani@redhat.com>
@openshift-ci openshift-ci Bot requested review from shajmakh and swatisehgal May 11, 2026 13:42
@openshift-ci

openshift-ci Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2026
@coderabbitai

coderabbitai Bot commented May 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds an "ignore" annotation value for SELinux policy configuration that allows skipping custom policy application in specific node groups. It defines a core constant and check function, wraps them for operator-level use, and integrates the ignore logic into MachineConfigPool processing with conditional skipping and logging.

Changes

Ignore Custom SELinux Policy Configuration

Layer / File(s) Summary
Core Ignore Policy Constant and Function
internal/api/annotations/annotations.go, internal/api/annotations/annotations_test.go
New constant SELinuxPolicyIgnore = "ignore" and function MustIgnoreCustomPolicy(annot map[string]string) bool check if the policy config annotation equals the ignore value. Test case verifies "ignore" does not enable custom policy; separate test function validates behavior across empty, "custom", arbitrary, and "ignore" values.
Operator-Level Helper Wrapper
internal/api/annotations/helper/helper.go, internal/api/annotations/helper/helper_test.go
New function MustIgnoreCustomPolicy(instance *nropv1.NUMAResourcesOperator) bool iterates through node groups and delegates to the base annotation function, returning true if any node group signals ignore. Table-driven tests verify correct return values for varied node group annotation maps.
MachineConfigPool Processing Integration
pkg/objectstate/rte/machineconfigpool.go
Computes per-tree mustIgnore flag by evaluating annotations.MustIgnoreCustomPolicy on node group annotations. Adds conditional guard in MCP loop that logs and skips processing when mustIgnore is true.

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: extending a SELinux annotation to control MachineConfig reconciliation in the RTE component.
Description check ✅ Passed The description clearly explains the new annotation value, its effects on the operand and MachineConfig reconciliation, and the benefits for the upgrade flow.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch selinux-upgrade-gradual

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pkg/objectstate/rte/machineconfigpool.go (1)

148-171: LGTM - Correct implementation of ignore logic.

The per-tree ignore check correctly short-circuits MachineConfig processing when the annotation is set to "ignore", which aligns with the PR objective to decouple SELinux upgrade from MachineConfig removal. The logic flow properly handles the three states:

  1. Ignore: skip reconciliation entirely
  2. Custom policy: create/update MC
  3. Default policy: delete MC

The V(4) log level means this skip will only appear in debug logs. Consider whether Info-level logging would provide better operational visibility when MachineConfigs are being ignored.

🤖 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 `@pkg/objectstate/rte/machineconfigpool.go` around lines 148 - 171, The V(4)
debug log in the MachineConfigPool reconciliation loop (where mustIgnore :=
annotations.MustIgnoreCustomPolicy(tree.NodeGroup.Annotations) and the loop over
tree.MachineConfigPools short-circuits when mustIgnore is true) is too quiet for
operational visibility; change the log call that currently uses
klog.V(4).Infof("ignoring MachineConfig for pool %q", mcp.Name) to a
higher-visibility level (for example klog.Infof) so operators can see when
MachineConfigs are being skipped, keeping the existing early-return behavior and
only adjusting the logging level.
🤖 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 `@pkg/objectstate/rte/machineconfigpool.go`:
- Around line 148-171: The V(4) debug log in the MachineConfigPool
reconciliation loop (where mustIgnore :=
annotations.MustIgnoreCustomPolicy(tree.NodeGroup.Annotations) and the loop over
tree.MachineConfigPools short-circuits when mustIgnore is true) is too quiet for
operational visibility; change the log call that currently uses
klog.V(4).Infof("ignoring MachineConfig for pool %q", mcp.Name) to a
higher-visibility level (for example klog.Infof) so operators can see when
MachineConfigs are being skipped, keeping the existing early-return behavior and
only adjusting the logging level.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: b53e995c-0431-41f2-8511-db10130cbe13

📥 Commits

Reviewing files that changed from the base of the PR and between cf7b39d and d0c1ee4.

📒 Files selected for processing (5)
  • internal/api/annotations/annotations.go
  • internal/api/annotations/annotations_test.go
  • internal/api/annotations/helper/helper.go
  • internal/api/annotations/helper/helper_test.go
  • pkg/objectstate/rte/machineconfigpool.go

@ffromani

Copy link
Copy Markdown
Member Author

/cc @Tal-or
/hold

tiny change, but not sure yet we want this

@openshift-ci openshift-ci Bot requested a review from Tal-or May 11, 2026 14:09
@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant