rpc/raft/cluster: start removing ADL encoding for RPCs #9121

jcsp · 2023-02-26T17:43:12Z

Since Redpanda 23.1, ADL encoded RPCs are no longer used. We no longer need to carry these deprecated serialization paths to enable rolling upgrades.

This PR doesn't remove all cases, but targets major ones + those that I have other PRs touching (disk usage in health messages for #6549 and raft messages for #6365), so that I can remove any ADL parts of those PRs.

Backports Required

UX Changes

None

Release Notes

none

emaxerrno · 2023-02-27T05:35:32Z

this is neat. it lasted a good while.

jcsp · 2023-02-28T12:03:02Z

This needs 23.1 to be released, so that upgrade tests that target the previous feature release will pick up 23.1 instead of 22.3

jcsp · 2023-03-09T13:54:45Z

Saw failures in https://buildkite.com/redpanda/redpanda/builds/24695#0186c5d2-5cfc-4092-bf76-a0e67a1fece9:

ScalingUpTest.test_on_demand_rebalancing -- no open ticket for this, but it's a a timeout in "wait_until_ready" in a test running in debug mode that doesn't do upgrades, so seems very unlikely to be related to this PR.
PartitionBalancerTest.test_maintenance_mode (CI Failure (TimeoutError: failed to wait until status condition) in PartitionBalancerTest.test_fuzz_admin_ops #9315)

jcsp · 2023-03-09T13:54:51Z

/ci-repeat 4

jcsp · 2023-03-09T18:41:04Z

On the repeats:

Two instances of CI Failure (TimeoutError: failed to wait until status condition) in PartitionBalancerTest.test_fuzz_admin_ops #9315
One failure of feature_barrier_test.cc:test_barrier_node_isolated that seems likely to be a result of tests: add a task queue flush helper + use in unit tests #8464 -- opened test_utils: sleep in debug mode in flush_tasks #9352

dotnwat

well, that's awesome!

dotnwat · 2023-03-10T03:09:13Z

src/v/rpc/transport.h

              transport_version effective_version) mutable {
-          /*
-           * enforce the rule that a transport configured as v0 behaves like
-           * a v0 client transport and sends v0 messages.
-           */
          vassert(
-            version != transport_version::v0
-              || effective_version == transport_version::v0,
+            version >= transport_version::min_supported,


the version and effective_version should now be the same, right? i'm also thinking that change may be coming up in a later commit.

That will become true when we strip out the adl-only RPCs, so that encode_for_version is never returning v0. Those are just the lingering coproc RPCs -- I think we can do that in a later PR.

dotnwat · 2023-03-10T04:50:21Z

src/v/rpc/parse_utils.h

+        vassert(version >= transport_version::v2, "Can't encode serde <= v2");
+        return ss::do_with(std::move(msg), [&out, version](T& msg) {
+            return serde::write_async(out, std::move(msg)).then([version] {
+                return version;


ah i think this is it here where the caller gets effective version to input version

dotnwat · 2023-03-10T04:54:10Z

src/v/raft/tests/type_serialization_tests.cc

-    auto d = serialize_roundtrip_rpc(std::move(metadata));
+    auto d = serialize_roundtrip_rpc_adl(std::move(metadata));


is this an example of one of the rpc messages that remain as adl (only?) ?

The snapshot_metadata is used in the on-disk format. I'll check if I'm using this method anywhere else and if not take the misleading 'rpc' out of the name

oh right yeh that makes sense

dotnwat · 2023-03-10T04:56:58Z

tests/rptest/tests/redpanda_test.py

+
+        return versions
+
+    def upgrade_through_versions(self, versions):


nice change promoting this up to generic test infra

Since Redpanda 23.2, all RPCs are serde encoded, and all versions we may upgrade from also speak serde.

All the versions we may do a rolling upgrade from now support v2, and as of Redpanda 23.1 they will also speak v2 by default on all new connections, so there are no circumstances where we need to speak RPC v1 during a rolling upgrade.

Tests that relate to version upgrade are removed, as we no longer support a range of versions. Tests that used ADL now use serde. The mock request/response classes that specified a particular encoding are gone in favor of using serde everywhere. The encode_for_version method now needs to pass through versions for v2 transports because we have to use a v2 transport for testing what happens when a client uses a too-new version.

This test covers the hop from 22.2 to 22.3 specifically: we do not need to carry the test indefinitely. We could retain this test by updating it to step through all intermediate versions, but it's not worth it: when we upgrade to latest redpanda, the system will have passed through the already-tested 22.3 code that handles the upgrade.

This <= should have been <, to avoid skipping through versions to quickly.

There is more than one test that should be stepping through all verisons in a series.

...to not skip straight from 22.2.x to HEAD, which will fail now that HEAD is truly incompatible with such old versions, without ADL RPC. The test now does 22.2.8->23.1.x, then downgrades, then goes all the way to HEAD via all intervening feature versions.

...to not try and skip straight from 22.1.x to HEAD.

...it is no longer used for any cases involving RPC, just structures that we serialize to disk.

jcsp · 2023-03-10T14:56:06Z

Failures are all CloudStorageUsageTest -- this PR should be good to go.

NyaliaLui · 2023-03-28T16:03:13Z

Do these changes need backport into v23.1.x?
@jcsp

jcsp · 2023-03-29T17:56:36Z

Do these changes need backport into v23.1.x?

Definitely not, 23.1.x needs to remain ADL compatible to upgrade cleanly from 22.3.x

dotnwat · 2023-03-29T18:17:02Z

Since Redpanda 23.1, ADL encoded RPCs are no longer used

I did a double take on this from the cover letter too.

redpanda-data#9121 Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

Seems to have been partially cleaned up in redpanda-data#9121. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

Seems to have been partially cleaned up in redpanda-data#9121. Signed-off-by: Noah Watkins <noahwatkins@gmail.com> (cherry picked from commit cfd6325)

jcsp added the area/redpanda label Feb 26, 2023

jcsp force-pushed the goodbye-adl-rpc branch 2 times, most recently from 31400dc to 35f4cb8 Compare February 27, 2023 13:31

jcsp changed the title ~~Goodbye adl rpc~~ rpc/raft/cluster: start removing ADL encoding for RPCs Feb 27, 2023

jcsp added kind/tech-debt area/replication and removed area/redpanda labels Feb 27, 2023

jcsp requested a review from mmaslankaprv February 28, 2023 13:41

jcsp force-pushed the goodbye-adl-rpc branch from 35f4cb8 to 47d97ae Compare March 8, 2023 10:24

github-actions bot added the area/redpanda label Mar 8, 2023

jcsp force-pushed the goodbye-adl-rpc branch from 47d97ae to 2fe9b19 Compare March 8, 2023 10:29

jcsp marked this pull request as ready for review March 8, 2023 10:29

jcsp force-pushed the goodbye-adl-rpc branch from b7d2b59 to 5c8ea2a Compare March 9, 2023 09:06

jcsp requested a review from dotnwat March 9, 2023 19:10

dotnwat reviewed Mar 10, 2023

View reviewed changes

jcsp added 10 commits March 10, 2023 10:23

raft: retire ADL RPC encoding code

c4805c8

Since Redpanda 23.2, all RPCs are serde encoded, and all versions we may upgrade from also speak serde.

rpc: retire all use of transport_version::v1

f4a85ec

All the versions we may do a rolling upgrade from now support v2, and as of Redpanda 23.1 they will also speak v2 by default on all new connections, so there are no circumstances where we need to speak RPC v1 during a rolling upgrade.

cluster: retire ADL RPC encoding for config, feature RPCs

9fb2b9b

cluster: retire ADL encoding for health+metadata RPCs

348f688

tests: fix version checks in UpgradeBackToBackTest

f290853

This <= should have been <, to avoid skipping through versions to quickly.

tests: refactor multi-version upgrade into helper

2a37ba5

There is more than one test that should be stepping through all verisons in a series.

tests: update PartitionMovementTest

7ca9c54

...to not try and skip straight from 22.1.x to HEAD.

test_utils: rename serialize_roundtrip_rpc_adl

f8b8e1f

...it is no longer used for any cases involving RPC, just structures that we serialize to disk.

jcsp force-pushed the goodbye-adl-rpc branch from 5c8ea2a to f8b8e1f Compare March 10, 2023 10:23

jcsp requested a review from dotnwat March 10, 2023 14:56

dotnwat approved these changes Mar 10, 2023

View reviewed changes

dotnwat merged commit da36c91 into redpanda-data:dev Mar 10, 2023

graphcareful mentioned this pull request Mar 28, 2023

[v23.1.x] Add cloud_storage statistics to usage reporting #9670

Merged

jcsp deleted the goodbye-adl-rpc branch March 29, 2023 17:56

dotnwat added a commit to dotnwat/redpanda that referenced this pull request Aug 11, 2023

remove unused constants from adl time

6587c69

redpanda-data#9121 Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

dotnwat added a commit to dotnwat/redpanda that referenced this pull request Aug 11, 2023

cluster: remove unused constants from adl time

7d3c332

Seems to have been partially cleaned up in redpanda-data#9121. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

dotnwat added a commit to dotnwat/redpanda that referenced this pull request Aug 14, 2023

cluster: remove unused constants from adl time

cfd6325

Seems to have been partially cleaned up in redpanda-data#9121. Signed-off-by: Noah Watkins <noahwatkins@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc/raft/cluster: start removing ADL encoding for RPCs #9121

rpc/raft/cluster: start removing ADL encoding for RPCs #9121

jcsp commented Feb 26, 2023 •

edited

Loading

emaxerrno commented Feb 27, 2023

jcsp commented Feb 28, 2023

jcsp commented Mar 9, 2023 •

edited

Loading

jcsp commented Mar 9, 2023

jcsp commented Mar 9, 2023 •

edited

Loading

dotnwat left a comment

dotnwat Mar 10, 2023

jcsp Mar 10, 2023

dotnwat Mar 10, 2023

dotnwat Mar 10, 2023

jcsp Mar 10, 2023

dotnwat Mar 10, 2023

dotnwat Mar 10, 2023

jcsp commented Mar 10, 2023

NyaliaLui commented Mar 28, 2023

jcsp commented Mar 29, 2023

dotnwat commented Mar 29, 2023

		auto d = serialize_roundtrip_rpc(std::move(metadata));
		auto d = serialize_roundtrip_rpc_adl(std::move(metadata));


		return versions

		def upgrade_through_versions(self, versions):

rpc/raft/cluster: start removing ADL encoding for RPCs #9121

rpc/raft/cluster: start removing ADL encoding for RPCs #9121

Conversation

jcsp commented Feb 26, 2023 • edited Loading

Backports Required

UX Changes

Release Notes

emaxerrno commented Feb 27, 2023

jcsp commented Feb 28, 2023

jcsp commented Mar 9, 2023 • edited Loading

jcsp commented Mar 9, 2023

jcsp commented Mar 9, 2023 • edited Loading

dotnwat left a comment

Choose a reason for hiding this comment

dotnwat Mar 10, 2023

Choose a reason for hiding this comment

jcsp Mar 10, 2023

Choose a reason for hiding this comment

dotnwat Mar 10, 2023

Choose a reason for hiding this comment

dotnwat Mar 10, 2023

Choose a reason for hiding this comment

jcsp Mar 10, 2023

Choose a reason for hiding this comment

dotnwat Mar 10, 2023

Choose a reason for hiding this comment

dotnwat Mar 10, 2023

Choose a reason for hiding this comment

jcsp commented Mar 10, 2023

NyaliaLui commented Mar 28, 2023

jcsp commented Mar 29, 2023

dotnwat commented Mar 29, 2023

jcsp commented Feb 26, 2023 •

edited

Loading

jcsp commented Mar 9, 2023 •

edited

Loading

jcsp commented Mar 9, 2023 •

edited

Loading