strip rootfs prefix for run in docker

[diafour]

relates to this: #66 (comment)

Beside that, I'm still open to have some 'stripping' of a prefix but everyone who was interested in this happily migrated to running the exporter on the host, so I'd like to close this issue after merging #376.

Introduce new argument path.rootfs. This argument must match the path in bind-mount of host root. The node_exporter will use path.rootfs as prefix to filter entries in ${path.procfs}/mounts and to
cleanup it from mountpoint label.

[diafour]

Run without path.rootfs:

$ docker run -ti --rm \
  --net="host" \
  --pid="host" \
  -v "/proc:/host/proc:ro" -v "/sys:/host/sys:ro" -v "/:/rootfs:ro" \
  node-exporter:master \
  --path.procfs /host/proc --path.sysfs /host/sys

Example result metrics:

node_filesystem_avail_bytes{device="/dev/sda1",fstype="ext2",mountpoint="/rootfs/boot"} 1.71153408e+08
node_filesystem_avail_bytes{device="/dev/mapper/ubuntu--vg-root",fstype="ext4",mountpoint="/rootfs"} 5.722222592e+09

Run with path.rootfs:

$ docker run -ti --rm \
  --net="host" \
  --pid="host" \
  -v "/proc:/host/proc:ro" -v "/sys:/host/sys:ro" -v "/:/rootfs:ro" \
  node-exporter:master \
  --path.procfs /host/proc --path.sysfs /host/sys \
  --path.rootfs /rootfs

example result metric:

node_filesystem_avail_bytes{device="/dev/sda1",fstype="ext2",mountpoint="/boot"} 1.71153408e+08
node_filesystem_avail_bytes{device="/dev/mapper/ubuntu--vg-root",fstype="ext4",mountpoint="/"} 5.722222592e+09

[discordianfish]

What about making using path.rootfs as prefix for all paths? In which case you wouldn't have to modify the procfs path flags but only set the prefix flag? Then there wouldn't be a need for detecting/skipping paths.

[discordianfish]

@SuperQ btw, you are okay with a change like this in general? Before we waste too much time.. I still think that node-exporter is ideally running on the host but not even I am doing this in most of my deployments because my host OS is immutable and I like to update node-exporter without rebuilding my VM..

[SuperQ]

I do think we should support docker, as long as the changes are sane. It's annoying and convoluted, but I do see the use case.

[discordianfish]

Yeah I think the best we can do is a flag which is used consistenly for all procfs stuff, that way you only need one flag and it's easier to handle in code too.

[diafour]

@discordianfish Do I understand correctly? One flag with prefix instead of 3 flags procfs, sysfs and rootfs. Something like this?

$ docker run -ti --rm \
  --net="host" \
  --pid="host" \
  -v "/:/host:ro,rslave" \
  node-exporter:master \
  --path.prefix /host

If you need to monitor host from container (for example with kubernetes DaemonSet) than you bind-mount host rootfs and use path.prefix. If this flag is set node_exporter will trim this prefix from /proc/mounts entries. procFilePath and sysFilePath methods are use path.prefix instead path.procfs and path.sysfs.

[discordianfish]

Yes though:

procFilePath and sysFilePath methods are use path.prefix instead path.procfs and path.sysfs.

I'm suggesting that they would take the path.prefix and prepend to path.procfs/path.sysfs.

@SuperQ Does that make sense to you too?

[SuperQ]

Hrm, I'm thinking we should keep the rootfs prefix separate from the other two.

[discordianfish]

@SuperQ Can you explain what exactly you suggest and the reason for it? My proposal is clear?

[cofyc]

I couldn't find another way to migrate node_exporter on the host to container without breaking metrics. I've built a custom image with this patch. Is it possible to merge this pr? Then I can use official image.

[SuperQ]

I would like to keep the docker flags as simple as possible.

I like this setup proposal:

docker run -ti --rm \
  --net="host" \
  --pid="host" \
  -v "/:/host:ro,rslave" \
  node-exporter:master \
  --path.prefix=/host

With this option do we need to prepend the --path.sysfs and --path.procfs with the prefix? It seems like we wouldn't need to.

[discordianfish]

@SuperQ That's what I was suggesting. Create a --path.prefix flag and prepend this in the code to the procfs/sysfs flags.

[SuperQ]

@discordianfish I don't think we need to add the prefix to --path.procfs and --path.sysfs because --pid="host" on docker makes them work in the default location.

[SuperQ]

I tested this PR with:

docker run -d \
   --net="host" \
   --pid="host" \
   -v "/:/host:ro,rslave" \
   node-exporter:pr-912 --path.rootfs=/host

I get the expected metrics and labels. It looks like there is no need to adjust the /proc and /sys mapping due to the --pid="host" option to docker.

[discordianfish]

Ah I see, so yeah guess then we don't need to prefix them. In this case, LGTM!

[marcbachmann]

Is there any reason this wasn't merged?

[SuperQ]

The example README update needs to be adjusted, there's no need to change volume mounts.

[marcbachmann]

I've rebased in here and updated the readme: https://github.com/prometheus/node_exporter/compare/master...marcbachmann:rootfs_prefix_66?expand=1

But with make build I get some errors that aren't related to that PR. And with go build it results in an executable where the arguments aren't present 😕.

[discordianfish]

@diafour Can you rebase and do the change SuperQ mentioned?

[discordianfish]

Going to close this for now. Freel free to update and re-open.