do not delete disabled resources by gazarenkov · Pull Request #2733 · redhat-developer/rhdh-operator

gazarenkov · 2026-04-29T16:06:22Z

Description

It excludes potential deleting the resources created when certain feature is disabled (such as local db, route, monitor)

Which issue(s) does this PR fix or relate to

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

PR acceptance criteria

Tests
Documentation

How to test changes / Special notes to the reviewer

For example:

Create empty CR (spec.database.enableLocalDb=true by default)
Check DB StatefulSet created
Modify CR with spec.database.enableLocalDb=false and apply
Check DB StatefulSet is still there

github-actions · 2026-04-29T16:12:36Z

PR images are available:

github-actions · 2026-04-29T17:32:18Z

PR images are available:

rm3l

Hmm.., it might be confusing to see that some resources are still there when the corresponding feature is purposely toggled from enabled to disabled. For example, I first create a Backstage CR on OCP (so with a Route created by default), then I set spec.application.route.enabled: false in the CR, and now the Route object is still present.
Maybe the Operator could add some uniquely identifiable labels on the resources it creates and manages, and only try to delete those to avoid conflict with user-created resources?
Or (thinking out loud) maybe just refuse to reconcile if there are conflicting resources that it does not manage, and thus avoid touching those?

gazarenkov · 2026-04-30T11:26:39Z

Hmm.., it might be confusing to see that some resources are still there when the corresponding feature is purposely toggled from enabled to disabled. For example, I first create a Backstage CR on OCP (so with a Route created by default), then I set spec.application.route.enabled: false in the CR, and now the Route object is still present. Maybe the Operator could add some uniquely identifiable labels on the resources it creates and manages, and only try to delete those to avoid conflict with user-created resources? Or (thinking out loud) maybe just refuse to reconcile if there are conflicting resources that it does not manage, and thus avoid touching those?

Let's just document it for clarity and keep simple until we really need to make it complex.
Those resources, with specific names, are "special", operator manages it if certain feature is enabled and ignores otherwise.

github-actions · 2026-05-06T13:40:17Z

PR images are available:

rm3l · 2026-05-12T13:56:45Z

Hmm.., it might be confusing to see that some resources are still there when the corresponding feature is purposely toggled from enabled to disabled. For example, I first create a Backstage CR on OCP (so with a Route created by default), then I set spec.application.route.enabled: false in the CR, and now the Route object is still present. Maybe the Operator could add some uniquely identifiable labels on the resources it creates and manages, and only try to delete those to avoid conflict with user-created resources? Or (thinking out loud) maybe just refuse to reconcile if there are conflicting resources that it does not manage, and thus avoid touching those?

Let's just document it for clarity and keep simple until we really need to make it complex. Those resources, with specific names, are "special", operator manages it if certain feature is enabled and ignores otherwise.

This would be kinda breaking change then, I guess.
But overall, I think that even with documentation, just leaving them as is might end up creating more confusion with orphaned resources that might still consume cluster resources. The example with the Route would be particularly confusing IMO as the route is still active (can accept external traffic) but is no longer managed by the Operator.
Since the operator already sets owner reference on the resources it creates, would it make sense to "just" check with metav1.IsControlledBy before deleting? I feel like this would be small targeted changed compared to the current PR which, IMHO, would be a behavioral change that might end up causing more confusion (in 1.10).

rm3l · 2026-05-18T15:30:38Z

Hmm.., it might be confusing to see that some resources are still there when the corresponding feature is purposely toggled from enabled to disabled. For example, I first create a Backstage CR on OCP (so with a Route created by default), then I set spec.application.route.enabled: false in the CR, and now the Route object is still present. Maybe the Operator could add some uniquely identifiable labels on the resources it creates and manages, and only try to delete those to avoid conflict with user-created resources? Or (thinking out loud) maybe just refuse to reconcile if there are conflicting resources that it does not manage, and thus avoid touching those?

Let's just document it for clarity and keep simple until we really need to make it complex. Those resources, with specific names, are "special", operator manages it if certain feature is enabled and ignores otherwise.

