From 5dff164f1c8fc08b66f0ea509db36561eaef464c Mon Sep 17 00:00:00 2001 From: Utku Ozdemir Date: Fri, 27 Oct 2023 18:42:12 +0200 Subject: [PATCH] fix: fix error output of cli action tracker Before we started a reboot/shutdown/reset/upgrade action with the action tracker (`--wait`), we were setting a flag to prevent cobra from printing the returned error from the command. This was to prevent the error from being printed twice, as the reporter of the action tracker already prints any errors occurred during the action execution. But if the error happens too early - i.e. before we even started the status printer goroutine, then that error wouldn't be printed at all, as we have suppressed the errors. This PR moves the suppression flag to be set after the status printer is started - so we still do not double-print the errors, but neither do we suppress any early-stage error from being printed. Closes siderolabs/talos#7900. Signed-off-by: Utku Ozdemir --- cmd/talosctl/cmd/talos/reboot.go | 3 --- cmd/talosctl/cmd/talos/reset.go | 3 --- cmd/talosctl/cmd/talos/shutdown.go | 3 --- cmd/talosctl/cmd/talos/upgrade.go | 3 --- cmd/talosctl/pkg/talos/action/tracker.go | 5 +++++ internal/integration/cli/reboot.go | 27 ++++++++++++++++++++++++ 6 files changed, 32 insertions(+), 12 deletions(-) diff --git a/cmd/talosctl/cmd/talos/reboot.go b/cmd/talosctl/cmd/talos/reboot.go index 10af104c6..d45207b34 100644 --- a/cmd/talosctl/cmd/talos/reboot.go +++ b/cmd/talosctl/cmd/talos/reboot.go @@ -10,7 +10,6 @@ import ( "github.com/spf13/cobra" - "github.com/siderolabs/talos/cmd/talosctl/cmd/common" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/action" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/helpers" "github.com/siderolabs/talos/pkg/machinery/client" @@ -57,8 +56,6 @@ var rebootCmd = &cobra.Command{ }) } - common.SuppressErrors = true - return action.NewTracker( &GlobalArgs, action.MachineReadyEventFn, diff --git a/cmd/talosctl/cmd/talos/reset.go b/cmd/talosctl/cmd/talos/reset.go index 8c1e0ca86..e76a7699f 100644 --- a/cmd/talosctl/cmd/talos/reset.go +++ b/cmd/talosctl/cmd/talos/reset.go @@ -13,7 +13,6 @@ import ( "github.com/siderolabs/gen/maps" "github.com/spf13/cobra" - "github.com/siderolabs/talos/cmd/talosctl/cmd/common" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/action" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/helpers" machineapi "github.com/siderolabs/talos/pkg/machinery/api/machine" @@ -140,8 +139,6 @@ var resetCmd = &cobra.Command{ } } - common.SuppressErrors = true - return action.NewTracker( &GlobalArgs, action.StopAllServicesEventFn, diff --git a/cmd/talosctl/cmd/talos/shutdown.go b/cmd/talosctl/cmd/talos/shutdown.go index 4027edbc3..bbbc1ee05 100644 --- a/cmd/talosctl/cmd/talos/shutdown.go +++ b/cmd/talosctl/cmd/talos/shutdown.go @@ -10,7 +10,6 @@ import ( "github.com/spf13/cobra" - "github.com/siderolabs/talos/cmd/talosctl/cmd/common" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/action" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/helpers" "github.com/siderolabs/talos/pkg/machinery/client" @@ -50,8 +49,6 @@ var shutdownCmd = &cobra.Command{ }) } - common.SuppressErrors = true - return action.NewTracker( &GlobalArgs, action.StopAllServicesEventFn, diff --git a/cmd/talosctl/cmd/talos/upgrade.go b/cmd/talosctl/cmd/talos/upgrade.go index b1afce7a7..c2143ee9c 100644 --- a/cmd/talosctl/cmd/talos/upgrade.go +++ b/cmd/talosctl/cmd/talos/upgrade.go @@ -19,7 +19,6 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/peer" - "github.com/siderolabs/talos/cmd/talosctl/cmd/common" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/action" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/helpers" "github.com/siderolabs/talos/pkg/cli" @@ -73,8 +72,6 @@ var upgradeCmd = &cobra.Command{ return runUpgradeNoWait(opts) } - common.SuppressErrors = true - return action.NewTracker( &GlobalArgs, action.MachineReadyEventFn, diff --git a/cmd/talosctl/pkg/talos/action/tracker.go b/cmd/talosctl/pkg/talos/action/tracker.go index 27feec0e0..9e766a2c2 100644 --- a/cmd/talosctl/pkg/talos/action/tracker.go +++ b/cmd/talosctl/pkg/talos/action/tracker.go @@ -23,6 +23,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/backoff" + "github.com/siderolabs/talos/cmd/talosctl/cmd/common" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/global" "github.com/siderolabs/talos/cmd/talosctl/pkg/talos/helpers" machineapi "github.com/siderolabs/talos/pkg/machinery/api/machine" @@ -155,6 +156,10 @@ func (a *Tracker) Run() error { return a.runReporter(ctx) }) + // Reporter is started, it will print the errors if there is any. + // So from here on we can suppress the command error to be printed to avoid it being printed twice. + common.SuppressErrors = true + var trackEg errgroup.Group for _, node := range a.cliContext.Nodes { diff --git a/internal/integration/cli/reboot.go b/internal/integration/cli/reboot.go index a48d7951b..1581f92c6 100644 --- a/internal/integration/cli/reboot.go +++ b/internal/integration/cli/reboot.go @@ -9,6 +9,7 @@ package cli import ( "fmt" + "path/filepath" "strings" "testing" @@ -82,6 +83,32 @@ func (suite *RebootSuite) TestReboot() { ) } +// TestRebootEarlyFailPrintsOutput tests the action tracker used by reboot command to track reboot status +// does not suppress the stderr output if there is an error occurring at an early stage, i.e. before the +// action status reporting starts. +func (suite *RebootSuite) TestRebootEarlyFailPrintsOutput() { + controlPlaneNode := suite.RandomDiscoveredNodeInternalIP(machine.TypeControlPlane) + invalidTalosconfig := filepath.Join(suite.T().TempDir(), "talosconfig.yaml") + + suite.T().Logf("attempting to reboot node %q using talosconfig %q", controlPlaneNode, invalidTalosconfig) + + suite.RunCLI([]string{"--talosconfig", invalidTalosconfig, "reboot", "-n", controlPlaneNode}, + base.ShouldFail(), + base.StdoutEmpty(), + base.StderrNotEmpty(), + base.StderrMatchFunc(func(stdout string) error { + if strings.Contains(stdout, "method is not supported") { + suite.T().Skip("reboot is not supported") + } + + if !strings.Contains(stdout, "failed to determine endpoints") { + return fmt.Errorf("expected to find 'failed to determine endpoints' in stderr") + } + + return nil + })) +} + func init() { allSuites = append(allSuites, new(RebootSuite)) }