Skip to content
77 changes: 77 additions & 0 deletions managed/services/agents/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package agents

import (
"slices"
"sort"

agentv1 "github.com/percona/pmm/api/agent/v1"
Expand All @@ -32,6 +33,67 @@ var (
v2_28_00 = version.MustParse("2.28.0-0")
)

// defaultEnabledNodeExporterCollectors lists collectors that node_exporter enables on Linux on its own,
// as of node_exporter 1.8.2. Dropping the "--collector.<name>" flag does not stop those, so disabling one
// means passing "--no-collector.<name>" explicitly. 14 of them are in the "disabled" block below already,
// which is why they are appended only when missing.
//
// Entries are exact node_exporter collector names, the same way DisabledCollectors is matched everywhere
// else, so a name here never stands for a family of collectors. In particular "textfile" is the upstream
// base collector alone: the textfile metrics PMM actually collects come from the separate, default-off
// "textfile.hr"/"textfile.mr"/"textfile.lr" collectors, and silencing those means listing them by name so
// that FilterOutCollectors drops their "--collector." flag. Disabling a collector must stay in sync with
// scrapeConfigsForNodeExporter, which filters the same names out of "collect[]" - naming a disabled
// collector there makes node_exporter answer the whole resolution endpoint with HTTP 400.
var defaultEnabledNodeExporterCollectors = []string{
"arp",
"bcache",
"bonding",
"btrfs",
"conntrack",
"cpu",
"cpufreq",
"diskstats",
"dmi",
"edac",
"entropy",
"fibrechannel",
"filefd",
"filesystem",
"hwmon",
"infiniband",
"ipvs",
"loadavg",
"mdadm",
"meminfo",
"netclass",
"netdev",
"netstat",
"nfs",
"nfsd",
"nvme",
"os",
"powersupplyclass",
"pressure",
"rapl",
"schedstat",
"selinux",
"sockstat",
"softnet",
"stat",
"tapestats",
"textfile", // the upstream base collector only, not PMM's textfile.hr/textfile.mr/textfile.lr
"thermal_zone",
"time",
"timex",
"udp_queues",
"uname",
"vmstat",
"watchdog",
"xfs",
"zfs",
}

func nodeExporterConfig(node *models.Node, exporter *models.Agent, agentVersion *version.Parsed) (*agentv1.SetStateRequest_AgentProcess, error) {
listenAddress := getExporterListenAddress(node, exporter)
tdp := models.TemplateDelimsPair(exporter.ExporterOptions.MetricsPath)
Expand Down Expand Up @@ -124,6 +186,21 @@ func nodeExporterConfig(node *models.Node, exporter *models.Agent, agentVersion

args = collectors.FilterOutCollectors("--collector.", args, exporter.ExporterOptions.DisabledCollectors)

// Collectors are not tweaked on macOS, where node_exporter enables a different set by default.
// Older pmm-agents ship node_exporter builds that do not know all of the flags below and would exit.
if node.Distro != "darwin" && agentVersion.IsFeatureSupported(version.NodeExporterV1_8) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Heads up on a second-order effect of this, not a blocker.

ChangeNodeExporter (managed/services/inventory/agents.go:238) never calls vmdb.RequestConfigurationUpdate(). The only thing that regenerates the VM scrape config is the StateChanged callback at managed/services/agents/handler.go:272, and that arrives as the exporter is restarting. So on pmm-admin inventory change agent node-exporter --disable-collectors=diskstats:

  • t+0: agent restarts node_exporter, toStarting emits STARTING, server queues a VM config update
  • t+~10ms: new process is bound and serving, now answering ?collect[]=...diskstats... with 400 disabled collector: diskstats
  • t+~3s: updateBatchDelay expires, VM config is rewritten without collect[]=diskstats

Since the 400 fails the whole resolution endpoint, the node loses every HR metric for that window, not just diskstats. Default HR is 5s (managed/models/settings.go:206), so the real cost is zero or one missed scrape on one node.

This is new behaviour: before this PR, disabling a collector could never produce a 400, because the exporter kept it enabled. I checked whether it can wedge and it can't, so I don't think it should hold the merge: toStarting emits STARTING before exec, stateChanged requests a config update on every status change and not just port changes, and SendActualStatuses (agent/client/client.go:274) re-fires on every reconnect, so a dropped connection at the wrong moment still recovers.

If you want it deterministic rather than merely self-healing, ForceConfigurationUpdate(ctx) before RequestStateUpdate in ChangeNodeExporter closes it, which is the same pattern already used for port changes in handler.go:262-268 (PMM-14267).

disableArgs := collectors.DisableDefaultEnabledCollectors(
"--no-collector.",
defaultEnabledNodeExporterCollectors,
exporter.ExporterOptions.DisabledCollectors,
)
for _, arg := range disableArgs {
if !slices.Contains(args, arg) { // some collectors are already disabled above
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
args = append(args, arg)
}
}
}

if exporter.ExporterOptions.MetricsPath != "" {
args = append(args, "--web.telemetry-path="+exporter.ExporterOptions.MetricsPath)
}
Expand Down
215 changes: 215 additions & 0 deletions managed/services/agents/node_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ func TestNodeExporterConfig(t *testing.T) {
require.Equal(t, expected, actual)
})

// pmm-agent 2.x ships a node_exporter that does not know all "--no-collector." flags,
// so disabled collectors only get their enable flag dropped there
t.Run("LinuxDisabledCollectors", func(t *testing.T) {
t.Parallel()
node := &models.Node{}
Expand Down Expand Up @@ -291,6 +293,219 @@ func TestNodeExporterConfig(t *testing.T) {
require.Equal(t, expected, actual)
})

// Disabling a collector that node_exporter enables on its own takes "--no-collector.<name>",
// dropping "--collector.<name>" is not enough. pmm-agent 3.x is the oldest one shipping a
// node_exporter that knows all of those flags.
t.Run("LinuxDisabledDefaultEnabledCollectors", func(t *testing.T) {
t.Parallel()
node := &models.Node{}
exporter := &models.Agent{
AgentID: "agent-id",
AgentType: models.NodeExporterType,
ExporterOptions: models.ExporterOptions{
// arp is disabled by us already, dmi is not passed by us at all,
// netstat.fields is a flag of the netstat collector, not a collector
DisabledCollectors: []string{"cpu", "netstat", "netstat.fields", "vmstat", "meminfo", "arp", "dmi"},
},
}
agentVersion := version.MustParse("3.0.0")

actual, err := nodeExporterConfig(node, exporter, agentVersion)
require.NoError(t, err, "Unable to build node exporter config")

expected := []string{
"--collector.bonding",
"--collector.buddyinfo",
"--collector.diskstats",
"--collector.entropy",
"--collector.filefd",
"--collector.filesystem",
"--collector.hwmon",
"--collector.loadavg",
"--collector.meminfo_numa",
"--collector.netdev",
"--collector.processes",
"--collector.standard.go",
"--collector.standard.process",
"--collector.stat",
"--collector.textfile.directory.hr=" + pathsBase(agentVersion, "{{", "}}") + "/collectors/textfile-collector/high-resolution",
"--collector.textfile.directory.lr=" + pathsBase(agentVersion, "{{", "}}") + "/collectors/textfile-collector/low-resolution",
"--collector.textfile.directory.mr=" + pathsBase(agentVersion, "{{", "}}") + "/collectors/textfile-collector/medium-resolution",
"--collector.textfile.hr",
"--collector.textfile.lr",
"--collector.textfile.mr",
"--collector.time",
"--collector.uname",
"--collector.vmstat.fields=^(pg(steal_(kswapd|direct)|refill|alloc)_(movable|normal|dma3?2?)" +
"|nr_(dirty.*|slab.*|vmscan.*|isolated.*|free.*|shmem.*|i?n?active.*|anon_transparent_.*|writeback.*|unstable" +
"|unevictable|mlock|mapped|bounce|page_table_pages|kernel_stack)|drop_slab|slabs_scanned|pgd?e?activate" +
"|pgpg(in|out)|pswp(in|out)|pgm?a?j?fault)$",
"--no-collector.arp",
"--no-collector.bcache",
"--no-collector.conntrack",
"--no-collector.cpu",
"--no-collector.dmi",
"--no-collector.drbd",
"--no-collector.edac",
"--no-collector.infiniband",
"--no-collector.interrupts",
"--no-collector.ipvs",
"--no-collector.ksmd",
"--no-collector.logind",
"--no-collector.mdadm",
"--no-collector.meminfo",
"--no-collector.mountstats",
"--no-collector.netclass",
"--no-collector.netstat",
"--no-collector.nfs",
"--no-collector.nfsd",
"--no-collector.ntp",
"--no-collector.qdisc",
"--no-collector.runit",
"--no-collector.sockstat",
"--no-collector.supervisord",
"--no-collector.systemd",
"--no-collector.tcpstat",
"--no-collector.timex",
"--no-collector.vmstat",
"--no-collector.wifi",
"--no-collector.xfs",
"--no-collector.zfs",
"--web.disable-exporter-metrics",
"--web.listen-address=0.0.0.0:{{ .listen_port }}",
"--web.config.file={{ .TextFiles.webConfig }}",
}

requireNoDuplicateFlags(t, actual.Args)
require.Equal(t, expected, actual.Args)
})

// "textfile" is the upstream base collector, not an umbrella over PMM's textfile.hr/mr/lr ones.
// Those three are default-off, so dropping their "--collector." flag is what disables them, and they
// have to be named individually - both here and in the "collect[]" filter built by
// scrapeConfigsForNodeExporter, which must never name a collector we disabled.
t.Run("LinuxDisabledTextfileCollectors", func(t *testing.T) {
t.Parallel()
node := &models.Node{}
exporter := &models.Agent{
AgentID: "agent-id",
AgentType: models.NodeExporterType,
ExporterOptions: models.ExporterOptions{
DisabledCollectors: []string{"textfile", "textfile.hr"},
},
}
agentVersion := version.MustParse("3.0.0")

actual, err := nodeExporterConfig(node, exporter, agentVersion)
require.NoError(t, err, "Unable to build node exporter config")

expected := []string{
"--collector.bonding",
"--collector.buddyinfo",
"--collector.cpu",
"--collector.diskstats",
"--collector.entropy",
"--collector.filefd",
"--collector.filesystem",
"--collector.hwmon",
"--collector.loadavg",
"--collector.meminfo",
"--collector.meminfo_numa",
"--collector.netdev",
"--collector.netstat",
"--collector.netstat.fields=^(.*_(InErrors|InErrs|InCsumErrors)" +
"|Tcp_(ActiveOpens|PassiveOpens|RetransSegs|CurrEstab|AttemptFails|OutSegs|InSegs|EstabResets|OutRsts|OutSegs)|Tcp_Rto(Algorithm|Min|Max)" +
"|Udp_(RcvbufErrors|SndbufErrors)|Udp(6?|Lite6?)_(InDatagrams|OutDatagrams|RcvbufErrors|SndbufErrors|NoPorts)" +
"|Icmp6?_(OutEchoReps|OutEchos|InEchos|InEchoReps|InAddrMaskReps|InAddrMasks|OutAddrMaskReps|OutAddrMasks|InTimestampReps|InTimestamps" +
"|OutTimestampReps|OutTimestamps|OutErrors|InDestUnreachs|OutDestUnreachs|InTimeExcds|InRedirects|OutRedirects|InMsgs|OutMsgs)" +
"|IcmpMsg_(InType3|OutType3)|Ip(6|Ext)_(InOctets|OutOctets)|Ip_Forwarding|TcpExt_(Listen.*|Syncookies.*|TCPTimeouts))$",
"--collector.processes",
"--collector.standard.go",
"--collector.standard.process",
"--collector.stat",
// the directory flags are inert once their collector is off, so they are left alone
"--collector.textfile.directory.hr=" + pathsBase(agentVersion, "{{", "}}") + "/collectors/textfile-collector/high-resolution",
"--collector.textfile.directory.lr=" + pathsBase(agentVersion, "{{", "}}") + "/collectors/textfile-collector/low-resolution",
"--collector.textfile.directory.mr=" + pathsBase(agentVersion, "{{", "}}") + "/collectors/textfile-collector/medium-resolution",
// "--collector.textfile.hr" is gone, while mr and lr keep collecting
"--collector.textfile.lr",
"--collector.textfile.mr",
"--collector.time",
"--collector.uname",
"--collector.vmstat",
"--collector.vmstat.fields=^(pg(steal_(kswapd|direct)|refill|alloc)_(movable|normal|dma3?2?)" +
"|nr_(dirty.*|slab.*|vmscan.*|isolated.*|free.*|shmem.*|i?n?active.*|anon_transparent_.*|writeback.*|unstable" +
"|unevictable|mlock|mapped|bounce|page_table_pages|kernel_stack)|drop_slab|slabs_scanned|pgd?e?activate" +
"|pgpg(in|out)|pswp(in|out)|pgm?a?j?fault)$",
"--no-collector.arp",
"--no-collector.bcache",
"--no-collector.conntrack",
"--no-collector.drbd",
"--no-collector.edac",
"--no-collector.infiniband",
"--no-collector.interrupts",
"--no-collector.ipvs",
"--no-collector.ksmd",
"--no-collector.logind",
"--no-collector.mdadm",
"--no-collector.mountstats",
"--no-collector.netclass",
"--no-collector.nfs",
"--no-collector.nfsd",
"--no-collector.ntp",
"--no-collector.qdisc",
"--no-collector.runit",
"--no-collector.sockstat",
"--no-collector.supervisord",
"--no-collector.systemd",
"--no-collector.tcpstat",
// the base collector is default-on, so it takes an explicit "--no-" flag ...
"--no-collector.textfile",
"--no-collector.timex",
"--no-collector.wifi",
"--no-collector.xfs",
"--no-collector.zfs",
"--web.disable-exporter-metrics",
"--web.listen-address=0.0.0.0:{{ .listen_port }}",
"--web.config.file={{ .TextFiles.webConfig }}",
}

requireNoDuplicateFlags(t, actual.Args)
require.Equal(t, expected, actual.Args)
// ... while textfile.hr is default-off, so it must not get one
require.NotContains(t, actual.Args, "--no-collector.textfile.hr")
})

t.Run("MacOSDisabledCollectors", func(t *testing.T) {
t.Parallel()
node := &models.Node{
Distro: "darwin",
}
exporter := &models.Agent{
AgentID: "agent-id",
AgentType: models.NodeExporterType,
ExporterOptions: models.ExporterOptions{
DisabledCollectors: []string{"cpu", "diskstats"},
},
}
agentVersion := version.MustParse("3.0.0")

actual, err := nodeExporterConfig(node, exporter, agentVersion)
require.NoError(t, err, "Unable to build node exporter config")

// collectors are not tweaked on macOS, where node_exporter enables a different set by default
expected := []string{
"--collector.textfile.directory.hr=" + pathsBase(agentVersion, "{{", "}}") + "/collectors/textfile-collector/high-resolution",
"--collector.textfile.directory.lr=" + pathsBase(agentVersion, "{{", "}}") + "/collectors/textfile-collector/low-resolution",
"--collector.textfile.directory.mr=" + pathsBase(agentVersion, "{{", "}}") + "/collectors/textfile-collector/medium-resolution",
"--web.disable-exporter-metrics",
"--web.listen-address=0.0.0.0:{{ .listen_port }}",
"--web.config.file={{ .TextFiles.webConfig }}",
}

require.Equal(t, expected, actual.Args)
})

t.Run("MacOS", func(t *testing.T) {
t.Parallel()
node := &models.Node{
Expand Down
4 changes: 4 additions & 0 deletions version/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ var (
MysqlExporterPluginCollector FeatureVersion = MustParse("2.36.0-0")
NomadAgentSupportVersion FeatureVersion = MustParse("3.2.0-0")
MongoDBRtaAgentSupportVersion FeatureVersion = MustParse("3.7.0-0")
// NodeExporterV1_8 is the first pmm-agent shipping node_exporter 1.8, the oldest build that knows
// every collector we may have to disable explicitly. In pmm-agent 2.x, which ships 1.4.0, flags
// such as "--no-collector.watchdog" do not exist and would make the exporter exit.
NodeExporterV1_8 FeatureVersion = MustParse("3.0.0-0")
)

// IsFeatureSupported checks if the feature is supported by the version.
Expand Down
Loading