very first version of tenant migration from one pageserver to another via remote storage #995

LizardWizzard · 2021-12-14T13:59:12Z

Currently this contains python test which makes some assumptions and uses quite hacky setup with second pageserver without zenith cli support

Resolves #896
Resolves #897
Resolves #898
Resolves #900

pageserver/src/http/routes.rs

SomeoneToIgnore

Looks nice, actually, despite all the WIP stuff.

I'm a bit concerned by the sleep(n) things, but this is not a problem right now.
IMO later, we could call some http method periodically and limit the number of calls instead.

pageserver/src/http/routes.rs

test_runner/batch_others/test_tenant_relocation.py

zenith_utils/src/zid.rs

SomeoneToIgnore · 2021-12-14T15:51:26Z

test_runner/batch_others/test_tenant_relocation.py

+
+    new_pageserver_config_file_path = new_pageserver_dir / 'pageserver.toml'
+
+    # add remote storage mock to pageserver config


Maybe worth a todo, IMO, we would like to enable remote storage for every Python IT tests with one env var/command, so the file appending way is rather crude and temporary.

Yeah, I think when your config changes land it will be convenient to support multiple pageservers directly from zenith cli

LizardWizzard · 2021-12-14T16:28:01Z

@SomeoneToIgnore Thanks for the review!

IMO later, we could call some http method periodically and limit the number of calls instead.

Yes, I fully agree with that, we can have some utility in our python tests to wait for certain predicate to become true with a timeout on a waiting period. When all the bits will be in place I'll remove these sleep calls

SomeoneToIgnore · 2022-01-01T15:23:53Z

#1079 helps with hacks around the remote storage

LizardWizzard · 2022-01-13T12:58:08Z

I think in this form this patch can be accepted and TODO's are closed with follow ups. I used links in them to corresponding issues.

@arssher @lubennikovaav Please take a look at safekeeper changes, this mostly touches callmemaybe stuff

There is also a small document in docs/ so if you have any questions I'll be glad to cover them

SomeoneToIgnore

I have not looked thoroughly at the Python code, but LGTM overall

docs/pageserver-tenant-migration.md

pageserver/src/http/routes.rs

pageserver/src/remote_storage/local_fs.rs

test_runner/fixtures/zenith_fixtures.py

walkeeper/src/callmemaybe.rs

walkeeper/src/handler.rs

LizardWizzard · 2022-01-17T12:28:40Z

I've rebased this on top of shutdown improvements patch and added margins for some exact checks to reduce possible flakiness

SomeoneToIgnore

If I compile walkeeper package alone, I get an error:

❯ cargo check --package walkeeper --all-targets
    Checking walkeeper v0.1.0 (/Users/someonetoignore/Work/zenith/zenith/walkeeper)
error[E0433]: failed to resolve: could not find `select` in `tokio`
   --> walkeeper/src/callmemaybe.rs:220:16
    |
