Skip to content
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

Use cgroups for limiting memory usage #2398

Merged
merged 2 commits into from Apr 15, 2024

Conversation

Williangalvani
Copy link
Member

@Williangalvani Williangalvani commented Feb 20, 2024

fix #2393
fix #2387
fix #2378

@Williangalvani Williangalvani marked this pull request as ready for review February 28, 2024 16:58
@Williangalvani
Copy link
Member Author

it seems good now. I'd appreciate some more testing =]
Btw, you can add a cgroup column to htop, but it does mess up the columns spacing =[
image

@joaoantoniocardoso
Copy link
Collaborator

joaoantoniocardoso commented Feb 28, 2024

We extensively tested (@Williangalvani and me) for two hours changing the memory limit, as well as playing with other controls not used on this PR, like the memory.high, memory.swap.high and memory.swap.max.

As a next improvement, we should be able to configure the memory.high, and move this run_service.sh to some robust application (I heard Rust?) instead of a bash script, possibly having a frontend to show the cgroups stats and events, and allow the command line, env variables, and the cgroups controls to be configured.

@joaoantoniocardoso
Copy link
Collaborator

joaoantoniocardoso commented Feb 28, 2024

Useful commands inside the docker:

# Check if any process has been hitting memory limits and/or has been killed by the OOM.
cat /sys/fs/cgroup/$DOCKER_CGROUP/*/memory.events
# Check the current memory of any service:
cat /sys/fs/cgroup/$DOCKER_CGROUP/<cgroupname>/memory.*current

...and RedHat has a good resource about how to monitor it here. The most useful command is, from outside the docker:

systemd-cgtop

@patrickelectric patrickelectric added the merge-after-stable Should be merged only after next stable release label Mar 7, 2024
@Williangalvani
Copy link
Member Author

time to get this in?

ssh -i /root/.config/.ssh/id_rsa -o StrictHostKeyChecking=no pi@localhost "$1"
}

prepare_cgroups() {
Copy link
Member

Choose a reason for hiding this comment

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

We should test the ssh_command is working, if not, we should abort this process and continue the start script.
The reason behind this is that the start script is the root of the entire BlueOS, it can't fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

done


echo "Creating a new child cgroup..."
# Create a new child cgroup
mkdir /sys/fs/cgroup/$DOCKER_CGROUP/child_cgroup
Copy link
Member

Choose a reason for hiding this comment

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

We should create a variable for /sys/fs/cgroup/$DOCKER_CGROUP, since it's used so many times

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@Williangalvani
Copy link
Member Author

@patrickelectric can you try it on docker compose? I'm having some iptables issues

@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Apr 15, 2024
@ES-Alexander
Copy link
Collaborator

Would be nice if we could include some notes about this in the BlueOS-core dev docs, if/when it gets merged in.

@patrickelectric
Copy link
Member

@patrickelectric can you try it on docker compose? I'm having some iptables issues

Everything appears to be fine with compose

@patrickelectric patrickelectric merged commit e047c01 into bluerobotics:master Apr 15, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented merge-after-stable Should be merged only after next stable release
Projects
None yet
4 participants