-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Adjust memory limit check in docker env #14895
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
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.
do we need something similar for other tests:
https://github.com/search?q=repo%3Aphp%2Fphp-src%20proc%2Fmeminfo&type=code
maybe the logic should be extracted for re-use ?
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.
maybe it would be even worth it to create a new php-src function which works on docker and regular unix?
that way the php-src tests could be simplified and php userland would also benefit
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.
Thats both a good idea IMHO, I'll create a separate PR for the other tests, once we have this one settled.
Just tried locally on Ubuntu 20.04 with Docker version 26.1.3 and getting a bit nonsense there:
docker run -ti nginx:alpine sh
/ # cat /sys/fs/cgroup/memory/memory.limit_in_bytes
9223372036854771712
So not sure if this is always working in Docker...
Forgot to provide kernel version: 5.15.0-113-generic
@bukka could you do me a favour and run with docker run -ti --memory 512m nginx:alpine sh and check again?
It looks like this is way more complex than I initially thought 😉
So the current theory is: you are on Linux 5, using cgroup v1 which exposes /sys/fs/cgroup/memory/memory.limit_in_bytes, cgroup v2 will instead expose /sys/fs/cgroup/memory.max for example, that holds either the string max in case there is no memory limit or an integer representing the memory limit in bytes.
Btw.: 9223372036854771712 represents "no limit" for a 64-bit machine according to https://unix.stackexchange.com/a/421182
Yeah this works. So I think this should be just an additional check and not primary check for docker.
What I mean is that the skip should first check the meminfo and if that is ok, then check the Docker one if on Docker. Would it maybe make sense to not limit on docker but always check /sys/fs/cgroup/memory/memory.limit_in_bytes if present?
ext/standard/tests/file/file_get_contents_file_put_contents_5gb.phpt
Outdated
Show resolved
Hide resolved
Hm, I copied the code already in 9435f4d#diff-8689e5b00cb8d845b3d27ac0094c9342d5cde859d0a6defa39147d23e18459c7R12 before I noticed this PR.
Probably, this should be factored out to a common helper file as cmb noted on the ML.
962218e to
cc21de4
Compare
7ef1834 to
c4c7421
Compare
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'm wondering if it would make sense to introduce section in run-tests for this. It might be more consistent but won't work when run directly so I guess I'm fine with both approaches.
c4c7421 to
eec45d8
Compare
ext/standard/tests/file/file_get_contents_file_put_contents_5gb.phpt
Outdated
Show resolved
Hide resolved
Co-authored-by: Jakub Zelenka <bukka@php.net>
Look at this, sapi/cli/tests/upload_2G.phpt is failing on Windows. From how the --SKIPIF-- section looked before this change, this test was always skipped in Windows. I could change it to behave like this again, but I was wondering if this should work. Also I have no experience with PHP on Windows, maybe @cmb69 has an idea whats going on?
========DIFF========
Test
002- HTTP/1.1 200 OK
003- Host: %s
004- Date: %s
005- Connection: close
006- X-Powered-By: PHP/%s
007- Content-type: text/html; charset=UTF-8
003+ Notice: fwrite(): Send of 100000 bytes failed with errno=10035 A non-blocking socket operation could not be completed immediately in D:\a\php-src\php-src\sapi\cli\tests\upload_2G.php on line 34
004+ write failed @ ([98](https://github.com/php/php-src/actions/runs/10555691715/job/29239745310?pr=14895#step:6:99)6900000)
009- array(1) {
010- ["file1"]=>
011- array(6) {
012- ["name"]=>
013- string(9) "file1.txt"
014- ["full_path"]=>
015- string(9) "file1.txt"
016- ["type"]=>
017- string(10) "text/plain"
018- ["tmp_name"]=>
019- string(%d) "%s"
020- ["error"]=>
021- int(0)
022- ["size"]=>
023- int(2150000000)
024- }
025- }
026- Done
========DONE========
FAIL file upload greater than 2G [D:\a\php-src\php-src\sapi\cli\tests\upload_2G.phpt]
Look at this,
sapi/cli/tests/upload_2G.phptis failing on Windows.
I guess that is an issue with the internal buffering and the pipe manager; I would need to check this, but that may take a while (backlog too long).
Uh oh!
There was an error while loading. Please reload this page.
In a CircleCI environment when running tests in docker and limited to a
mediuminstance (4GB memory limit),freeand/proc/meminfoare not reflecting the memory limit of the container, but the host:MemFreein this case with ~8.9GB prevents the execution of the test, but depending on the order of execution and maybe other jobs on that host,MemFreesometimes is above the 10GB threshold. This will lead to the test being started and then OOM killed.So in case we run in docker, we need to check the cgroup memory limit:
Related to #13203