From e102a01487efd6fdacea7447aa3565d1843093d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20=C4=8Ctvrtka?= Date: Wed, 29 Jul 2026 11:24:38 +0200 Subject: [PATCH 1/4] PMM-15201 Fix done status issue. --- .../inventory/change_agent_valkey_exporter.go | 4 +- .../change_agent_valkey_exporter_test.go | 2 +- managed/services/agents/log_level.go | 37 ++++-- managed/services/agents/log_level_test.go | 119 ++++++++++++++++++ managed/services/agents/valkey.go | 2 +- managed/services/agents/valkey_test.go | 47 +++++++ 6 files changed, 197 insertions(+), 14 deletions(-) create mode 100644 managed/services/agents/log_level_test.go diff --git a/admin/commands/inventory/change_agent_valkey_exporter.go b/admin/commands/inventory/change_agent_valkey_exporter.go index 385bbcd9798..f73f1ff5902 100644 --- a/admin/commands/inventory/change_agent_valkey_exporter.go +++ b/admin/commands/inventory/change_agent_valkey_exporter.go @@ -62,7 +62,9 @@ func (res *changeAgentValkeyExporterResult) String() string { // ChangeAgentValkeyExporterCommand is used by Kong for CLI flags and commands. type ChangeAgentValkeyExporterCommand struct { // Embedded flags - flags.LogLevelFatalChangeFlags + // valkey_exporter has no fatal level - it silently falls back to info - so the flag + // offers the same levels as `pmm-admin inventory add agent valkey-exporter`. + flags.LogLevelNoFatalChangeFlags AgentID string `arg:"" help:"Valkey Exporter Agent ID"` diff --git a/admin/commands/inventory/change_agent_valkey_exporter_test.go b/admin/commands/inventory/change_agent_valkey_exporter_test.go index e4174c59a7d..2628aaf65a4 100644 --- a/admin/commands/inventory/change_agent_valkey_exporter_test.go +++ b/admin/commands/inventory/change_agent_valkey_exporter_test.go @@ -43,7 +43,7 @@ func TestValkeyExporterChangeAgent(t *testing.T) { Password: new("redis_pass"), TLS: new(true), PushMetrics: new(false), - LogLevelFatalChangeFlags: flags.LogLevelFatalChangeFlags{ + LogLevelNoFatalChangeFlags: flags.LogLevelNoFatalChangeFlags{ LogLevel: new(flags.LogLevel("debug")), }, CustomLabels: &map[string]string{"environment": "test"}, diff --git a/managed/services/agents/log_level.go b/managed/services/agents/log_level.go index 6a2e25f3a66..bcfde952f39 100644 --- a/managed/services/agents/log_level.go +++ b/managed/services/agents/log_level.go @@ -24,20 +24,35 @@ import ( // Log level available in exporters with pmm 2.28. var exporterLogLevelCommandVersion = version.MustParse("2.27.99") -// withLogLevel - append CLI args --log.level -// mysqld_exporter, node_exporter, postgres_exporter and valkey_exporter don't support --log.level=fatal. +const ( + // Flag used by exporters which parse their command line with kingpin. + logLevelFlag = "--log.level" + + // Flag used by valkey_exporter, which parses its command line with the standard library + // flag package. That package rejects --log.level and exits with code 2, which used to + // kill the exporter right after start and leave the agent DONE (PMM-15201). + valkeyLogLevelFlag = "--log-level" +) + +// withLogLevel appends the --log.level CLI arg. The mysqld_exporter, node_exporter and +// postgres_exporter binaries don't support --log.level=fatal. func withLogLevel(args []string, logLevel *string, pmmAgentVersion *version.Parsed, supportLogLevelFatal bool) []string { - level := pointer.GetString(logLevel) + return withLogLevelFlag(args, logLevelFlag, logLevel, pmmAgentVersion, supportLogLevelFatal) +} - if level != "" && !pmmAgentVersion.Less(exporterLogLevelCommandVersion) { - // exists exporters that not support --log.level=fatal anymore after last update - // so replace "fatal" to "error" for previous stored state - if !supportLogLevelFatal && level == "fatal" { - level = "error" - } +// withLogLevelFlag appends "=" for exporters which spell the log level flag +// differently than the kingpin-based majority. +func withLogLevelFlag(args []string, flag string, logLevel *string, pmmAgentVersion *version.Parsed, supportLogLevelFatal bool) []string { + level := pointer.GetString(logLevel) + if level == "" || pmmAgentVersion.Less(exporterLogLevelCommandVersion) { + return args + } - args = append(args, "--log.level="+level) + // Some exporters dropped support for the fatal level, so fall back to error to keep a + // previously stored "fatal" working. + if !supportLogLevelFatal && level == "fatal" { + level = "error" } - return args + return append(args, flag+"="+level) } diff --git a/managed/services/agents/log_level_test.go b/managed/services/agents/log_level_test.go new file mode 100644 index 00000000000..86ae210a38a --- /dev/null +++ b/managed/services/agents/log_level_test.go @@ -0,0 +1,119 @@ +// Copyright (C) 2023 Percona LLC +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package agents + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/percona/pmm/version" +) + +func TestWithLogLevel(t *testing.T) { + t.Parallel() + + supported := version.MustParse("2.28.0") + + for name, tc := range map[string]struct { + level *string + pmmAgentVersion *version.Parsed + supportLogLevelFatal bool + expected []string + }{ + "debug": { + level: new("debug"), + pmmAgentVersion: supported, + expected: []string{"--log.level=debug"}, + }, + "fatal supported": { + level: new("fatal"), + pmmAgentVersion: supported, + supportLogLevelFatal: true, + expected: []string{"--log.level=fatal"}, + }, + "fatal falls back to error": { + // Exporters which dropped the fatal level would refuse to start otherwise. + level: new("fatal"), + pmmAgentVersion: supported, + expected: []string{"--log.level=error"}, + }, + "no level": { + level: nil, + pmmAgentVersion: supported, + expected: nil, + }, + "empty level": { + level: new(""), + pmmAgentVersion: supported, + expected: nil, + }, + "pmm-agent too old": { + // The flag only exists from PMM 2.28 onwards. + level: new("debug"), + pmmAgentVersion: version.MustParse("2.27.0"), + expected: nil, + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + actual := withLogLevel(nil, tc.level, tc.pmmAgentVersion, tc.supportLogLevelFatal) + assert.Equal(t, tc.expected, actual) + }) + } +} + +// TestWithLogLevelFlagValkey covers PMM-15201: valkey_exporter parses its command line with the +// standard library flag package, which rejects --log.level and exits with code 2, leaving the +// agent in the DONE state. +func TestWithLogLevelFlagValkey(t *testing.T) { + t.Parallel() + + supported := version.MustParse("2.28.0") + + for name, tc := range map[string]struct { + level *string + expected []string + }{ + "debug": {new("debug"), []string{"--log-level=debug"}}, + "info": {new("info"), []string{"--log-level=info"}}, + "warn": {new("warn"), []string{"--log-level=warn"}}, + "error": {new("error"), []string{"--log-level=error"}}, + // valkey_exporter silently falls back to info on an unknown level, so a stored + // "fatal" must be translated rather than passed through. + "fatal falls back to error": {new("fatal"), []string{"--log-level=error"}}, + "no level": {nil, nil}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + actual := withLogLevelFlag(nil, valkeyLogLevelFlag, tc.level, supported, false) + assert.Equal(t, tc.expected, actual) + for _, arg := range actual { + assert.NotContains(t, arg, "--log.level", "valkey_exporter rejects the dotted flag") + } + }) + } +} + +// TestWithLogLevelAppends makes sure existing args are preserved. +func TestWithLogLevelAppends(t *testing.T) { + t.Parallel() + + args := withLogLevel([]string{"--web.listen-address=:42000"}, new("info"), version.MustParse("2.28.0"), false) + assert.Equal(t, []string{"--web.listen-address=:42000", "--log.level=info"}, args) +} diff --git a/managed/services/agents/valkey.go b/managed/services/agents/valkey.go index 263e97d92d6..b5e0573fc8c 100644 --- a/managed/services/agents/valkey.go +++ b/managed/services/agents/valkey.go @@ -45,7 +45,7 @@ func valkeyExporterConfig(node *models.Node, service *models.Service, exporter * args = append(args, "--redis.addr="+exporter.DSN(service, dsnParams, nil, pmmAgentVersion)) args = append(args, "--connection-timeout="+connectionTimeout.String()) - args = withLogLevel(args, exporter.LogLevel, pmmAgentVersion, false) + args = withLogLevelFlag(args, valkeyLogLevelFlag, exporter.LogLevel, pmmAgentVersion, false) sort.Strings(args) res := &agentv1.SetStateRequest_AgentProcess{ diff --git a/managed/services/agents/valkey_test.go b/managed/services/agents/valkey_test.go index 73229e581ce..0ec8c8534f7 100644 --- a/managed/services/agents/valkey_test.go +++ b/managed/services/agents/valkey_test.go @@ -76,4 +76,51 @@ func TestValkeyExporterConfig(t *testing.T) { require.Contains(t, actual.Args, "--connection-timeout=1.5s") require.Contains(t, actual.Args, "--redis.addr=redis://username:secret@1.2.3.4:6379") }) + + // PMM-15201: valkey_exporter only knows --log-level. Passing --log.level made it print + // its usage, exit with code 2 and land the agent in the DONE state. + t.Run("LogLevel", func(t *testing.T) { + t.Parallel() + + for name, tc := range map[string]struct { + logLevel string + expected string + }{ + "debug": {"debug", "--log-level=debug"}, + "info": {"info", "--log-level=info"}, + "warn": {"warn", "--log-level=warn"}, + "error": {"error", "--log-level=error"}, + // valkey_exporter has no fatal level and silently falls back to info. + "fatal": {"fatal", "--log-level=error"}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + exporter := &models.Agent{ + AgentID: "agent-id", + AgentType: models.ValkeyExporterType, + LogLevel: new(tc.logLevel), + } + + actual := valkeyExporterConfig(node, service, exporter, redactSecrets, pmmAgentVersion) + require.Contains(t, actual.Args, tc.expected) + require.NotContains(t, actual.Args, "--log.level="+tc.logLevel) + }) + } + }) + + t.Run("NoLogLevel", func(t *testing.T) { + t.Parallel() + + exporter := &models.Agent{ + AgentID: "agent-id", + AgentType: models.ValkeyExporterType, + } + + actual := valkeyExporterConfig(node, service, exporter, redactSecrets, pmmAgentVersion) + for _, arg := range actual.Args { + require.NotContains(t, arg, "log-level") + require.NotContains(t, arg, "log.level") + } + }) } From 96c91f5330b34480cd003761a82f7901ded03ea4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20=C4=8Ctvrtka?= Date: Thu, 30 Jul 2026 09:43:39 +0200 Subject: [PATCH 2/4] PMM-15201 Add withValkeyLogLevel helper and cover the flag in tests. --- .../change_agent_valkey_exporter_test.go | 16 ++++++++++++++++ managed/services/agents/log_level.go | 12 +++++++++--- managed/services/agents/log_level_test.go | 14 ++++++-------- managed/services/agents/valkey.go | 2 +- managed/services/agents/valkey_test.go | 4 +++- 5 files changed, 35 insertions(+), 13 deletions(-) diff --git a/admin/commands/inventory/change_agent_valkey_exporter_test.go b/admin/commands/inventory/change_agent_valkey_exporter_test.go index 2628aaf65a4..7eee3516544 100644 --- a/admin/commands/inventory/change_agent_valkey_exporter_test.go +++ b/admin/commands/inventory/change_agent_valkey_exporter_test.go @@ -281,5 +281,21 @@ Configuration changes applied: require.Error(t, err) assert.Contains(t, strings.ToLower(err.Error()), "log-level") }) + + // valkey_exporter has no fatal level, so the flag must reject it the same way + // `pmm-admin inventory add agent valkey-exporter` does. + t.Run("FatalLogLevelRejected", func(t *testing.T) { + t.Parallel() + + cli := []string{"change-agent", "valkey-exporter", "test-agent-id", "--log-level=fatal"} + + var cmd ChangeAgentValkeyExporterCommand + parser, err := kong.New(&cmd) + require.NoError(t, err) + + _, err = parser.Parse(cli[2:]) + require.Error(t, err) + assert.Contains(t, strings.ToLower(err.Error()), "log-level") + }) }) } diff --git a/managed/services/agents/log_level.go b/managed/services/agents/log_level.go index bcfde952f39..0b374c5a33e 100644 --- a/managed/services/agents/log_level.go +++ b/managed/services/agents/log_level.go @@ -40,9 +40,15 @@ func withLogLevel(args []string, logLevel *string, pmmAgentVersion *version.Pars return withLogLevelFlag(args, logLevelFlag, logLevel, pmmAgentVersion, supportLogLevelFatal) } -// withLogLevelFlag appends "=" for exporters which spell the log level flag +// withValkeyLogLevel appends the --log-level CLI arg for valkey_exporter, which spells the flag +// differently than the kingpin-based exporters and has no fatal level. +func withValkeyLogLevel(args []string, logLevel *string, pmmAgentVersion *version.Parsed) []string { + return withLogLevelFlag(args, valkeyLogLevelFlag, logLevel, pmmAgentVersion, false) +} + +// withLogLevelFlag appends "=" for exporters which spell the log level flag // differently than the kingpin-based majority. -func withLogLevelFlag(args []string, flag string, logLevel *string, pmmAgentVersion *version.Parsed, supportLogLevelFatal bool) []string { +func withLogLevelFlag(args []string, flagName string, logLevel *string, pmmAgentVersion *version.Parsed, supportLogLevelFatal bool) []string { level := pointer.GetString(logLevel) if level == "" || pmmAgentVersion.Less(exporterLogLevelCommandVersion) { return args @@ -54,5 +60,5 @@ func withLogLevelFlag(args []string, flag string, logLevel *string, pmmAgentVers level = "error" } - return append(args, flag+"="+level) + return append(args, flagName+"="+level) } diff --git a/managed/services/agents/log_level_test.go b/managed/services/agents/log_level_test.go index 86ae210a38a..222246364d3 100644 --- a/managed/services/agents/log_level_test.go +++ b/managed/services/agents/log_level_test.go @@ -77,10 +77,11 @@ func TestWithLogLevel(t *testing.T) { } } -// TestWithLogLevelFlagValkey covers PMM-15201: valkey_exporter parses its command line with the +// TestWithValkeyLogLevel covers PMM-15201: valkey_exporter parses its command line with the // standard library flag package, which rejects --log.level and exits with code 2, leaving the -// agent in the DONE state. -func TestWithLogLevelFlagValkey(t *testing.T) { +// agent in the DONE state. The per-level matrix lives in TestValkeyExporterConfig, so only the +// flag spelling and the fatal fallback are checked here. +func TestWithValkeyLogLevel(t *testing.T) { t.Parallel() supported := version.MustParse("2.28.0") @@ -89,10 +90,7 @@ func TestWithLogLevelFlagValkey(t *testing.T) { level *string expected []string }{ - "debug": {new("debug"), []string{"--log-level=debug"}}, - "info": {new("info"), []string{"--log-level=info"}}, - "warn": {new("warn"), []string{"--log-level=warn"}}, - "error": {new("error"), []string{"--log-level=error"}}, + "dashed flag": {new("info"), []string{"--log-level=info"}}, // valkey_exporter silently falls back to info on an unknown level, so a stored // "fatal" must be translated rather than passed through. "fatal falls back to error": {new("fatal"), []string{"--log-level=error"}}, @@ -101,7 +99,7 @@ func TestWithLogLevelFlagValkey(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - actual := withLogLevelFlag(nil, valkeyLogLevelFlag, tc.level, supported, false) + actual := withValkeyLogLevel(nil, tc.level, supported) assert.Equal(t, tc.expected, actual) for _, arg := range actual { assert.NotContains(t, arg, "--log.level", "valkey_exporter rejects the dotted flag") diff --git a/managed/services/agents/valkey.go b/managed/services/agents/valkey.go index b5e0573fc8c..4712dc2b01f 100644 --- a/managed/services/agents/valkey.go +++ b/managed/services/agents/valkey.go @@ -45,7 +45,7 @@ func valkeyExporterConfig(node *models.Node, service *models.Service, exporter * args = append(args, "--redis.addr="+exporter.DSN(service, dsnParams, nil, pmmAgentVersion)) args = append(args, "--connection-timeout="+connectionTimeout.String()) - args = withLogLevelFlag(args, valkeyLogLevelFlag, exporter.LogLevel, pmmAgentVersion, false) + args = withValkeyLogLevel(args, exporter.LogLevel, pmmAgentVersion) sort.Strings(args) res := &agentv1.SetStateRequest_AgentProcess{ diff --git a/managed/services/agents/valkey_test.go b/managed/services/agents/valkey_test.go index 0ec8c8534f7..c5f6b796ad8 100644 --- a/managed/services/agents/valkey_test.go +++ b/managed/services/agents/valkey_test.go @@ -104,7 +104,9 @@ func TestValkeyExporterConfig(t *testing.T) { actual := valkeyExporterConfig(node, service, exporter, redactSecrets, pmmAgentVersion) require.Contains(t, actual.Args, tc.expected) - require.NotContains(t, actual.Args, "--log.level="+tc.logLevel) + for _, arg := range actual.Args { + require.NotContains(t, arg, "--log.level", "valkey_exporter rejects the dotted flag") + } }) } }) From 8dc3d6c62a7b5c07605e9d8a4128252bb9c7afbd Mon Sep 17 00:00:00 2001 From: Alex Demidoff Date: Fri, 31 Jul 2026 16:02:06 +0300 Subject: [PATCH 3/4] PMM-15201 Simplify the log level helper and deduplicate its tests. Keep log_level.go exporter-agnostic: the valkey flag spelling now lives at its only call site in valkey.go. Drop the test cases that duplicated the coverage already provided by TestValkeyExporterConfig and DefaultTimeoutUsesFlag. --- .../inventory/change_agent_valkey_exporter.go | 2 - .../change_agent_valkey_exporter_test.go | 45 +++++++----------- managed/services/agents/log_level.go | 29 +++--------- managed/services/agents/log_level_test.go | 47 ++----------------- managed/services/agents/valkey.go | 4 +- managed/services/agents/valkey_test.go | 25 ++-------- 6 files changed, 35 insertions(+), 117 deletions(-) diff --git a/admin/commands/inventory/change_agent_valkey_exporter.go b/admin/commands/inventory/change_agent_valkey_exporter.go index f73f1ff5902..57b6bd802dd 100644 --- a/admin/commands/inventory/change_agent_valkey_exporter.go +++ b/admin/commands/inventory/change_agent_valkey_exporter.go @@ -62,8 +62,6 @@ func (res *changeAgentValkeyExporterResult) String() string { // ChangeAgentValkeyExporterCommand is used by Kong for CLI flags and commands. type ChangeAgentValkeyExporterCommand struct { // Embedded flags - // valkey_exporter has no fatal level - it silently falls back to info - so the flag - // offers the same levels as `pmm-admin inventory add agent valkey-exporter`. flags.LogLevelNoFatalChangeFlags AgentID string `arg:"" help:"Valkey Exporter Agent ID"` diff --git a/admin/commands/inventory/change_agent_valkey_exporter_test.go b/admin/commands/inventory/change_agent_valkey_exporter_test.go index 7eee3516544..6c76960d679 100644 --- a/admin/commands/inventory/change_agent_valkey_exporter_test.go +++ b/admin/commands/inventory/change_agent_valkey_exporter_test.go @@ -268,34 +268,25 @@ Configuration changes applied: assert.Contains(t, strings.ToLower(err.Error()), "agent-id") }) - t.Run("InvalidLogLevel", func(t *testing.T) { - t.Parallel() - - cli := []string{"change-agent", "valkey-exporter", "test-agent-id", "--log-level=invalid"} - - var cmd ChangeAgentValkeyExporterCommand - parser, err := kong.New(&cmd) - require.NoError(t, err) - - _, err = parser.Parse(cli[2:]) - require.Error(t, err) - assert.Contains(t, strings.ToLower(err.Error()), "log-level") - }) - // valkey_exporter has no fatal level, so the flag must reject it the same way // `pmm-admin inventory add agent valkey-exporter` does. - t.Run("FatalLogLevelRejected", func(t *testing.T) { - t.Parallel() - - cli := []string{"change-agent", "valkey-exporter", "test-agent-id", "--log-level=fatal"} - - var cmd ChangeAgentValkeyExporterCommand - parser, err := kong.New(&cmd) - require.NoError(t, err) - - _, err = parser.Parse(cli[2:]) - require.Error(t, err) - assert.Contains(t, strings.ToLower(err.Error()), "log-level") - }) + for name, level := range map[string]string{ + "InvalidLogLevel": "invalid", + "FatalLogLevelRejected": "fatal", + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + cli := []string{"change-agent", "valkey-exporter", "test-agent-id", "--log-level=" + level} + + var cmd ChangeAgentValkeyExporterCommand + parser, err := kong.New(&cmd) + require.NoError(t, err) + + _, err = parser.Parse(cli[2:]) + require.Error(t, err) + assert.Contains(t, strings.ToLower(err.Error()), "log-level") + }) + } }) } diff --git a/managed/services/agents/log_level.go b/managed/services/agents/log_level.go index 0b374c5a33e..da8ce148049 100644 --- a/managed/services/agents/log_level.go +++ b/managed/services/agents/log_level.go @@ -24,38 +24,21 @@ import ( // Log level available in exporters with pmm 2.28. var exporterLogLevelCommandVersion = version.MustParse("2.27.99") -const ( - // Flag used by exporters which parse their command line with kingpin. - logLevelFlag = "--log.level" - - // Flag used by valkey_exporter, which parses its command line with the standard library - // flag package. That package rejects --log.level and exits with code 2, which used to - // kill the exporter right after start and leave the agent DONE (PMM-15201). - valkeyLogLevelFlag = "--log-level" -) - -// withLogLevel appends the --log.level CLI arg. The mysqld_exporter, node_exporter and -// postgres_exporter binaries don't support --log.level=fatal. +// withLogLevel appends the --log.level CLI arg used by the kingpin-based exporters, of which +// mysqld_exporter, node_exporter and postgres_exporter don't support --log.level=fatal. func withLogLevel(args []string, logLevel *string, pmmAgentVersion *version.Parsed, supportLogLevelFatal bool) []string { - return withLogLevelFlag(args, logLevelFlag, logLevel, pmmAgentVersion, supportLogLevelFatal) -} - -// withValkeyLogLevel appends the --log-level CLI arg for valkey_exporter, which spells the flag -// differently than the kingpin-based exporters and has no fatal level. -func withValkeyLogLevel(args []string, logLevel *string, pmmAgentVersion *version.Parsed) []string { - return withLogLevelFlag(args, valkeyLogLevelFlag, logLevel, pmmAgentVersion, false) + return withLogLevelFlag(args, "--log.level", logLevel, pmmAgentVersion, supportLogLevelFatal) } -// withLogLevelFlag appends "=" for exporters which spell the log level flag -// differently than the kingpin-based majority. +// withLogLevelFlag appends "=" if pmm-agent is new enough, downgrading fatal +// to error for exporters which don't support it. func withLogLevelFlag(args []string, flagName string, logLevel *string, pmmAgentVersion *version.Parsed, supportLogLevelFatal bool) []string { level := pointer.GetString(logLevel) if level == "" || pmmAgentVersion.Less(exporterLogLevelCommandVersion) { return args } - // Some exporters dropped support for the fatal level, so fall back to error to keep a - // previously stored "fatal" working. + // Keep a previously stored 'fatal' working on exporters which dropped that level. if !supportLogLevelFatal && level == "fatal" { level = "error" } diff --git a/managed/services/agents/log_level_test.go b/managed/services/agents/log_level_test.go index 222246364d3..b51d5dfd737 100644 --- a/managed/services/agents/log_level_test.go +++ b/managed/services/agents/log_level_test.go @@ -29,15 +29,17 @@ func TestWithLogLevel(t *testing.T) { supported := version.MustParse("2.28.0") for name, tc := range map[string]struct { + args []string level *string pmmAgentVersion *version.Parsed supportLogLevelFatal bool expected []string }{ - "debug": { + "debug appended to existing args": { + args: []string{"--web.listen-address=:42000"}, level: new("debug"), pmmAgentVersion: supported, - expected: []string{"--log.level=debug"}, + expected: []string{"--web.listen-address=:42000", "--log.level=debug"}, }, "fatal supported": { level: new("fatal"), @@ -71,47 +73,8 @@ func TestWithLogLevel(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - actual := withLogLevel(nil, tc.level, tc.pmmAgentVersion, tc.supportLogLevelFatal) + actual := withLogLevel(tc.args, tc.level, tc.pmmAgentVersion, tc.supportLogLevelFatal) assert.Equal(t, tc.expected, actual) }) } } - -// TestWithValkeyLogLevel covers PMM-15201: valkey_exporter parses its command line with the -// standard library flag package, which rejects --log.level and exits with code 2, leaving the -// agent in the DONE state. The per-level matrix lives in TestValkeyExporterConfig, so only the -// flag spelling and the fatal fallback are checked here. -func TestWithValkeyLogLevel(t *testing.T) { - t.Parallel() - - supported := version.MustParse("2.28.0") - - for name, tc := range map[string]struct { - level *string - expected []string - }{ - "dashed flag": {new("info"), []string{"--log-level=info"}}, - // valkey_exporter silently falls back to info on an unknown level, so a stored - // "fatal" must be translated rather than passed through. - "fatal falls back to error": {new("fatal"), []string{"--log-level=error"}}, - "no level": {nil, nil}, - } { - t.Run(name, func(t *testing.T) { - t.Parallel() - - actual := withValkeyLogLevel(nil, tc.level, supported) - assert.Equal(t, tc.expected, actual) - for _, arg := range actual { - assert.NotContains(t, arg, "--log.level", "valkey_exporter rejects the dotted flag") - } - }) - } -} - -// TestWithLogLevelAppends makes sure existing args are preserved. -func TestWithLogLevelAppends(t *testing.T) { - t.Parallel() - - args := withLogLevel([]string{"--web.listen-address=:42000"}, new("info"), version.MustParse("2.28.0"), false) - assert.Equal(t, []string{"--web.listen-address=:42000", "--log.level=info"}, args) -} diff --git a/managed/services/agents/valkey.go b/managed/services/agents/valkey.go index 4712dc2b01f..ea21abd8643 100644 --- a/managed/services/agents/valkey.go +++ b/managed/services/agents/valkey.go @@ -45,7 +45,9 @@ func valkeyExporterConfig(node *models.Node, service *models.Service, exporter * args = append(args, "--redis.addr="+exporter.DSN(service, dsnParams, nil, pmmAgentVersion)) args = append(args, "--connection-timeout="+connectionTimeout.String()) - args = withValkeyLogLevel(args, exporter.LogLevel, pmmAgentVersion) + // valkey_exporter parses flags with the stdlib flag package, which rejects --log.level + // and has no fatal level (PMM-15201). + args = withLogLevelFlag(args, "--log-level", exporter.LogLevel, pmmAgentVersion, false) sort.Strings(args) res := &agentv1.SetStateRequest_AgentProcess{ diff --git a/managed/services/agents/valkey_test.go b/managed/services/agents/valkey_test.go index c5f6b796ad8..77c465a811c 100644 --- a/managed/services/agents/valkey_test.go +++ b/managed/services/agents/valkey_test.go @@ -16,6 +16,7 @@ package agents import ( + "strings" "testing" "time" @@ -86,10 +87,7 @@ func TestValkeyExporterConfig(t *testing.T) { logLevel string expected string }{ - "debug": {"debug", "--log-level=debug"}, - "info": {"info", "--log-level=info"}, - "warn": {"warn", "--log-level=warn"}, - "error": {"error", "--log-level=error"}, + "info": {"info", "--log-level=info"}, // valkey_exporter has no fatal level and silently falls back to info. "fatal": {"fatal", "--log-level=error"}, } { @@ -104,25 +102,8 @@ func TestValkeyExporterConfig(t *testing.T) { actual := valkeyExporterConfig(node, service, exporter, redactSecrets, pmmAgentVersion) require.Contains(t, actual.Args, tc.expected) - for _, arg := range actual.Args { - require.NotContains(t, arg, "--log.level", "valkey_exporter rejects the dotted flag") - } + require.NotContains(t, strings.Join(actual.Args, " "), "--log.level") }) } }) - - t.Run("NoLogLevel", func(t *testing.T) { - t.Parallel() - - exporter := &models.Agent{ - AgentID: "agent-id", - AgentType: models.ValkeyExporterType, - } - - actual := valkeyExporterConfig(node, service, exporter, redactSecrets, pmmAgentVersion) - for _, arg := range actual.Args { - require.NotContains(t, arg, "log-level") - require.NotContains(t, arg, "log.level") - } - }) } From fd15603481ee8f10093e01cf3413eed1fd45ea22 Mon Sep 17 00:00:00 2001 From: Alex Demidoff Date: Fri, 31 Jul 2026 17:59:15 +0300 Subject: [PATCH 4/4] PMM-15201 Document TestWithLogLevel. --- managed/services/agents/log_level_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/managed/services/agents/log_level_test.go b/managed/services/agents/log_level_test.go index b51d5dfd737..d99a420b64e 100644 --- a/managed/services/agents/log_level_test.go +++ b/managed/services/agents/log_level_test.go @@ -23,6 +23,8 @@ import ( "github.com/percona/pmm/version" ) +// TestWithLogLevel covers the pmm-agent version gate, the fatal downgrade and that args +// passed in are preserved. func TestWithLogLevel(t *testing.T) { t.Parallel()