Skip to content

Fix incomplete Mounts attributes #25697

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix incomplete Mounts attributes #25697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion libpod/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,16 @@ func getTestContainer(id, name string, manager lock.Manager) (*Container, error)
RootfsImageID: id,
RootfsImageName: "testimg",
StaticDir: "/does/not/exist/",
Mounts: []string{"/does/not/exist"},
Mounts: []define.InspectMount{
{
Type: "bind",
Source: "/dummy/source",
Destination: "/dummy/destination",
Mode: "",
RW: true,
Propagation: "rprivate",
},
},
},
ContainerMiscConfig: ContainerMiscConfig{
LogPath: "/does/not/exist/",
Expand Down
4 changes: 2 additions & 2 deletions libpod/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,10 +379,10 @@ func (c *Container) Spec() *spec.Spec {
return returnSpec
}

// specFromState returns the unmarshalled json config of the container. If the
// SpecFromState returns the unmarshalled json config of the container. If the
// config does not exist (e.g., because the container was never started) return
// the spec from the config.
func (c *Container) specFromState() (*spec.Spec, error) {
func (c *Container) SpecFromState() (*spec.Spec, error) {
returnSpec := c.config.Spec

if f, err := os.Open(c.state.ConfigPath); err == nil {
Expand Down
2 changes: 1 addition & 1 deletion libpod/container_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ type ContainerRootFSConfig struct {
// Mounts contains all additional mounts into the container rootfs.
// It is presently only used for the container's SHM directory.
// These must be unmounted before the container's rootfs is unmounted.
Mounts []string `json:"mounts,omitempty"`
Copy link
Member

@mheon mheon Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is going to break upgrades from previous versions - you can't deserialize a JSON string into a define.InspectMount. It should be possible to, instead of this, make a func (c *Container) InspectMounts() ([]define.InspectMount, error) that takes the libpod ContainerConfig and converts it into InspectMount in the same way Inspect does (with a minor refactor, you could use the same code for both). That would probably be sufficient for your needs here, no?

Copy link
Author

Choose a reason for hiding this comment

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

Yes I can try. I understand it would be a breaking change. As I reported in my comment below, it would be great if, at least, it could be applied on the next major release.

Copy link
Author

@D3vil0p3r D3vil0p3r Mar 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

in the same way Inspect does (with a minor refactor, you could use the same code for both)

to which specific Inspect function you refer? This one?

podman/libpod/container_inspect.go

Line 39 in 490eb47

func (c *Container) Inspect(size bool) (*define.InspectContainerData, error) {

Mounts []define.InspectMount `json:"mounts,omitempty"`
// NamedVolumes lists the Libpod named volumes to mount into the
// container. Each named volume is guaranteed to exist so long as this
// container exists.
Expand Down
6 changes: 3 additions & 3 deletions libpod/container_inspect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (c *Container) Inspect(size bool) (*define.InspectContainerData, error) {
}

func (c *Container) volumesFrom() ([]string, error) {
ctrSpec, err := c.specFromState()
ctrSpec, err := c.SpecFromState()
if err != nil {
return nil, err
}
Expand All @@ -63,7 +63,7 @@ func (c *Container) volumesFrom() ([]string, error) {
func (c *Container) getContainerInspectData(size bool, driverData *define.DriverData) (*define.InspectContainerData, error) {
config := c.config
runtimeInfo := c.state
ctrSpec, err := c.specFromState()
ctrSpec, err := c.SpecFromState()
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -664,7 +664,7 @@ func (c *Container) inHostPidNS() (bool, error) {
if c.config.PIDNsCtr != "" {
return false, nil
}
ctrSpec, err := c.specFromState()
ctrSpec, err := c.SpecFromState()
if err != nil {
return false, err
}
Expand Down
6 changes: 3 additions & 3 deletions libpod/container_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ func (c *Container) processLabel(processLabel string) (string, error) {
if !c.Systemd() && !c.ociRuntime.SupportsKVM() {
return processLabel, nil
}
ctrSpec, err := c.specFromState()
ctrSpec, err := c.SpecFromState()
if err != nil {
return "", err
}
Expand Down Expand Up @@ -2068,7 +2068,7 @@ func (c *Container) cleanupStorage() error {
}

for _, containerMount := range c.config.Mounts {
if err := c.unmountSHM(containerMount); err != nil {
if err := c.unmountSHM(containerMount.Destination); err != nil {
reportErrorf("unmounting container %s: %w", c.ID(), err)
}
}
Expand Down Expand Up @@ -2870,7 +2870,7 @@ func (c *Container) update(updateOptions *entities.ContainerUpdateOptions) error
(updateOptions.Resources != nil || updateOptions.Env != nil || updateOptions.UnsetEnv != nil) {
// So `podman inspect` on running containers sources its OCI spec from disk.
// To keep inspect accurate we need to update the on-disk OCI spec.
onDiskSpec, err := c.specFromState()
onDiskSpec, err := c.SpecFromState()
if err != nil {
return fmt.Errorf("retrieving on-disk OCI spec to update: %w", err)
}
Expand Down
2 changes: 1 addition & 1 deletion libpod/oci_conmon_exec_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
)

func (c *Container) setProcessCapabilitiesExec(options *ExecOptions, user string, execUser *user.ExecUser, pspec *spec.Process) error {
ctrSpec, err := c.specFromState()
ctrSpec, err := c.SpecFromState()
if err != nil {
return err
}
Expand Down
10 changes: 8 additions & 2 deletions libpod/runtime_ctr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,13 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (_ *Contai
return nil, fmt.Errorf("unable to create shm dir: %w", err)
}
}
ctr.config.Mounts = append(ctr.config.Mounts, ctr.config.ShmDir)
ctr.config.Mounts = append(ctr.config.Mounts, define.InspectMount{ // HERE I ADDED SOME DEFAULT VALUES. MUST THEY BE CHANGED? If yes, where should I take those values?
Type: "bind", // default mount type
Source: "/opt", // assuming source is same as destination, or set appropriately
Destination: ctr.config.ShmDir, // the destination in the container
RW: true, // default read/write setting
Propagation: "rprivate", // default propagation
})
}

// Add the container to the state
Expand Down Expand Up @@ -1167,7 +1173,7 @@ func (r *Runtime) evictContainer(ctx context.Context, idOrName string, removeVol

// Unmount container mount points
for _, mount := range c.config.Mounts {
Unmount(mount)
Unmount(mount.Destination)
}

// Remove container from c/storage
Expand Down
3 changes: 2 additions & 1 deletion pkg/domain/entities/types/container_ps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"time"

netTypes "github.com/containers/common/libnetwork/types"
pod_define "github.com/containers/podman/v5/libpod/define"
define "github.com/containers/podman/v5/pkg/ps/define"
)

Expand Down Expand Up @@ -41,7 +42,7 @@ type ListContainer struct {
// Labels for container
Labels map[string]string
// User volume mounts
Mounts []string
Mounts []pod_define.InspectMount
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned on the issue these are breaking changes to a user facing API and cannot be done. We canont just break things.

Copy link
Author

@D3vil0p3r D3vil0p3r Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My 2cent: I understand, but at least for a major release. Each major release of all software could introduce breaking changes, but the fear to keep everything "unbreakable" just stops any improvement on the software itself (and on podman). But still, those "Mounts" attributes as-is, that are just destination directories, are unusable, simply because the same call does not return the related "Source", it is just an example.

For this particular case, I'm happily spending my small free time to enrich those data. I don't do this only because I need, I do because I "believe" on podman project and I use it as a standard over docker

Copy link
Member

Choose a reason for hiding this comment

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

but the fear to keep everything "unbreakable" just stops any improvement on the software itself (and on podman)

This isn't a fear, this is plain and simple a breaking API change. An old remote client would be unable to parse this json and thus break podman ps and the other way around the same way.

We cannot just break API users just because you happen to need different data in the field. There are ways to introduce new features without breaking existing ones, we do that all the time. New options and new fields would be fine (still assumes they make sense overall and are maintainable) but you cannot break the format of an existing one.

Yes we can break things in a major release but a new major release is still far away, not even planned yet. And if we like to break things that there must be a strong reason and we should do a careful audit of similar problems in the list API. Otherwise we have the same issue on a different field again.

For this particular case, I'm happily spending my small free time to enrich those data. I don't do this only because I need, I do because I "believe" on podman project and I use it as a standard over docker

I am thankful that you take the time to contribute to the project, but compatibility is an important topic for us, so you would need to find another way to move this forward. Adding a new API field that has the data you need could be an option.

// The names assigned to the container
Names []string
// Namespaces the container belongs to. Requires the
Expand Down
18 changes: 17 additions & 1 deletion pkg/ps/ps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
psdefine "github.com/containers/podman/v5/pkg/ps/define"
"github.com/containers/storage"
"github.com/containers/storage/types"
spec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/sirupsen/logrus"
)

Expand Down Expand Up @@ -153,6 +154,10 @@ func ListContainerBatch(rt *libpod.Runtime, ctr *libpod.Container, opts entities
healthStatus string
restartCount uint
podName string
ctrSpec *spec.Spec
namedVolumes []*libpod.ContainerNamedVolume
mounts []spec.Mount
inspectMounts []define.InspectMount
)

batchErr := ctr.Batch(func(c *libpod.Container) error {
Expand Down Expand Up @@ -206,6 +211,17 @@ func ListContainerBatch(rt *libpod.Runtime, ctr *libpod.Container, opts entities
return err
}

ctrSpec, err = c.SpecFromState()
if err != nil {
return err
}

namedVolumes, mounts = c.SortUserVolumes(ctrSpec)
inspectMounts, err = c.GetMounts(namedVolumes, conConfig.ImageVolumes, mounts)
if err != nil {
return err
}

if opts.Namespace {
ctrPID := strconv.Itoa(pid)
cgroup, _ = getNamespaceInfo(filepath.Join("/proc", ctrPID, "ns", "cgroup"))
Expand Down Expand Up @@ -260,7 +276,7 @@ func ListContainerBatch(rt *libpod.Runtime, ctr *libpod.Container, opts entities
ImageID: conConfig.RootfsImageID,
IsInfra: conConfig.IsInfra,
Labels: conConfig.Labels,
Mounts: ctr.UserVolumes(),
Mounts: inspectMounts,
Names: []string{conConfig.Name},
Networks: networks,
Pid: pid,
Expand Down
2 changes: 1 addition & 1 deletion pkg/specgen/generate/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ func ConfigToSpec(rt *libpod.Runtime, specg *specgen.SpecGenerator, containerID
tmpMounts := conf.Mounts

conf.Systemd = nil
conf.Mounts = []string{}
conf.Mounts = []define.InspectMount{}

if specg == nil {
specg = &specgen.SpecGenerator{}
Expand Down