diff --git a/libpod/boltdb_state_internal.go b/libpod/boltdb_state_internal.go index d656cc316e..4eb6b620c1 100644 --- a/libpod/boltdb_state_internal.go +++ b/libpod/boltdb_state_internal.go @@ -7,11 +7,13 @@ import ( "os" "sort" "strings" + "sync" "github.com/sirupsen/logrus" bolt "go.etcd.io/bbolt" "go.podman.io/common/libnetwork/types" "go.podman.io/podman/v6/libpod/define" + "go.podman.io/podman/v6/libpod/plugin" ) const ( @@ -297,21 +299,12 @@ func (s *BoltState) getVolumeFromDB(name []byte, volume *Volume, volBkt *bolt.Bu // Need this for UsesVolumeDriver() so set it now. volume.runtime = s.runtime - - // Retrieve volume driver - if volume.UsesVolumeDriver() { - plugin, err := s.runtime.getVolumePlugin(volume.config) - if err != nil { - // We want to fail gracefully here, to ensure that we - // can still remove volumes even if their plugin is - // missing. Otherwise, we end up with volumes that - // cannot even be retrieved from the database and will - // cause things like `volume ls` to fail. - logrus.Errorf("Volume %s uses volume plugin %s, but it cannot be accessed - some functionality may not be available: %v", volume.Name(), volume.config.Driver, err) - } else { - volume.plugin = plugin + volume.volumePlugin = sync.OnceValues(func() (*plugin.VolumePlugin, error) { + if !volume.UsesVolumeDriver() { + return nil, nil } - } + return volume.runtime.getVolumePlugin(volume.config) + }) // Get the lock lock, err := s.runtime.lockManager.RetrieveLock(volume.config.LockID) diff --git a/libpod/runtime_volume_common.go b/libpod/runtime_volume_common.go index 74e005a621..151b2d9f58 100644 --- a/libpod/runtime_volume_common.go +++ b/libpod/runtime_volume_common.go @@ -78,13 +78,14 @@ func (r *Runtime) newVolume(ctx context.Context, noCreatePluginVolume bool, opti } } - // Plugin can be nil if driver is local, but that's OK - superfluous - // assignment doesn't hurt much. - plugin, err := r.getVolumePlugin(volume.config) - if err != nil { - return nil, fmt.Errorf("volume %s uses volume plugin %s but it could not be retrieved: %w", volume.config.Name, volume.config.Driver, err) + var plugin *volplugin.VolumePlugin + if volume.UsesVolumeDriver() { + var err error + plugin, err = volume.volumePlugin() + if err != nil { + return nil, fmt.Errorf("volume %s uses volume plugin %s but it could not be retrieved: %w", volume.config.Name, volume.config.Driver, err) + } } - volume.plugin = plugin if volume.config.Driver == define.VolumeDriverLocal { logrus.Debugf("Validating options for local driver") @@ -158,13 +159,13 @@ func (r *Runtime) newVolume(ctx context.Context, noCreatePluginVolume bool, opti // Now we get conditional: we either need to make the volume in the // volume plugin, or on disk if not using a plugin. - if volume.plugin != nil && !noCreatePluginVolume { + if volume.UsesVolumeDriver() && !noCreatePluginVolume { // We can't chown, or relabel, or similar the path the volume is // using, because it's not managed by us. // TODO: reevaluate this once we actually have volume plugins in // use in production - it may be safe, but I can't tell without // knowing what the actual plugin does... - if err := makeVolumeInPluginIfNotExist(volume.config.Name, volume.config.Options, volume.plugin); err != nil { + if err := makeVolumeInPluginIfNotExist(volume.config.Name, volume.config.Options, plugin); err != nil { return nil, err } } else { @@ -445,19 +446,14 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool, timeo if v.UsesVolumeDriver() && !ignoreVolumePlugin { canRemove := true - // Do we have a volume driver? - if v.plugin == nil { + plugin, err := v.volumePlugin() + if err != nil { canRemove = false - removalErr = fmt.Errorf("cannot remove volume %s from plugin %s, but it has been removed from Podman: %w", v.Name(), v.Driver(), define.ErrMissingPlugin) + removalErr = fmt.Errorf("cannot remove volume %s from plugin %s, but it has been removed from Podman: %w", v.Name(), v.Driver(), err) } else { - // Ping the plugin first to verify the volume still - // exists. - // We're trying to be very tolerant of missing volumes - // in the backend, to avoid the problems we see with - // sync between c/storage and the Libpod DB. getReq := new(pluginapi.GetRequest) getReq.Name = v.Name() - if _, err := v.plugin.GetVolume(getReq); err != nil { + if _, err := plugin.GetVolume(getReq); err != nil { canRemove = false removalErr = fmt.Errorf("volume %s could not be retrieved from plugin %s, but it has been removed from Podman: %w", v.Name(), v.Driver(), err) } @@ -465,7 +461,7 @@ func (r *Runtime) removeVolume(ctx context.Context, v *Volume, force bool, timeo if canRemove { req := new(pluginapi.RemoveRequest) req.Name = v.Name() - if err := v.plugin.RemoveVolume(req); err != nil { + if err := plugin.RemoveVolume(req); err != nil { return fmt.Errorf("volume %s could not be removed from plugin %s: %w", v.Name(), v.Driver(), err) } } diff --git a/libpod/sqlite_state_internal.go b/libpod/sqlite_state_internal.go index d6f1032359..d9bcd6143d 100644 --- a/libpod/sqlite_state_internal.go +++ b/libpod/sqlite_state_internal.go @@ -11,10 +11,12 @@ import ( "slices" "sort" "strings" + "sync" "github.com/sirupsen/logrus" "go.podman.io/common/libnetwork/types" "go.podman.io/podman/v6/libpod/define" + "go.podman.io/podman/v6/libpod/plugin" "go.podman.io/storage/pkg/fileutils" // SQLite backend for database/sql @@ -350,22 +352,13 @@ func finalizeVolumeSqlite(vol *Volume) error { } vol.lock = lock - // Retrieve volume driver - if vol.UsesVolumeDriver() { - plugin, err := vol.runtime.getVolumePlugin(vol.config) - if err != nil { - // We want to fail gracefully here, to ensure that we - // can still remove volumes even if their plugin is - // missing. Otherwise, we end up with volumes that - // cannot even be retrieved from the database and will - // cause things like `volume ls` to fail. - logrus.Errorf("Volume %s uses volume plugin %s, but it cannot be accessed - some functionality may not be available: %v", vol.Name(), vol.config.Driver, err) - } else { - vol.plugin = plugin - } - } - vol.valid = true + vol.volumePlugin = sync.OnceValues(func() (*plugin.VolumePlugin, error) { + if !vol.UsesVolumeDriver() { + return nil, nil + } + return vol.runtime.getVolumePlugin(vol.config) + }) return nil } diff --git a/libpod/volume.go b/libpod/volume.go index c383b7ec34..c464c3f42b 100644 --- a/libpod/volume.go +++ b/libpod/volume.go @@ -27,7 +27,7 @@ type Volume struct { ignoreIfExists bool valid bool - plugin *plugin.VolumePlugin + volumePlugin func() (*plugin.VolumePlugin, error) runtime *Runtime lock lock.Locker } diff --git a/libpod/volume_inspect.go b/libpod/volume_inspect.go index d8e9260903..6db9a4604e 100644 --- a/libpod/volume_inspect.go +++ b/libpod/volume_inspect.go @@ -32,15 +32,16 @@ func (v *Volume) Inspect() (*define.InspectVolumeData, error) { logrus.Debugf("Querying volume plugin %s for status", v.config.Driver) data.Mountpoint = v.state.MountPoint - if v.plugin == nil { - return nil, fmt.Errorf("volume %s uses volume plugin %s but it is not available, cannot inspect: %w", v.Name(), v.config.Driver, define.ErrMissingPlugin) + plugin, err := v.volumePlugin() + if err != nil { + return nil, fmt.Errorf("volume %s uses volume plugin %s but it is not available, cannot inspect: %w", v.Name(), v.config.Driver, err) } // Retrieve status for the volume. // Need to query the volume driver. req := new(pluginapi.GetRequest) req.Name = v.Name() - resp, err := v.plugin.GetVolume(req) + resp, err := plugin.GetVolume(req) if err != nil { return nil, fmt.Errorf("retrieving volume %s information from plugin %s: %w", v.Name(), v.Driver(), err) } diff --git a/libpod/volume_internal.go b/libpod/volume_internal.go index b43954a57e..a9931e034e 100644 --- a/libpod/volume_internal.go +++ b/libpod/volume_internal.go @@ -5,8 +5,10 @@ package libpod import ( "fmt" "os" + "sync" "go.podman.io/podman/v6/libpod/define" + "go.podman.io/podman/v6/libpod/plugin" ) // Creates a new volume @@ -19,6 +21,12 @@ func newVolume(runtime *Runtime) *Volume { volume.config.Options = make(map[string]string) volume.state.NeedsCopyUp = true volume.state.NeedsChown = true + volume.volumePlugin = sync.OnceValues(func() (*plugin.VolumePlugin, error) { + if !volume.UsesVolumeDriver() { + return nil, nil + } + return volume.runtime.getVolumePlugin(volume.config) + }) return volume } diff --git a/libpod/volume_internal_common.go b/libpod/volume_internal_common.go index d7453b0a0c..ec9d9ff99a 100644 --- a/libpod/volume_internal_common.go +++ b/libpod/volume_internal_common.go @@ -50,14 +50,15 @@ func (v *Volume) mount() error { // ours more. So hardcode container ID to something reasonable, and use // the same one for everything. if v.UsesVolumeDriver() { - if v.plugin == nil { - return fmt.Errorf("volume plugin %s (needed by volume %s) missing: %w", v.Driver(), v.Name(), define.ErrMissingPlugin) + plugin, err := v.volumePlugin() + if err != nil { + return fmt.Errorf("volume plugin %s (needed by volume %s) missing: %w", v.Driver(), v.Name(), err) } req := new(pluginapi.MountRequest) req.Name = v.Name() req.ID = pseudoCtrID - mountPoint, err := v.plugin.MountVolume(req) + mountPoint, err := plugin.MountVolume(req) if err != nil { return err } @@ -156,14 +157,15 @@ func (v *Volume) unmount(force bool) error { if v.state.MountCount == 0 { if v.UsesVolumeDriver() { - if v.plugin == nil { - return fmt.Errorf("volume plugin %s (needed by volume %s) missing: %w", v.Driver(), v.Name(), define.ErrMissingPlugin) + plugin, err := v.volumePlugin() + if err != nil { + return fmt.Errorf("volume plugin %s (needed by volume %s) missing: %w", v.Driver(), v.Name(), err) } req := new(pluginapi.UnmountRequest) req.Name = v.Name() req.ID = pseudoCtrID - if err := v.plugin.UnmountVolume(req); err != nil { + if err := plugin.UnmountVolume(req); err != nil { return err } diff --git a/test/e2e/volume_plugin_test.go b/test/e2e/volume_plugin_test.go index e2ab89ae87..ad9f759bb7 100644 --- a/test/e2e/volume_plugin_test.go +++ b/test/e2e/volume_plugin_test.go @@ -301,4 +301,88 @@ Removed: Expect(volInspect2).Should(ExitCleanly()) Expect(volInspect2.OutputToString()).To(ContainSubstring("3")) }) + + It("unrelated command after refresh does not log plugin errors for plugin volumes", func() { + podmanTest.AddImageToRWStore(volumeTest) + + pluginStatePath := filepath.Join(podmanTest.TempDir, "volumes") + err := os.Mkdir(pluginStatePath, 0o755) + Expect(err).ToNot(HaveOccurred()) + + pluginName := "testvol7" + ctrName := "pluginCtrRefresh" + podmanTest.PodmanExitCleanly( + "run", "--name", ctrName, "--security-opt", "label=disable", + "-v", "/run/docker/plugins:/run/docker/plugins", + "-v", fmt.Sprintf("%v:%v", pluginStatePath, pluginStatePath), + "-d", volumeTest, "--sock-name", pluginName, "--path", pluginStatePath, + ) + + err = WaitForFile(fmt.Sprintf("/run/docker/plugins/%s.sock", pluginName)) + Expect(err).ToNot(HaveOccurred()) + + vol1 := "refreshVol1-" + stringid.GenerateRandomID() + vol2 := "refreshVol2-" + stringid.GenerateRandomID() + podmanTest.PodmanExitCleanly("volume", "create", "--driver", pluginName, vol1) + podmanTest.PodmanExitCleanly("volume", "create", "--driver", pluginName, vol2) + + podmanTest.StopContainer(ctrName) + + // Simulate post-reboot: tmp state is gone but RunRoot (volume DB) persists. + // Use a fresh tmpdir instead of deleting the alive file from setup. + refreshTmp := filepath.Join(podmanTest.TempDir, "refresh-tmp") + err = os.MkdirAll(refreshTmp, 0o755) + Expect(err).ToNot(HaveOccurred()) + + // Unrelated command triggers refresh; must succeed with no stderr. + podmanTest.PodmanExitCleanly("--log-level=error", "--tmpdir", refreshTmp, "version") + + for _, vol := range []string{vol1, vol2} { + remove := podmanTest.Podman([]string{"volume", "rm", vol}) + remove.WaitWithDefaultTimeout() + Expect(remove).To(ExitWithErrorRegex(125, "has been removed from Podman")) + } + ls := podmanTest.PodmanExitCleanly("volume", "ls", "-q") + Expect(ls.OutputToStringArray()).To(BeEmpty()) + }) + + It("volume inspect and ls with stopped plugin fail without finalize spam", func() { + podmanTest.AddImageToRWStore(volumeTest) + + pluginStatePath := filepath.Join(podmanTest.TempDir, "volumes") + err := os.Mkdir(pluginStatePath, 0o755) + Expect(err).ToNot(HaveOccurred()) + + pluginName := "testvol8" + ctrName := "pluginCtrInspect" + podmanTest.PodmanExitCleanly( + "run", "--name", ctrName, "--security-opt", "label=disable", + "-v", "/run/docker/plugins:/run/docker/plugins", + "-v", fmt.Sprintf("%v:%v", pluginStatePath, pluginStatePath), + "-d", volumeTest, "--sock-name", pluginName, "--path", pluginStatePath, + ) + + err = WaitForFile(fmt.Sprintf("/run/docker/plugins/%s.sock", pluginName)) + Expect(err).ToNot(HaveOccurred()) + + volName := "inspectVol-" + stringid.GenerateRandomID() + podmanTest.PodmanExitCleanly("volume", "create", "--driver", pluginName, volName) + + podmanTest.StopContainer(ctrName) + + inspect := podmanTest.Podman([]string{"volume", "inspect", volName}) + inspect.WaitWithDefaultTimeout() + Expect(inspect).To(ExitWithErrorRegex(125, "cannot inspect")) + + ls := podmanTest.Podman([]string{"volume", "ls", "-q"}) + ls.WaitWithDefaultTimeout() + Expect(ls).To(ExitWithErrorRegex(125, "cannot inspect")) + + remove := podmanTest.Podman([]string{"volume", "rm", volName}) + remove.WaitWithDefaultTimeout() + Expect(remove).To(ExitWithErrorRegex(125, "has been removed from Podman")) + + lsAfter := podmanTest.PodmanExitCleanly("volume", "ls", "-q") + Expect(lsAfter.OutputToStringArray()).To(BeEmpty()) + }) })