fix: Correctly read container limits when running under k8s #1692
Conversation
Signed-off-by: talbii <ido@dragonflydb.io>
|
I would be happy if someone suggested a way to run Dragonfly under Kubernetes, as I'm rather noobish in that area 😄 |
hahaha, I missed this and I explicitly asked for this! I am afraid I can't help much with k8 :( |
|
Please expand in the commit message what was the bug and what has been changed. It's not really easy to understand what the fix is just by reading the code. |
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <ido@dragonflydb.io>
src/server/dfly_main.cc
Outdated
| @@ -515,8 +515,10 @@ error_code GetCGroupPath(string* memory_path, string* cpu_path) { | |||
| if (groups.size() == 1) { | |||
| // for v2 we only read 0::<name> | |||
| size_t pos = cgv.rfind(':'); | |||
| if (pos == string::npos) | |||
| if (pos == string::npos) { | |||
| *memory_path = cgv; | |||
There was a problem hiding this comment.
this is a bit hacky but I feel like it would save a bit of hassle in the future. Happy to hear some comments
(fyi: this is used in line 582)
There was a problem hiding this comment.
I am not a big fun of this because you transform a hard error to a passing condition.
Furthermore isn't this a bug? I mean both v1 and v2 must be of the form:
// N:<cgroup name>:<path> -- in case of v1, in many lines
// 0::<cgroup name> -- in case of v2, in a single line
So if : is missing, then something was read wrong?
There was a problem hiding this comment.
realistically this branch should never enter because that is simply the expected format of cgroups. this is assuming the function which reads is correct, but I think its a fair assumption 😸
I guess originally I added that check as a sanity check. I will change this to LOG(ERROR) and exit, instead of returning.
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <ido@dragonflydb.io>
Signed-off-by: talbii <ido@dragonflydb.io>
src/server/dfly_main.cc
Outdated
| @@ -515,8 +515,10 @@ error_code GetCGroupPath(string* memory_path, string* cpu_path) { | |||
| if (groups.size() == 1) { | |||
| // for v2 we only read 0::<name> | |||
| size_t pos = cgv.rfind(':'); | |||
| if (pos == string::npos) | |||
| if (pos == string::npos) { | |||
| *memory_path = cgv; | |||
There was a problem hiding this comment.
I am not a big fun of this because you transform a hard error to a passing condition.
Furthermore isn't this a bug? I mean both v1 and v2 must be of the form:
// N:<cgroup name>:<path> -- in case of v1, in many lines
// 0::<cgroup name> -- in case of v2, in a single line
So if : is missing, then something was read wrong?
| if (mmax && !absl::StartsWith(*mmax, "max")) { | ||
| CHECK(absl::SimpleAtoi(*mmax, &mdata->mem_total)); | ||
| } | ||
| mdata->mem_avail = min(mdata->mem_avail, mdata->mem_total); |
There was a problem hiding this comment.
Can the total memory ever be less than the available memory?
There was a problem hiding this comment.
oh good catch! because read_mem() also updates via min I think this min is actually redundant!
There was a problem hiding this comment.
edit: wait no that is not right. If we set memory.max to 8G, not set memory.high, and the system has more than 8G of available memory then mem_avail > mem_total.
On the other hand, if the available memory is less than what was set in mem_total, then mem_avail <= mem_total.
So I don't think its possible to remove the min() here.
if you have any issues feel free to contact internally :) When I first introduced this I wrote a small guide for testing: e646476 Edit: yes I have tested, inside and outside a K8s pod, everything worked well. |
Signed-off-by: talbii <ido@dragonflydb.io>
Closes #1683.
Background: In k8s, the memory/CPU cgroups which are reported by
/proc/self/cgroupdo not exist, i.e., the path/sys/fs/cgroup/<resource>/<k8s cgroup>doesn't exist. This causes Dragonfly to not read container limits at all, hence missing potential limits set by the user.Fix: Begin by first reading
/sys/fs/cgroup/<resource>/<limit>, which, in words, means "read the resource limits of the system". Later the path/sys/fs/cgroup/<resource>/<specific group>is read, updating the limit if needed.