Commit e0ad962a authored by Raphael 'kena' Poss's avatar Raphael 'kena' Poss

kv,adminserver: properly remove the no-nodeid semantics

The Decommission RPC doesn't need to accept an empty node ID list
as of v20.2 since `cockroach quit --decommission` was removed.

However, `TestDecommission` still used that case and started
failing as a result. This patch fixes the test to not rely
on the behavior.

Additionally, this patch causes the RPC to return an error when no
node ID is specified, instead of silently turning into a no-op.

A discussion remains of whether the RPC should accept a way to specify
the "local" node (maybe more explicitly than via an empty list of node
IDs), like many of the other RPCs already do.

This discussion came up in a separate issues which wants that
behavior for the `node drain` command. I am expecting that `node
decommission` will want that option too. However let's address that at
that time.

Release note: None
parent d5bd8545
......@@ -3025,7 +3025,10 @@ func TestDecommission(t *testing.T) {
admin := serverpb.NewAdminClient(cc)
// Decommission the first node, which holds most of the leases.
_, err = admin.Decommission(
ctx, &serverpb.DecommissionRequest{Decommissioning: true},
ctx, &serverpb.DecommissionRequest{
NodeIDs: []roachpb.NodeID{1},
Decommissioning: true,
},
)
require.NoError(t, err)
......@@ -3059,7 +3062,10 @@ func TestDecommission(t *testing.T) {
ts := timeutil.Now()
_, err = admin.Decommission(
ctx, &serverpb.DecommissionRequest{NodeIDs: []roachpb.NodeID{2}, Decommissioning: true},
ctx, &serverpb.DecommissionRequest{
NodeIDs: []roachpb.NodeID{2},
Decommissioning: true,
},
)
require.NoError(t, err)
......@@ -3084,7 +3090,10 @@ func TestDecommission(t *testing.T) {
// Decommission two more nodes. Only n5 is left; getting the replicas there
// can't use atomic replica swaps because the leaseholder can't be removed.
_, err = admin.Decommission(
ctx, &serverpb.DecommissionRequest{NodeIDs: []roachpb.NodeID{3, 4}, Decommissioning: true},
ctx, &serverpb.DecommissionRequest{
NodeIDs: []roachpb.NodeID{3, 4},
Decommissioning: true,
},
)
require.NoError(t, err)
......
......@@ -1714,6 +1714,10 @@ func (s *adminServer) Decommission(
) (*serverpb.DecommissionStatusResponse, error) {
nodeIDs := req.NodeIDs
if len(nodeIDs) == 0 {
return nil, status.Errorf(codes.InvalidArgument, "no node ID specified")
}
// Mark the target nodes as decommissioning. They'll find out as they
// heartbeat their liveness.
if err := s.server.Decommission(ctx, req.Decommissioning, nodeIDs); err != nil {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment