Migrations memory bugfixes #23556

bashtanov · 2024-09-30T09:24:20Z

A few fixes where objects lifetime was not taken care of.

Backports Required

Release Notes

none

vbotbuildovich · 2024-09-30T12:08:40Z

new failures in https://buildkite.com/redpanda/redpanda/builds/55426#0192428c-958e-42ed-afa3-cd32bcbd9924:

"rptest.tests.delete_records_test.DeleteRecordsTest.test_delete_records_concurrent_truncations.cloud_storage_enabled=True.truncate_point=start_offset"

new failures in https://buildkite.com/redpanda/redpanda/builds/55631#01924cca-eee3-41a6-980c-35bef00a6724:

"rptest.tests.partition_force_reconfiguration_test.PartitionForceReconfigurationTest.test_basic_reconfiguration.acks=-1.restart=False.controller_snapshots=False"

mmaslankaprv · 2024-09-30T12:40:10Z

src/v/ssx/async_algorithm.h

@@ -106,6 +106,10 @@ template<std::random_access_iterator I, typename Fn>
 iter_size<I> for_each_limit(const I begin, const I end, ssize_t limit, Fn f) {
    auto chunk_size = std::min(limit, end - begin);
    I chunk_end = begin + chunk_size;
+    // std::for_each discards [[nodiscard]] callable result, check explicitly
+    if (false) {


would it be possible to check if function passed to for each limit returns void by using a cpp concept ?

there is another flavor of this function which isn't validated

would it be possible to check if function passed to for each limit returns void by using a cpp concept?

Probably, but I think this would be too strict. One may want to use a function that actually returns a value which is by design okay to discard.

there is another flavor of this function which isn't validated

The one below? It is not affected by the problem, as f is called explicitly. If f or its return type is marked [[nodiscard]] the call will emit warning (as error) on compilation, I have checked. future<...> class is declared [[nodiscard]] in Seastar.

Does this even work?

It looks like the moment you convert the function to a function pointer the no discard attribute gets lost https://gcc.godbolt.org/z/jWbKo59r8

Yes it does work. Nodiscard attribute gets lost from a function if you convert it to a pointer indeed, as it's stored neither in the pointer nor in the pointer type. It is not, however, lost if the return type itself is marked as nodiscard: https://gcc.godbolt.org/z/bGq1ordvj. In this case the function pointer type retains the information about the function return type, which is known to be nodiscard.

vbotbuildovich · 2024-09-30T12:58:15Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55426#0192428d-54c4-46ad-8b66-41ece58a7436

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55522#0192479c-be73-41b3-8cde-5210398ab363

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55522#0192479f-050e-4fd6-aaba-aa70f5c92b4f

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/55573#01924966-16db-48ed-8607-ba552116e3fa

dotnwat · 2024-09-30T22:10:52Z

src/v/cluster/data_migration_backend.cc

-                      _topic_migration_map.emplace(nt, metadata.id);
-                      return reconcile_topic(
-                        nt, tstate, metadata.id, mrstate.scope, true);
+                      return reconcile_topic(metadata.id, idx, nt, mrstate);


fix tstate lifetime

in the first commit, i have a guess about what the memory lifetime issue is that is being fixed, but i'm not certain. please state in the commit message what the issue is and how it's being fixed.

dotnwat · 2024-09-30T22:12:24Z

src/v/cluster/data_migration_backend.cc

-      [this, &metadata, &mrstate](const auto& migration) mutable {
+      [this, migration_id = metadata.id, &mrstate](
+        const auto& migration) mutable {
          return ss::do_with(
-            migration.topic_nts(),
-            [this, &metadata, &mrstate](const auto& nts) {
-                // poor man's `nts | std::views::enumerate`
-                auto enumerated_nts = std::views::transform(
-                  nts, [index = -1](const auto& nt) mutable {
-                      return std::forward_as_tuple(++index, nt);
-                  });
-                return ssx::async_for_each(
+            // poor man's `migration.topic_nts() | std::views::enumerate`
+            std::views::transform(
+              migration.topic_nts(),
+              [index = -1](const auto& nt) mutable {
+                  return std::forward_as_tuple(++index, nt);
+              }),
+            [this, migration_id, &mrstate](auto& enumerated_nts) {
+                return ss::do_for_each(
                  enumerated_nts,
-                  [this, &metadata, &mrstate](const auto& idx_nt) {
+                  [this, migration_id, &mrstate](const auto& idx_nt) {
                      auto& [idx, nt] = idx_nt;
-                      return reconcile_topic(metadata.id, idx, nt, mrstate);
+                      return reconcile_topic(migration_id, idx, nt, mrstate);


maybe split out the reformatting? it's kinda hard to tell what is changing here

I tried playing with formatting, but it did not make things any cleaner. I'd suggest switching to side-by-side mode for this change.

Essentially, since ss::do_for_each does not extend the container's lifetime, we should pass exactly its container argument via ss::do_with, that's what the change is about. I can create ssx::do_for_each_with if you think it has potential for reuse.

dotnwat · 2024-09-30T22:17:46Z

src/v/ssx/async_algorithm.h

+    // std::for_each discards [[nodiscard]] callable result, check explicitly
+    if (false) {
+        f(*begin);
+    }
    std::for_each(begin, chunk_end, std::move(f));


i'm slightly confused about what is going on. reading the commit message, it seems like it would be sufficient to disallow this function from being used for an f that returns a future, which would seem like the most common mistake?

then the other thing you could do is

f(*begin); std::for_each(++begin, chunk_end, std::move(f));

?

The original problem is indeed mistakenly supplying a future-returning function to ssx::async_for_each or one of its flavours. But I see no reason to limit the fix to future-returning functions specifically, and rather do it for any functions returning with [[nodiscard]]. It fixes the original problem and any other potential mistakes where the function result is silently lost but is not meant to.

Moreover, I don't think it would be unreasonable to create an improved version of std::for_each (with this static check) in our base library and use it everywhere instead.

As for your second proposal, is it because you are worried the call is optimized out before the warning is emitted? I would imagine the warning is emitted by the frontend, while any optimization, even that simple, is in the later stages. Your suggestion will also UB on an empty chunk (I'm not sure if it's called at all in this case but I'd rather not break it).

As for your second proposal, is it because you are worried the call is optimized out before the warning is emitted? I would imagine the warning is emitted by the frontend, while any optimization, even that simple, is in the later stages. Your suggestion will also UB on an empty chunk (I'm not sure if it's called at all in this case but I'd rather not break it).

nah, just that if (false) looks weird. fine i guess if it works, just wondering if there were a better option.

bashtanov · 2024-10-01T21:18:10Z

Hi all, the last commit seems to have attracted most comments, so I reworded it.

@travisdowns was there any particular reason for using std::for_each instead of a for loop with iterators? Would you mind if I changed it to a loop similar to its implementation in the standard library:

  for (; __first != __last; ++__first)
    __f(*__first);

?

It was shorter than the one of the coroutine using it.

copy migration definition from migrations table, as it may disappear

- do not use ssx::async_for_each for coroutines - extend container lifetime for ss::do_for_each using ss::do_with

bashtanov · 2024-10-02T08:59:34Z

I've removed the controversial change and will rework it. This PR remains to only fix migrations problems.

mmaslankaprv · 2024-10-02T13:24:18Z

please update release notes

bashtanov · 2024-10-02T13:37:41Z

failure: https://redpandadata.atlassian.net/browse/CORE-6906
@mmaslankaprv could you merge please?

bashtanov requested a review from mmaslankaprv September 30, 2024 09:24

github-actions bot added the area/redpanda label Sep 30, 2024

bashtanov requested review from ztlpn, bharathv and BenPope September 30, 2024 09:24

mmaslankaprv reviewed Sep 30, 2024

View reviewed changes

dotnwat reviewed Sep 30, 2024

View reviewed changes

bashtanov force-pushed the migrations-memory-bugfixes branch from 200eab0 to 7d13f36 Compare October 1, 2024 09:15

bashtanov requested review from mmaslankaprv and dotnwat October 1, 2024 09:22

bashtanov force-pushed the migrations-memory-bugfixes branch from 7d13f36 to c4a12d3 Compare October 1, 2024 17:07

bashtanov requested a review from StephanDollberg October 2, 2024 08:49

bashtanov added 3 commits October 2, 2024 09:56

c/migrations: fix tstate lifetime in backend::reconcile_migration

ed0a9f8

It was shorter than the one of the coroutine using it.

c/migrations: fix memory management in handle_migration_update

8565c73

copy migration definition from migrations table, as it may disappear

c/migrations: memory management fixes

ec5538d

- do not use ssx::async_for_each for coroutines - extend container lifetime for ss::do_for_each using ss::do_with

bashtanov force-pushed the migrations-memory-bugfixes branch from c4a12d3 to ec5538d Compare October 2, 2024 08:58

bashtanov removed request for dotnwat, BenPope and StephanDollberg October 2, 2024 08:58

bashtanov requested a review from dotnwat October 2, 2024 08:59

bashtanov mentioned this pull request Oct 2, 2024

ssx/async_algorithm: avoid std::for_each to honor [[nodiscard]] #23598

Merged

7 tasks

mmaslankaprv approved these changes Oct 2, 2024

View reviewed changes

mmaslankaprv merged commit 97b9e0d into redpanda-data:dev Oct 2, 2024
15 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrations memory bugfixes #23556

Migrations memory bugfixes #23556

bashtanov commented Sep 30, 2024 •

edited

Loading

vbotbuildovich commented Sep 30, 2024 •

edited

Loading

mmaslankaprv Sep 30, 2024

mmaslankaprv Sep 30, 2024

bashtanov Sep 30, 2024

StephanDollberg Oct 1, 2024

bashtanov Oct 1, 2024

vbotbuildovich commented Sep 30, 2024 •

edited

Loading

dotnwat Sep 30, 2024

dotnwat Sep 30, 2024

bashtanov Oct 1, 2024

dotnwat Sep 30, 2024

bashtanov Oct 1, 2024

dotnwat Oct 2, 2024

bashtanov commented Oct 1, 2024

bashtanov commented Oct 2, 2024

mmaslankaprv commented Oct 2, 2024

bashtanov commented Oct 2, 2024

Migrations memory bugfixes #23556

Migrations memory bugfixes #23556

Conversation

bashtanov commented Sep 30, 2024 • edited Loading

Backports Required

Release Notes

vbotbuildovich commented Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbotbuildovich commented Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bashtanov commented Oct 1, 2024

bashtanov commented Oct 2, 2024

mmaslankaprv commented Oct 2, 2024

bashtanov commented Oct 2, 2024

bashtanov commented Sep 30, 2024 •

edited

Loading

vbotbuildovich commented Sep 30, 2024 •

edited

Loading

vbotbuildovich commented Sep 30, 2024 •

edited

Loading