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

docs(docker): mount /sys/class/powercap and /proc read-only #57

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bdossantos
Copy link

@bdossantos bdossantos commented Jan 17, 2021

Hi! Really cool tool!

Here is a small suggestion: it's probably "better" to mount /sys/class/powercap and /proc in containter ro

@bpetit
Copy link
Contributor

bpetit commented Jan 18, 2021

Hi,

Thanks a lot for the PR and the suggestion. I tried it and ran into an issue with apparmor:

docker: Error response from daemon: OCI runtime create failed: container_linux.go:349: starting container process caused "process_linux.go:449: container init caused "apply apparmor profile: apparmor failed to apply profile: open /proc/self/attr/exec: read-only file system"": unknown.

It seems like apparmor, in the container, needs to write on /proc, so this is a blocker.

However, you raise an important topic. We need to find a way to make the configuration of the container less "invasive". I wonder if there is a "clean" way to mount only the parts of /proc that we need for tracking the other processes on the host. We could then maybe mount them RO.

Any thoughts ?

@bdossantos
Copy link
Author

Hi,

Thanks a lot for the PR and the suggestion. I tried it and ran into an issue with apparmor:

docker: Error response from daemon: OCI runtime create failed: container_linux.go:349: starting container process caused "process_linux.go:449: container init caused "apply apparmor profile: apparmor failed to apply profile: open /proc/self/attr/exec: read-only file system"": unknown.

It seems like apparmor, in the container, needs to write on /proc, so this is a blocker.

However, you raise an important topic. We need to find a way to make the configuration of the container less "invasive". I wonder if there is a "clean" way to mount only the parts of /proc that we need for tracking the other processes on the host. We could then maybe mount them RO.

Any thoughts ?

Hi, nice catch, I've not tested on an host with apparmor.

I usually run my containers read-only. Another way to be less invasive, running the image as non-root like node_exporter:

https://github.com/prometheus/node_exporter/blob/master/Dockerfile#L11

@bpetit bpetit added this to Triage in General Jan 18, 2021
@bpetit
Copy link
Contributor

bpetit commented Jan 20, 2021

You're right. We have to optimize on that part too.

I may give it a try next week. Regarding the RO volumes, we need to find a solution to do that without crashing apparmor or any other service that needs access to some /proc subset at a given time to crash. Maybe mounting the /proc/self files as RW and the rest as RO could work here ?

@bdossantos
Copy link
Author

bdossantos commented Jan 20, 2021

You're right. We have to optimize on that part too.

I may give it a try next week. Regarding the RO volumes, we need to find a solution to do that without crashing apparmor or any other service that needs access to some /proc subset at a given time to crash. Maybe mounting the /proc/self files as RW and the rest as RO could work here ?

Another possibility is to mount /proc:/host/proc:ro and /sys:/host/sys:ro with a prefix like netdata. To be able to do this scaphrandre probably need a small modification to accept an option flag to set a prefix for /proc and /sys (null by default)

https://learn.netdata.cloud/docs/agent/packaging/docker
https://learn.netdata.cloud/docs/agent/daemon (see -s path option)

edit: metricbeat seem to do the same https://www.elastic.co/guide/en/beats/metricbeat/current/running-on-docker.html

@bpetit
Copy link
Contributor

bpetit commented Jan 20, 2021

Good idea. It shouldn't be a problem to implement that. I'll try to start working on it next week then, if nobody jumps on it first.
Thanks for the reporting and ideas :)

@bpetit bpetit moved this from Triage to To do in General Jan 20, 2021
@bpetit bpetit moved this from To do to In progress in General Jan 29, 2021
bpetit added a commit that referenced this pull request Jan 29, 2021
to ask scaph to look in PREFIX/proc and PREFIX/sys/class/powercap
instead of the default paths. This enables to run scaph in docker with
only RO mountpoins. (see discussion
#57 for more details)
@bpetit
Copy link
Contributor

bpetit commented Jan 29, 2021

trying a PR for that topic: #64

@bpetit
Copy link
Contributor

bpetit commented Jan 30, 2021

I'm building a new image to try the use case you thought about.

@bdossantos
Copy link
Author

I'm building a new image to try the use case you thought about.

Nice job 👍

@bpetit
Copy link
Contributor

bpetit commented Jan 30, 2021

It will be a bit more complex than anticipated, because we use the procfs crate to get metrics from /proc and will need to find a way to tell it to consider the prefix too... I'll update here about the progress.

@bpetit bpetit self-assigned this Feb 8, 2021
@bpetit bpetit moved this from In progress to To do in General Mar 11, 2021
@bpetit bpetit added this to the Release v0.5.0 milestone Dec 16, 2021
@bpetit bpetit moved this from To do to In progress in General Mar 5, 2022
@bpetit
Copy link
Contributor

bpetit commented Jul 14, 2022

An appropriate feature has been implemented in procfs. I should be able to fix this for release 0.5

Stay tuned !

@bpetit bpetit modified the milestones: Release v0.5.0, Release v0.6.0 Oct 10, 2022
@bpetit bpetit moved this from In progress to To do in General Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
General
To do
Development

Successfully merging this pull request may close these issues.

None yet

2 participants