Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: fully and properly drain target node of decommission #141411

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

tbg
Copy link
Member

@tbg tbg commented Feb 13, 2025

With this patch, at the end of decommissioning, we call the drain step as we
would for ./cockroach node drain:

[...]
.....
id	is_live	replicas	is_decommissioning	membership	is_draining	readiness	blocking_ranges
1	true	2	true	decommissioning	false	ready	0
.....
id	is_live	replicas	is_decommissioning	membership	is_draining	readiness	blocking_ranges
1	true	1	true	decommissioning	false	ready	0
......
id	is_live	replicas	is_decommissioning	membership	is_draining	readiness	blocking_ranges
1	true	0	true	decommissioning	false	ready	0
draining node n2
node is draining... remaining: 26
node is draining... remaining: 0 (complete)
node n2 drained successfully

No more data reported on target nodes. Please verify cluster health before removing the nodes.

In particular, note how the first invocation returns a RemainingIndicator of
26, so before this patch, we had initiated draining, but it hadn't fully completed.

I thought for a while that this could explain #140774, i.e. that #138732 was
insufficient as it did not guarantee that the node had actually drained fully
by the time it was marked as fully decommissioned and the node decommission
had returned. But I found that fully draining did not fix the test, and
ultimately tracked the issue down to a test infra problem. Still, this PR is
a good change, that brings the drain experience in decommission on par with
the standalone CLI.

See #140774.

I verified that the modified decommission/drains roachtest passes via

./pkg/cmd/roachtest/roachstress.sh -l -c 1 decommission/drains/alive

Touches #140774.
Touches #139411.
Touches #139413.
Closes #140098 (since we no longer fail decommission on drain failure)

PR #138732 already fixed most of the drain issues, but since the
decommissioning process still went ahead and shut the node out
from the cluster, SQL connections that drain was still waiting
for would likely hit errors (since the gateway node would not
be able to connect to the rest of the cluster any more due to
having been flipped to fully decommissioned). So there's a new
release note for the improvement in this PR, which avoids that.

We should consider backporting this new set of changes to 25.1
to address flakes (and the corresponding poor UX that could occur
in production) such as #141578 (comment).

Release note (bug fix): previously, a node that was drained as part
of decommissioning may have interrupted SQL connections that were
still active during drain (and for which drain would have been
expected to wait).
Epic: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg force-pushed the drain-fix-for-reals branch 2 times, most recently from cfb1b1e to 7700799 Compare February 13, 2025 11:12
With this patch, at the end of decommissioning, we call the drain step as we
would for `./cockroach node drain`:

```
[...]
.....
id	is_live	replicas	is_decommissioning	membership	is_draining	readiness	blocking_ranges
1	true	2	true	decommissioning	false	ready	0
.....
id	is_live	replicas	is_decommissioning	membership	is_draining	readiness	blocking_ranges
1	true	1	true	decommissioning	false	ready	0
......
id	is_live	replicas	is_decommissioning	membership	is_draining	readiness	blocking_ranges
1	true	0	true	decommissioning	false	ready	0
draining node n2
node is draining... remaining: 26
node is draining... remaining: 0 (complete)
node n2 drained successfully

No more data reported on target nodes. Please verify cluster health before removing the nodes.
```

In particular, note how the first invocation returns a RemainingIndicator of
26, so before this patch, we had initiated draining, but it hadn't fully completed.

I thought for a while that this could explain cockroachdb#140774, i.e. that cockroachdb#138732 was
insufficient as it did not guarantee that the node had actually drained fully
by the time it was marked as fully decommissioned and the `node decommission`
had returned. But I found that fully draining did not fix the test, and
ultimately tracked the issue down to a test infra problem. Still, this PR is
a good change, that brings the drain experience in decommission on par with
the standalone CLI.

See cockroachdb#140774.

I verified that the modified decommission/drains roachtest passes via

```
./pkg/cmd/roachtest/roachstress.sh -l -c 1 decommission/drains/alive
```

Touches cockroachdb#140774.
Touches cockroachdb#139411.
Touches cockroachdb#139413.

PR cockroachdb#138732 already fixed most of the drain issues, but since the
decommissioning process still went ahead and shut the node out
from the cluster, SQL connections that drain was still waiting
for would likely hit errors (since the gateway node would not
be able to connect to the rest of the cluster any more due to
having been flipped to fully decommissioned). So there's a new
release note for the improvement in this PR, which avoids that.

Release note (bug fix): previously, a node that was drained as part
of decommissioning may have interrupted SQL connections that were
still active during drain (and for which drain would have been
expected to wait).
Epic: None
@tbg tbg force-pushed the drain-fix-for-reals branch from 7700799 to 5319f88 Compare February 13, 2025 11:35
@tbg tbg marked this pull request as ready for review February 17, 2025 15:50
@tbg tbg added the backport-25.1.x Flags PRs that need to be backported to 25.1 label Feb 17, 2025
@tbg tbg requested a review from arulajmani February 17, 2025 15:52
Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

@tbg
Copy link
Member Author

tbg commented Feb 20, 2025

TFTR!

bors r+

@craig craig bot merged commit 52b1df0 into cockroachdb:master Feb 20, 2025
24 checks passed
Copy link

blathers-crl bot commented Feb 20, 2025

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.


Issue #140098: branch-release-25.1.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-25.1.x Flags PRs that need to be backported to 25.1 target-release-25.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roachtest: decommission/randomized failed
3 participants