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.

Sign up for GitHub

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

Jump to bottom

Improve migrations API #23479

Merged
merged 9 commits into from
Oct 2, 2024
Merged

Improve migrations API #23479

merged 9 commits into from
Oct 2, 2024

Conversation

bashtanov
Copy link
Contributor

  1. implement API to show a single migration
  2. on migration creation validate that cluster and topic config are supported
  3. add tests for these and for deleting active migrations

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
  • v24.2.x
  • v24.1.x
  • v23.3.x

Release Notes

  • none

@bashtanov bashtanov changed the title Imrove migrations API Improve migrations API Sep 25, 2024
@bashtanov bashtanov requested a review from ztlpn September 25, 2024 15:17
@bashtanov bashtanov assigned bharathv and unassigned bharathv Sep 25, 2024
@bashtanov bashtanov requested a review from bharathv September 25, 2024 15:18
@bashtanov bashtanov force-pushed the migrations-implement-get-single branch from af6af78 to 6bee80a Compare September 25, 2024 16:30
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Sep 25, 2024 •
edited
Loading

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55119#01922a52-d9ec-4c72-a21d-e7c653424b2d

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55598#01924a32-bcb7-408e-807c-db5c0858d011

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55638#01924ce7-fab6-4de2-b08e-653f773f9e09

mmaslankaprv
mmaslankaprv reviewed Sep 30, 2024
View reviewed changes
tests/rptest/tests/data_migrations_api_test.py Outdated
@@ -33,6 +33,7 @@

MIGRATION_LOG_ALLOW_LIST = [
'Error during log recovery: cloud_storage::missing_partition_exception',
'Requested data migration does not exist',
Copy link
Member

Choose a reason for hiding this comment

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

this shouldn't be logged with an error severity as this is a result of user interacting with an API endpoint

mmaslankaprv
mmaslankaprv reviewed Sep 30, 2024
View reviewed changes
tests/rptest/tests/data_migrations_api_test.py Outdated
@@ -34,6 +34,7 @@
MIGRATION_LOG_ALLOW_LIST = [
'Error during log recovery: cloud_storage::missing_partition_exception',
'Requested data migration does not exist',
'Invalid data migration state transition requested',
Copy link
Member

Choose a reason for hiding this comment

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

this is similar, shouldn't be an error

mmaslankaprv
mmaslankaprv reviewed Sep 30, 2024
View reviewed changes
src/v/cluster/data_migration_table.cc Outdated
@@ -201,6 +201,14 @@ migrations_table::apply(create_data_migration_cmd cmd) {
std::optional<migrations_table::validation_error>
migrations_table::validate_migrated_resources(
const data_migration& migration) const {
// cloud_storage_api is checked on startup
if (!config::shard_local_cfg().cloud_storage_enabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

can we pass a configuration::binding for those two values ?, the code will be much easier to test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it won't work, as these settings fail base_property::assert_live_settable()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used a boolean as discussed

mmaslankaprv
mmaslankaprv reviewed Sep 30, 2024
View reviewed changes
src/v/cluster/tests/data_migration_table_test.cc Outdated
Comment on lines 87 to 89
config::shard_local_cfg().cloud_storage_enabled.set_value(true);
config::shard_local_cfg()
.cloud_storage_disable_archiver_manager.set_value(true);
config::shard_local_cfg().cloud_storage_enable_remote_write.set_value(
true);
Copy link
Member

Choose a reason for hiding this comment

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

this should be expressed as config::mock_binding passed as an argument to the table constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

used a boolean ctor argument as discussed

@bashtanov
misc: fix typos in comments and error messages
2875c26
@bashtanov
c/migrations: implement API to get a single migration
5b5bd7f
@bashtanov
c/migrations: improve migration validation on create
cf46cac 
Check the same conditions as checked when creating archival STM:
cluster-wide and topic configuration on cloud storage.
@bashtanov
admin/server: translate data migration errors to http return codes
3cd43a8
@bashtanov
tests/migrations: test individual migration get API endpoint
8073365
@bashtanov
tests/migrations: check migrations are not deletable in transient states
ca17854
@bashtanov
tests/migrations: test migration validation
f946ebc 
refuse to create migrations if cluster-wide or topic configs are unsupported
@bashtanov
c/migrations/tests: configure for cloud in data migration table test
d728b81 
otherwise migrations don't pass validation
@bashtanov
tests/migrations: test getting a migration that does not exist
68889db
@bashtanov bashtanov force-pushed the migrations-implement-get-single branch from 6bee80a to 68889db Compare October 1, 2024 21:14
@bashtanov bashtanov requested a review from mmaslankaprv October 1, 2024 21:15
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Oct 1, 2024 •
edited
Loading

new failures in https://buildkite.com/redpanda/redpanda/builds/55598#01924a2e-8c27-4ae0-a670-1ab05ea78794:

"rptest.tests.debug_bundle_test.DebugBundleTest.test_post_debug_bundle.ignore_none=False"

new failures in https://buildkite.com/redpanda/redpanda/builds/55598#01924a2e-8c2a-4b5c-96fd-c11d43af00b4:

"rptest.tests.debug_bundle_test.DebugBundleTest.test_post_debug_bundle.ignore_none=True"

new failures in https://buildkite.com/redpanda/redpanda/builds/55598#01924a32-bcb1-4bcb-8d7f-f6b0ada432ce:

"rptest.tests.debug_bundle_test.DebugBundleTest.test_post_debug_bundle.ignore_none=True"

new failures in https://buildkite.com/redpanda/redpanda/builds/55598#01924a32-bcae-4083-b72d-c98b7b38d900:

"rptest.tests.debug_bundle_test.DebugBundleTest.test_post_debug_bundle.ignore_none=False"

@bashtanov
Copy link
Contributor Author

/ci-repeat

@bashtanov bashtanov merged commit e042836 into redpanda-data:dev Oct 2, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmaslankaprv mmaslankaprv mmaslankaprv approved these changes

@ztlpn ztlpn Awaiting requested review from ztlpn

@bharathv bharathv Awaiting requested review from bharathv

Assignees
No one assigned
Labels
area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bashtanov @vbotbuildovich @mmaslankaprv @bharathv