OCPBUGS-82532: Skip adoption of missing or terminal generated-by installplans

[haklein]

ensureSubscriptionInstallPlanState adopts the installplan named by the olm.generated-by annotation whenever a subscription has no installplan reference. That is only valid while the generating plan is still in progress. Once the plan has been GC'd or has reached a terminal phase, adoption wedges the namespace:

• plan missing: the NotFound error aborts syncResolvingNamespace before resolution, so no installplan is ever created again for any subscription in the namespace.
• plan complete: the adoption resets the subscription to UpgradePending with currentCSV=startingCSV, which no longer exists after upgrades. The change is never persisted and the sync bails out before resolution on every iteration.

Both are hit by deleting an unapproved installplan (a documented way to get a plan for a newer version when one is pending), on any namespace with dependency-generated subscriptions, e.g. ODF.

Description of the change:

Treat a missing plan as "nothing to adopt" and skip adoption of plans in a terminal phase so resolution can create a fresh installplan. Failed plans are still adopted when fail-forward is enabled, which depends on the subscription referencing them.

Motivation for the change:

Fixes OCPBUGS-82532

Architectural changes:

n/a

Testing remarks:

Unit tests cover the full decision table of the adoption path:

generated-by plan	fail-forward	expected
missing	-	no error, no adoption, resolution proceeds
exists, in progress	-	adopted (existing behavior)
lookup fails (non-404)	-	sync error propagated (existing behavior)
exists, Complete	-	not adopted, resolution proceeds
exists, Failed	off	not adopted, resolution proceeds
exists, Failed	on	adopted, subscription marked Failed (existing behavior)

The two "not adopted"/"no error" rows are regression tests and fail against the previous code.

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Bug fixes are accompanied by regression test(s)
• e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
• tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
• Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
• Docs updated or added to /doc
• Commit messages sensible and descriptive
• Tests marked as [FLAKE] are truly flaky and have an issue
• Code is properly formatted

Summary by CodeRabbit

• Bug Fixes

  • Improved InstallPlan reconciliation when annotated plans are missing, preventing unnecessary namespace resolution failures.
  • Safely handles completed and failed plans by verifying the subscription’s starting version before adoption.
  • Continues reporting unexpected API errors while ignoring expected missing-resource responses.

• Tests

  • Added coverage for InstallPlan adoption, fail-forward behavior, no-op scenarios, and transient API or version lookup errors.

[openshift-ci]

Hi @haklein. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[Copilot]

Pull request overview

This PR fixes a resolution-wedging edge case in the Catalog Operator’s namespace resolution loop by preventing ensureSubscriptionInstallPlanState from “adopting” an olm.generated-by InstallPlan when that InstallPlan is missing (GC’d) or has already reached a terminal phase, allowing normal resolution to proceed and create a fresh InstallPlan.

Changes:

• Treat NotFound when fetching the olm.generated-by InstallPlan as “nothing to adopt” (no error), so namespace resolution can continue.
• Skip adoption when the generating InstallPlan is in a terminal phase (Complete, and Failed when fail-forward is disabled).
• Add unit tests covering missing, transient-error, terminal-phase, and fail-forward adoption behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
pkg/controller/operators/catalog/operator.go	Avoids wedging namespace resolution by skipping adoption for missing or terminal olm.generated-by InstallPlans.
pkg/controller/operators/catalog/operator_test.go	Adds regression/unit tests for the new adoption-skipping behavior and error handling.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[tmshort]

/ok-to-test

[haklein]

/jira refresh

[tmshort]

/approve
(Assuming CI passes)

[joelanford]

@haklein, the OCPBUGS issue mentions ODF. However ODF does some funky stuff where it creates more Subscriptions in Manual approval mode and then has its own logic for automatically approving those manual subscriptions. I don't know if that functionality of ODF is at play with this bug, but this seems like a rather nuanced change in an area of the codebase that even the current SMEs lack deep understanding of the blast radius.

Before we move forward with this, I would ask that we try to reproduce with:

• a "simple" operator that doesn't have dependencies
• another operator that does have dependencies, but not named ODF :)

[joelanford]

/hold

For the above request.

[tmshort]

