NO-ISSUE: Fix storage unit test panic

[pmtk]

Summary by CodeRabbit

• Bug Fixes
  • Improved reliability of configuration file handling to better manage edge cases with file notifications.
  • Enhanced validation for configuration files with clearer error reporting for invalid inputs.

[openshift-ci-robot]

@pmtk: This pull request explicitly references no jira issue.

Details

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Walkthrough

The PR improves lvmd configuration robustness by: validating that config YAML parses to a non-nil struct, making file-watch remove-event handling check file existence before defaulting, and refactoring watch tests to poll for expected state instead of using fixed delays.

Changes

LVMD Config Robustness

Layer / File(s)	Summary
Empty config validation pkg/config/lvmd/lvmd.go	NewLvmdConfigFromFile adds a post-unmarshal nil guard and returns an explicit error if the parsed Lvmd struct is nil.
Remove-event file existence check pkg/components/storage.go	Fsnotify Remove event handling now calls os.Stat on usrCfg to decide: if the file is gone, apply defaults; if it still exists, re-copy the user config.
Watch test polling infrastructure pkg/components/storage_test.go	Introduces timing constants and two helper functions to poll runtime config until it matches expected state or times out, and to load config after a settle delay. All watch test assertions replace fixed-delay waits with these polling helpers.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels

