Refactor use of system.hostfs to fix cgroup metrics (elastic#24334)

* refactor use of system.hostfs to fix cgroup metrics

* add changelog

* remove comment

* add cfgwarn

* move changelog

* shift around CLI config location and rep warning

* add comment about system.hostfs usage

* update docs

* capitalization

* fix grammar, add conditional

* change docs phrasing

(cherry picked from commit c70fe5c)

CHANGELOG.next.asciidoc

@@ -147,6 +147,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
 - Fix panic due to unhandled DeletedFinalStateUnknown in k8s OnDelete {pull}23419[23419]
 - Fix error loop with runaway CPU use when the Kafka output encounters some connection errors {pull}23484[23484]
 - Allow configuring credential_profile_name and shared_credential_file when using role_arn. {pull}24174[24174]
+- Fix issue discovering docker containers and metadata after reconnections {pull}24318[24318]
+- Allow cgroup self-monitoring to see alternate `hostfs` paths {pull}24334[24334]
 
 
 *Auditbeat*

libbeat/cmd/instance/beat.go

@@ -1102,13 +1102,22 @@ func initPaths(cfg *common.Config) error {
 	// the paths field. After we will unpack the complete configuration and keystore reference
 	// will be correctly replaced.
 	partialConfig := struct {
-		Path paths.Path `config:"path"`
+		Path   paths.Path `config:"path"`
+		Hostfs string     `config:"system.hostfs"`
 	}{}
 
+	if paths.IsCLISet() {
+		cfgwarn.Deprecate("8.0.0", "This flag will be removed in the future and replaced by a config value.")
+	}
+
 	if err := cfg.Unpack(&partialConfig); err != nil {
 		return fmt.Errorf("error extracting default paths: %+v", err)
 	}
 
+	// Read the value for hostfs as `system.hostfs`
+	// In the config, there is no `path.hostfs`, as we're merely using the path struct to carry the hostfs variable.
+	partialConfig.Path.Hostfs = partialConfig.Hostfs
+
 	if err := paths.InitPaths(&partialConfig.Path); err != nil {
 		return fmt.Errorf("error setting default paths: %+v", err)
 	}

libbeat/cmd/instance/metrics/metrics.go

@@ -29,6 +29,7 @@ import (
 	"github.com/elastic/beats/v7/libbeat/metric/system/cpu"
 	"github.com/elastic/beats/v7/libbeat/metric/system/process"
 	"github.com/elastic/beats/v7/libbeat/monitoring"
+	"github.com/elastic/beats/v7/libbeat/paths"
 	"github.com/elastic/gosigar/cgroup"
 )
 
@@ -285,6 +286,7 @@ func reportBeatCgroups(_ monitoring.Mode, V monitoring.Visitor) {
 	}
 
 	cgroups, err := cgroup.NewReaderOptions(cgroup.ReaderOptions{
+		RootfsMountpoint:         paths.Paths.Hostfs,
 		IgnoreRootCgroups:        true,
 		CgroupsHierarchyOverride: os.Getenv(libbeatMonitoringCgroupsHierarchyOverride),
 	})

libbeat/cmd/root.go

@@ -93,6 +93,7 @@ func GenRootCmdWithSettings(beatCreator beat.Creator, settings instance.Settings
 	rootCmd.PersistentFlags().AddGoFlag(flag.CommandLine.Lookup("path.data"))
 	rootCmd.PersistentFlags().AddGoFlag(flag.CommandLine.Lookup("path.logs"))
 	rootCmd.PersistentFlags().AddGoFlag(flag.CommandLine.Lookup("path.home"))
+	rootCmd.PersistentFlags().AddGoFlag(flag.CommandLine.Lookup("system.hostfs"))
 	rootCmd.PersistentFlags().AddGoFlag(flag.CommandLine.Lookup("strict.perms"))
 	if f := flag.CommandLine.Lookup("plugin"); f != nil {
 		rootCmd.PersistentFlags().AddGoFlag(f)

libbeat/docs/command-reference.asciidoc

@@ -704,12 +704,10 @@ the end of the file is reached. By default harvesters are closed after
 `close_inactive` is reached.
 endif::[]
 
-ifeval::["{beatname_lc}"=="metricbeat"]
 *`--system.hostfs MOUNT_POINT`*::
 
-Specifies the mount point of the host's filesystem for use in monitoring a host
-from within a container.
-endif::[]
+Specifies the mount point of the host's filesystem for use in monitoring a host.
+
 
 ifeval::["{beatname_lc}"=="packetbeat"]
 *`-t`*::

libbeat/docs/shared-path-config.asciidoc

@@ -106,3 +106,20 @@ Example:
 ------------------------------------------------------------------------------
 path.logs: /var/log/beats
 ------------------------------------------------------------------------------
+
+[float]
+==== `system.hostfs`
+
+Specifies the mount point of the host's filesystem for use in monitoring a host.
+This can either be set in the config, or with the `--system.hostfs` CLI flag. This is used for cgroup self-monitoring.
+ifeval::["{beatname_lc}"=="metricbeat"]
+This is also used by the system module to read files from `/proc` and `/sys`.
+endif::[]
+
+
+Example:
+
+[source,yaml]
+------------------------------------------------------------------------------
+path.logs: /var/log/beats
+------------------------------------------------------------------------------

libbeat/paths/paths.go

@@ -28,6 +28,8 @@
 //
 // path.config - Configuration files and Elasticsearch template default location
 //
+// system.hostfs - supplies an alternate filesystem root for containerized environments
+//
 // These settings can be set via the configuration file or via command line flags.
 // The CLI flags overwrite the configuration file options.
 //
@@ -38,27 +40,45 @@
 package paths
 
 import (
+	"flag"
 	"fmt"
 	"os"
 	"path/filepath"
 )
 
+var (
+	// TODO: remove this flag in 8.0 since it should be replaced by system.hostfs configuration option (config.HostFS)
+	// HostFS is an alternate mountpoint for the filesytem root, for when metricbeat is running inside a container.
+	hostFS = flag.String("system.hostfs", "", "Mount point of the host's filesystem for use in monitoring a host from within a container")
+)
+
+// Path tracks user-configurable path locations and directories
 type Path struct {
 	Home   string
 	Config string
 	Data   string
 	Logs   string
+	Hostfs string
 }
 
 // FileType is an enumeration type representing the file types.
 // Currently existing file types are: Home, Config, Data
 type FileType string
 
 const (
-	Home   FileType = "home"
+	// Home is the "root" directory for the running beats instance
+	Home FileType = "home"
+	// Config is the path to the beat config
 	Config FileType = "config"
-	Data   FileType = "data"
-	Logs   FileType = "logs"
+	// Data is the path to the beat data directory
+	Data FileType = "data"
+	// Logs is the path to the beats logs directory
+	Logs FileType = "logs"
+	// Hostfs is an alternate path to the filesystem root,
+	// used for system metrics that interact with procfs and sysfs.
+	// Unlike the other values here, this corrisponds to `system.hostfs`
+	// and not `path.hostfs`.
+	Hostfs FileType = "hostfs"
 )
 
 // Paths is the Path singleton on which the top level functions from this
@@ -115,15 +135,31 @@ func (paths *Path) initPaths(cfg *Path) error {
 		paths.Logs = filepath.Join(paths.Home, "logs")
 	}
 
+	if *hostFS != "" {
+		paths.Hostfs = *hostFS
+	}
+
+	if paths.Hostfs == "" {
+		paths.Hostfs = "/"
+	}
+
 	return nil
 }
 
+// IsCLISet returns true if the CLI system.hostfs value has been set
+func IsCLISet() bool {
+	if *hostFS != "" {
+		return true
+	}
+	return false
+}
+
 // Resolve resolves a path to a location in one of the default
 // folders. For example, Resolve(Home, "test") returns an absolute
 // path for "test" in the home path.
 func (paths *Path) Resolve(fileType FileType, path string) string {
-	// absolute paths are not changed
-	if filepath.IsAbs(path) {
+	// absolute paths are not changed for non-hostfs file types, since hostfs is a little odd
+	if filepath.IsAbs(path) && fileType != Hostfs {
 		return path
 	}
 
@@ -136,6 +172,8 @@ func (paths *Path) Resolve(fileType FileType, path string) string {
 		return filepath.Join(paths.Data, path)
 	case Logs:
 		return filepath.Join(paths.Logs, path)
+	case Hostfs:
+		return filepath.Join(paths.Hostfs, path)
 	default:
 		panic(fmt.Sprintf("Unknown file type: %s", fileType))
 	}

metricbeat/module/system/diskio/diskio_test.go

@@ -26,16 +26,23 @@ import (
 
 	"github.com/stretchr/testify/assert"
 
+	"github.com/elastic/beats/v7/libbeat/paths"
 	mbtest "github.com/elastic/beats/v7/metricbeat/mb/testing"
-	"github.com/elastic/beats/v7/metricbeat/module/system"
 )
 
+func setHostfs(pathString string) {
+	path := paths.Path{
+		Hostfs: pathString,
+	}
+	paths.InitPaths(&path)
+}
+
 func TestDataNameFilter(t *testing.T) {
-	oldFS := system.HostFS
-	newFS := "_meta/testdata"
-	system.HostFS = &newFS
+	oldFS := paths.Paths.Hostfs
+	setHostfs("_meta/testdata")
+
 	defer func() {
-		system.HostFS = oldFS
+		setHostfs(oldFS)
 	}()
 
 	conf := map[string]interface{}{
@@ -51,11 +58,11 @@ func TestDataNameFilter(t *testing.T) {
 }
 
 func TestDataEmptyFilter(t *testing.T) {
-	oldFS := system.HostFS
-	newFS := "_meta/testdata"
-	system.HostFS = &newFS
+	oldFS := paths.Paths.Hostfs
+	setHostfs("_meta/testdata")
+
 	defer func() {
-		system.HostFS = oldFS
+		setHostfs(oldFS)
 	}()
 
 	conf := map[string]interface{}{
@@ -67,6 +74,7 @@ func TestDataEmptyFilter(t *testing.T) {
 	data, errs := mbtest.ReportingFetchV2Error(f)
 	assert.Empty(t, errs)
 	assert.Equal(t, 10, len(data))
+
 }
 
 func TestFetch(t *testing.T) {

metricbeat/module/system/entropy/entropy.go

@@ -29,8 +29,8 @@ import (
 
 	"github.com/elastic/beats/v7/libbeat/common"
 	"github.com/elastic/beats/v7/libbeat/common/cfgwarn"
+	"github.com/elastic/beats/v7/libbeat/paths"
 	"github.com/elastic/beats/v7/metricbeat/mb"
-	"github.com/elastic/beats/v7/metricbeat/module/system"
 )
 
 // init registers the MetricSet with the central registry as soon as the program
@@ -55,12 +55,7 @@ type MetricSet struct {
 func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
 	cfgwarn.Beta("The system entropy metricset is beta.")
 
-	systemModule, ok := base.Module().(*system.Module)
-	if !ok {
-		return nil, errors.New("unexpected module type")
-	}
-
-	totalPath := path.Join(systemModule.HostFS, "/proc/sys/kernel/random")
+	totalPath := paths.Resolve(paths.Hostfs, "/proc/sys/kernel/random")
 
 	return &MetricSet{
 		BaseMetricSet: base,

metricbeat/module/system/entropy/entropy_test.go

@@ -24,13 +24,19 @@ import (
 
 	"github.com/stretchr/testify/assert"
 
+	"github.com/elastic/beats/v7/libbeat/paths"
 	mbtest "github.com/elastic/beats/v7/metricbeat/mb/testing"
-	"github.com/elastic/beats/v7/metricbeat/module/system"
 )
 
 func TestData(t *testing.T) {
-	testdata := "./_meta/testdata"
-	system.HostFS = &testdata
+	testPath := paths.Path{
+		Hostfs: "./_meta/testdata",
+	}
+
+	if err := paths.InitPaths(&testPath); err != nil {
+		t.Errorf("error setting default paths: %+v", err)
+		t.FailNow()
+	}
 	f := mbtest.NewReportingMetricSetV2Error(t, getConfig())
 	err := mbtest.WriteEventsReporterV2Error(f, t, ".")
 	if err != nil {
@@ -39,8 +45,14 @@ func TestData(t *testing.T) {
 }
 
 func TestFetch(t *testing.T) {
-	testdata := "./_meta/testdata"
-	system.HostFS = &testdata
+	testPath := paths.Path{
+		Hostfs: "./_meta/testdata",
+	}
+
+	if err := paths.InitPaths(&testPath); err != nil {
+		t.Errorf("error setting default paths: %+v", err)
+		t.FailNow()
+	}
 	f := mbtest.NewReportingMetricSetV2Error(t, getConfig())
 	events, errs := mbtest.ReportingFetchV2Error(f)
 

metricbeat/module/system/filesystem/helper.go

@@ -22,15 +22,14 @@ package filesystem
 import (
 	"bufio"
 	"os"
-	"path"
 	"path/filepath"
 	"strings"
 	"time"
 
 	"runtime"
 
 	"github.com/elastic/beats/v7/libbeat/common"
-	"github.com/elastic/beats/v7/metricbeat/module/system"
+	"github.com/elastic/beats/v7/libbeat/paths"
 	sigar "github.com/elastic/gosigar"
 )
 
@@ -194,7 +193,7 @@ func BuildTypeFilter(ignoreType ...string) Predicate {
 func DefaultIgnoredTypes() (types []string) {
 	// If /proc/filesystems exist, default ignored types are all marked
 	// as nodev
-	fsListFile := path.Join(*system.HostFS, "/proc/filesystems")
+	fsListFile := paths.Resolve(paths.Hostfs, "/proc/filesystems")
 	if f, err := os.Open(fsListFile); err == nil {
 		scanner := bufio.NewScanner(f)
 		for scanner.Scan() {

metricbeat/module/system/process/process.go

@@ -28,6 +28,7 @@ import (
 	"github.com/elastic/beats/v7/libbeat/common"
 	"github.com/elastic/beats/v7/libbeat/logp"
 	"github.com/elastic/beats/v7/libbeat/metric/system/process"
+	"github.com/elastic/beats/v7/libbeat/paths"
 	"github.com/elastic/beats/v7/metricbeat/mb"
 	"github.com/elastic/beats/v7/metricbeat/mb/parse"
 	"github.com/elastic/beats/v7/metricbeat/module/system"
@@ -83,8 +84,8 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
 
 	if runtime.GOOS == "linux" {
 		if config.Cgroups == nil || *config.Cgroups {
-			debugf("process cgroup data collection is enabled, using hostfs='%v'", systemModule.HostFS)
-			m.cgroup, err = cgroup.NewReader(systemModule.HostFS, true)
+			debugf("process cgroup data collection is enabled, using hostfs='%v'", paths.Paths.Hostfs)
+			m.cgroup, err = cgroup.NewReader(paths.Paths.Hostfs, true)
 			if err != nil {
 				if err == cgroup.ErrCgroupsMissing {
 					logp.Warn("cgroup data collection will be disabled: %v", err)

metricbeat/module/system/raid/raid.go

@@ -24,9 +24,9 @@ import (
 	"github.com/prometheus/procfs"
 
 	"github.com/elastic/beats/v7/libbeat/common"
+	"github.com/elastic/beats/v7/libbeat/paths"
 	"github.com/elastic/beats/v7/metricbeat/mb"
 	"github.com/elastic/beats/v7/metricbeat/mb/parse"
-	"github.com/elastic/beats/v7/metricbeat/module/system"
 	"github.com/elastic/beats/v7/metricbeat/module/system/raid/blockinfo"
 )
 
@@ -45,10 +45,6 @@ type MetricSet struct {
 
 // New creates a new instance of the raid metricset.
 func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
-	systemModule, ok := base.Module().(*system.Module)
-	if !ok {
-		return nil, errors.New("unexpected module type")
-	}
 
 	// Additional configuration options
 	config := struct {
@@ -60,7 +56,7 @@ func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
 	}
 
 	if config.MountPoint == "" {
-		config.MountPoint = systemModule.HostFS
+		config.MountPoint = paths.Paths.Hostfs
 	}
 
 	mountPoint := filepath.Join(config.MountPoint, procfs.DefaultMountPoint)