This would be kinda breaking change then, I guess. But overall, I think that even with documentation, just leaving them as is might end up creating more confusion with orphaned resources that might still consume cluster resources. The example with the Route would be particularly confusing IMO as the route is still active (can accept external traffic) but is no longer managed by the Operator. Since the operator already sets owner reference on the resources it creates, would it make sense to "just" check with metav1.IsControlledBy before deleting? I feel like this would be small targeted changed compared to the current PR which, IMHO, would be a behavioral change that might end up causing more confusion (in 1.10).

As discussed today, let's document this here (and also consider mentioning this in the official product docs).

rm3l · 2026-05-18T15:31:52Z

/cherry-pick release-1.10

openshift-cherrypick-robot · 2026-05-18T15:31:56Z

@rm3l: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-1.10

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.

sonarqubecloud · 2026-05-18T15:32:29Z

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 · 2026-05-18T15:36:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.05%. Comparing base (0e1d1db) to head (056699b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2733      +/-   ##
==========================================
- 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 · 2026-05-18T15:37:19Z

PR images are available:

rm3l · 2026-05-19T10:20:33Z

/agentic_review

rhdh-qodo-merge · 2026-05-19T10:20:43Z

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

✅ Tickets: RHDHBUGS-2781

✅ Compliance rules (platform): 18 rules

1. Monitoring disable doesn’t deactivate 🐞 Bug ≡ Correctness

Description

applyServiceMonitor now returns nil when spec.monitoring.enabled is false, leaving any
previously-created ServiceMonitor in place so Prometheus scraping continues even though monitoring
is disabled in the CR. This contradicts the field’s documented intent (“Enable ServiceMonitor for
Prometheus scraping”) and makes toggling monitoring off ineffective without manual cleanup.

Code

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

	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
	}

	sm := &monitoringv1.ServiceMonitor{

Relevance

⭐⭐ Medium
Historically monitoring-disable deleted ServiceMonitor (PR #1374), but later changes reduced
deletion behavior focus (PR #1515/#1552).
PR-#1374
PR-#1515
PR-#1552

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The controller now does nothing on monitoring disable, while the CRD field description indicates
this flag controls whether ServiceMonitor-based scraping is enabled; keeping an existing
ServiceMonitor means scraping remains enabled. The unit test still creates an existing
ServiceMonitor in the disabled case and no longer verifies any cleanup, consistent with the new
(potentially incorrect) behavior.

internal/controller/monitor.go[18-23]
api/v1alpha5/backstage_types.go[94-99]
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
`applyServiceMonitor()` now returns early when `spec.monitoring.enabled` is false, leaving any existing `ServiceMonitor` untouched. As a result, disabling monitoring in the CR does not actually stop scraping if a `ServiceMonitor` already exists.

### Issue Context
- The API field is documented as enabling ServiceMonitor-based scraping; users will reasonably expect toggling it off to stop scraping.
- The previous implementation deleted the ServiceMonitor when disabled; that behavior was removed.

### Fix Focus Areas
- internal/controller/monitor.go[18-27]
- api/v1alpha5/backstage_types.go[94-99]

### What to change
Implement a deterministic “disabled” behavior that matches the CR semantics:
1. When `spec.monitoring.enabled == false`, actively ensure scraping is disabled by either:
  - deleting the `ServiceMonitor` (restore prior behavior), or
  - patching it into a non-scraping state (e.g., selector that matches nothing) while keeping the object.
2. Update the CRD field description (and/or operator docs) to explicitly state the behavior on disable if you intentionally keep the object.
3. Add/adjust unit tests to validate the chosen behavior when toggling from enabled -> disabled.

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

2. Missing local DB toggle test 🐞 Bug ☼ Reliability

Description

The integration test that exercised toggling enableLocalDb from default true to false was removed,
leaving no integration coverage for the PR’s stated behavior (resources should remain after
disabling). This makes it easy to regress the intended behavior without detection.

Code

integration_tests/db_test.go[L42-87]

-	It("creates default Backstage and then update CR to not to use local DB", func() {
-		backstageName := createAndReconcileBackstage(ctx, ns, api.BackstageSpec{}, "")
-
-		Eventually(func(g Gomega) {
-			By("creating Deployment with database.enableLocalDb=true by default")
-
-			err := k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)}, &appsv1.StatefulSet{})
-			g.Expect(err).To(Not(HaveOccurred()))
-
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)}, &corev1.Service{})
-			g.Expect(err).To(Not(HaveOccurred()))
-
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-secret-%s", backstageName)}, &corev1.Secret{})
-			g.Expect(err).To(Not(HaveOccurred()))
-
-		}, time.Minute, time.Second).Should(Succeed())
-
-		By("updating Backstage")
-		update := &api.Backstage{}
-		err := k8sClient.Get(ctx, types.NamespacedName{Name: backstageName, Namespace: ns}, update)
-		Expect(err).To(Not(HaveOccurred()))
-		update.Spec.Database = &api.Database{}
-		update.Spec.Database.EnableLocalDb = ptr.To(false)
-		err = k8sClient.Update(ctx, update)
-		Expect(err).To(Not(HaveOccurred()))
-		_, err = NewTestBackstageReconciler(ns).ReconcileAny(ctx, reconcile.Request{
-			NamespacedName: types.NamespacedName{Name: backstageName, Namespace: ns},
-		})
-		Expect(err).To(Not(HaveOccurred()))
-
-		Eventually(func(g Gomega) {
-			By("deleting Local Db StatefulSet, Service and Secret")
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)}, &appsv1.StatefulSet{})
-			g.Expect(err).To(HaveOccurred())
-			g.Expect(errors.IsNotFound(err))
-
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-%s", backstageName)}, &corev1.Service{})
-			g.Expect(err).To(HaveOccurred())
-			g.Expect(errors.IsNotFound(err))
-
-			err = k8sClient.Get(ctx, types.NamespacedName{Namespace: ns, Name: fmt.Sprintf("backstage-psql-secret-%s", backstageName)}, &corev1.Secret{})
-			g.Expect(err).To(HaveOccurred())
-			g.Expect(errors.IsNotFound(err))
-		}, time.Minute, time.Second).Should(Succeed())
-
-	})

