From 7d8cb5bf846a23a15a309bc57ed392dcc505e64f Mon Sep 17 00:00:00 2001 From: Patryk Matuszak Date: Wed, 27 May 2026 15:53:53 +0200 Subject: [PATCH 1/3] Handle nil Lvmd after unmarshalling empty config file yaml.Unmarshal can set the Lvmd pointer to nil when the file is empty (e.g. read during concurrent truncation by the config watcher). This caused a nil pointer dereference panic at l.SocketName. Return an error instead. Co-Authored-By: Claude Opus 4.6 --- pkg/config/lvmd/lvmd.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/config/lvmd/lvmd.go b/pkg/config/lvmd/lvmd.go index 57ed6615e1..b08b3c9474 100644 --- a/pkg/config/lvmd/lvmd.go +++ b/pkg/config/lvmd/lvmd.go @@ -41,6 +41,9 @@ func NewLvmdConfigFromFile(p string) (*Lvmd, error) { if err != nil { return nil, fmt.Errorf("unmarshalling lvmd file: %w", err) } + if l == nil { + return nil, fmt.Errorf("lvmd config file %q is empty", p) + } if l.SocketName == "" { l.SocketName = defaultSockName } From 4b3eb9226d323cc5cc952567699620bac21f76ed Mon Sep 17 00:00:00 2001 From: Patryk Matuszak Date: Wed, 27 May 2026 15:54:20 +0200 Subject: [PATCH 2/3] Guard lvmd watcher against spurious inotify Remove events os.WriteFile with O_TRUNC can produce spurious IN_DELETE inotify events on Linux. The watcher would treat these as real deletions and fall back to default config, overwriting the user's config. Verify the file is actually gone before falling back to defaults; if it still exists, re-apply the user config. Co-Authored-By: Claude Opus 4.6 --- pkg/components/storage.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/components/storage.go b/pkg/components/storage.go index 038a9b2a86..28caa1ccb2 100644 --- a/pkg/components/storage.go +++ b/pkg/components/storage.go @@ -261,9 +261,14 @@ func loadCSIPluginConfig(ctx context.Context, copyUserCfg(usrCfg, runtimeCfg, cancel) } if event.Has(fsnotify.Remove) { - klog.Warningf("lvmd config file %q was removed, this may be due to a reset user configuration or by mistake; "+ - "now, the new config will be applied from inbuilt defaults", event.Name) - copyDefaultCfg(runtimeCfg, cancel) + if _, statErr := os.Stat(usrCfg); statErr != nil { + klog.Warningf("lvmd config file %q was removed, this may be due to a reset user configuration or by mistake; "+ + "now, the new config will be applied from inbuilt defaults", event.Name) + copyDefaultCfg(runtimeCfg, cancel) + } else { + klog.Infof("lvmd config file %q received remove event but file still exists, re-applying user config", event.Name) + copyUserCfg(usrCfg, runtimeCfg, cancel) + } } if event.Has(fsnotify.Chmod) { klog.Warningf("permissions were modified for %q, this may cause side-effects", event.Name) From 1a68caa5c29026ec6fcb5b63f4ab974d2d45e9c1 Mon Sep 17 00:00:00 2001 From: Patryk Matuszak Date: Wed, 27 May 2026 15:54:46 +0200 Subject: [PATCH 3/3] Fix flaky Test_loadCSIPluginConfig by replacing sleep with polling The test used a 50ms time.Sleep to wait for the fsnotify watcher to process events, which was unreliable on loaded CI nodes. Replace with: - waitForRuntimeCfg: polls until runtime config matches expected value (5s timeout), used for positive assertions where a change is expected. - readRuntimeCfgAfterSettle: waits 500ms then reads, used for negative assertions where nothing should change (chmod, shutdown tests). Confirmed stable over 100 consecutive runs. Co-Authored-By: Claude Opus 4.6 --- pkg/components/storage_test.go | 68 ++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/pkg/components/storage_test.go b/pkg/components/storage_test.go index 9c74b7c65e..b6c6446222 100644 --- a/pkg/components/storage_test.go +++ b/pkg/components/storage_test.go @@ -11,11 +11,11 @@ import ( "github.com/openshift/microshift/pkg/config/lvmd" ) -// runtimeCfgUpdateDelay is the delay to wait for the runtime config to be updated via the watch -// in the tests. This is a bit of a hack to avoid having to wait for the watch to trigger. -// In case of failures on loads, consider increasing this value. -// In a normal I/O environment, this value should never be a problem. -var runtimeCfgUpdateDelay = 50 * time.Millisecond +const ( + runtimeCfgPollInterval = 50 * time.Millisecond + runtimeCfgPollTimeout = 5 * time.Second + runtimeCfgSettleDelay = 500 * time.Millisecond +) func Test_loadCSIPluginConfig(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) @@ -34,15 +34,33 @@ func Test_loadCSIPluginConfig(t *testing.T) { SocketName: "the socket should not matter!", } - getRuntimeCfgAfterDelay := func() *lvmd.Lvmd { - // wait for the runtime config to be updated via the watch - time.Sleep(runtimeCfgUpdateDelay) - runtimeLvmd, err := lvmd.NewLvmdConfigFromFile(testRuntimeCfg) + waitForRuntimeCfg := func(expected *lvmd.Lvmd) *lvmd.Lvmd { + t.Helper() + deadline := time.Now().Add(runtimeCfgPollTimeout) + for { + cfg, err := lvmd.NewLvmdConfigFromFile(testRuntimeCfg) + if err == nil { + cfg.Message = "" + if reflect.DeepEqual(cfg, expected) { + return cfg + } + } + if time.Now().After(deadline) { + t.Fatalf("Timed out waiting for runtime config to match expected config") + } + time.Sleep(runtimeCfgPollInterval) + } + } + + readRuntimeCfgAfterSettle := func() *lvmd.Lvmd { + t.Helper() + time.Sleep(runtimeCfgSettleDelay) + cfg, err := lvmd.NewLvmdConfigFromFile(testRuntimeCfg) if err != nil { t.Fatalf("Failed to load runtime config: %v", err) } - runtimeLvmd.Message = "" // ignore message field - return runtimeLvmd + cfg.Message = "" + return cfg } previousCfgLoader := defaultCfgLoader @@ -71,11 +89,7 @@ func Test_loadCSIPluginConfig(t *testing.T) { t.Fatalf("Failed to open runtime config file: %v", err) } - runtimeLvmd = getRuntimeCfgAfterDelay() - - if !reflect.DeepEqual(runtimeLvmd, testDefaultCfgLvmd) { - t.Fatalf("Expected runtime config to match default config") - } + runtimeLvmd = waitForRuntimeCfg(testDefaultCfgLvmd) }) var usrLvmd *lvmd.Lvmd @@ -101,12 +115,13 @@ func Test_loadCSIPluginConfig(t *testing.T) { } t.Run("chmod to write only should not affect the watch", func(t *testing.T) { - // wait for the runtime config to be updated via the watch but it should not trigger anything + runtimeLvmd = waitForRuntimeCfg(usrLvmd) + if err := os.Chmod(testUsrCfg, 0400); err != nil { t.Fatalf("Failed to change user config permissions: %v", err) } - runtimeLvmd = getRuntimeCfgAfterDelay() + runtimeLvmd = readRuntimeCfgAfterSettle() if !reflect.DeepEqual(runtimeLvmd, usrLvmd) { t.Fatalf("Expected runtime config to match user config") } @@ -115,14 +130,12 @@ func Test_loadCSIPluginConfig(t *testing.T) { t.Fatalf("expected usr config to have been updated to Readonly from Chmod (400) and not be writable: %v", err) } - // wait for the runtime config to be updated via the watch but it should not trigger anything if err := os.Chmod(testUsrCfg, 0600); err != nil { t.Fatalf("Failed to change user config permissions: %v", err) } }) t.Run("user config update should trigger a reload", func(t *testing.T) { - // switch the defaults to trigger a reload usrLvmd.DeviceClasses[1].Default = true usrLvmd.DeviceClasses[0].Default = false @@ -130,10 +143,7 @@ func Test_loadCSIPluginConfig(t *testing.T) { t.Fatalf("Failed to save user config: %v", err) } - runtimeLvmd = getRuntimeCfgAfterDelay() - if !reflect.DeepEqual(runtimeLvmd, usrLvmd) { - t.Fatalf("Expected runtime config to match user config") - } + runtimeLvmd = waitForRuntimeCfg(usrLvmd) }) }) @@ -141,13 +151,7 @@ func Test_loadCSIPluginConfig(t *testing.T) { if err := os.Remove(testUsrCfg); err != nil { t.Fatalf("Failed to remove user config: %v", err) } - runtimeLvmd = getRuntimeCfgAfterDelay() - if reflect.DeepEqual(runtimeLvmd, usrLvmd) { - t.Fatalf("Expected runtime config to now match default config again, but was equal to old usr config") - } - if !reflect.DeepEqual(runtimeLvmd, testDefaultCfgLvmd) { - t.Fatalf("Expected runtime config to now match default config again, but was not equal to the default cfg") - } + runtimeLvmd = waitForRuntimeCfg(testDefaultCfgLvmd) }) t.Run("user config watch should be stopped after shutdown", func(t *testing.T) { @@ -158,7 +162,7 @@ func Test_loadCSIPluginConfig(t *testing.T) { t.Fatalf("Failed to save user config after watch shutdown: %v", err) } - runtimeLvmd = getRuntimeCfgAfterDelay() + runtimeLvmd = readRuntimeCfgAfterSettle() if reflect.DeepEqual(runtimeLvmd, usrLvmd) { t.Fatalf("Expected runtime config to not have been updated after watch shutdown, but was equal to new usr config") }