PMM-15201 Correct status after change valkey in PMM admin.

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"`
 

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"},
@@ -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")
+		})
 	})
 }

managed/services/agents/log_level.go

@@ -24,20 +24,41 @@ 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"
-		}
+// 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 "<flagName>=<level>" for exporters which spell the log level flag
+// differently than the kingpin-based majority.
+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)
+	// 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, flagName+"="+level)
 }

managed/services/agents/log_level_test.go

@@ -0,0 +1,117 @@
+// 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 <https://www.gnu.org/licenses/>.
+
+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)
+		})
+	}
+}
+
+// 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)
+}

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 = withValkeyLogLevel(args, exporter.LogLevel, pmmAgentVersion)
 	sort.Strings(args)
 
 	res := &agentv1.SetStateRequest_AgentProcess{

managed/services/agents/valkey_test.go

@@ -76,4 +76,53 @@ 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)
+				for _, arg := range actual.Args {
+					require.NotContains(t, arg, "--log.level", "valkey_exporter rejects the dotted flag")
+				}
+			})
+		}
+	})
+
+	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")
+		}
+	})
 }