[release-1.10] fix: do not delete disabled resources [RHDHBUGS-2781]

[rm3l]

Manual cherry-pick of #2733

ref: https://redhat.atlassian.net/browse/RHDHBUGS-2781

[rhdh-qodo-merge]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0)

Context used

✅ Compliance rules (platform): 18 rules

1. Docs contradict monitoring behavior 🐞 Bug ≡ Correctness

Description

When spec.monitoring.enabled=false, applyServiceMonitor now returns without deleting an existing
ServiceMonitor, but docs state the operator will automatically clean up ServiceMonitor resources
when monitoring is disabled. This mismatch will mislead users and can leave monitoring active even
after they follow the documented disable procedure.

Code

internal/controller/monitor.go[R21-23]

	if !backstage.Spec.IsMonitoringEnabled() {
-		lg.Info("monitoring disabled, deleting any existing ServiceMonitor")
-		return r.tryToDelete(ctx,
-			&monitoringv1.ServiceMonitor{},
-			utils.GenerateRuntimeObjectName(backstage.Name, "metrics"),
-			backstage.Namespace,
-		)
+		return nil
	}

Relevance

⭐ Low

Same behavior/doc mismatch (ServiceMonitor not deleted when disabled) already merged; doc-update
suggestion rejected in PR #2733.

PR-#2733

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The controller now performs no cleanup when monitoring is disabled, while the documentation
explicitly promises automatic cleanup on disable.

internal/controller/monitor.go[18-23]
docs/monitoring.md[87-115]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`applyServiceMonitor()` no longer cleans up an existing `ServiceMonitor` when monitoring is disabled, but `/docs/monitoring.md` explicitly states the operator will automatically clean up existing `ServiceMonitor` resources when monitoring is disabled. Users following the docs will expect the ServiceMonitor to be removed.

### Issue Context
The PR removed deletion behavior (now returns `nil` when disabled). If the intended new contract is “do not delete disabled resources”, documentation must be updated. If the intended contract is “do not delete *user-created* resources”, the safer implementation is to delete **only** ServiceMonitors that are controlled (ownerRef) by the Backstage CR.

### Fix Focus Areas
- internal/controller/monitor.go[18-26]
- docs/monitoring.md[87-115]
- internal/controller/monitor_test.go[48-72]

### Suggested fix approach
Option A (recommended if you still want cleanup of operator-owned resources):
1. When monitoring is disabled, `GET` the ServiceMonitor by name.
2. If not found: return nil.
3. If found and `metav1.IsControlledBy(sm, backstage)` is true: delete it.
4. If found but not controlled by the Backstage CR: do **not** delete (preserves user-managed resources).
5. Keep ignoring `meta.IsNoMatchError` (CRD missing) to avoid failures on clusters without the CRD.
6. Update `TestApplyServiceMonitor_MonitoringDisabled` to assert the correct behavior (deleted only if controlled, preserved otherwise).

Option B (if the new contract is “never delete on disable”):
1. Update `/docs/monitoring.md` to remove the claim about automatic cleanup on disable and explicitly instruct users to manually delete the ServiceMonitor if they want it removed.
2. Update the unit test comments/assertions to reflect the new behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Docs contradict localDB disable 🐞 Bug ⚙ Maintainability

Description

BackstageReconciler no longer runs any cleanup after reconciliation, so disabling local DB
(spec.database.enableLocalDb=false) will not delete the local DB StatefulSet/Service/Secret
anymore. This conflicts with the DB migration doc, which instructs users that the operator will
delete the StatefulSet/Pod(s) when local DB is disabled, affecting operational workflows like PVC
cleanup.

Code

internal/controller/backstage_controller.go[R114-118]

		return ctrl.Result{}, errorAndStatus(&backstage, "failed to apply backstage objects", err)
	}

-	if err := r.cleanObjects(ctx, backstage); err != nil {
-		return ctrl.Result{}, errorAndStatus(&backstage, "failed to clean backstage objects ", err)
-	}
-
	r.setDeploymentStatus(ctx, &backstage, *bsModel)
	return ctrl.Result{}, nil

Relevance

⭐ Low

Similar doc-mismatch warning was definitely rejected in PR #2733 (team chose not to update docs).

PR-#2733

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Reconcile now applies objects and exits with no cleanup step; documentation still states disabling
local DB deletes StatefulSet/Pods. Also, operator-managed objects are normally owner-referenced to
the Backstage CR, enabling safe “delete-only-if-owned” semantics if desired.

internal/controller/backstage_controller.go[89-119]
docs/db_migration.md[98-105]
pkg/model/runtime.go[203-215]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Cleanup of local DB (and route) resources on feature disable was removed from the reconcile loop. Existing docs (`/docs/db_migration.md`) describe the old behavior (StatefulSet/Pods are deleted when `enableLocalDb=false`). This mismatch can cause users to follow incorrect procedures (e.g., attempting PVC cleanup while the StatefulSet still exists).

### Issue Context
The PR goal suggests preserving disabled resources to avoid deleting user-created objects. However, docs and possibly user expectations still reflect cleanup-on-disable.

### Fix Focus Areas
- internal/controller/backstage_controller.go[89-119]
- docs/db_migration.md[98-105]
- pkg/model/runtime.go[203-215]

### Suggested fix approach
Choose one and make it explicit:

Option A (keep cleanup for operator-owned resources only):
- Reintroduce a cleanup phase that deletes resources **only if** they are controlled by the Backstage CR (ownerRef), leveraging the fact that the model sets controller references for operator-managed objects.
- This preserves user-created resources (no ownerRef), while still preventing resource leaks for operator-created ones.

Option B (no cleanup on disable is the new contract):
- Update `/docs/db_migration.md` to state that disabling local DB will **not** delete the StatefulSet/Pods automatically, and provide manual deletion steps and PVC cleanup prerequisites accordingly.

In both options:
- Add/adjust tests (integration or unit) to lock in the intended contract.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Monitoring test loses assertion 🐞 Bug ⚙ Maintainability

Description

TestApplyServiceMonitor_MonitoringDisabled still claims it "should delete the existing one" but no
longer verifies whether the ServiceMonitor was deleted or preserved. This removes coverage for the
PR’s core behavioral change and leaves the intended contract ambiguous.

Code

internal/controller/monitor_test.go[R69-72]

	// Apply service monitor (should delete the existing one)
	err = r.applyServiceMonitor(ctx, backstage)
	assert.NoError(t, err)
-
-	// Verify ServiceMonitor was deleted
-	sm := &monitoringv1.ServiceMonitor{}
-	err = r.Get(ctx, types.NamespacedName{
-		Name:      utils.GenerateRuntimeObjectName(backstage.Name, "metrics"),
-		Namespace: backstage.Namespace,
-	}, sm)
-	assert.True(t, apierrors.IsNotFound(err))
}

Relevance

⭐ Low

PR #2733 intentionally removed the ServiceMonitor deletion assertion; indicates team accepts
dropping this check.

PR-#2733

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test explicitly sets up an existing ServiceMonitor for deletion but no longer checks whether it
was deleted, despite comments stating that was the purpose.

internal/controller/monitor_test.go[48-72]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The monitoring-disabled unit test no longer asserts the post-condition (deleted vs preserved ServiceMonitor) and retains outdated comments. This makes it easy for future changes to unintentionally reintroduce deletion or break preservation without detection.

### Issue Context
The PR changes semantics around deletion. Tests should explicitly encode the intended behavior:
- If the new behavior is “never delete on disable”: assert the ServiceMonitor still exists.
- If the new behavior is “delete only operator-owned”: add two cases (controlled-by Backstage => deleted; not controlled => preserved).

### Fix Focus Areas
- internal/controller/monitor_test.go[48-72]

### Suggested fix approach
- Update the test name/comment to match the intended behavior.
- Add `Get` + assertions after calling `applyServiceMonitor` to verify the ServiceMonitor state.
- If implementing ownership-based deletion, construct one ServiceMonitor with ownerRef set to the Backstage object and another without, and assert correct outcomes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.05%. Comparing base (52ebcf6) to head (bdb2862).

Additional details and impacted files

@@               Coverage Diff                @@
##           release-1.10    #2863      +/-   ##
================================================
- Coverage         61.18%   61.05%   -0.13%     
================================================
  Files                37       37              
  Lines              2128     2098      -30     
================================================
- Hits               1302     1281      -21     
+ Misses              692      683       -9     
  Partials            134      134              

Flag	Coverage Δ
nightly	?
unittests	61.05% <100.00%> (-0.13%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
internal/controller/backstage_controller.go	0.00% <ø> (-10.21%)	⬇️
internal/controller/monitor.go	94.87% <100.00%> (-0.59%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

PR images are available:

1. https://quay.io/rhdh-community/operator:0.10.0-pr-2863-bdb2862
2. https://quay.io/rhdh-community/operator-bundle:0.10.0-pr-2863-bdb2862
3. https://quay.io/rhdh-community/operator-catalog:0.10.0-pr-2863-bdb2862

[rhdh-qodo-merge]

Review Summary by Qodo

Prevent automatic deletion of disabled resources in CR

🐞 Bug fix

Walkthroughs

Description

• Remove automatic deletion of disabled resources when features are disabled
• Resources now persist when corresponding CR features are disabled
• Eliminates unintended cleanup of user-created resources
• Simplifies monitoring and database cleanup logic

Diagram

flowchart LR
  A["Disabled Features in CR"] -->|Previously| B["Auto-delete Resources"]
  A -->|Now| C["Preserve Resources"]
  B --> D["User Data Loss"]
  C --> E["User Control"]

Loading

File Changes

1. internal/controller/backstage_controller.go 🐞 Bug fix +1/-58

Remove automatic resource cleanup logic

• Removed cleanObjects() method that automatically deleted disabled resources
• Removed tryToDelete() helper method for resource cleanup
• Removed cleanup call from Reconcile() method
• Removed unused imports related to deletion logic

internal/controller/backstage_controller.go

---

2. internal/controller/monitor.go 🐞 Bug fix +1/-6

Stop deleting ServiceMonitor when disabled

• Changed applyServiceMonitor() to return early when monitoring is disabled
• Removed automatic deletion of ServiceMonitor when monitoring feature is disabled
• Simplified logic to preserve existing resources

internal/controller/monitor.go

---

3. integration_tests/db_test.go 🧪 Tests +0/-51

Remove test for resource deletion behavior

• Removed test case that verified deletion of local DB resources
• Removed unused imports (corev1, reconcile)
• Kept test for creating Backstage with disabled local DB

integration_tests/db_test.go

---

View more (2)

4. internal/controller/backstage_controller_test.go 🧪 Tests +0/-83

Remove tryToDelete unit tests

• Deleted entire test file containing unit tests for tryToDelete() method
• Tests covered success, not found, no match, and error scenarios

internal/controller/backstage_controller_test.go

---

5. internal/controller/monitor_test.go 🧪 Tests +0/-9

Remove ServiceMonitor deletion verification

• Removed verification that ServiceMonitor was deleted when monitoring disabled
• Removed unused import apierrors
• Kept test setup but removed deletion assertions

internal/controller/monitor_test.go

---