Return non-zero exit code in case of errors (#690)

* Return non-zero exit code in case of errors

At the moment, k8s and k8sd return 0 even if the command fails,
which is a problem especially when used inside scripts.

We'll ensure that a non-zero exit code is returned if the commands
fail.

* Update the cluster recovery command to use cobra "Run"

The cluster recovery command currently uses "RunE" and returns an
error in case of failures.

To stay consistent with other commands, we'll use "Run" and call
env.Exit as part of the command callback instead of returning the
errors.

src/k8s/cmd/k8sd/k8sd.go

@@ -91,7 +91,7 @@ func NewRootCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
 	addCommands(
 		cmd,
 		&cobra.Group{ID: "cluster", Title: "K8sd clustering commands:"},
-		newClusterRecoverCmd(),
+		newClusterRecoverCmd(env),
 	)
 
 	return cmd

src/k8s/cmd/k8sd/k8sd_cluster_recover.go

@@ -24,6 +24,7 @@ import (
 	"golang.org/x/sys/unix"
 	"gopkg.in/yaml.v2"
 
+	cmdutil "github.com/canonical/k8s/cmd/util"
 	"github.com/canonical/k8s/pkg/log"
 	"github.com/canonical/k8s/pkg/utils"
 )
@@ -100,26 +101,28 @@ func logDebugf(format string, args ...interface{}) {
 
 }
 
-func newClusterRecoverCmd() *cobra.Command {
+func newClusterRecoverCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "cluster-recover",
 		Short: "Recover the cluster from this member if quorum is lost",
-		RunE: func(cmd *cobra.Command, args []string) error {
+		Run: func(cmd *cobra.Command, args []string) {
 			log.Configure(log.Options{
 				LogLevel:     rootCmdOpts.logLevel,
 				AddDirHeader: true,
 			})
 
 			if err := recoveryCmdPrechecks(cmd.Context()); err != nil {
-				return err
+				cmd.PrintErrf("Recovery precheck failed: %v\n", err)
+				env.Exit(1)
 			}
 
 			if clusterRecoverOpts.SkipK8sd {
-				cmd.Printf("Skipping k8sd recovery.")
+				cmd.Printf("Skipping k8sd recovery.\n")
 			} else {
 				k8sdTarballPath, err := recoverK8sd()
 				if err != nil {
-					return fmt.Errorf("failed to recover k8sd, error: %w", err)
+					cmd.PrintErrf("Failed to recover k8sd, error: %v\n", err)
+					env.Exit(1)
 				}
 				cmd.Printf("K8sd cluster changes applied.\n")
 				cmd.Printf("New database state saved to %s\n", k8sdTarballPath)
@@ -130,15 +133,15 @@ func newClusterRecoverCmd() *cobra.Command {
 			}
 
 			if clusterRecoverOpts.SkipK8sDqlite {
-				cmd.Printf("Skipping k8s-dqlite recovery.")
+				cmd.Printf("Skipping k8s-dqlite recovery.\n")
 			} else {
 				k8sDqlitePreRecoveryTarball, k8sDqlitePostRecoveryTarball, err := recoverK8sDqlite()
 				if err != nil {
-					return fmt.Errorf(
-						"failed to recover k8s-dqlite, error: %w, "+
-							"pre-recovery backup: %s",
-						err, k8sDqlitePreRecoveryTarball,
-					)
+					cmd.PrintErrf(
+						"Failed to recover k8s-dqlite, error: %v, "+
+							"pre-recovery backup: %s\n",
+						err, k8sDqlitePreRecoveryTarball)
+					env.Exit(1)
 				}
 				cmd.Printf("K8s-dqlite cluster changes applied.\n")
 				cmd.Printf("New database state saved to %s\n",
@@ -148,8 +151,6 @@ func newClusterRecoverCmd() *cobra.Command {
 					k8sDqlitePostRecoveryTarball, clusterRecoverOpts.K8sDqliteStateDir)
 				cmd.Printf("Pre-recovery database backup: %s\n\n", k8sDqlitePreRecoveryTarball)
 			}
-
-			return nil
 		},
 	}
 

src/k8s/cmd/main.go

@@ -30,14 +30,26 @@ func main() {
 
 	// choose command based on the binary name
 	base := filepath.Base(os.Args[0])
+	var err error
 	switch base {
 	case "k8s-apiserver-proxy":
-		k8s_apiserver_proxy.NewRootCmd(env).ExecuteContext(ctx)
+		err = k8s_apiserver_proxy.NewRootCmd(env).ExecuteContext(ctx)
 	case "k8sd":
-		k8sd.NewRootCmd(env).ExecuteContext(ctx)
+		err = k8sd.NewRootCmd(env).ExecuteContext(ctx)
 	case "k8s":
-		k8s.NewRootCmd(env).ExecuteContext(ctx)
+		err = k8s.NewRootCmd(env).ExecuteContext(ctx)
 	default:
 		panic(fmt.Errorf("invalid entrypoint name %q", base))
 	}
+
+	// Although k8s commands typically use Run instead of RunE and handle
+	// errors directly within the command execution, this acts as a safeguard in
+	// case any are overlooked.
+	//
+	// Furthermore, the Cobra framework may not invoke the "Run*" entry points
+	// at all in case of argument parsing errors, in which case we *need* to
+	// handle the errors here.
+	if err != nil {
+		env.Exit(1)
+	}
 }