/approve cancel
The e2e tests have been failing, so revoking approval.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joelanford 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]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: db142cda-1cef-4e64-b70b-c4cbf8a02a74

📥 Commits

Reviewing files that changed from the base of the PR and between 1247251 and 8ef96ef.

📒 Files selected for processing (2)

• pkg/controller/operators/catalog/operator.go
• pkg/controller/operators/catalog/operator_test.go

---

📝 Walkthrough

Walkthrough

The subscription install-plan reconciliation now skips missing generated InstallPlans, adopts terminal plans only when startingCSV exists under applicable fail-forward rules, and propagates other errors. Tests cover adoption, no-op, missing-resource, and transient-error cases.

Changes

InstallPlan adoption reconciliation

Layer / File(s)	Summary
InstallPlan adoption rules pkg/controller/operators/catalog/operator.go	Missing annotated InstallPlans no longer block reconciliation. Terminal Complete plans and non-fail-forward Failed plans require an existing startingCSV; other lookup errors remain propagated.
Adoption behavior validation pkg/controller/operators/catalog/operator_test.go	Tests cover generated-plan adoption, missing plans, terminal-plan rules, fail-forward behavior, no-op cases, and transient InstallPlan or CSV lookup errors.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: fgiudici, pedjak, twogiants

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: skipping adoption of missing or terminal generated-by InstallPlans.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

Error: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:

• 'output.formats' expected a map, got 'string'
  The command is terminated due to an error: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:

• 'output.formats' expected a map, got 'string'

---

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

[haklein]

@joelanford thanks a lot for your feedback. Please bear with the slow reply, I was on PTO last week.

the OCPBUGS issue mentions ODF. However ODF does some funky stuff where it creates more Subscriptions in Manual approval mode and then has its own logic for automatically approving those manual subscriptions.

I've deployed ODF 4.20 in a lab, and so far the approval mode was automatic out of the box. We do have an ODF doc section "Changing the update approval strategy" which shows how to change to manual. So that's likely done by customers on numerous ODF installations

I don't know if that functionality of ODF is at play with this bug, but this seems like a rather nuanced change in an area of the codebase that even the current SMEs lack deep understanding of the blast radius.

This does reproduce outside of ODF, see below

Before we move forward with this, I would ask that we try to reproduce with:

* a "simple" operator that doesn't have dependencies

This doesn't shop up with a simple operator, only if one depends on another and when OLM itself creates that subscription. Tested with a dummy operator, and replacing the versions v0.1.0 to v0.4.0 (via replaces chain). That didn’t reproduce, as it was missing the olm.generated-by annotation. This annotation only gets added by dependency created subscriptions.

* another operator that _does_ have dependencies, but not named ODF :)

Results from a test with two operators, one depending on the other. testop v0.5.0 declares an olm.package dependency on testdep v0.1.0. A second catalog tag adds testop v0.6.0 and testdep v0.2.0, so that one pending InstallPlan references both subscriptions.

1. Create a subscription for testop v0.5.0 with installPlanApproval: Manual
2. Approve the InstallPlan. OLM installs both operators. OLM creates the testdep subscription and writes olm.generated-by on it
3. Change the catalog to the tag with v0.6.0. OLM creates a pending InstallPlan for both packages
4. Delete the InstallPlan that the annotation references. On longer running clusters, GC would have done this step (when an unapproved plan was lingering around while multiple z releases were released)
5. Delete the pending InstallPlan

Result: OLM does not create a new InstallPlan again. The subscriptions stay without a plan reference. The catalog-operator log shows this loop:

level=warning msg="unable to get installplan from cache" installplan=install-mtjsd sub=testdep-stable-testop-catalog-repro-dep
level=info msg="error ensuring installplan state: installplans.operators.coreos.com \"install-mtjsd\" not found"

As soon as the olm.generated-by is removed, a new plan is created and executed.

[haklein]

A new dependency subscription syncs after its generating installplan completes, and it needs adoption of that installplan to link to its installed CSV. The first version skipped all terminal InstallPlans, so these subscriptions failed with ConstraintsNotSatisfiable. The guard now skips a terminal InstallPlan only when spec.startingCSV no longer names an existing CSV (which is a failure condition of OCPBUGS-82532)
I added a unit test for the adoption case and rebased the branch onto master.