Relevance

⭐⭐⭐ High
Team often accepts adding/strengthening integration test coverage (e.g., added explicit integration
coverage in PR #1406).
PR-#1406
PR-#2102

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The current integration tests only cover the 'created with local DB disabled' case and do not cover
toggling the flag after initial creation, which is the scenario the PR is intended to change.

integration_tests/db_test.go[38-79]

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 integration test validating behavior when toggling `spec.database.enableLocalDb` at runtime was removed instead of being updated to assert the new expected behavior (DB resources are retained).

### Issue Context
The PR description/acceptance criteria explicitly calls out the toggle scenario (enableLocalDb true by default, then set to false, DB StatefulSet should still exist). There is currently only coverage for creating a CR with local DB disabled from the start.

### Fix Focus Areas
- integration_tests/db_test.go[22-81]

### What to change
1. Reintroduce an integration test that:
  - Creates a Backstage CR with default DB settings (local DB enabled by default).
  - Verifies the DB StatefulSet exists.
  - Updates the CR to `spec.database.enableLocalDb=false` and reconciles.
  - Verifies the DB StatefulSet still exists (and optionally Service/Secret as well, depending on desired behavior).
2. Ensure the test uses the same reconciliation helper patterns as existing integration tests (create, update, reconcile, Eventually assertions).

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

3. Misleading monitoring test comments 🐞 Bug ⚙ Maintainability

Description

TestApplyServiceMonitor_MonitoringDisabled still contains comments stating it tests deletion, but
the deletion assertions were removed. This makes the test intent unclear and risks future incorrect
modifications.

Code

internal/controller/monitor_test.go[R70-74]

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

func TestApplyServiceMonitor_MonitoringEnabled_NoCRD(t *testing.T) {

Relevance

⭐⭐⭐ High
They’ve accepted fixing misleading/typo comments in tests/docs for clarity (PR #1759).
PR-#1759

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test still sets up an existing ServiceMonitor and has comments claiming it is testing deletion,
but it now only asserts that applyServiceMonitor returns no error, leaving the behavior unverified
and the comments inaccurate.

internal/controller/monitor_test.go[58-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
`TestApplyServiceMonitor_MonitoringDisabled` retains comments indicating it should delete an existing ServiceMonitor, but the test no longer verifies deletion.

### Issue Context
This mismatch makes it unclear whether the intended behavior is deletion, no-op, or something else when monitoring is disabled.

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

### What to change
1. Update comments (and optionally the test name) to reflect the actual expected behavior.
2. Add an explicit assertion matching the chosen semantics (e.g., ServiceMonitor still exists, or is deleted, etc.), so the test validates behavior rather than only "no error".

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

rm3l

Getting this merged (so we can have it in time for 1.10) and I'll create a follow up issue to clarify the behavior in the docs (can be done in 1.10.z or 2.y)
EDIT: Created https://redhat.atlassian.net/browse/RHIDP-13670

rm3l · 2026-05-19T13:03:09Z

/hold cancel

openshift-cherrypick-robot · 2026-05-19T13:04:45Z

@rm3l: #2733 failed to apply on top of branch "release-1.10":

Applying: do not delete disabled resources
Using index info to reconstruct a base tree...
M	bundle/rhdh/manifests/backstage-operator.clusterserviceversion.yaml
M	internal/controller/backstage_controller.go
Falling back to patching base and 3-way merge...
Removing internal/controller/backstage_controller_test.go
Auto-merging internal/controller/backstage_controller.go
Auto-merging bundle/rhdh/manifests/backstage-operator.clusterserviceversion.yaml
CONFLICT (content): Merge conflict in bundle/rhdh/manifests/backstage-operator.clusterserviceversion.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 do not delete disabled resources

Details

In response to this:

/cherry-pick release-1.10

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.

do not delete disabled resources

3ef8b9f

openshift-ci Bot requested review from Fortune-Ndlovu and subhashkhileri April 29, 2026 16:06

gazarenkov temporarily deployed to internal April 29, 2026 16:06 — with GitHub Actions Inactive

do not delete disabled resources

d3dd75e

gazarenkov temporarily deployed to internal April 29, 2026 17:26 — with GitHub Actions Inactive

gazarenkov requested review from rm3l April 30, 2026 06:48

rm3l reviewed Apr 30, 2026

View reviewed changes

openshift-ci Bot added the needs-rebase label Apr 30, 2026

Merge branch 'main' into do-not-delete-local-db-resources

3ce8528

gazarenkov temporarily deployed to internal May 6, 2026 13:34 — with GitHub Actions Inactive

openshift-ci Bot removed the needs-rebase label May 6, 2026

Merge branch 'main' into do-not-delete-local-db-resources

056699b

rm3l requested a review from a team as a code owner May 18, 2026 15:32

rm3l temporarily deployed to internal May 18, 2026 15:32 — with GitHub Actions Inactive

rhdh-qodo-merge Bot reviewed May 19, 2026

View reviewed changes

Comment thread internal/controller/monitor.go

rm3l approved these changes May 19, 2026

View reviewed changes

openshift-ci Bot assigned rm3l May 19, 2026

openshift-ci Bot added the lgtm label May 19, 2026

rm3l added the do-not-merge/hold label May 19, 2026

openshift-ci Bot removed the do-not-merge/hold label May 19, 2026

rm3l merged commit 4fd1e80 into redhat-developer:main May 19, 2026
10 of 11 checks passed

rm3l mentioned this pull request May 19, 2026

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

Merged

gazarenkov deleted the do-not-delete-local-db-resources branch June 17, 2026 12:50

Uh oh!

Conversation

gazarenkov commented Apr 29, 2026

Description

Which issue(s) does this PR fix or relate to

PR acceptance criteria

How to test changes / Special notes to the reviewer

Uh oh!

github-actions Bot commented Apr 29, 2026

Uh oh!

github-actions Bot commented Apr 29, 2026

Uh oh!

rm3l left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gazarenkov commented Apr 30, 2026

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

rm3l commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rm3l commented May 18, 2026

Uh oh!

rm3l commented May 18, 2026

Uh oh!

openshift-cherrypick-robot commented May 18, 2026

Uh oh!

sonarqubecloud Bot commented May 18, 2026

Quality Gate passed

Uh oh!

codecov Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

rm3l commented May 19, 2026

Uh oh!

rhdh-qodo-merge Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

rm3l left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rm3l commented May 19, 2026

Uh oh!

Uh oh!

openshift-cherrypick-robot commented May 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rm3l left a comment •

edited

Loading

rm3l commented May 12, 2026 •

edited

Loading

codecov Bot commented May 18, 2026 •

edited

Loading

rhdh-qodo-merge Bot commented May 19, 2026 •

edited

Loading

rm3l left a comment •

edited

Loading