OCPBUGS-97592: skip plugin execution when backup does not target HCP resources

[Alcamech]

Summary

• Remove the CRD-existence fallback from ShouldEndPluginExecution. The plugin now requires includedResources to list HCP types (hostedcontrolplane, hostedcluster, or nodepool) to proceed.
• MC-wide backups (includedResources: null) are correctly skipped, eliminating ~16K spurious errors and the misleading PartiallyFailed status. Backup data was never affected.
• Update test backup specs to include HCP resource types, matching real-world HC backup configurations.

Context

ShouldEndPluginExecution falls back to a CRD-existence check when includedResources is empty. On management clusters the HCP CRD is always present, so MC-wide backups (daily-full/weekly-full) are never skipped. The plugin runs on every item (~16K), errors on each because no HostedControlPlane can be associated, and marks the backup PartiallyFailed.

Every HC backup schedule on the staging MC (240 schedules) and every example in this repo specifies includedResources with HCP types. MC-wide schedules have includedResources: null.

Open question for the team: Can we require includedResources to list HCP types for the plugin to run? If an HC backup omits includedResources, the plugin would silently skip. All current schedules and examples already include them.

Test plan

• make verify passes
• MC-wide backup completes as Completed (not PartiallyFailed)
• HC backup with includedResources containing HCP types proceeds normally
• HC restore from a backup with includedResources proceeds normally

Summary by CodeRabbit

• Bug Fixes
  • Improved backup and restore plugin early-exit behavior to use the backup’s configured included resources (including hosted control plane-related resources) instead of checking for cluster resource availability.
  • Made skip/error behavior more consistent when namespaces or included resources are missing, leading to more predictable execution outcomes.
• Tests
  • Updated backup and restore test fixtures and assertions to match the new included-resources-driven stop condition, including wildcard and hosted resource scenarios.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@Alcamech: This pull request references Jira Issue OCPBUGS-97592, which is invalid:

• expected the bug to target the "5.0.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

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

Details

In response to this:

Summary

• Remove the CRD-existence fallback from ShouldEndPluginExecution. The plugin now requires includedResources to list HCP types (hostedcontrolplane, hostedcluster, or nodepool) to proceed.
• MC-wide backups (includedResources: null) are correctly skipped, eliminating ~16K errors and PartiallyFailed status.
• Update test backup specs to include HCP resource types, matching real-world HC backup configurations.

Context

ShouldEndPluginExecution falls back to a CRD-existence check when includedResources is empty. On management clusters the HCP CRD is always present, so MC-wide backups (daily-full/weekly-full) are never skipped. The plugin runs on every item (~16K), errors on each because no HostedControlPlane can be associated, and marks the backup PartiallyFailed.

Every HC backup schedule on the staging MC (240 schedules) and every example in this repo specifies includedResources with HCP types. MC-wide schedules have includedResources: null.

Open question for the team: Can we require includedResources to list HCP types for the plugin to run? If an HC backup omits includedResources, the plugin would silently skip. All current schedules and examples already include them.

Test plan

• make verify passes
• MC-wide backup completes as Completed (not PartiallyFailed)
• HC backup with includedResources containing HCP types proceeds normally
• HC restore from a backup with includedResources proceeds normally

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]

Warning

Review limit reached

@Alcamech, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 76a0cc27-0e2f-46db-b652-4d9be758e667

📥 Commits

Reviewing files that changed from the base of the PR and between ca29049 and e55e499.

📒 Files selected for processing (6)

• pkg/common/utils.go
• pkg/common/utils_test.go
• pkg/core/backup.go
• pkg/core/backup_test.go
• pkg/core/restore.go
• pkg/core/restore_test.go

Walkthrough

ShouldEndPluginExecution now determines execution solely from backup namespaces and included resources. Backup and restore callers use the reduced signature, with tests and fixtures updated for hypershift resource selections.

Changes

Spec-driven execution gating

Layer / File(s)	Summary
Resource specification gating pkg/common/utils.go	ShouldEndPluginExecution validates namespaces and continues for wildcard or hypershift-related resources without checking CRD existence or requiring client, context, or logger parameters.
Backup and restore integration pkg/core/backup.go, pkg/core/restore.go	Backup and restore execution paths call the reduced helper signature and retain the existing early-skip behavior.
Gating tests and fixtures pkg/common/utils_test.go, pkg/core/backup_test.go, pkg/core/restore_test.go	Tests cover namespace and resource-based decisions, and fixtures include hostedcontrolplane, hostedcluster, and nodepool.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: sjenning, celebdor, enxebre, jparrill

