[CORE-7743] `cluster`: tiered storage topic property update fixes #23545

WillemKauf · 2024-09-27T19:32:15Z

Relevant JIRA ticket: https://redpandadata.atlassian.net/browse/CORE-7743

Relevant historical threads: #9191, #23220 (comment), #5846.

This PR fixes some bugs that have been present in how redpanda handles AlterConfig and IncrementalAlterConfig API calls and the tiered storage topic properties redpanda.remote.read, redpanda.remote.write.

These bugs are well described in the above links, but the TL;DR is that a command like rpk topic alter-config t --set redpanda.remote.write=true --set redpanda.remote.read=false cannot properly describe both the addition of write permissions and the removal of read permissions due to the binary implementation of shadow_indexing_mode.

The topic_property layer is left untouched, but the incremental_topic_updates has had its property_update<shadow_indexing_mode> field deprecated (privatized in the struct, explicit getters/setters must be used to access it) and two new fields property_update<bool> remote_read/write added.

These fields will be used for topic property updates from now on, for clusters past v24.3 with the feature flag shadow_indexing_split_topic_property_update active. Unfortunately, we must keep the shadow_indexing related updates in the code to maintain backwards compatibility with older redpanda versions.

Another important change to note is that rpk topic alter-config t --delete redpanda.remote.write --delete redpanda.remote.read now behaves as expected, in resetting those topic level permissions to the cluster default. DescribeConfigs requests will also now accurately print the topic level permissions (these bugs were being chased in closed PR #23220. This current PR should be considered a replacement for that one).

Closes #5846, closes #4499, closes #11749

Backports Required

Release Notes

Bug Fixes

Fixes a bug in which AlterConfig and IncrementalAlterConfig requests modifying redpanda.remote.read and redpanda.remote.write simultaneously were not being properly respected.
Fixes a bug in which the responses to DescribeConfigs requests would misrepresent the state of redpanda.remote.read and redpanda.remote.write.

pgellert

Thanks for picking this up, it's looking good.

Do we want to backport this all the way? I think there's no technical reason why we couldn't, but I'm wondering if this should go into a patch release.

src/v/cluster/topic_table.cc

tests/rptest/tests/offset_for_leader_epoch_archival_test.py

src/v/migrations/cloud_storage_config.cc

src/go/rpk/pkg/cli/topic/config.go

src/v/cluster/topic_table.cc

src/v/cluster/tests/cluster_tests.cc

src/v/kafka/server/tests/alter_config_test.cc

tests/rptest/tests/alter_topic_configuration_test.py

WillemKauf · 2024-10-02T18:10:11Z

Force push to:

Add the new feature flag shadow_indexing_split_topic_property_update, which is used at the kafka layer in order to determine which code-path should be followed for the shadow_indexing topic property update- the old, bug prone code path, or the updated one.

src/v/cluster/types.h

src/go/rpk/pkg/cli/topic/config.go

WillemKauf · 2024-10-03T15:57:02Z

Force push to:

Make restart_node_and_await_stable_leader() in alter_topic_configuration_test.py deterministic by checking the cluster health overview in order to assert on controller stabilization.

pgellert

Some final minor things left but otherwise it looks good to me.

src/v/cluster/topic_table.cc

tests/rptest/tests/alter_topic_configuration_test.py

pgellert · 2024-10-03T16:03:31Z

src/go/rpk/pkg/cli/topic/config.go

@@ -76,6 +76,8 @@ valid, but does not apply it.
 			// at the same time, so we issue first the set request for write,
 			// then the rest of the requests.
 			// See https://github.com/redpanda-data/redpanda/issues/9191
+			// TODO: Remove this once v24.2 is EOL.
+			// See https://github.com/redpanda-data/redpanda/pull/23545.
 			_, isRRR := setKVs["redpanda.remote.read"]


We can't backport this PR because of the feature gate, but if we wanted to fix these bugs in older versions we could implement this request-splitting logic that rpk is doing inside redpanda in 24.1/24.2/23.3. It might be worth asking product if we want to do that.

To be clear, I think we should get this PR in because this seems like the right long-term fix, but since these bugs are important we might want to release a backportable fix for them as well.

We can't backport this PR because of the feature gate, but if we wanted to fix these bugs in older versions we could implement this request-splitting logic that rpk is doing inside redpanda in 24.1/24.2/23.3. It might be worth asking product if we want to do that.

I think it's a good idea, so that it can work with other tools that support setting arbitrary config.

Agreed, these are pretty important bugs, it would be good to fix them in previous versions.

This should be done in a future PR, but for now my focus is on v24.3.

WillemKauf · 2024-10-03T20:47:20Z

Force push to:

Deflake race-y test in alter_topic_configuration_test.py - trim_logs() does not appear to be blocking, and was causing unreliable test outcomes when checking node logs for the deprecated log line.

WillemKauf · 2024-10-04T13:09:34Z

Force push to:

Rebase to upstream dev (sorry, forgot to apply --keep-base) in order to add required license to alter_topic_configuration_test.py here.

To feature guard the new code path taken for the `shadow_indexing` topic property, add it to the feature table. Clusters past version `v24.3` will follow the new, bug free code path in the `topic_table` for their `AlterConfig`/`IncrementalAlterConfig` requests, while older clusters will follow the now deprecated code-path. Also, update `partition_properties_stm` to use the new `release_version` enum.

In order to deprecate and better control call sites that use this field, move it to a `private` field and add get/set functions. Also modify use-sites and constructors that can no longer treat `incremental_topic_updates` as an aggregate for initialization.

To deal with the bugs that have come about due to use of the `shadow_indexing` field in the `AlterConfig`/`IncrementalAlterConfig` APIs, split the update field into two seperate `property_update<bool>`s, `remote_read` and `remote_write`. This fixes any bugs that were present and described in the JIRA thread here: https://redpandadata.atlassian.net/browse/CORE-7743 For legacy reasons, the old `incremental_update` function for `shadow_indexing` must still be available, in case of an older broker sending topic property updates to an updated broker. However, updated clusters with the new feature flag `shadow_indexing_split_topic_property_update` active will use the `remote_read` and `remote_write` fields for their `incremental_update`s.

Adds `test_shadow_indexing_alter_configs` and `test_shadow_indexing_incremental_alter_configs`.

Also add some `node` parameters to `KCL` related functions in order to allow the issuing of Kafka requests to specific brokers.

Kafka ignores case when setting a boolean property: https://github.com/apache/kafka/blob/master/clients/src/main/... ...java/org/apache/kafka/common/config/ConfigDef.java#L690-L710 Realign ourselves with Kafka by transforming the string value to lowercase before parsing it in `parse_and_set_bool()`.

These workarounds should be removed in a future version of `redpanda` (once `v24.2` is EOL), now that bug fixes are in place in the core. They are left here for now to ensure backwards compatibility between an upgraded version of `rpk` and an un-upgraded `redpanda` cluster.

WillemKauf · 2024-10-04T14:36:00Z

Force push to:

Resolve merge conflicts with upstream dev
Use new release_version enum in feature_table

pgellert

lgtm, but let's get someone else to review this as well

Lazin · 2024-10-07T18:12:05Z

src/v/kafka/server/handlers/alter_configs.cc

@@ -152,8 +167,17 @@ create_topic_properties_update(alter_configs_resource& resource) {
                continue;
            }
            if (cfg.name == topic_property_remote_read) {
-                // Legacy update for shadow indexing field
-                {
+                if (shadow_indexing_split_update) {


nit: can we get rid of 'shadow indexing' in the name? maybe just 'tiered-storage'?
the SI comes from the very first implementation which was called 'shadow indexing' and since then it was renamed everywhere. I don't mind if the name will stay though. It will be easier to connect it with the old flag.

also the 'split' in the name focuses on the low level detail, it will probably be confusing for some people

Given that the topic property is still named shadow_indexing, I think it would be best if this rename was applied there as well in a future PR. What do you think?

Lazin

LGTM, one minor nit regarding the name

WillemKauf requested review from Lazin and pgellert September 27, 2024 19:32

WillemKauf requested review from twmb, r-vasquez, gene-redpanda and Deflaimun as code owners September 27, 2024 19:32

github-actions bot added area/rpk area/redpanda labels Sep 27, 2024

WillemKauf mentioned this pull request Sep 27, 2024

kafka: fix alter-config --delete for some "sticky" properties #23220

Closed

7 tasks

WillemKauf force-pushed the si_alter_config_fix branch from 136f9b6 to 941ca96 Compare September 27, 2024 19:40

This comment was marked as outdated.

Sign in to view

WillemKauf force-pushed the si_alter_config_fix branch 2 times, most recently from 980b9d1 to b33245c Compare September 28, 2024 00:25

This comment was marked as outdated.

Sign in to view

WillemKauf force-pushed the si_alter_config_fix branch from b33245c to 21ce425 Compare September 28, 2024 04:15

WillemKauf changed the title ~~[CORE-7743] cluster: tiered storage topic property fixes~~ [CORE-7743] cluster: tiered storage topic update fixes Sep 28, 2024

WillemKauf changed the title ~~[CORE-7743] cluster: tiered storage topic update fixes~~ [CORE-7743] cluster: tiered storage topic property update fixes Sep 28, 2024

pgellert reviewed Sep 30, 2024

View reviewed changes

WillemKauf force-pushed the si_alter_config_fix branch from 21ce425 to ae4d02c Compare September 30, 2024 19:43

WillemKauf requested a review from pgellert September 30, 2024 20:14

WillemKauf force-pushed the si_alter_config_fix branch from 91d67b6 to 9d34ec8 Compare October 1, 2024 01:24

pgellert reviewed Oct 1, 2024

View reviewed changes

WillemKauf force-pushed the si_alter_config_fix branch from 9d34ec8 to 32eb837 Compare October 1, 2024 16:12

dotnwat requested review from bharathv, mmaslankaprv and BenPope October 1, 2024 17:21

WillemKauf force-pushed the si_alter_config_fix branch from 32eb837 to 0faad94 Compare October 1, 2024 19:51

michael-redpanda requested a review from pgellert October 2, 2024 13:56

WillemKauf force-pushed the si_alter_config_fix branch 2 times, most recently from 1ecaefc to 7957f50 Compare October 2, 2024 18:08

mmaslankaprv reviewed Oct 3, 2024

View reviewed changes

src/v/cluster/types.h Outdated Show resolved Hide resolved

WillemKauf force-pushed the si_alter_config_fix branch from 7957f50 to c50d1fd Compare October 3, 2024 14:06

twmb reviewed Oct 3, 2024

View reviewed changes

src/go/rpk/pkg/cli/topic/config.go Show resolved Hide resolved

WillemKauf force-pushed the si_alter_config_fix branch 2 times, most recently from 9d107ae to 141c43b Compare October 3, 2024 15:55

pgellert reviewed Oct 3, 2024

View reviewed changes

WillemKauf force-pushed the si_alter_config_fix branch 2 times, most recently from 84283da to feea5e4 Compare October 3, 2024 17:33

WillemKauf requested a review from pgellert October 3, 2024 17:37

WillemKauf force-pushed the si_alter_config_fix branch from feea5e4 to cfd0f59 Compare October 3, 2024 20:45

WillemKauf force-pushed the si_alter_config_fix branch 2 times, most recently from f6e2b1c to be4a8c2 Compare October 4, 2024 13:07

WillemKauf added 7 commits October 4, 2024 09:58

kafka: add shadow indexing tests to alter_config_test.cc

ad801dd

Adds `test_shadow_indexing_alter_configs` and `test_shadow_indexing_incremental_alter_configs`.

rptest: add AlterConfigMixedNodeTest

a7f2fa0

Also add some `node` parameters to `KCL` related functions in order to allow the issuing of Kafka requests to specific brokers.

WillemKauf force-pushed the si_alter_config_fix branch from be4a8c2 to 540ae13 Compare October 4, 2024 14:35

pgellert approved these changes Oct 4, 2024

View reviewed changes

WillemKauf requested a review from dotnwat October 7, 2024 15:54

Lazin reviewed Oct 7, 2024

View reviewed changes

Lazin approved these changes Oct 7, 2024

View reviewed changes

WillemKauf merged commit 58f68dd into redpanda-data:dev Oct 7, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-7743] `cluster`: tiered storage topic property update fixes #23545

[CORE-7743] `cluster`: tiered storage topic property update fixes #23545

WillemKauf commented Sep 27, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

pgellert left a comment

WillemKauf commented Oct 2, 2024

WillemKauf commented Oct 3, 2024

pgellert left a comment

pgellert Oct 3, 2024

BenPope Oct 3, 2024

WillemKauf Oct 3, 2024

WillemKauf commented Oct 3, 2024

WillemKauf commented Oct 4, 2024

WillemKauf commented Oct 4, 2024 •

edited

Loading

pgellert left a comment

Lazin Oct 7, 2024

Lazin Oct 7, 2024

WillemKauf Oct 7, 2024 •

edited

Loading

Lazin left a comment

[CORE-7743] cluster: tiered storage topic property update fixes #23545

[CORE-7743] cluster: tiered storage topic property update fixes #23545

Conversation

WillemKauf commented Sep 27, 2024 • edited Loading

Backports Required

Release Notes

Bug Fixes

This comment was marked as outdated.

This comment was marked as outdated.

pgellert left a comment

Choose a reason for hiding this comment

WillemKauf commented Oct 2, 2024

WillemKauf commented Oct 3, 2024

pgellert left a comment

Choose a reason for hiding this comment

pgellert Oct 3, 2024

Choose a reason for hiding this comment

BenPope Oct 3, 2024

Choose a reason for hiding this comment

WillemKauf Oct 3, 2024

Choose a reason for hiding this comment

WillemKauf commented Oct 3, 2024

WillemKauf commented Oct 4, 2024

WillemKauf commented Oct 4, 2024 • edited Loading

pgellert left a comment

Choose a reason for hiding this comment

Lazin Oct 7, 2024

Choose a reason for hiding this comment

Lazin Oct 7, 2024

Choose a reason for hiding this comment

WillemKauf Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Lazin left a comment

Choose a reason for hiding this comment

[CORE-7743] `cluster`: tiered storage topic property update fixes #23545

[CORE-7743] `cluster`: tiered storage topic property update fixes #23545

WillemKauf commented Sep 27, 2024 •

edited

Loading

WillemKauf commented Oct 4, 2024 •

edited

Loading

WillemKauf Oct 7, 2024 •

edited

Loading