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

node wise recovery improvements #15394

Merged
merged 8 commits into from
Dec 13, 2023
Merged

Conversation

bharathv
Copy link
Contributor

@bharathv bharathv commented Dec 11, 2023

Some changes from previous defunct nodes implementation.

  • defunct nodes is no longer a thing, will be reimplemented as offline nodes in a future PR.
  • node wise recovery and offlining nodes are two separate user workflows. Earlier node wise recovering partitions and marking nodes as defunct were combined into a single command.
  • node wise recovery no longer touches partitions with majority, balancer should automatically recover them after unavailability timeout.
  • corresponding controller command for node wise recovery is idempotent (can be issued as many times as needed)

API examples:

fetch a list of partitions losing majority - GET http://:9644/v1/partitions/majority_lost?dead_nodes=5,6,7
node wise recovery them - POST http://:9644/v1/partitions/force_recover_from_nodes

example payload.

{'dead_nodes': [5], 'partitions_to_force_recover': [{'ntp': {'ns': 'kafka', 'topic': 'topic-3', 'partition': 0     }, 'topic_revision': 42, 'replicas': [{'node_id': 5, 'core': 0}], 'dead_nodes': [5]}, {'ntp': {'ns': 'kafka', 'topic': 'topic-7', 'partition': 0}, 'topic_revision': 49, 'replicas': [{'node_id': 5, 'core': 1}], 'dead_nodes': [5]}, {'     ntp': {'ns': 'kafka', 'topic': 'topic-5', 'partition': 0}, 'topic_revision': 47, 'replicas': [{'node_id': 5, 'core': 1}], 'dead_nodes': [5]}, {'ntp': {'ns': 'kafka', 'topic': 'topic-1', 'partition': 0}, 'topic_revision': 39, 'replic     as': [{'node_id': 5, 'core': 1}], 'dead_nodes': [5]}, {'ntp': {'ns': 'kafka', 'topic': 'topic-17', 'partition': 1}, 'topic_revision': 59, 'replicas': [{'node_id': 5, 'core': 1}], 'dead_nodes': [5]}]}

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.2.x
  • v23.1.x
  • v22.3.x

Release Notes

Features

  • Adds the ability to force recover all partitions from a set of nodes (aka node wise recovery). This bulk recovers all partitions that lost majority because the nodes holding the majority replicas are no longer available.

@bharathv
Copy link
Contributor Author

/dt

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Dec 12, 2023

new failures in https://buildkite.com/redpanda/redpanda/builds/42572#018c5c79-699d-4e7f-95f6-317755a710a1:

"rptest.tests.cluster_config_test.ClusterConfigAliasTest.test_aliasing_with_upgrade.wipe_cache=False.prop_set=PropertyAliasData.primary_name=.log_retention_ms.aliased_name=.delete_retention_ms.redpanda_version=.23.3.test_values=.1000000.300000.500000"

new failures in https://buildkite.com/redpanda/redpanda/builds/42572#018c5c79-69a4-4294-9b51-0d811376a80f:

"rptest.tests.cluster_config_test.ClusterConfigAliasTest.test_aliasing_with_upgrade.wipe_cache=True.prop_set=PropertyAliasData.primary_name=.log_retention_ms.aliased_name=.delete_retention_ms.redpanda_version=.23.3.test_values=.1000000.300000.500000"

@bharathv bharathv marked this pull request as ready for review December 12, 2023 08:33
@bharathv
Copy link
Contributor Author

/dt

auto validation_err
= _topics.local().validate_force_reconfigurable_partitions(result);
if (validation_err) {
co_return errc::concurrent_modification_error;
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can return a validation_err here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was intentional because if a user makes a get_majority_lost_partitions() they don't expect an error unless something concurrently modified the state while processing the request (hence concurrent_modification_error).. this validation_error is only possible in that situation.

@mmaslankaprv
Copy link
Member

just two comments, otherwise looks good

This command will replace defunct_nodes_cmd in the subsequent commits.
Adds logic for processing force reconfiguration partitions. Additionally
undos most of the defunct_node_cmd state processing.
Additionally renames defunct_node -> dead_node in most APIs to clearup
terminology.
@bharathv
Copy link
Contributor Author

/dt

@vbotbuildovich
Copy link
Collaborator

@bharathv bharathv merged commit a2a2dd4 into redpanda-data:dev Dec 13, 2023
20 checks passed
@bharathv bharathv deleted the defunct_node_fixes branch December 13, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants