Refine concurrency logic

[adamfarley]

This change ensures that we identify the correct memory size when running tests in containers, by setting CGMEM to be the number of memory bytes in all circumstances.

We could remove the "-lt" logic altogether, but we shouldn't because sometimes the cgroup limits are set far beyond the machine's maximum memory, for reasons we do not yet understand.

[adamfarley]

Linked to #5012

May not close it. Further review required. Should improve matters, though.

[adamfarley]

Testing here: https://ci.adoptium.net/job/Grinder/8831/console

[adamfarley]

Yep, that worked. Now sets concurrency to a more sensible level. Ready to merge.

[jerboaa]

/sys/fs/cgroup/memory/memory.limit_in_bytes is inherently cgv1 (legacy) specific. This won't work on cgroup v2 systems. FYI.

[sxa]

Ref Severin's comment above, /sys/fs/cgroup/memory.max appears to be the right thing for V2 so we ought to change that (either in this PR or a separate one)
Noting that this is a follow on to other fixes in this area under #4933 and #5035

[andrew-m-leonard]

lgtm

[sxa]

@adamfarley Because I'm paranoid about these things can you fire this off on a subset of diverse machines similar to the table at the top of #4933 to check that this won't regress anything please?

Suggest one of the PLCT RISC-V machines (4 core, 16Gb), test-sxa-armv7l-ubuntu2004-odroid-2 (8 core, 2Gb), something running on an Ubuntu 2004 host and something on Ubuntu 22.04 as a minimum.

[adamfarley]

Ref Severin's comment above, /sys/fs/cgroup/memory.max appears to be the right thing for V2 so we ought to change that (either in this PR or a separate one) Noting that this is a follow on to other fixes in this area under #4933 and #5035

Okie dokie. Will fix.

[adamfarley]

@adamfarley Because I'm paranoid about these things can you fire this off on a subset of diverse machines similar to the table at the top of #4933 to check that this won't regress anything please?

Suggest one of the PLCT RISC-V machines (4 core, 16Gb), test-sxa-armv7l-ubuntu2004-odroid-2 (8 core, 2Gb), something running on an Ubuntu 2004 host and something on Ubuntu 22.04 as a minimum.

Gotcha. Will run with the updated fix.

[adamfarley]

Ok, given that we (a) want to tolerate both memory.max AND memory.etcetcbytes, and (b) that memory.max can be both present, readable and empty, here's a proposal for the new script:

KMEMMB=`awk '/^MemTotal:/{print int($$2/1024)}' /proc/meminfo`; 

if [[ -r /sys/fs/cgroup/memory.max && $(cat /sys/fs/cgroup/memory.max) =~ ^[0-9]+$ ]]; 
    then CGMEM=`cat /sys/fs/cgroup/memory.max`; 
elif [[ -r /sys/fs/cgroup/memory/memory.limit_in_bytes && $(cat /sys/fs/cgroup/memory/memory.limit_in_bytes) =~ ^[0-9]+$ ]]; 
    then CGMEM=`cat /sys/fs/cgroup/memory/memory.limit_in_bytes`; 
else 
    CGMEM=`expr $${KMEMMB} \* 1024 \* 1024`; 
fi; 

CGMEMMB=`expr $${CGMEM} / 1024 / 1024`; 

if [ "$${KMEMMB}" -lt "$${CGMEMMB}" ]; then 
    echo "$${KMEMMB}"; else echo "$${CGMEMMB}"; 
fi

I added formatting for an easier review.

What do you think?

[adamfarley]

P.S. If I ever complain I have too much spare time, remind me of this makefile. Turning it into a bash script would remove the need to put all of this script into a single line.

[adamfarley]

Test run: https://ci.adoptium.net/job/Grinder/8832/

Will check across other machines once I know the new code works.

[jerboaa]

Ok, given that we (a) want to tolerate both memory.max AND memory.etcetcbytes, and (b) that memory.max can be both present, readable and empty, here's a proposal for the new script:

KMEMMB=`awk '/^MemTotal:/{print int($$2/1024)}' /proc/meminfo`; 

if [[ -r /sys/fs/cgroup/memory.max && $(cat /sys/fs/cgroup/memory.max) =~ ^[0-9]+$ ]]; 
    then CGMEM=`cat /sys/fs/cgroup/memory.max`; 
elif [[ -r /sys/fs/cgroup/memory/memory.limit_in_bytes && $(cat /sys/fs/cgroup/memory/memory.limit_in_bytes) =~ ^[0-9]+$ ]]; 
    then CGMEM=`cat /sys/fs/cgroup/memory/memory.limit_in_bytes`; 
else 
    CGMEM=`expr $${KMEMMB} \* 1024 \* 1024`; 
fi; 

CGMEMMB=`expr $${CGMEM} / 1024 / 1024`; 

if [ "$${KMEMMB}" -lt "$${CGMEMMB}" ]; then 
    echo "$${KMEMMB}"; else echo "$${CGMEMMB}"; 
fi

I added formatting for an easier review.

What do you think?

You should test on the FS type on the cgroup root. In pseudo code:

if [ "$(stat -f --format '%T' /sys/fs/cgroup)_" == "cgroup2fs_" ]; then
   #cgroup v2 branch
else
   #cgroup v1 branch
fi

[adamfarley]

Ok, had a ponder and came up with this:

KMEMMB=`awk '/^MemTotal:/{print int($$2/1024)}' /proc/meminfo`; 

if [[ -f "/.dockerenv" ]]; then
    CGRPV=`stat -f --format '%T' /sys/fs/cgroup`;
    if [[ "_$$CGRPV" == "_cgroup2fs" ]]; then
        CGMEM=`cat /sys/fs/cgroup/memory.max 2>1`; 
    elif [[ "_$$CGRPV" == "_tmpfs" ]]; then 
        CGMEM=`cat /sys/fs/cgroup/memory/memory.limit_in_bytes 2>1`; 
    fi; 
fi;

if [[ ! $$(CGMEM) =~ ^[0-9]+$$ ]]; then
    CGMEM=`expr $${KMEMMB} \* 1024 \* 1024`; 
fi; 

CGMEMMB=`expr $${CGMEM} / 1024 / 1024`; 

if [ "$${KMEMMB}" -lt "$${CGMEMMB}" ]; then 
    echo "$${KMEMMB}"; else echo "$${CGMEMMB}"; 
fi

This way we:

• Check if we're in a container before running the stat command, which not all OS' have.
• Can handle things if the file is missing, unreadable, or empty.
• Don't use cgroups contents if we're not in a container (some OS' have that folder even outside of any containers).

Added some debug code (so we know we're following the right code path) and ran to test it here: https://ci.adoptium.net/job/Grinder/8836/

[jerboaa]

if [[ -f "/.dockerenv" ]]; then

Means you assume docker as the runtime. Fails once moved to podman or something else, FWIW. Not sure if that's an issue worth addressing at this point.

[sxa]

You should test on the FS type on the cgroup root. In pseudo code:

Do you think that's going to trap additional situations compared to just checking for the presence of memory.max? Given where this code is (on one line for now) my preference would probably be to just check one or the other given that this is already quite complex for one line unless we need both checks.

if [[ -r /sys/fs/cgroup/memory.max && $(cat /sys/fs/cgroup/memory.max) =~ ^[0-9]+$ ]]; 
    then CGMEM=`cat /sys/fs/cgroup/memory.max`; 

We should avoid mixing $(...) and the backquote syntax in this. I also don't personally like using bash regex is bash but I guess that's just personal preference and that ship has sailed elsewhere in our code.

if [[ -f "/.dockerenv" ]]; then

Means you assume docker as the runtime. Fails once moved to podman or something else

Is that check even necessary?

[adamfarley]

Taking into consideration Stewart and Severin's points, I'm testing this:

KMEMMB=`awk '/^MemTotal:/{print int($$2/1024)}' /proc/meminfo`; 

if [[ -r /sys/fs/cgroup/memory.max ]]; then
    CGMEM=`cat /sys/fs/cgroup/memory.max 2>1`; 
else
    CGMEM=`cat /sys/fs/cgroup/memory/memory.limit_in_bytes 2>1`; 
fi; 

if [[ ! $$(CGMEM) =~ ^[0-9]+$$ ]]; then
    CGMEM=`expr $${KMEMMB} \* 1024 \* 1024`; 
fi; 

CGMEMMB=`expr $${CGMEM} / 1024 / 1024`; 

if [ "$${KMEMMB}" -lt "$${CGMEMMB}" ]; then 
    echo "$${KMEMMB}"; else echo "$${CGMEMMB}"; 
fi

This removes the cgroup version check, but also removes the need for it (as we now have error-handling).

One fewer man-page check (for stat) and one fewer "if" layer, to make debugging easier.

Test run: https://ci.adoptium.net/job/Grinder/8847/

[adamfarley]

Also, I added the formatted copy of the script into a big comment, to make maintenance easier in the future.

[adamfarley]

Ok, typo fixed and code works. All debug comments removed, and tested here:

https://ci.adoptium.net/job/Grinder/8850/console

Requesting merge now.