search

DOMjudge / domjudge

DOMjudge programming contest jury system
https://www.domjudge.org
GNU General Public License v2.0
739 stars 258 forks source link

Error when missing cgroup value memory.peak #2780

Closed eldering closed 2 days ago

eldering commented 2 weeks ago

This replaces the memory.memsw.max_usage_in_bytes value from cgroup v1. We always assume it's there so we should fail if it is not. Otherwise things silently degrade in a way they would not have before.

Addresses #2763

nickygerritsen commented 2 weeks ago

But it did 'work' without this thing, it just reported it as 0? I think it's better to document that than to not allow cgroupv2 at all (which is basically what this PR does) on older kernels (like the one in Ubuntu 22.04).

Or did it fully break for you?

eldering commented 2 weeks ago

But it did 'work' without this thing, it just reported it as 0? I think it's better to document that than to not allow cgroupv2 at all (which is basically what this PR does) on older kernels (like the one in Ubuntu 22.04).

Or did it fully break for you?

Yes, it was working because we allowed it to. But we didn't allow it in the cgroup v1 implementation either, so there you were guaranteed to always get real memory usage value. I think silently reporting zero when the kernel does not support it, creates a confusing situation (which happened to us at the BAPC). So better to explicitly bail out and let the user switch to cgroup v1. You're on an older kernel anyways.

nickygerritsen commented 2 weeks ago

If others think the same I think we need to re update the docs to tell them how to switch to v1. I think we removed that now

meisterT commented 2 weeks ago

If we do this (I don't feel strongly), then we should change the script that I touched in https://github.com/DOMjudge/domjudge/commit/8f8f4c06f1ec85a37a7c56460a1fccc3132ab7c6 from warning to error to get this error before you start judging.

vmcj commented 2 weeks ago

Is this only a problem for people upgrading DOMjudge on their current OS or also for people installing a new server (Debian stable, current Ubuntu LTS)? In case you upgrade you already have cgroupV1, keeping this in the docs that this still works seems fine for me (and crashing if we can't guarantee the same information is fine by me)

If we also do this for new installations on newer/current versions seems a bit hard for informational level of info (assuming that runguard still does its job and still terminates submissions taking too much memory) I'm not sure if the IMO minor decrease in memory is worth the discussion people need to have to have cgroupV1 enabled again with their sysadmin so I would at least give an override flag for this behaviour or an explicit question when the judgedaemon boots?

nickygerritsen commented 2 weeks ago

Ubuntu 22.04 at least doesn't support it. It's not the latest LTS, but I think it's still widely used, especially since there is no ICPC 24.04 image yet (although it is coming).

vmcj commented 2 weeks ago

Ubuntu 22.04 at least doesn't support it. It's not the latest LTS, but I think it's still widely used, especially since there is no ICPC 24.04 image yet (although it is coming).

In that case I think terminating like @eldering suggests is the better option but also adding an override flag for when you accept this (minor) decrease in performance if its only informational.

@eldering did the submissions with memory-limit version still get detected?