Skip to content

Docker compat: Return null when swappiness is unset instead of -1#24126

Merged
Luap99 merged 1 commit into
podman-container-tools:mainfrom
inknos:issue-23824
Apr 10, 2026
Merged

Docker compat: Return null when swappiness is unset instead of -1#24126
Luap99 merged 1 commit into
podman-container-tools:mainfrom
inknos:issue-23824

Conversation

@inknos

@inknos inknos commented Oct 1, 2024

Copy link
Copy Markdown
Contributor

Fixes: #23824

Does this PR introduce a user-facing change?

`podman inspect` returns MemorySwappiness: null when it's not set

@inknos

inknos commented Oct 1, 2024

Copy link
Copy Markdown
Contributor Author

I speak a very broken Go, and I am not sure whether this is the right place to fix it, so I need some expertise here. @baude @Luap99

(It makes some sense in my head though)

Comment thread pkg/specgenutil/specgen.go Outdated
Comment thread libpod/define/container_inspect.go Outdated
@github-actions github-actions Bot added the kind/api-change Change to remote API; merits scrutiny label Oct 3, 2024
@inknos

inknos commented Oct 3, 2024

Copy link
Copy Markdown
Contributor Author

@Luap99 makes more sense now.

However, I am checking it on my system and MemorySwappiness is 0...

$ ./bin/podman inspect practical_germain | grep Swap
               "MemorySwap": 0,
               "MemorySwappiness": 0,

Not sure if I am missing something or if this is part of a bigger problem. Shouldn't it be -1 on systems with cgroupsv2?

$ grep cgroup /proc/filesystems
nodev   cgroup
nodev   cgroup2

@inknos inknos changed the title Docker compat: Return null when swappiness is 0 Docker compat: Return null when swappiness is -1 Oct 3, 2024
@Luap99

Luap99 commented Oct 4, 2024

Copy link
Copy Markdown
Member

@Luap99 makes more sense now.

However, I am checking it on my system and MemorySwappiness is 0...

$ ./bin/podman inspect practical_germain | grep Swap
               "MemorySwap": 0,
               "MemorySwappiness": 0,

Not sure if I am missing something or if this is part of a bigger problem. Shouldn't it be -1 on systems with cgroupsv2?

$ grep cgroup /proc/filesystems
nodev   cgroup
nodev   cgroup2

Good question, I guess so.

@giuseppe @mheon WDYT?

@giuseppe

giuseppe commented Oct 4, 2024

Copy link
Copy Markdown
Contributor

looks like a bug. We set the memory swappiness only when there is a memory limit:

$ podman run --rm -d --name foo alpine sleep 10
3f97720574b8eab140107b2f89e7cf7e6f3944af9c780369d47f51f9e22fde2b
$ podman inspect foo | grep Swap
               "MemorySwap": 0,
               "MemorySwappiness": 0,

$ podman run --rm -d --name foo --memory 1G alpine sleep 10
6c50d6921e80e7278e191128db74e01e2f38793a73fe7d3c25fcf4fba1c1a572
$ podman inspect foo | grep Swap
               "MemorySwap": 2147483648,
               "MemorySwappiness": -1,

@inknos

inknos commented Oct 4, 2024

Copy link
Copy Markdown
Contributor Author

looks like a bug. We set the memory swappiness only when there is a memory limit:

thanks. I can look for a fix for it

@giuseppe

giuseppe commented Oct 4, 2024

Copy link
Copy Markdown
Contributor

I think we could just error out when users try to set it on cgroup v2 (crun does it anyway), and hard code it to -1 for inspect.

@Luap99 @mheon what do you think?

@inknos

inknos commented Oct 4, 2024

Copy link
Copy Markdown
Contributor Author
$ podman run --rm -d --name foo --memory-swappiness 4 alpine sleep 3
Error: OCI runtime error: crun: cannot set memory swappiness with cgroupv2

It errors out already no?

@mheon

mheon commented Oct 4, 2024

Copy link
Copy Markdown
Contributor

Might be cleanest to hard-code to -1 just for inspect, the resource limit code in SpecGen is pretty ugly

@github-actions

github-actions Bot commented Nov 4, 2024

Copy link
Copy Markdown

A friendly reminder that this PR had no activity for 30 days.

@baude baude added the 5.3 label Nov 4, 2024
@baude

baude commented Nov 4, 2024

Copy link
Copy Markdown
Contributor

This PR has been marked for 5.3 inclusion but it must be merged prior to Nov 5 for inclusion in 5.3 RC3. PRs not merged by that date are considered on a case by case basis for backporting.

@openshift-ci

openshift-ci Bot commented May 27, 2025

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: inknos
Once this PR has been reviewed and has the lgtm label, please assign mheon for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@TomSweeneyRedHat

Copy link
Copy Markdown
Contributor

@inknos should this be closed or is this still on your todo list?

@mheon

mheon commented Aug 8, 2025

Copy link
Copy Markdown
Contributor

Ooof. I'll get to this on Monday.

@Honny1

Honny1 commented Mar 4, 2026

Copy link
Copy Markdown
Contributor

@inknos The related issue is marked for 6.0. I think now is the best time to restart work on it.

This hard codes -1 on inspect for Docker's API to return `null` and be compatible

Fixes: podman-container-tools#23824

Signed-off-by: Nicola Sella <nsella@redhat.com>
@inknos inknos changed the title Docker compat: Return null when swappiness is -1 Docker compat: Return null when swappiness is unset instead of -1 Apr 8, 2026

@Honny1 Honny1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@Honny1

Honny1 commented Apr 10, 2026

Copy link
Copy Markdown
Contributor

PTAL @containers/podman-maintainers

@inknos inknos removed the 5.3 label Apr 10, 2026
@inknos inknos added the 6.0 Breaking changes for Podman 6.0 label Apr 10, 2026
@inknos

inknos commented Apr 10, 2026

Copy link
Copy Markdown
Contributor Author

Issue tags 6.0 so I added the tag here as well, and this is a breaking change

@Luap99 Luap99 left a comment

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.

LGTM

@Luap99 Luap99 merged commit 80df8f2 into podman-container-tools:main Apr 10, 2026
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.0 Breaking changes for Podman 6.0 kind/api-change Change to remote API; merits scrutiny release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

podman inspect should return null on some value instead of 0

7 participants