Commit d25c4638 authored by craig[bot]'s avatar craig[bot]

Merge #49912

49912: kv,adminserver: properly remove the no-nodeid semantics r=tbg a=knz

Fixes #49896

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 issue 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
Co-authored-by: default avatarRaphael 'kena' Poss <[email protected]>
parents 326cb320 e0ad962a
......@@ -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