Skip to content

Conversation

talbii
Copy link
Contributor

@talbii talbii commented May 31, 2023

Following some community interaction, this PR adds detection for cgroup v1 as well, where previously there was only v2 support.

@talbii talbii force-pushed the cgroup-v1-detection branch from 88c3d5d to 79b3366 Compare May 31, 2023 12:30
chakaz
chakaz previously approved these changes Jun 1, 2023
Copy link
Contributor

@chakaz chakaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it's not trivial to add unit tests for this change. Have you tested it end-to-end?

@royjacobson
Copy link
Contributor

Unfortunately it's not trivial to add unit tests for this change. Have you tested it end-to-end?

We can split the parsing part to a unit-testable function and copy paste output samples of /proc/self/cgroup there, don't have to actually bootstrap a V1 docker or something 😅

@chakaz
Copy link
Contributor

chakaz commented Jun 1, 2023

I actually think that testing this on a V1 docker is something we should do before committing this :) Not necessarily in a pytest / integration test, but at least manually.
Ideally we could add a unit-test where we mock the file system as a whole, providing the files we need. But it doesn't seem to be supported in Helio.

@talbii
Copy link
Contributor Author

talbii commented Jun 1, 2023

Sure, it's possible to test the parsing. Is this needed?

About the actual detection: yes, I did test it on both v1 and v2 (with v1 being inside a VM). All seemed well.

I guess it would be possible to unit test as a whole (creating Docker image, and running Dragonfly inside to it and checking limits, etc.), but this seems like overkill.

@chakaz
Copy link
Contributor

chakaz commented Jun 1, 2023

I agree that it's an over kill (and not a unit test, more of an end to end test :).
So long as you've tested it manually, I think we're good here. @royjacobson are you ok with this as well?

@talbii talbii force-pushed the cgroup-v1-detection branch from 79b3366 to ba92c9b Compare June 5, 2023 12:17
@talbii talbii requested a review from chakaz June 5, 2023 12:22
@talbii talbii force-pushed the cgroup-v1-detection branch from 32c0d61 to a0a86dd Compare June 5, 2023 12:22
@talbii
Copy link
Contributor Author

talbii commented Jun 5, 2023

Welp... not really sure what to do here. The test rdb_test seems to fail now. It runs just fine in my machine. 🙁

@chakaz
Copy link
Contributor

chakaz commented Jun 5, 2023

I don't even see the output for rdb_test. Perhaps you could rebase on main and see if it's resolved? I see that you're out of date anyway

@talbii talbii force-pushed the cgroup-v1-detection branch from 55577bd to 249468d Compare June 7, 2023 11:45
@romange romange force-pushed the cgroup-v1-detection branch from 249468d to 6750606 Compare June 7, 2023 12:14
For future reference: testing. In order to test this feature (both for
v1 and v2). To do this, create a cgroup and move Dragonfly into it
before running.

One way to do this is using a script, like: cat; ./dragonfly
--logtostderr

Enabling v1 (by default, you should have v2): Edit `/etc/default/grub`,
and look for `GRUB_CMDLINE_LINUX_DEFAULT`. Append to it
`systemd.unified_cgroup_hierarchy=0`, update GRUB using `sudo
updat-grub`, and reboot.

In v1, create a group `mycgroup` and add PID 1234 to it:

```
sudo mkdir -p /sys/fs/cgroup/memory/mycgroup
sudo bash -c "echo 8589934592 >
/sys/fs/cgroup/memory/mycgroup/memory.limit_in_bytes" # set mem limit to
8G
sudo bash -c "echo 1234 > /sys/fs/cgroup/memory/mycgroup/tasks" # assign
PID 1234 to this cgroup
```

In v2:

```
sudo mkdir -p /sys/fs/cgroup/mycgroup
sudo bash -c "echo 8589934592 > /sys/fs/cgroup/mycgroup/memory.max" #
set mem limit to 8G
sudo bash -c "echo 1234 > /sys/fs/cgroup/mycgroup/cgroup.procs" # assign
PID 1234 to this cgroup
```

then test by (using script example from before):

```
$ ./run.sh # contains: cat; ./dragonfly --logtostderr
[1] 1234
[1]  + 1234 suspended (tty input)  ./run.sh
fg
^D
<look for Dragonfly memory limit>
```

Signed-off-by: talbii <[email protected]>
@talbii talbii force-pushed the cgroup-v1-detection branch from 6750606 to b393884 Compare June 7, 2023 12:55
@romange romange merged commit e646476 into main Jun 7, 2023
@romange romange deleted the cgroup-v1-detection branch June 7, 2023 13:38
dranikpg pushed a commit that referenced this pull request Jun 9, 2023
Add support for cgroup v1 limit checking

For future reference: testing. In order to test this feature (both for
v1 and v2). To do this, create a cgroup and move Dragonfly into it
before running.

One way to do this is using a script, like: cat; ./dragonfly
--logtostderr

Enabling v1 (by default, you should have v2): Edit `/etc/default/grub`,
and look for `GRUB_CMDLINE_LINUX_DEFAULT`. Append to it
`systemd.unified_cgroup_hierarchy=0`, update GRUB using `sudo
updat-grub`, and reboot.

In v1, create a group `mycgroup` and add PID 1234 to it:

```
sudo mkdir -p /sys/fs/cgroup/memory/mycgroup
sudo bash -c "echo 8589934592 >
/sys/fs/cgroup/memory/mycgroup/memory.limit_in_bytes" # set mem limit to
8G
sudo bash -c "echo 1234 > /sys/fs/cgroup/memory/mycgroup/tasks" # assign
PID 1234 to this cgroup
```

In v2:

```
sudo mkdir -p /sys/fs/cgroup/mycgroup
sudo bash -c "echo 8589934592 > /sys/fs/cgroup/mycgroup/memory.max" #
set mem limit to 8G
sudo bash -c "echo 1234 > /sys/fs/cgroup/mycgroup/cgroup.procs" # assign
PID 1234 to this cgroup
```

then test by (using script example from before):

```
$ ./run.sh # contains: cat; ./dragonfly --logtostderr
[1] 1234
[1]  + 1234 suspended (tty input)  ./run.sh
fg
^D
<look for Dragonfly memory limit>
```

Signed-off-by: talbii <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants