Skip to content

Commit

Permalink
fix: fix error output of cli action tracker
Browse files Browse the repository at this point in the history
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 #7900.

Signed-off-by: Utku Ozdemir <utku.ozdemir@siderolabs.com>
(cherry picked from commit 5dff164)
  • Loading branch information
utkuozdemir authored and smira committed Nov 8, 2023
1 parent 059823c commit 6be1e58
Show file tree
Hide file tree
Showing 6 changed files with 32 additions and 12 deletions.
3 changes: 0 additions & 3 deletions cmd/talosctl/cmd/talos/reboot.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -57,8 +56,6 @@ var rebootCmd = &cobra.Command{
})
}

common.SuppressErrors = true

return action.NewTracker(
&GlobalArgs,
action.MachineReadyEventFn,
Expand Down
3 changes: 0 additions & 3 deletions cmd/talosctl/cmd/talos/reset.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -140,8 +139,6 @@ var resetCmd = &cobra.Command{
}
}

common.SuppressErrors = true

return action.NewTracker(
&GlobalArgs,
action.StopAllServicesEventFn,
Expand Down
3 changes: 0 additions & 3 deletions cmd/talosctl/cmd/talos/shutdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -50,8 +49,6 @@ var shutdownCmd = &cobra.Command{
})
}

common.SuppressErrors = true

return action.NewTracker(
&GlobalArgs,
action.StopAllServicesEventFn,
Expand Down
3 changes: 0 additions & 3 deletions cmd/talosctl/cmd/talos/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -73,8 +72,6 @@ var upgradeCmd = &cobra.Command{
return runUpgradeNoWait(opts)
}

common.SuppressErrors = true

return action.NewTracker(
&GlobalArgs,
action.MachineReadyEventFn,
Expand Down
5 changes: 5 additions & 0 deletions cmd/talosctl/pkg/talos/action/tracker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down
27 changes: 27 additions & 0 deletions internal/integration/cli/reboot.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ package cli

import (
"fmt"
"path/filepath"
"strings"
"testing"

Expand Down Expand Up @@ -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))
}

0 comments on commit 6be1e58

Please sign in to comment.