220 |         tokio::select! {
    |                ^^^^^^ could not find `select` in `tokio`

feels like tokio is missing a feature there?

Otherwise, the same "LGTM but not sure about Python magic code" impression.

walkeeper/src/send_wal.rs

LizardWizzard · 2022-01-18T10:49:03Z

Thanks for the review @SomeoneToIgnore!

If I compile walkeeper package alone, I get an error:

It seems that the error appears on main too. I wonder should we pin somewhere (in zeinth_utils?) tokio dependency with all the needed features. This should simplify things a bit, and AFAIK cargo uses union of all features needed by crate/it's dependencies so this shouldn't affect compile times, but worth checking

docs/pageserver-tenant-migration.md

lubennikovaav · 2022-01-18T11:34:01Z

docs/pageserver-tenant-migration.md

+
+### Implementation details
+
+Now safekeeper needs to track which pageserver it is replicating to. This introduces complications into replication code and callmemaybe subscription state handling.


Could you elaborate on the "complications in the replication code"?

I wonder if we need special treatment of pageserver's feedback in get_replicas_state() on safekeeper, given that two streams can exist for the same timeline?

By complications I mean additional handling of pageserver connection string and the requirement to track which pageserver is actually a primary one, to avoid reconnections to it (which is not currently implemented but is present in the epic in the follow ups section)

given that two streams can exist for the same timeline?

At a first glance everything looks OK because we keep a Vec of replicas for particular timeline. Do you have some invariants in mind that should be checked?

test_runner/batch_others/test_tenant_relocation.py

LizardWizzard · 2022-01-18T13:12:55Z

walkeeper/src/send_wal.rs

-                        let timelineid = spg.timeline.get().timelineid;
+                        // TODO this expect is ugly, will it be better with node id?
+                        //   can we guarantee that this code is not executed during walproposer recovery?
+                        let pageserver_connstr = pageserver_connstr


@lubennikovaav what do you think about this expect call? It was not triggered in tests so I assume this code path it not followed in walproposer recovery but can we guarantee somehow that recovery won't touch this?

If walproposer is in recovery stop_lsn is set and this is checked in the code above this line.

kelvich · 2022-01-20T07:07:02Z

docs/pageserver-tenant-migration.md

+
+### Implementation details
+
+Now safekeeper needs to track which pageserver it is replicating to. This introduces complications into replication code:


BTW do you think we can address both of that with neondatabase/rfcs#16 ?

For the first one, if callmemaybe goes away there is no need to weak it for these changes.

The second one can go away too if safekeeper doesnt initiate connections (and doesnt reconnect) to pageserver. As you've answered this is exactly how it is proposed in the RFC, so yeah, both problems are absent with this RFC being implemented

This patch includes attach/detach http endpoints in pageservers. Some changes in callmemaybe handling inside safekeeper and an integrational test to check migration with and without load. There are still some rough edges that will be addressed in follow up patches

LizardWizzard requested a review from SomeoneToIgnore December 14, 2021 13:59

LizardWizzard force-pushed the tenant-rebalancing-v0 branch from 9c430b5 to 5b5ca4f Compare December 14, 2021 14:01

SomeoneToIgnore reviewed Dec 14, 2021

View reviewed changes

pageserver/src/http/routes.rs Outdated Show resolved Hide resolved

SomeoneToIgnore reviewed Dec 14, 2021

View reviewed changes

LizardWizzard mentioned this pull request Dec 14, 2021

Refactor on demand nature of Repository::get_timeline #997

Closed

SomeoneToIgnore mentioned this pull request Jan 1, 2022

Add basic remote storage test #1079

Merged

LizardWizzard force-pushed the tenant-rebalancing-v0 branch 4 times, most recently from ac3c7d2 to 189924e Compare January 13, 2022 08:57

LizardWizzard mentioned this pull request Jan 13, 2022

Epic: tenant relocation between pageserver nodes #886

Closed

1 task

LizardWizzard marked this pull request as ready for review January 13, 2022 12:48

LizardWizzard requested review from SomeoneToIgnore, arssher and lubennikovaav January 13, 2022 12:58

SomeoneToIgnore approved these changes Jan 13, 2022

View reviewed changes

LizardWizzard force-pushed the tenant-rebalancing-v0 branch 5 times, most recently from b2c1ca3 to b10dc00 Compare January 17, 2022 11:29

stepashka linked an issue Jan 17, 2022 that may be closed by this pull request

Epic: tenant relocation between pageserver nodes #886

Closed

1 task

LizardWizzard force-pushed the tenant-rebalancing-v0 branch from 443194c to 1d60d95 Compare January 17, 2022 20:19

SomeoneToIgnore reviewed Jan 17, 2022

View reviewed changes

walkeeper/src/send_wal.rs Outdated Show resolved Hide resolved

walkeeper/src/send_wal.rs Outdated Show resolved Hide resolved

lubennikovaav reviewed Jan 18, 2022

View reviewed changes

docs/pageserver-tenant-migration.md Outdated Show resolved Hide resolved

lubennikovaav reviewed Jan 18, 2022

View reviewed changes

test_runner/batch_others/test_tenant_relocation.py Show resolved Hide resolved

lubennikovaav reviewed Jan 18, 2022

View reviewed changes

test_runner/batch_others/test_tenant_relocation.py Outdated Show resolved Hide resolved

lubennikovaav approved these changes Jan 18, 2022

View reviewed changes

LizardWizzard commented Jan 18, 2022

View reviewed changes

LizardWizzard force-pushed the tenant-rebalancing-v0 branch from 1d60d95 to 3b8494c Compare January 18, 2022 15:11

kelvich reviewed Jan 20, 2022

View reviewed changes

SomeoneToIgnore mentioned this pull request Jan 21, 2022

Ensure every submodule compiles on its own #1160

Merged

LizardWizzard mentioned this pull request Jan 21, 2022

refactoring of timeline memory state management in layered repo #1163

Merged

LizardWizzard force-pushed the tenant-rebalancing-v0 branch from 3b8494c to 17300a0 Compare January 24, 2022 09:54

LizardWizzard added 3 commits January 24, 2022 13:00

reduce flakiness

26ff323

walkeeper: use named type as a key in callmemaybe subscriptions hashmap

ae9bc4a

LizardWizzard force-pushed the tenant-rebalancing-v0 branch from 17300a0 to ae9bc4a Compare January 24, 2022 10:01

LizardWizzard merged commit 458bc0c into main Jan 24, 2022

LizardWizzard deleted the tenant-rebalancing-v0 branch January 24, 2022 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

very first version of tenant migration from one pageserver to another via remote storage #995

very first version of tenant migration from one pageserver to another via remote storage #995

LizardWizzard commented Dec 14, 2021 •

edited

Loading

SomeoneToIgnore left a comment

SomeoneToIgnore Dec 14, 2021

LizardWizzard Dec 14, 2021

LizardWizzard commented Dec 14, 2021

SomeoneToIgnore commented Jan 1, 2022

LizardWizzard commented Jan 13, 2022 •

edited

Loading

SomeoneToIgnore left a comment

LizardWizzard commented Jan 17, 2022

SomeoneToIgnore left a comment

LizardWizzard commented Jan 18, 2022

lubennikovaav Jan 18, 2022

LizardWizzard Jan 18, 2022

LizardWizzard Jan 18, 2022

LizardWizzard Jan 18, 2022

lubennikovaav Jan 19, 2022

kelvich Jan 20, 2022

LizardWizzard Jan 20, 2022


		new_pageserver_config_file_path = new_pageserver_dir / 'pageserver.toml'

		# add remote storage mock to pageserver config


		### Implementation details

		Now safekeeper needs to track which pageserver it is replicating to. This introduces complications into replication code and callmemaybe subscription state handling.

very first version of tenant migration from one pageserver to another via remote storage #995

very first version of tenant migration from one pageserver to another via remote storage #995

Conversation

LizardWizzard commented Dec 14, 2021 • edited Loading

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LizardWizzard commented Dec 14, 2021

SomeoneToIgnore commented Jan 1, 2022

LizardWizzard commented Jan 13, 2022 • edited Loading

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

LizardWizzard commented Jan 17, 2022

SomeoneToIgnore left a comment

Choose a reason for hiding this comment

LizardWizzard commented Jan 18, 2022

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

Choose a reason for hiding this comment

LizardWizzard commented Dec 14, 2021 •

edited

Loading

LizardWizzard commented Jan 13, 2022 •

edited

Loading