ready-for-human-review

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title refers to fixing a storage unit test panic, which is addressed but represents only part of the changeset; the broader fixes involve LVMD config handling and fsnotify robustness.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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 test names in storage_test.go are stable and deterministic, using static descriptive strings with no dynamic content, timestamps, UUIDs, or changing values.
Test Structure And Quality	✅ Passed	Tests meet all quality standards: single responsibility per block, proper timeouts (5s polling), descriptive assertion messages, auto-cleanup via t.TempDir(), helper functions marked correctly.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests are added in this PR. Changes are limited to Go unit tests and implementation code in pkg/components and pkg/config directories.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests added; PR contains only unit test and production code changes. SNO compatibility check is not applicable.
Topology-Aware Scheduling Compatibility	✅ Passed	PR contains only config handling and test improvements with no deployment manifests, scheduling constraints, or topology-aware concerns.
Ote Binary Stdout Contract	✅ Passed	PR modifies component packages and unit tests, not OTE test binaries. No process-level code (main, init, TestMain, suite setup) that could violate stdout JSON contract exists in modified files.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No Ginkgo e2e tests added. Changes are unit tests using Go's standard testing package, not Ginkgo tests. Check not applicable.
No-Weak-Crypto	✅ Passed	No weak cryptographic algorithms (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom crypto implementations, or unsafe secret comparisons found in the modified files.
Container-Privileges	✅ Passed	The PR modifies only Go source/test files (storage.go, storage_test.go, lvmd.go) with no container/K8s manifests or privileged settings present.
No-Sensitive-Data-In-Logs	✅ Passed	All new logging statements only log file paths, not sensitive data like passwords, tokens, API keys, PII, session IDs, internal hostnames, or customer configuration data.

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

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

level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2."
level=warning msg="Suggested new configuration:\nlinters:\n enable:\n - gomodguard_v2\n"
level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/apparentlymart/go-cidr@v1.1.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/go-systemd@v0.0.0-20190321100706-95778dfbb74e: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/google/go-cmp@v0.7.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/miekg/dns@v1.1.63: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/openshift/api@v0.0.0-20260408092441-8b086e6b9eb9: is

... [truncated 31032 characters] ...

elet: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-cli-plugin: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-controller: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n"

---

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/config/lvmd/lvmd.go (1)

40-46: ⚠️ Potential issue | 🔴 Critical

Fix yaml.Unmarshal(buf, &l) to yaml.Unmarshal(buf, l) and make the “empty file” check actually work

l := new(Lvmd) already yields a non-nil *Lvmd; passing &l gives **Lvmd, and the subsequent if l == nil check won’t reliably trigger on empty YAML, so empty lvmd files may be treated as successfully parsed. Use yaml.Unmarshal(buf, l) and detect empty input via buf (or validate required fields) rather than relying on the pointer being nilled by unmarshal.

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

In `@pkg/config/lvmd/lvmd.go` around lines 40 - 46, The current code passes &l to
yaml.Unmarshal and then checks if l == nil, which is wrong because l is created
via l := new(Lvmd) (a non-nil *Lvmd) and &l yields **Lvmd; change the Unmarshal
call to yaml.Unmarshal(buf, l) to pass the *Lvmd, and replace the ineffective
nil check with a real empty-file/validation check — for example, return an error
if len(buf) == 0 or validate required fields on the Lvmd struct (e.g.,
requiredName/ID fields) after unmarshalling; update error messages accordingly
to reference the lvmd file path p and the failing validation.

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

Outside diff comments:
In `@pkg/config/lvmd/lvmd.go`:
- Around line 40-46: The current code passes &l to yaml.Unmarshal and then
checks if l == nil, which is wrong because l is created via l := new(Lvmd) (a
non-nil *Lvmd) and &l yields **Lvmd; change the Unmarshal call to
yaml.Unmarshal(buf, l) to pass the *Lvmd, and replace the ineffective nil check
with a real empty-file/validation check — for example, return an error if
len(buf) == 0 or validate required fields on the Lvmd struct (e.g.,
requiredName/ID fields) after unmarshalling; update error messages accordingly
to reference the lvmd file path p and the failing validation.

---

ℹ️ Review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Enterprise

Run ID: 4cfdf85a-a192-46f3-ba53-729c97891d06

📥 Commits

Reviewing files that changed from the base of the PR and between a890f0c and 1a68caa.

📒 Files selected for processing (3)

• pkg/components/storage.go
• pkg/components/storage_test.go
• pkg/config/lvmd/lvmd.go

[pmtk]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/config/lvmd/lvmd.go (1)> 40-46: ⚠️ Potential issue | 🔴 Critical

Fix yaml.Unmarshal(buf, &l) to yaml.Unmarshal(buf, l) and make the “empty file” check actually work
l := new(Lvmd) already yields a non-nil *Lvmd; passing &l gives **Lvmd, and the subsequent if l == nil check won’t reliably trigger on empty YAML, so empty lvmd files may be treated as successfully parsed. Use yaml.Unmarshal(buf, l) and detect empty input via buf (or validate required fields) rather than relying on the pointer being nilled by unmarshal.

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

In `@pkg/config/lvmd/lvmd.go` around lines 40 - 46, The current code passes &l to
yaml.Unmarshal and then checks if l == nil, which is wrong because l is created
via l := new(Lvmd) (a non-nil *Lvmd) and &l yields **Lvmd; change the Unmarshal
call to yaml.Unmarshal(buf, l) to pass the *Lvmd, and replace the ineffective
nil check with a real empty-file/validation check — for example, return an error
if len(buf) == 0 or validate required fields on the Lvmd struct (e.g.,
requiredName/ID fields) after unmarshalling; update error messages accordingly
to reference the lvmd file path p and the failing validation.

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

Outside diff comments:
In `@pkg/config/lvmd/lvmd.go`:
- Around line 40-46: The current code passes &l to yaml.Unmarshal and then
checks if l == nil, which is wrong because l is created via l := new(Lvmd) (a
non-nil *Lvmd) and &l yields **Lvmd; change the Unmarshal call to
yaml.Unmarshal(buf, l) to pass the *Lvmd, and replace the ineffective nil check
with a real empty-file/validation check — for example, return an error if
len(buf) == 0 or validate required fields on the Lvmd struct (e.g.,
requiredName/ID fields) after unmarshalling; update error messages accordingly
to reference the lvmd file path p and the failing validation.

ℹ️ Review info

The finding is incorrect. yaml.Unmarshal(buf, &l) where l is *Lvmd passes a **Lvmd, which allows the unmarshaller to set the pointer to nil on empty input. This is the intended behavior — the if l == nil check works correctly.

Empirical verification:

package main

import (
      "fmt"
      "sigs.k8s.io/yaml"
)

type Lvmd struct {
      SocketName string `json:"socketName"`
}

func main() {
      l := new(Lvmd)
      fmt.Printf("before: l=%p, *l=%+v\n", l, *l)

      err := yaml.Unmarshal([]byte(""), &l)
      fmt.Printf("after empty unmarshal: l=%v, err=%v\n", l, err)

      l2 := new(Lvmd)
      err = yaml.Unmarshal([]byte("---\n"), &l2)
      fmt.Printf("after '---' unmarshal: l2=%v, err=%v\n", l2, err)

      l3 := new(Lvmd)
      err = yaml.Unmarshal(nil, &l3)
      fmt.Printf("after nil unmarshal: l3=%v, err=%v\n", l3, err)
}

Output:

before: l=0xc000012140, *l={SocketName:}
after empty unmarshal: l=<nil>, err=<nil>
after '---' unmarshal: l2=<nil>, err=<nil>
after nil unmarshal: l3=<nil>, err=<nil>

Changing to yaml.Unmarshal(buf, l) as suggested would break the nil check — the unmarshaller would write into the existing struct and l would never be nil.

[pmtk]

/test ?

[pmtk]

/test e2e-aws-tests-bootc-periodic-arm-el9 e2e-aws-tests-bootc-periodic-el10

[ggiguash]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggiguash, pmtk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ggiguash,pmtk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[copejon]

/verified by ci

[openshift-ci-robot]

@copejon: This PR has been marked as verified by ci.

Details

In response to this:

/verified by ci

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-merge-bot]

/retest-required

Remaining retests: 0 against base HEAD 0ce9584 and 2 for PR HEAD 1a68caa in total

[openshift-ci]

@pmtk: all tests passed!

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.