🚥 Pre-merge checks | ✅ 14 | ❌ 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 (14 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: skipping plugin execution when the backup lacks HCP-targeted resources.
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.
Stable And Deterministic Test Names	✅ Passed	No changed test title uses runtime data; all added/edited names are static string literals and none are fmt.Sprintf-generated.
Test Structure And Quality	✅ Passed	Changed tests are table-driven unit tests, use fake clients/TempDir cleanup, and add no cluster waits or leaked resources.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the changed tests are plain testing unit tests and use no MicroShift-unavailable APIs or assumptions.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the modified tests are unit tests only and include no SNO-specific multi-node assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	Only backup/restore early-exit logic and tests changed; no pod scheduling, affinity, node selectors, or topology-aware manifest code was introduced.
Ote Binary Stdout Contract	✅ Passed	No touched process-level code writes to stdout; changed files only adjust helper logic and tests, and the main entrypoint contains no Print/klog stdout calls.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the touched tests are unit tests and add only HCP resource specs, with no IPv4-only or external network assumptions.
No-Weak-Crypto	✅ Passed	The PR only changes backup-spec gating/tests; no weak crypto, custom crypto, or secret/token comparisons appear in the touched code.
Container-Privileges	✅ Passed	The PR only changes Go code and tests; no YAML/JSON manifests were modified and no privilege settings appear in the changed files.
No-Sensitive-Data-In-Logs	✅ Passed	Patch only changes plugin gating/tests; it adds no new log statements or sensitive-value logging.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Alcamech
Once this PR has been reviewed and has the lgtm label, please assign joeavaikath for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/common/utils.go (1)

212-221: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Treat * as a match here. Backup.Spec.IncludedResources can legally be ["*"], but this loop only recognizes explicit HCP resource names, so those backups fall through and get skipped. Short-circuit on * (or otherwise handle the all-resources case) so HCP restores still run.

🤖 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/common/utils.go` around lines 212 - 221, The resource filter in the
IncludedResources check currently only matches explicit hostedcluster,
hostedcontrolplane, and nodepool names, so backups configured with "*" are
incorrectly skipped. Update the logic in the loop inside the relevant utils
helper to treat "*" as a valid match for all resources and short-circuit to
allow the restore path, while keeping the existing explicit resource-name checks
intact.

🤖 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 `@pkg/common/utils.go`:
- Around line 212-221: The resource filter in the IncludedResources check
currently only matches explicit hostedcluster, hostedcontrolplane, and nodepool
names, so backups configured with "*" are incorrectly skipped. Update the logic
in the loop inside the relevant utils helper to treat "*" as a valid match for
all resources and short-circuit to allow the restore path, while keeping the
existing explicit resource-name checks intact.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8a6b9ffa-d7bc-4df2-9ee3-9a79cf7e776d

📥 Commits

Reviewing files that changed from the base of the PR and between e9dbba8 and 5fccb61.

📒 Files selected for processing (3)

• pkg/common/utils.go
• pkg/core/backup_test.go
• pkg/core/restore_test.go

[jparrill]

/uncc @enxebre

[jparrill]

/uncc @sjenning

[jparrill]

/cc @jparrill

[jparrill]

Dropped some comments. Thanks!

[coderabbitai]

🧹 Nitpick comments (1)

pkg/core/restore.go (1)

171-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove dead code.

This block is now unreachable. The common.ShouldEndPluginExecution(backup) check above returns early if len(backup.Spec.IncludedNamespaces) == 0, which inherently catches the case where the slice is nil.

♻️ Proposed refactor

-	// if the IncludedNamespaces field is nil, return error
-	if backup.Spec.IncludedNamespaces == nil {
-		p.log.Error("IncludedNamespaces from backup object is nil")
-		return nil, fmt.Errorf("included namespaces from backup object is nil")
-	}
-

🤖 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/core/restore.go` around lines 171 - 175, Remove the unreachable nil check
and its associated log/error return following the
common.ShouldEndPluginExecution(backup) check in the restore flow. Leave the
existing early-return behavior and subsequent restore logic unchanged.

🤖 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/core/restore.go`:
- Around line 171-175: Remove the unreachable nil check and its associated
log/error return following the common.ShouldEndPluginExecution(backup) check in
the restore flow. Leave the existing early-return behavior and subsequent
restore logic unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cfb9d369-cf00-449a-a481-bdeff89d2d18

📥 Commits

Reviewing files that changed from the base of the PR and between 5fccb61 and e9e35f3.

📒 Files selected for processing (6)

• pkg/common/utils.go
• pkg/common/utils_test.go
• pkg/core/backup.go
• pkg/core/backup_test.go
• pkg/core/restore.go
• pkg/core/restore_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/core/restore_test.go

[Alcamech]

@jparrill ready to tag

[openshift-ci]

@Alcamech: 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.