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

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

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

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 "<flagName>=<level>" 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)
 }

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 <https://www.gnu.org/licenses/>.
+
+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)
+		})
+	}
+}

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{

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