path.rootfs stripping in func mountPointDetails() was not merged properly

[dt-rush]

Host operating system

Linux ip-10-0-93-21.ec2.internal 4.14.11-coreos #1 SMP Fri Jan 5 11:00:14 UTC 2018 x86_64 GNU/Linux

node_exporter version: output of node_exporter --version

node_exporter, version 0.18.0 (branch: , revision: )
  build user:       
  build date:       
  go version:       go1.12.5

node_exporter command line flags

working:

ExecStart=/bin/node_exporter \
    --web.listen-address :9000 \
    --web.telemetry-path /metrics \
    --log.level debug

pathological:

ExecStart=/bin/node_exporter \
    --web.listen-address :9000 \
    --web.telemetry-path /metrics \
    --path.rootfs /host \
    --log.level debug

Are you running node_exporter in Docker?

yes

What did you do that produced an error?

Attempt to export the node_filesystem_avail_bytes metric for a volume bind-mounted into the container via docker -v "/:/host:ro,rslave" (on the host machine, it lives at /media/MONITORDisk, so inside the container, it's at /host/media/MONITORDisk)

What did you expect to see?

The filesystem info for /host/media/MONITORDisk should be found and exported at {mountpoint="/media/MONITORDisk"} (/host being stripped, because it's the path.rootfs argument)

What did you see instead?

If path.rootfs is defined, we get the following errors in debug logs:

level=debug msg="Error on statfs() system call for \"/host/host/media/MONITORDisk\": no such file or directory" source="filesystem_linux.go:90"

If path.rootfs is undefined, we export the metric, but it doesn't have /host stripped from the mountpoint label value.

Analysis of why this is happening:

The ability to monitor bind-mounted volumes from host properly relies on removing a prefix directory they're mounted at.

In this case, we pass the parameter, --path.rootfs /host to the node_exporter binary so that /host/media/MONITORDisk can be checked and reported at /media/MONITORDisk in the labels of the resulting metric.

...but this feature was added in a PR that was closed, and then merged in another PR trying to replicate it after some discussion.

The second PR did not properly integrate the changes - so that instead of stripping /host from the metric on output, it appends it before trying to run statfs, and the binary ends up trying to run statfs on /host/host/media/MONITORDisk which of course doesn't exist.

The first (closed) PR had a function called rootfsStripPrefix which was used to strip the prefix for the mountpoint property of the filesystemLabels object which func mountPointDetails() outputs, so that when we run syscall.Statfs(rootfsFilePath(labels.mountPoint), buf), the fact that the rootfsFilePath function re-appends the stripped prefix is not a problem, as it allows the statfs() call to find the proper path on disk.

In the PR which was eventually merged instead, which was made as an imitation of this closed PR, the rootfsStripPrefix function was not included. Compare first PR and second PR (does not modify that line, which it should).

Suggested fix

Reinstate the rootfsStripPrefix function so that the code behaves as intended.

I'll be opening a PR for this shortly linking to this issue.

[dt-rush]

In addition:

• the files collector/filesystem_freebsd.go and collector/filesystem_bsd.go were never modified, as they should have been, to include the stripping function.

• there were not tests included to verify the behaviour of the stripping function given a certain procfs mounts file.

The PR I wrote (#1421) has corrected both of these issues as well.

[kinghrothgar]

Now that #1421 is merged this issue resolved?

[dt-rush]

@kinghrothgar yes, good check

[kinghrothgar]

Is there any ETA on when a new minor release will happen? This is a significant bug for anyone running node_exporter inside of docker. It is preventing me to upgrading to get other added features / metrics.

[dt-rush]

@kingkrothgar, I'm not sure as I'm not on the project but for my use case, I just clone at the specific commit where this was merged and build, doing a multi-stage build in docker so I just copy the built binary on into the runtime container image. Maybe that'll work for you for now as well.

[dt-rush]

(It builds a mostly-static binary, but it is still linked to musl-libc in my case (not sure if that's inherent or just a consequence of the way I build it in golang:alpine?)

So here's what I do:

#######################################################
# BUILD STAGE 1: go build environment for node_exporter
#######################################################

FROM golang:alpine AS gobuild

# install packages needed to build
RUN apk add make curl git && \
	git clone https://github.com/prometheus/node_exporter && \
	cd node_exporter && \
	git checkout 5d3e2ce2ef927eeb44fc409d3cd1fbba884571f3 && \
	make build && \
	cp node_exporter /usr/bin/

#######################################################
# BUILD STAGE 2: open-balena-base (runtime image)
#######################################################

FROM balena/open-balena-base:v8.0.1

#
#	MUSL-LIBC (for running 99%-static binary built in gobuild stage)
#

RUN wget http://www.musl-libc.org/releases/musl-1.1.10.tar.gz \
	&& tar -xvf musl-1.1.10.tar.gz \
	&& rm musl-1.1.10.tar.gz \
	&& cd musl-1.1.10 \
	&& ./configure \
	&& make \
	&& make install \
	&& cd .. \
	&& rm -rf musl-1.1.10

# copy node-exporter binary
COPY --from=gobuild /usr/bin/node_exporter /bin/

...

[discordianfish]

No time set but we'll release 1.0.0 probably in the next weeks.