From 84d9c12bdce0747026d232c9fba3918bd1d91ab0 Mon Sep 17 00:00:00 2001
From: irfan sharif <irfanmahmoudsharif@gmail.com>
Date: Thu, 24 Sep 2020 18:00:00 -0400
Subject: [PATCH] server,cli: improve {d,r}ecommissioning user errors

Specifically around {d,r}ecommissioning non-existent nodes.
We're able to do this now because we can always assume that a missing
liveness record, as seen in the decommission/recommission codepaths,
imply that the user is trying to decommission/recommission a
non-existent node.

Release note: None
---
 pkg/cli/node.go      | 22 ++++++++++++++++++++--
 pkg/server/server.go |  5 +++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/pkg/cli/node.go b/pkg/cli/node.go
index 8ed8f735362b..3d7fd3863cac 100644
--- a/pkg/cli/node.go
+++ b/pkg/cli/node.go
@@ -337,7 +337,17 @@ func runDecommissionNode(cmd *cobra.Command, args []string) error {
 	}
 
 	c := serverpb.NewAdminClient(conn)
-	return runDecommissionNodeImpl(ctx, c, nodeCtx.nodeDecommissionWait, nodeIDs)
+	if err := runDecommissionNodeImpl(ctx, c, nodeCtx.nodeDecommissionWait, nodeIDs); err != nil {
+		cause := errors.UnwrapAll(err)
+		if s, ok := status.FromError(cause); ok && s.Code() == codes.NotFound {
+			// Are we trying to decommision a node that does not
+			// exist? See Server.Decommission for where this specific grpc error
+			// code is generated.
+			return errors.New("node does not exist")
+		}
+		return err
+	}
+	return nil
 }
 
 func handleNodeDecommissionSelf(
@@ -566,13 +576,21 @@ func runRecommissionNode(cmd *cobra.Command, args []string) error {
 	}
 	resp, err := c.Decommission(ctx, req)
 	if err != nil {
+		cause := errors.UnwrapAll(err)
 		// If it's a specific illegal membership transition error, we try to
 		// surface a more readable message to the user. See
 		// ValidateLivenessTransition in kvserverpb/liveness.go for where this
 		// error is generated.
-		if s, ok := status.FromError(err); ok && s.Code() == codes.FailedPrecondition {
+		if s, ok := status.FromError(cause); ok && s.Code() == codes.FailedPrecondition {
 			return errors.Newf("%s", s.Message())
 		}
+		if s, ok := status.FromError(cause); ok && s.Code() == codes.NotFound {
+			// Are we trying to recommission node that does not
+			// exist? See Server.Decommission for where this specific grpc error
+			// code is generated.
+			fmt.Fprintln(stderr)
+			return errors.New("node does not exist")
+		}
 		return err
 	}
 	return printDecommissionStatus(*resp)
diff --git a/pkg/server/server.go b/pkg/server/server.go
index 46ddcf4fe877..389e3b73a45e 100644
--- a/pkg/server/server.go
+++ b/pkg/server/server.go
@@ -86,7 +86,9 @@ import (
 	gwruntime "github.com/grpc-ecosystem/grpc-gateway/runtime"
 	"github.com/opentracing/opentracing-go"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
+	grpcstatus "google.golang.org/grpc/status"
 )
 
 var (
@@ -1965,6 +1967,9 @@ func (s *Server) Decommission(
 	for _, nodeID := range nodeIDs {
 		statusChanged, err := s.nodeLiveness.SetMembershipStatus(ctx, nodeID, targetStatus)
 		if err != nil {
+			if errors.Is(err, kvserver.ErrMissingLivenessRecord) {
+				return grpcstatus.Error(codes.NotFound, kvserver.ErrMissingLivenessRecord.Error())
+			}
 			return err
 		}
 		if statusChanged {