Skip to content

Commit

Permalink
server,cli: improve {d,r}ecommissioning user errors
Browse files Browse the repository at this point in the history
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
  • Loading branch information
irfansharif committed Sep 29, 2020
1 parent d631239 commit 84d9c12
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 2 deletions.
22 changes: 20 additions & 2 deletions pkg/cli/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 84d9c12

Please sign in to comment.