[mclasmeier]

Summary

Delete scanner-v4-db PVC during both Central and SecuredCluster teardown to prevent stale ownerReference conflicts on redeploy

Unlike central-db, the scanner-v4-db PVC is not managed by the operator's ReconcilePVCExtension — its ownerReference cleanup currently relies solely on K8s garbage collection. On quick redeploy (e.g. in CI), asynchronously executing GC may not haev finished before the new CR is created, leaving a stale ownerReference. The operator then tries to add a second controller: true reference, which K8s rejects, causing an error loop (#291).

Owner-name guards ensure the PVC is only deleted if owned by the component being torn down, so shared-namespace deployments remain safe.

Fixes #291, fixes #321.

Test plan

• Verify TestSecuredClusterWithOLMSwitch passes on OpenShift CI

[coderabbitai]

Walkthrough

Central and SecuredCluster teardown now request deletion of the scanner-v4-db PVC when it is owned by the corresponding custom resource. Existing owner checks and deletion-error handling remain in place.

Changes: Scanner PVC teardown

Layer / File(s)	Summary
Owner-scoped PVC deletion internal/deployer/deployer.go	Central and SecuredCluster teardown add scanner-v4-db to their respective deletion lists. The existing owner checks determine whether deletion proceeds.

Priority: Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: vladbologa

Merge Risk: Minimal · up to ff565

Central and SecuredCluster teardown now also delete the scanner-v4-db PVC when the corresponding custom resource owns it. This should reduce stale owner-reference conflicts on redeploy. Cleanup failures are still only logged and redeploy continues, as before, so a failed deletion can still leave a stale PVC. That is existing behavior rather than something this change introduces, so there is no merge-blocking risk.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly identifies the scanner-v4-db PVC cleanup addressed by the pull request. It does not state that teardown deletes the PVC, but it remains related to the main change.
Linked Issues check		The changes satisfy [#291] and [#321]. deleteCentralResources adds scanner-v4-db with OwnerName: centralCrName. deleteSecuredClusterResources adds the same PVC with `OwnerName: securedClusterC…
Out of Scope Changes check		The change is limited to teardown cleanup in internal/deployer/deployer.go. It adds deletion requests for the PVC required by [#291] and [#321], with owner guards that implement the stated safety re…
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…

Full details: Linked Issues check

Explanation

The changes satisfy [#291] and [#321]. deleteCentralResources adds scanner-v4-db with OwnerName: centralCrName. deleteSecuredClusterResources adds the same PVC with OwnerName: securedClusterCrName. The existing owner-name check skips deletion when retrieval fails or ownership does not match. This addresses stale owner references while protecting shared-namespace resources. No linked issue requires a specific automated test beyond this coding behavior.

Full details: Out of Scope Changes check

Explanation

The change is limited to teardown cleanup in internal/deployer/deployer.go. It adds deletion requests for the PVC required by [#291] and [#321], with owner guards that implement the stated safety requirement. No unrelated behavior or files are changed.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

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

[coderabbitai]

🔇 Additional comments: 1

internal/deployer/deployer.go-124-124: 1

124-124: Major · Functional Correctness · Quick win

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Stop redeployment when PVC cleanup cannot be verified.

If the owner lookup fails, the existing loop skips this PVC. If deletion fails, deployCentral and deploySecuredCluster log the teardown error and continue. Either path can recreate the CR while the stale PVC remains, causing the owner-reference conflict this change targets. Return lookup errors and prevent redeployment when cleanup fails. (raw.githubusercontent.com)

Also applies to: 187-187

---

Review info

Run configuration

• Source mode: Auto
• Repository: stackrox/roxie
• Branch: mc/scanner-v4-db-pvc-cleanup
• File: .coderabbit.yml
• Commit: ff5656c
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7d449a87-86eb-4142-91c3-42a85f9af86d

Commits

Reviewing files that changed from the base of the PR and between f78bf93 and ff5656c.

Files selected for processing: 1

• internal/deployer/deployer.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

[mclasmeier]

@coderabbitai, this (#324 (review)) is in some sense a pre-existing issue. Since the kubectl call involved here implements retries, I think the chances of this posing a problem in practice is quite low.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
gh api repos/stackrox/roxie/pulls/324/reviews/5684538414
gh api --paginate repos/stackrox/roxie/pulls/324/reviews/5684538414/comments

Length of output: 6059

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
ast-grep outline internal/deployer/deployer.go
sed -n '1,260p' internal/deployer/deployer.go
rg -n -C3 --type go 'func .*([Kk]ubectl|[Rr]etry)|retry\.|Retry\(|func .*deployCentral|func .*deploySecuredCluster' .

Length of output: 18148

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
ast-grep outline internal/k8s --items all
sed -n '1,230p' internal/k8s/kubectl.go
sed -n '417,451p' internal/deployer/deployer.go
rg -n -A45 -B3 --type go 'func RetrieveResourceFromCluster\(' internal/k8s

Length of output: 9281

---

@mclasmeier Agreed. Both the owner lookup and deletion use RunKubectl, which makes up to three attempts for recognized transient errors. This PR also reuses the existing cleanup error handling.

Retries do not eliminate failures, but the remaining risk does not justify my “Major” classification or requiring broader error-handling changes in this PR. I retract that recommendation.

_{You are interacting with an AI system.}