Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 7 additions & 14 deletions libpod/boltdb_state_internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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)
Expand Down
32 changes: 14 additions & 18 deletions libpod/runtime_volume_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -445,27 +446,22 @@ 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)
}
}
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)
}
}
Expand Down
23 changes: 8 additions & 15 deletions libpod/sqlite_state_internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion libpod/volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type Volume struct {

ignoreIfExists bool
valid bool
plugin *plugin.VolumePlugin
volumePlugin func() (*plugin.VolumePlugin, error)
runtime *Runtime
lock lock.Locker
}
Expand Down
7 changes: 4 additions & 3 deletions libpod/volume_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
8 changes: 8 additions & 0 deletions libpod/volume_internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}

Expand Down
14 changes: 8 additions & 6 deletions libpod/volume_internal_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}

Expand Down
84 changes: 84 additions & 0 deletions test/e2e/volume_plugin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is somewhat dangerous and should need at least comments why this is done


// 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())
})
})