fix: handle missing LVMVolumeGroupNodeStatus gracefully in vg-manager

[lmiccini]

The vg-manager reconciler errors hard when the LVMVolumeGroupNodeStatus for its node does not yet exist, causing a permanent stuck state on some nodes during bootstrap. This happens because the LVMCluster controller creates NodeStatus resources in parallel with other resources, and the vg-manager can start reconciling before its NodeStatus is created.

Handle the NotFound case by requeueing after 10 seconds instead of returning a hard error, giving the LVMCluster controller time to create the resource.

Summary by CodeRabbit

• Bug Fixes
  • When a required node status is not yet present, operations now log this and automatically retry after 10 seconds instead of treating it as an error.
  • Improved handling of other status retrieval failures so they surface as errors; tests updated to reflect the new retry behavior and to separate “missing status” and “retrieval failure” cases.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: dd0eafd5-946e-467d-95b6-d7f266e83695

📥 Commits

Reviewing files that changed from the base of the PR and between d309454 and c0c1f5d.

📒 Files selected for processing (2)

• internal/controllers/vgmanager/controller.go
• internal/controllers/vgmanager/controller_test.go

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

• internal/controllers/vgmanager/controller.go
• internal/controllers/vgmanager/controller_test.go

---

Walkthrough

The Reconciler.Reconcile method now distinguishes missing LVMVolumeGroupNodeStatus from other retrieval errors: if not found it logs and requeues after 10 seconds returning no error; other Get errors are still returned as before.

Changes

LVMVolumeGroupNodeStatus Not Found Error Handling

Layer / File(s)	Summary
Status not found requeue path and tests internal/controllers/vgmanager/controller.go, internal/controllers/vgmanager/controller_test.go	Reconcile checks for apierrors.IsNotFound when fetching LVMVolumeGroupNodeStatus, logs that the status hasn't been created yet, and requeues after 10 seconds returning nil error. Tests updated to expect a successful reconcile with RequeueAfter: 10 * time.Second for the missing-status case and to cover non-NotFound Get failures.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

ready-for-human-review

🚥 Pre-merge checks | ✅ 13 | ❌ 2

❌ Failed checks (2 warnings)

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.
Test Structure And Quality	⚠️ Warning	testNodeSelector violates single responsibility (6 unrelated behaviors in 1 test) and lacks meaningful assertion messages on 5 of 10 Expect statements testing equality assertions.	Split testNodeSelector into focused tests (one per behavior) and add failure messages to all Expect assertions, e.g., Expect(res).To(Equal(...), "expected RequeueAfter 10s when status not found").

✅ Passed checks (13 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly describes the main change: handling missing LVMVolumeGroupNodeStatus gracefully instead of returning errors.
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	All Ginkgo test names are stable and deterministic with no dynamic information like pod names, timestamps, UUIDs, node names, or variable interpolation.
Microshift Test Compatibility	✅ Passed	The PR modifies internal/controllers/vgmanager/controller_test.go, which contains unit tests (not e2e tests). The custom check applies only to new Ginkgo e2e tests, which this PR does not add.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new e2e tests added. PR modifies existing unit/controller tests in internal/controllers/vgmanager/, not e2e tests. Check not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	PR modifies error handling in vg-manager controller for missing LVMVolumeGroupNodeStatus—no scheduling constraints introduced.
Ote Binary Stdout Contract	✅ Passed	Tests use GinkgoWriter for logging; main code uses controller-runtime logger (stderr). No stdout writes detected in process-level code or tests.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PR does not add new e2e tests; it only modifies unit tests in internal/controllers/vgmanager/. Check applies only to new e2e tests in test/e2e/.
No-Weak-Crypto	✅ Passed	PR introduces no cryptographic code. Changes only affect LVMVolumeGroupNodeStatus retrieval error handling in the vg-manager controller.
Container-Privileges	✅ Passed	PR only modifies Go controller code, not container/K8s manifests. No privileged settings, hostPID/Network/IPC, SYS_ADMIN capabilities, or allowPrivilegeEscalation introduced.
No-Sensitive-Data-In-Logs	✅ Passed	Reviewed all logging statements in the PR. No passwords, tokens, API keys, PII, session IDs, internal hostnames, or customer data found in any logs.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 and usage tips.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

[openshift-ci]

Welcome @lmiccini! It looks like this is your first PR to openshift/lvm-operator 🎉

[openshift-ci]

Hi @lmiccini. Thanks for your PR.

I'm waiting for a openshift 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.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

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.

[pacevedom]

/ok-to-test

[pacevedom]

/ok-to-test

[coderabbitai]

Actionable comments posted: 1

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

Inline comments:
In `@internal/controllers/vgmanager/controller_test.go`:
- Around line 804-815: The Reconciler in this test is built without Namespace so
getLVMVolumeGroupNodeStatus() queries the empty namespace and the test misses
the real race; set Namespace on the Reconciler to the volumeGroup's namespace
(e.g., Namespace: volumeGroup.GetNamespace()) when constructing the Reconciler
used for the "status does not exist yet" case and do the same for the subsequent
variant that injects a Get error so both use the correct namespace for Get
calls; update the Reconciler construction lines referencing Reconciler,
volumeGroup, and the injected-Get-error test to include Namespace:
volumeGroup.GetNamespace().

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 432d2290-34cc-4f5d-afed-a865628ee683

📥 Commits

Reviewing files that changed from the base of the PR and between 5e56bfe and d309454.

📒 Files selected for processing (2)

• internal/controllers/vgmanager/controller.go
• internal/controllers/vgmanager/controller_test.go

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

• internal/controllers/vgmanager/controller.go

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.92%. Comparing base (7aa53f9) to head (c0c1f5d).
⚠️ Report is 146 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2462      +/-   ##
==========================================
+ Coverage   52.25%   56.92%   +4.66%     
==========================================
  Files          52       53       +1     
  Lines        3896     4374     +478     
==========================================
+ Hits         2036     2490     +454     
- Misses       1697     1708      +11     
- Partials      163      176      +13     

Files with missing lines	Coverage Δ
internal/controllers/vgmanager/controller.go	77.60% <100.00%> (+7.35%)	⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[lmiccini]

/test e2e-aws-hypershift

[openshift-ci]

@lmiccini: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/snyk-deps	c0c1f5d	link	false	/test snyk-deps

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.

[lmiccini]

/retest

[pacevedom]

/ok-to-test

[lmiccini]

/retest