Add system.hostfs configuration option for system module

[ChrsMark]

What does this PR do?

This PR adds system.hostfs configuration option for system module.
It will be used instead of the flag -system.hostfs=/hostfs where adding a flag is not possible ie in Agent. Using the -system.hostfs will still be available until we completely remove it in 8.0

Why is it important?

In some cases setting the proc filesystem using the -system.hostfs is not possible like when running with Agent, see #22915. A configuration option should be used instead.

How to test this PR manually

Note: Testing alongside with elastic/integrations#673 is recommended, means that testing the package directly is required for test-plan phase of this PR.

1. Build Metricbeat: GOOS=linux GOARCH=amd64 go build
2. Prepare a configuration file vim system.yml:

- module: system
  period: 10s
  metricsets:
    - process
  # Configure the mount point of the host’s filesystem for use in monitoring a host from within a container
  system.hostfs: "/hostfs"

3. Use the following Dockerfile.debug to build a custom image locally:

FROM docker.elastic.co/beats/metricbeat:7.10.2
COPY metricbeat.yml /usr/share/metricbeat/metricbeat.yml
COPY metricbeat /usr/share/metricbeat/metricbeat
COPY system.yml /usr/share/metricbeat/modules.d/system.yml
USER root
RUN chown root:metricbeat /usr/share/metricbeat/metricbeat.yml
USER metricbeat

4. Build the image: docker build -f Dockefile.debug . -t metricbeat-hostfs
5. Run Metricbeat and verify that process metricset collects data:

docker run --mount type=bind,source=/proc,target=/hostfs/proc,readonly --mount type=bind,source=/sys/fs/cgroup,target=/hostfs/sys/fs/cgroup,readonly --mount type=bind,source=/,target=/hostfs,readonly --net=host metricbeat-hostfs -e -d "*" 

6. Try to break Metricbeat by adding a random hostpath flag: -system.hostfs=/hostfs45

Related issues

• Closes Add Metricbeat --system.hostfs equivalent to Elastic Agent #22915
• Relates [Elastic Agent] [Meta] Gathering logs and metrics from Kubernetes without using fleet generated agent config and without central agent management via fleet #23613

cc: @blakerouse

[elasticmachine]

Pinging @elastic/integrations (Team:Integrations)

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: Pull request #23831 updated

• • Start Time: 2021-02-05T14:25:39.215+0000

• Duration: 35 min 55 sec

• Commit: 582cc3f

Test stats 🧪

Test	Results
Failed	0
Passed	8424
Skipped	2116
Total	10540

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	8424
Skipped	2116
Total	10540

[fearful-symmetry]

We're probably gonna have to work a little harder to merge the values from the beat config and the CLI. First of all, system_linux.go will need to be changed to grab its value from somewhere else, presumably the merged config. Secondly, we might want to be a little more clear about what value takes precedence, since one value can override another here. Maybe some logic for if *HostFS != "" && config.HostFS != "" { error() } or something like that? Just worried about one implicitly overriding the other.

[fearful-symmetry]

👍

[jsoriano]

Have you considered adding this option to all beats as a global setting?

Apart of the system module, other features can benefit of accessing the host fs, as add_docker_metadata, that actually has another setting for this, or the internal metrics, that rely on the global gosigar.Procd to get the pid of the process. gosigar.Procd is modified in Metricbeat by the system module, what can lead to inconsistent behaviour between Beats (Metricbeat managed by an agent in a container can be monitored from the host pid namespace, but other Beats cannot).

An option can be to have explicit settings for each feature (as there is one for the system module and another one for add_docker_metadata) but I wonder if when wanting to override this setting this is not wanted for all features.

In any case, just food for thought, this could be decided later.

[ChrsMark]

Thanks for commenting @jsoriano ! Your suggestion sounds reasonable and definitely this configuration option could be revisited/refactored.

For now I would avoid adding more complexity on this since this issue is quite blocking for running system module through Agent in containerized environements.

@fearful-symmetry do you think we should create a follow-up issue to file what Jaime suggested?

[ChrsMark]

Thanks @fearful-symmetry for the review! I updated the linux subsystem initialisation so as to take into account the configuration option. As far as the check is concerned I don't think we should add a check like if *HostFS != "" && config.HostFS != "" { error() } since I don't see the reason of someone having both set. My thought initially was:

1. unpack the config with default value of HostFS to empty string.
2. check if CLI flag is set by checking if !="" and if so replace the configuration field no matter what it had before.

I see CLI flags as more powerful since anyone adding them should know the reason for this. If there is a case of this won't work I'm happy to change it.

Let me know what you think!

[fearful-symmetry]

Changes look good. As far as merging the configs, I think it's fine if we don't throw an error, but I've seen enough weird and crufty container deployments that I think at least printing a little warning message when the CLI overrides the config might be a good idea. I might be overreacting, but it can't hurt.

As far as a further issue to deal with the gosigar stuff, unless it's an immediate need, I'm not sure it's needed. We're planning to do a fairly deep refactor of the system module sometime soon, and we might render a lot of the conversation obsolete.

[ChrsMark]

@fearful-symmetry I added the warn message. Feel free to have another look.

Also after we merge this we need the corresponding change in system package definition too so as to make it work with Agent, right?

[fearful-symmetry]

LGTM @jsoriano you're right, the metadata processors are all over the place when it comes to how we're gathering host data. We might want to consider some kind of holdover until we refactor a lot of the metricbeat code.