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.

Sign up for GitHub

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

Jump to bottom

Add system.hostfs configuration option for system module #23831

Merged
merged 5 commits into from
Feb 8, 2021
Merged

Add system.hostfs configuration option for system module #23831

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b5b5b3
Add system.hostfs configuration option for system module
ChrsMark Feb 3, 2021
dbb9ce3
lint + update
ChrsMark Feb 3, 2021
bc104de
Fix subsystems' config
ChrsMark Feb 4, 2021
f30c810
add changelog
ChrsMark Feb 4, 2021
582cc3f
Add warn message when both cli and config is used
ChrsMark Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions metricbeat/docs/modules/system.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,9 @@ metricbeat.modules:
period: 10s
processes: ['.*']

# Configure the mount point of the host’s filesystem for use in monitoring a host from within a container
#system.hostfs: "/hostfs"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


# Configure the metric types that are included by these metricsets.
cpu.metrics: ["percentages","normalized_percentages"] # The other available option is ticks.
core.metrics: ["percentages"] # The other available option is ticks.
Expand Down
3 changes: 3 additions & 0 deletions metricbeat/metricbeat.reference.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ metricbeat.modules:
period: 10s
processes: ['.*']

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

# Configure the metric types that are included by these metricsets.
cpu.metrics: ["percentages","normalized_percentages"] # The other available option is ticks.
core.metrics: ["percentages"] # The other available option is ticks.
Expand Down
3 changes: 3 additions & 0 deletions metricbeat/module/system/_meta/config.reference.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
period: 10s
processes: ['.*']

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

# Configure the metric types that are included by these metricsets.
cpu.metrics: ["percentages","normalized_percentages"] # The other available option is ticks.
core.metrics: ["percentages"] # The other available option is ticks.
Expand Down
2 changes: 2 additions & 0 deletions metricbeat/module/system/_meta/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
process.include_top_n:
by_cpu: 5 # include top 5 processes by CPU
by_memory: 5 # include top 5 processes by memory
# Configure the mount point of the host’s filesystem for use in monitoring a host from within a container
#system.hostfs: "/hostfs"

- module: system
period: 1m
Expand Down
19 changes: 18 additions & 1 deletion metricbeat/module/system/system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
)

var (
// TODO: remove this flag in 8.0 since it should be replaced by system.hostfs configuration option (config.HostFS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

// HostFS is an alternate mountpoint for the filesytem root, for when metricbeat is running inside a container.
HostFS = flag.String("system.hostfs", "", "mountpoint of the host's filesystem for use in monitoring a host from within a container")
)
Expand All @@ -39,6 +40,11 @@ func init() {
}
}

// Config for the system module.
type Config struct {
HostFS string `config:"system.hostfs"` // Specifies the mount point of the host’s filesystem for use in monitoring a host from within a container.
}

// Module represents the system module
type Module struct {
mb.BaseModule
Expand All @@ -53,5 +59,16 @@ func NewModule(base mb.BaseModule) (mb.Module, error) {
initModule()
})

return &Module{BaseModule: base, HostFS: *HostFS, IsAgent: fleetmode.Enabled()}, nil
config := Config{
HostFS: "",
}
err := base.UnpackConfig(&config)
if err != nil {
return nil, err
}
if *HostFS != "" {
config.HostFS = *HostFS
}

return &Module{BaseModule: base, HostFS: config.HostFS, IsAgent: fleetmode.Enabled()}, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
2 changes: 2 additions & 0 deletions metricbeat/modules.d/system.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
process.include_top_n:
by_cpu: 5 # include top 5 processes by CPU
by_memory: 5 # include top 5 processes by memory
# Configure the mount point of the host’s filesystem for use in monitoring a host from within a container
#system.hostfs: "/hostfs"

- module: system
period: 1m
Expand Down
3 changes: 3 additions & 0 deletions x-pack/metricbeat/metricbeat.reference.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ metricbeat.modules:
period: 10s
processes: ['.*']

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

# Configure the metric types that are included by these metricsets.
cpu.metrics: ["percentages","normalized_percentages"] # The other available option is ticks.
core.metrics: ["percentages"] # The other available option is ticks.
Expand Down