feat: Yield inside huge values migration serialization #4197

chakaz · 2024-11-26T14:27:47Z

With #4144 we break huge values slot migration into multiple commands. This PR now adds yield between those commands.
It also adds a test that checks that modifying huge values while doing a migration works well, and that RSS doesn't grow too much.

Fixes #4100

With #4144 we break huge values slot migration into multiple commands. This PR now adds yield between those commands. It also adds a test that checks that modifying huge values while doing a migration works well, and that RSS doesn't grow too much. Fixes #4100

chakaz · 2024-11-26T14:29:42Z

@adiholden re/ locks during serialization:

DbSlice::PreUpdate() locks local_mu_ before calling OnDbChange, so we are already covered there implicitly. In the previous PR perhaps you were thinking about registering for journal changes? We don't do that here...

kostasrim · 2024-11-26T15:53:22Z

@adiholden re/ locks during serialization:

DbSlice::PreUpdate() locks local_mu_ before calling OnDbChange, so we are already covered there implicitly. In the previous PR perhaps you were thinking about registering for journal changes? We don't do that here...

don't worry about local_mutex it's removed in my PR (assuming by local_mutex you mean the db_slice member)

chakaz · 2024-11-26T18:52:16Z

@adiholden re/ locks during serialization:
DbSlice::PreUpdate() locks local_mu_ before calling OnDbChange, so we are already covered there implicitly. In the previous PR perhaps you were thinking about registering for journal changes? We don't do that here...

don't worry about local_mutex it's removed in my PR (assuming by local_mutex you mean the db_slice member)

I do refer to DbSlice's local_mu_, and I actually do need to lock it for the same reason PreUpdate locks it today.

adiholden · 2024-11-28T12:26:14Z

tests/dragonfly/cluster_test.py

+    if seed_during_migration:
+        await stop_seed()
+    else:
+        # Only verify memory growth if we haven't pushed new data during migration


I am not sure we need this option of not running seeder during migration
you added this option inorder to check the rss compared to rss before migration right? but we can compare peak rss to peak used memory

more importantly is the comparison - we compare before and after if we don't seed during the migration

Before migrating to Seeder and comparison, I used custom logic for checking the data that was inserted. With the seeder I'm afraid it is no longer possible to check, unless you have an idea on how to do that.

I personally vote for keeping the previous logic, let me know if you want that as well..

adiholden · 2024-11-28T12:29:33Z

tests/dragonfly/cluster_test.py

+
+    insert_task = asyncio.create_task(insert_data(instances[0].cluster_client()))
+
+    async def get_rss(client, field):


get_meomory_info maybe?

adiholden · 2024-11-28T12:30:29Z

tests/dragonfly/cluster_test.py

+    while True:
+        rss = await get_rss(nodes[0].client, "used_memory_rss")
+        logging.debug(f"Current rss: {rss}")
+        if rss > 1_000_000_000:


explain why are you waiting for 1G rss?

adiholden · 2024-11-28T12:32:49Z

tests/dragonfly/cluster_test.py

+    # Insert data to containers with a gaussian distribution: some will be small and other big
+    stop = False
+
+    async def insert_data(client):


Kostas is working on seeder improvement for containers with different size
I prefer to have something generic and this logic inside test

chakaz · 2024-12-08T13:38:08Z

Please do not review yet. I still haven't modified the tests to use Kostas' new framework. This is just a merge of changes from main.

adiholden · 2024-12-23T06:56:07Z

src/server/journal/streamer.cc

    if (fiber_cancelled_)
      return;
    cursor = pt->TraverseBuckets(cursor, [&](PrimeTable::bucket_iterator it) {
+      std::lock_guard guard(big_value_mu_);


I believe the big_value_mu_ should protect only the WriteBucket to make sure that we do not write to the serializer from different fibers once we yield

Idealy it would be inside the WriteBucket function but I think we might have some failures in the flow of CVCUponInsert which could be fixed but not in this PR for sure

you will also need to call db_slice_->BlockingCounter() , check out SliceSnapshot::BucketSaveCb

This last comment makes me think that we are missing cluster migration test for dragonfly running in cache mode and for testing expire logic. Lets open a separate task for that and do this in a separate PR

I mimicked the locking pattern in snapshot.cc.

There, we acquire the lock before calling FlushChangeToEarlierCallbacks(), which I imagine is needed? If we'll lock inside WriteBucket() the call to FlushChangeToEarlierCallbacks() will be unguarded.

Also, by locking inside WriteBucket() we risk releasing the lock when we call it multiple times in CVCUponInsert() (so that the operation will no longer be atomic, if another fiber locks the mutex in between)

I added "locking" the blocking counter, good catch!

Will file an issue shortly. Edit: #4354

adiholden · 2024-12-26T11:49:42Z

tests/dragonfly/seeder/__init__.py

-        await asyncio.gather(
-            *(self._run_unit(client, sha, unit, using_stopkey, args) for unit in self.units)
-        )
+        for unit in self.units:


you can remove this changes right? we are not using the seeder v2 in cluster now

It's true that we don't, but it took me forever to understand this is where the bug is, and we might use it in the future, so I think it's a good idea to keep it as is..

adiholden · 2024-12-26T11:53:17Z

tests/dragonfly/cluster_test.py


+@dfly_args({"proactor_threads": 2, "cluster_mode": "yes"})
+@pytest.mark.asyncio
+async def test_cluster_migration_huge_container_while_seeding(


where is the huge container in this test?

Right, I'll rename.

adiholden · 2024-12-26T12:00:27Z

tests/dragonfly/utility.py

+                        pipe.execute_command(*cmd)
+                    await pipe.execute()
+                else:
+                    # To mirror consistently to Fake Redis we must only send to it successful


this is not so good that we dont test multi flow but its better than nothing..
I will continue thinking if we have a better approach to testing correctness of migraion so we will not have this limitaion

adiholden · 2024-12-26T12:12:54Z

tests/dragonfly/cluster_test.py

+    seeder = df_seeder_factory.create(
+        keys=100, port=instances[0].port, cluster_mode=True, mirror_to_fake_redis=True
+    )
+    seed = asyncio.create_task(seeder.run())


first call
await seeder.run(target_deviation=0.1)
To fill the data
and than
seed = asyncio.create_task(seeder.run())

you will not need the sleep in line 2054 after this change

adiholden · 2024-12-26T12:13:03Z

tests/dragonfly/cluster_test.py

+
+    logging.debug("Seeding cluster")
+    seeder = df_seeder_factory.create(
+        keys=100, port=instances[0].port, cluster_mode=True, mirror_to_fake_redis=True


more keys please

adiholden · 2024-12-26T12:15:46Z

tests/dragonfly/seeder_test.py

+    df_factory.start_all([instance])
+
+    seeder = df_seeder_factory.create(
+        keys=100, port=instance.port, unsupported_types=[ValueType.JSON], mirror_to_fake_redis=True


why not json?

FakeRedis doesn't work with JSON in my setup, I don't know why..
Since JSON is disabled in cluster anyway, I thought that it wouldn't matter too much, see:

dragonfly/tests/dragonfly/utility.py

Line 430 in 3b082e4

unsupported_types.append(ValueType.JSON) # Cluster aio client doesn't support JSON

adiholden · 2025-01-02T10:32:29Z

tests/dragonfly/utility.py

+                    # To mirror consistently to Fake Redis we must only send to it successful
+                    # commands. We can't use pipes because they might succeed partially.
+                    for cmd in tx_data[0]:
+                        await client.execute_command(*cmd)


maybe we can add here result compare between the different servers, as if the result is different this will probably lead to capture check fail and adding this check will help catching the reason in this case

That's a great idea! I'll do that now.

adiholden · 2025-01-02T11:15:40Z

tests/dragonfly/cluster_test.py

+    logging.debug("Seeding finished")
+
+    assert (
+        await get_memory(client0, "used_memory_peak_rss")


I ran the test on your branch , the check does not fail if chunk_size=0
maybe the data pushed by seeder is not big enough to trigger this check?

did you mean that you ran this test on the main branch?

anyway, yeah, this could be possible. I added this check back when we had huge values in the test. I think it's worthwhile to keep it though, for if/when we will add huge values to the v1 seeder

I ran it on your branch.
So do we have any test that we can this check to make sure we actually reduce the memory usage with your changes?

That was the original test that I wrote :)
I filled with custom (huge) data, verified consistency, and checked that memory did not increase too much.
We can't properly test that we did not increase in memory if we don't migrate huge values... but we can do that in another test, that does not migrate while seeding. I'll do that here.

Yes this are 2 different checks that we dont increase rss to much and check the migration is working correctly while sending traffic when the migration is in progress.
It does not have to be in the same test

adiholden · 2025-01-02T14:53:57Z

tests/dragonfly/cluster_test.py

+    )
+    await seeder.run(target_deviation=0.1)
+
+    seed = asyncio.create_task(seeder.run())


when I run the test I also see that RestoreStreamer::OnDbChange and the journal calls are not executed.
Only after adding a sleep here after this line I started seeing them executed.
Please add this sleep and we should also add stats and print them to log and check the stats after running the test but lets do that in another PR

shahar added 2 commits November 26, 2024 21:57

test timeout

e79309e

fix

4e0763a

chakaz requested a review from adiholden November 28, 2024 10:50

adiholden reviewed Nov 28, 2024

View reviewed changes

Merge branch 'main' into chakaz/huge-values-migration-3

496c4a7

shahar added 10 commits December 9, 2024 09:03

WIP WIP WIP

0aaaabc

Merge branch 'main' into chakaz/huge-values-migration-3

be98628

Merge branch 'main' into chakaz/huge-values-migration-3

1e5d628

Merge branch 'main' into chakaz/huge-values-migration-3

9ad4ea1

huge_value_target

30c58fd

half tests pass

b323464

Merge branch 'main' into chakaz/huge-values-migration-3

efc25dc

fix tests!

f33dfb1

cleanup:

920d2cf

remove comment

f7265d5

chakaz requested a review from adiholden December 22, 2024 13:36

adiholden reviewed Dec 23, 2024

View reviewed changes

blocking counter

fbd3550

chakaz mentioned this pull request Dec 23, 2024

Add cluster migration test in cache mode #4354

Open

shahar added 3 commits December 24, 2024 12:28

test not working yet

ecf70c7

HUGE VALUES TEST!

2193977

Merge branch 'main' into chakaz/huge-values-migration-3

785bcc5

shahar added 2 commits December 26, 2024 11:32

fix tests

9b3940f

mirror consistency

add8ebb

chakaz requested a review from adiholden December 26, 2024 11:39

adiholden reviewed Dec 26, 2024

View reviewed changes

PR comments

e2d795d

chakaz requested a review from adiholden January 2, 2025 08:19

chunk size

5493cbf

adiholden reviewed Jan 2, 2025

View reviewed changes

shahar added 3 commits January 2, 2025 13:44

Merge branch 'main' into chakaz/huge-values-migration-3

0b07108

compare results

2ccc30f

peak vs before

c22e985

chakaz requested a review from adiholden January 2, 2025 13:20

adiholden reviewed Jan 2, 2025

View reviewed changes

shahar added 3 commits January 5, 2025 10:27

100 keys

d57440f

opt only

71acec6

sleep

7ea8adc

chakaz requested a review from adiholden January 5, 2025 13:32

adiholden approved these changes Jan 5, 2025

View reviewed changes

chakaz enabled auto-merge (squash) January 5, 2025 13:38

chakaz merged commit 7860a16 into main Jan 5, 2025
9 checks passed

chakaz deleted the chakaz/huge-values-migration-3 branch January 5, 2025 14:28


		insert_task = asyncio.create_task(insert_data(instances[0].cluster_client()))

		async def get_rss(client, field):

feat: Yield inside huge values migration serialization #4197

feat: Yield inside huge values migration serialization #4197

Uh oh!

Conversation

chakaz commented Nov 26, 2024

Uh oh!

chakaz commented Nov 26, 2024

Uh oh!

kostasrim commented Nov 26, 2024

Uh oh!

chakaz commented Nov 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chakaz commented Dec 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adiholden Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chakaz Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chakaz Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

adiholden Dec 23, 2024 •

edited

Loading

chakaz Dec 23, 2024 •

edited

Loading

chakaz Jan 2, 2025 •

edited

Loading