-
Notifications
You must be signed in to change notification settings - Fork 2.9k
tmpfs: Add support for noatime mount option #26238
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.
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
Conversation
Luap99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have a proper integration test in test/e2e or test/system for this to make sure the mount is actually correctly passed to the oci runtime and it mounted the tmpfs with it.
|
I don't see anything wrong with the patch. The tests are missing as @Luap99 pointed out. |
| }) | ||
|
|
||
| It("podman run --tmpfs with noatime option", func() { | ||
| session := podmanTest.Podman([]string{"run", "--rm", "--tmpfs", "/mytmpfs:noatime", ALPINE, "grep", "mytmpfs", "/proc/self/mountinfo"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider podmanTest.PodmanExitCleanly here ... I think you should also test the negative here too and as a result of the test, ensure the error message you added has been emitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@baude
Thank you for reviewing the code and providing your suggestions!
I'd like to share my findings step by step:
1. Define Negative Test Case
podman run -v ./testdir:/testdir:noatime alpineI expect an error like
the 'noatime' option is only allowed with tmpfs mounts
However, the actual error was:
Error: invalid option type "noatime"
2. Investigation
I found this mount error already returned by ValidateVolumeOpts, just before processOptionsInternal. Also podman create... has the same result.

3. Expected Error via Rest API
Interestingly, I tried podman system serivce... and it returned the expected error message.
cd <PATH>/podman/bin
./podman system service --time=0 tcp:127.0.0.1:8080# Another terminal
# Create a conatiner
curl -X POST http://127.0.0.1:8080/v4.5.1/libpod/containers/create -H "Content-Type: application/json" -d '{ "name": "myalpine", "image": "alpine", "command": ["sleep", "3600"], "mounts": [{ "type": "bind", "source": "/home/arthur/podman/bin/testdir", "destination": "/testdir", "options": ["noatime"]}]}'
# Response JSON
{"cause":"invalid mount option","message":"the 'noatime' option is only allowed with tmpfs mounts: invalid mount option","response":500}4. Next Step
I'd like to add a negative test using curl(Step 3.). Since it needs podman system service... running at the beginning, I think I can use BATS...still look around test/system directory.. possibly adding a new bats file like 770-create.bats for noatime test.
Let me know if this direction makes sense, or if you have any suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the negative case is that without adding noatime then checking that mountinfo should not include the noatime mount option.
I'd like to add a negative test using curl(Step 3.). Since it needs podman system service... running at the beginning, I think I can use BATS...still look around test/system directory.. possibly adding a new bats file like 770-create.bats for noatime test.
That is not right, test/system runs podman just like the e2e it should not be used for direct API calls generally. If you want api calls look at test/apiv2
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
@Luap99
Let me summarize all tests I added for this pull request. Unit Test
make localunitE2E
make localintegration FOCUS="podman run --tmpfs with noatime option"API
cd test/apiv2
./test-apiv2 44-mounts |
'noatime' flag disables updates to file access times when files are read. This can reduce unnecessary writes and improve performance, especially in read-heavy workloads. Previously, tmpfs did not recognize the 'noatime' mount option and would return an error. With this change, tmpfs now properly accepts and handles the 'noatime' option. Fixes: containers#26102 Signed-off-by: Arthur Wu <lion811004@gmail.com>
Luap99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@giuseppe PTAL, this should be good to merge |
giuseppe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ArthurWuTW, giuseppe, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
00c431c
into
containers:main
What I did
How I did it
podman run --network host --tmpfs /mytmpfs:noatime alpine sh -c "mount | grep mytmpfs"Output:
Code Trace
Acceptable mount options defined in
mount_opts.go(processOptionsInternal).Unit Test
There is an existing unit test code pattern for mount options in
utils_test.go(TestProcessOptions)I added a new test in
testsarray.Code Change
add
noatimeoption inmount_opts.go(processOptionsInternal).Documentation
Updated supported mount options to include noatime with a short description.
How to verify it
make localunitpasses without errors.make validateprwithout errors.make podmanand check noatime mount option./home/arthur/podman/bin/podman run --network host --tmpfs /mytmpfs:noatime alpine sh -c "mount | grep mytmpfs"Output: As we can see,
/mytmpfshas thenoatimeoptionRelease Note