OCPBUGS-62496: fix: delete old profile during replacement

[Tal-or]

What this PR does / why we need it:

When a PerformanceProfile is being replaced for a NodePool, hypershift controller lack the logic to delete the old profile from the HCP namespace.

This commit resolve this gap.
A dedicated e2e test coverage was added on NTO repo: openshift/cluster-node-tuning-operator#1413

Which issue(s) this PR fixes:

Fixes
OCPBUGS-62496

Special notes for your reviewer:

Checklist:

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.

[openshift-ci-robot]

@Tal-or: This pull request references Jira Issue OCPBUGS-62496, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.21.0) matches configured target version for branch (4.21.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (liqcui@redhat.com), skipping review request.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

What this PR does / why we need it:

When a PerformanceProfile is being replaced for a NodePool, hypershift controller lack the logic to delete the old profile from the HCP namespace.

This commit resolve this gap.
A dedicated e2e test coverage was added on NTO repo: openshift/cluster-node-tuning-operator#1413

Which issue(s) this PR fixes:

Fixes
OCPBUGS-62496

Special notes for your reviewer:

Checklist:

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

[jparrill]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jparrill, Tal-or

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:

• OWNERS [jparrill]

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

[jparrill]

Verify is failing due to:

1: CT1 Title does not follow ConventionalCommits.org format 'type(optional-scope): description': "nto:delete old profile during replacement"
make: *** [Makefile:366: run-gitlint] Error 1
{"component":"entrypoint","error":"wrapped process failed: exit status 

Check the commit follows the conventional commits (Doc here)

[jparrill]

@coderabitai review

[openshift-ci-robot]

@Tal-or: No Jira issue is referenced in the title of this pull request.
To reference a jira issue, add 'XYZ-NNN:' to the title of this pull request and request another refresh with /jira refresh.

Details

In response to this:

What this PR does / why we need it:

When a PerformanceProfile is being replaced for a NodePool, hypershift controller lack the logic to delete the old profile from the HCP namespace.

This commit resolve this gap.
A dedicated e2e test coverage was added on NTO repo: openshift/cluster-node-tuning-operator#1413

Which issue(s) this PR fixes:

Fixes
OCPBUGS-62496

Special notes for your reviewer:

Checklist:

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Walkthrough

ntoReconcile now lists PerformanceProfile ConfigMaps for the NodePool, deletes any that don't match the newly computed ConfigMap name, then proceeds to create or update the target PerformanceProfile ConfigMap with existing error handling and logging.

Changes

Cohort / File(s)	Change Summary
PerformanceProfile ConfigMap reconciliation hypershift-operator/controllers/nodepool/nto.go	Added logic to list ConfigMaps labeled as PerformanceProfileConfigMap for the current NodePool, delete any whose names differ from the computed target ConfigMap, and then perform CreateOrUpdate on the target PerformanceProfile ConfigMap; preserves prior error handling and logging.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ 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%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	The pull request description is clearly related to the changeset. It explains that the hypershift controller was lacking logic to delete old profiles when a PerformanceProfile is replaced for a NodePool, and notes that this commit resolves that gap. The description appropriately references the Jira ticket OCPBUGS-62496 and mentions that e2e test coverage was added in the related Node Tuning Operator repository, providing meaningful context about the change without being vague or off-topic.
Title Check	✅ Passed	The PR title "OCPBUGS-62496: fix: delete old profile during replacement" is fully aligned with the changeset content. According to the raw summary, the change adds logic to delete old PerformanceProfile ConfigMaps when a profile is replaced for a NodePool, which is precisely what the title describes. The title is concise, specific, and includes the Jira reference, making it clear to teammates scanning history what the primary change addresses. The title avoids vague terminology and noise, providing meaningful context about both the fix (deletion of old profiles) and the scenario (during replacement).

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between e7286c1 and eb473ef.

📒 Files selected for processing (1)

• hypershift-operator/controllers/nodepool/nto.go (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request

🔇 Additional comments (2)

hypershift-operator/controllers/nodepool/nto.go (2)

526-549: The cleanup logic correctly addresses the PR objective.

The implementation properly handles deletion of old PerformanceProfile ConfigMaps when a profile is replaced:

• Lists existing ConfigMaps for the NodePool
• Deletes any that don't match the current desired name
• Creates or updates the desired ConfigMap

This ensures only the relevant PerformanceProfile ConfigMap remains for the NodePool.

---

526-534: Verify the intentionality of the IsNotFound check in List operations.

The pattern err != nil && !apierrors.IsNotFound(err) is used consistently across the codebase (found in at least hostedcluster_controller.go:3107 and nto.go:57-58). While List operations typically return empty lists rather than NotFound errors per Kubernetes API conventions, this defensive check appears to be intentional—either as a safety measure or legacy pattern. Confirm whether this is deliberate defensive programming or can be simplified to just err != nil.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

hypershift-operator/controllers/nodepool/nto.go (1)

536-543: Consider adding a log message when deleting old ConfigMaps.

Adding a log entry when deleting mismatched ConfigMaps would improve debuggability and provide better visibility into the cleanup operation.

Apply this diff to add logging:

 	for i := range existingPerformanceProfileConfigMapList.Items {
 		ppConfigMap := &existingPerformanceProfileConfigMapList.Items[i]
 		if ppConfigMap.Name != performanceProfileConfigMap.Name {
+			log.Info("Deleting old PerformanceProfile ConfigMap", "name", ppConfigMap.Name)
 			if _, err := supportutil.DeleteIfNeeded(ctx, r.Client, ppConfigMap); err != nil {
 				return fmt.Errorf("failed to delete performanceProfile ConfigMap: %w", err)
 			}
 		}
 	}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between eb473ef and ed3c3cc.

📒 Files selected for processing (1)

• hypershift-operator/controllers/nodepool/nto.go (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request

🔇 Additional comments (3)

hypershift-operator/controllers/nodepool/nto.go (3)

526-534: LGTM! Proper scoping and error handling for listing ConfigMaps.

The List operation correctly filters for PerformanceProfile ConfigMaps belonging to the current NodePool, and error handling appropriately ignores NotFound while returning other errors.

---

536-543: LGTM! Correctly implements cleanup of old PerformanceProfile ConfigMaps.

The loop correctly addresses the previous review concern by using for i := range and taking the address of the slice element directly. The logic properly deletes any existing ConfigMaps that don't match the target name, which resolves OCPBUGS-62496.

---

544-550: LGTM! Proper reconciliation with error handling and logging.

The CreateOrUpdate operation correctly reconciles the target PerformanceProfile ConfigMap after cleanup, with appropriate error handling and logging.

[bryan-cox]

/retest-required

[bryan-cox]

/retitle OCPBUGS-62496: fix: delete old profile during replacement

[openshift-ci-robot]

@Tal-or: This pull request references Jira Issue OCPBUGS-62496, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.21.0) matches configured target version for branch (4.21.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (liqcui@redhat.com), skipping review request.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

What this PR does / why we need it:

When a PerformanceProfile is being replaced for a NodePool, hypershift controller lack the logic to delete the old profile from the HCP namespace.

This commit resolve this gap.
A dedicated e2e test coverage was added on NTO repo: openshift/cluster-node-tuning-operator#1413

Which issue(s) this PR fixes:

Fixes
OCPBUGS-62496

Special notes for your reviewer:

Checklist:

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

[bryan-cox]

/lgtm

[Tal-or]

/retest

[Tal-or]

@jparrill @bryan-cox Thank you for the approval folks, who should add the verified label?

[mrniranjan]

/verified by @mrniranjan

[openshift-ci-robot]

@mrniranjan: This PR has been marked as verified by @mrniranjan.

Details

In response to this:

/verified by @mrniranjan

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD 05a2fcd and 2 for PR HEAD ed3c3cc in total

[Tal-or]

/retest

[Tal-or]

/retest

[openshift-ci]

@Tal-or: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions 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.

[openshift-ci-robot]

@Tal-or: Jira Issue OCPBUGS-62496: Some pull requests linked via external trackers have merged:

The following pull request, linked via external tracker, has not merged:

• openshift/cluster-node-tuning-operator#1413 is open

All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-62496 has not been moved to the MODIFIED state.

This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload.

Details

In response to this:

What this PR does / why we need it:

When a PerformanceProfile is being replaced for a NodePool, hypershift controller lack the logic to delete the old profile from the HCP namespace.

This commit resolve this gap.
A dedicated e2e test coverage was added on NTO repo: openshift/cluster-node-tuning-operator#1413

Which issue(s) this PR fixes:

Fixes
OCPBUGS-62496

Special notes for your reviewer:

Checklist:

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

[Tal-or]

/cherry-pick release-4.20, release-4.19

[openshift-cherrypick-robot]

@Tal-or: cannot checkout release-4.20,: error checking out "release-4.20,": exit status 1 error: pathspec 'release-4.20,' did not match any file(s) known to git

Details

In response to this:

/cherry-pick release-4.20, release-4.19

Instructions 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.

[Tal-or]

/cherry-pick release-4.20 release-4.19

[openshift-cherrypick-robot]

@Tal-or: new pull request created: #7428

Details

In response to this:

/cherry-pick release-4.20 release-4.19

Instructions 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.