diff --git a/admin/commands/inventory/change_agent_valkey_exporter.go b/admin/commands/inventory/change_agent_valkey_exporter.go index 385bbcd979..57b6bd802d 100644 --- a/admin/commands/inventory/change_agent_valkey_exporter.go +++ b/admin/commands/inventory/change_agent_valkey_exporter.go @@ -62,7 +62,7 @@ func (res *changeAgentValkeyExporterResult) String() string { // ChangeAgentValkeyExporterCommand is used by Kong for CLI flags and commands. type ChangeAgentValkeyExporterCommand struct { // Embedded flags - flags.LogLevelFatalChangeFlags + 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 e4174c59a7..6c76960d67 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"}, @@ -268,18 +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. + 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 6a2e25f3a6..da8ce14804 100644 --- a/managed/services/agents/log_level.go +++ b/managed/services/agents/log_level.go @@ -24,20 +24,24 @@ 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. +// 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 { - level := pointer.GetString(logLevel) + return withLogLevelFlag(args, "--log.level", 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 "=" 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 + } - args = append(args, "--log.level="+level) + // Keep a previously stored 'fatal' working on exporters which dropped that level. + if !supportLogLevelFatal && level == "fatal" { + level = "error" } - return args + return append(args, flagName+"="+level) } diff --git a/managed/services/agents/log_level_test.go b/managed/services/agents/log_level_test.go new file mode 100644 index 0000000000..d99a420b64 --- /dev/null +++ b/managed/services/agents/log_level_test.go @@ -0,0 +1,82 @@ +// 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" +) + +// TestWithLogLevel covers the pmm-agent version gate, the fatal downgrade and that args +// passed in are preserved. +func TestWithLogLevel(t *testing.T) { + t.Parallel() + + 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 appended to existing args": { + args: []string{"--web.listen-address=:42000"}, + level: new("debug"), + pmmAgentVersion: supported, + expected: []string{"--web.listen-address=:42000", "--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(tc.args, tc.level, tc.pmmAgentVersion, tc.supportLogLevelFatal) + assert.Equal(t, tc.expected, actual) + }) + } +} diff --git a/managed/services/agents/valkey.go b/managed/services/agents/valkey.go index 263e97d92d..ea21abd864 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 = withLogLevel(args, exporter.LogLevel, pmmAgentVersion, false) + // 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 73229e581c..77c465a811 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" @@ -76,4 +77,33 @@ 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 + }{ + "info": {"info", "--log-level=info"}, + // 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, strings.Join(actual.Args, " "), "--log.level") + }) + } + }) }