allow property aliases in redpanda.yaml, fix aliases not being applied #15605

andijcr · 2023-12-14T17:22:52Z

Property aliases are not included in the ignored items when reading a node yaml config or cluster yaml config.

This pr fixes 3 distinct but related bugs:

aliases not being being applied

solved with the first commit. when searching for a property with an alias, do not skip applying the value

aliases in bootstrapping redpanda.yaml causing std::illegal_argument exceptions

with commit 4-5 include aliases in the list of names to be ignored when setting node config

~~3. properties set with aliases reverting to a previous value after a restart, under some conditions.~~
~~- with commit 6 translate aliases to the main name before applying it to the controller log. this is more a workaround than a proper fix of the underlying issue, as described in #13362~~

edit: #15725 should be a proper fix for #13362. so the third commit is no longer necessary. once that merge, this can merge on top

Fixes #15603

Backports Required

Release Notes

Bug Fixes

cluster config aliases are accepted while reading from yaml

vbotbuildovich · 2023-12-14T19:37:17Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42800#018c6996-a6a5-4fe2-82ca-24148ae86c69

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42800#018c699f-45d7-4173-bda3-29f7ba7285a5

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42800#018c6996-a6a8-4b28-ab7d-30107fa4fc68

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42800#018c699f-45db-4b52-95ca-73a6dd85f183

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42894#018c6edb-deab-4763-8cde-33a1c3641652

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42894#018c6ed5-a356-46ca-b70e-4a95a7196713

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42894#018c6ed5-a359-4db4-81e0-04b99f0cf2f3

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/42894#018c6edb-deae-4924-8859-31f360cfabbe

dotnwat · 2023-12-15T05:10:43Z

src/v/config/config_store.h

-            auto found = _properties.find(name);
-            if (found == _properties.end()) {
-                found = _aliases.find(name);
-                if (found == _aliases.end()) {
-                    if (!ignore_missing.contains(name)) {
-                        throw std::invalid_argument(
-                          fmt::format("Unknown property {}", name));
-                    }
+            auto* prop = [&]() -> base_property* {
+                auto primary = _properties.find(name);
+                if (primary != _properties.end()) {
+                    return primary->second;
                }
-            } else {
-                bool ok = false;
-                try {
-                    auto validation_err = found->second->validate(node.second);
-                    if (validation_err.has_value()) {
-                        errors[name] = fmt::format(
-                          "Validation error: {}",
-                          validation_err.value().error_message());
-                    }
-
-                    found->second->set_value(node.second);
-                    ok = true;


config_store bug about not considering aliases for bootstrap

please describe the problem that is fixed in this commit, and how it is fixed. it also looks like there is some code moved / refactored? if so, that could be a separate commit to make it easier to understand the change.

done, splitted it apart. the first commit has the first fix, the following are some cosmetic change to bring it up smoothly at this stage

dotnwat

what test(s) should we add?

andijcr · 2023-12-15T18:30:26Z

what test(s) should we add?

e132fa0

and another where we trigger the invalid argument check.

in cluster_config_test.py

dotnwat · 2023-12-15T18:35:30Z

what test(s) should we add?
e132fa0
and another where we trigger the invalid argument check.

As far as I can tell the issue linked to in this PR doesn't mention any failing tests. So maybe it's changing / fixing some behavior? If so, that would qualify for a test or update to a test?

andrwng

The changes look good, but will wait on a test to approve. It'd be great to at least cover the behavior in the issue, where delete_retention_ms is set in the config

src/v/config/config_store.h

before this commit, finding the name in the alias table would not throw the std::invalid_argument exception. but would not set the property.

this is a non-functional change

non functional change, the optional was just used inside a code block

property_aliases() mirrors property_names() and both get merged in property_names_and_aliases(), to be used instead of property_names when filtering/scanning over user input

this allows to include prop aliases when validating external input

andijcr · 2023-12-15T23:11:38Z

the last commit address issue #15263 and #13362
by converting aliases to the primary name before writing them to the log.

it's a partial solution, because if the cluster is in the middle of an upgrade, the conversion is skipped (and this in not strictly required. we could save the version when an alias is introduced, to skip conversion only for the latest)

andrwng

The first few commits lgtm, but I've got some questions about the last one.

Still would be great to have a test for the functional changes

src/v/cluster/config_frontend.cc

andijcr · 2023-12-18T17:09:17Z

force push: removed last commit, the issue was solved with #15725

andrwng

I think these changes look good. Would be great to have some tests

src/v/config/config_store.h

andrwng

Thanks for adding some tests! There is probably more to be done, e.g. in ducktape tests, but I don't think that has to block this

vbotbuildovich · 2023-12-18T21:14:55Z

new failures in https://buildkite.com/redpanda/redpanda/builds/42986#018c7e88-7056-4487-ac0c-889b74651af3:

"rptest.tests.consumer_group_recovery_test.ConsumerOffsetsRecoveryTest.test_consumer_offsets_partition_recovery"

piyushredpanda · 2023-12-19T06:44:08Z

Failure here seems to be #15261

vbotbuildovich · 2023-12-19T06:44:31Z

/backport v23.3.x

github-actions bot added the area/redpanda label Dec 14, 2023

andijcr mentioned this pull request Dec 14, 2023

delete_retention_ms migration is not working when included in redpanda.yaml #15603

Closed

andijcr requested review from graphcareful, dotnwat and StephanDollberg December 14, 2023 17:28

dotnwat reviewed Dec 15, 2023

View reviewed changes

andijcr force-pushed the issue/15603/config_tests branch from fb0bce5 to c6f5348 Compare December 15, 2023 17:48

andijcr changed the title ~~Issue/15603/config tests~~ allow property aliases in redpanda.yaml Dec 15, 2023

andijcr requested review from dotnwat and andrwng December 15, 2023 18:21

andrwng reviewed Dec 15, 2023

View reviewed changes

src/v/config/config_store.h Outdated Show resolved Hide resolved

andijcr mentioned this pull request Dec 15, 2023

alias property names do not work correctly while bootstrapping from redpanda.yaml #13362

Closed

andijcr and others added 5 commits December 15, 2023 21:07

config/config_store: handle aliases for property names

f8a1ebf

before this commit, finding the name in the alias table would not throw the std::invalid_argument exception. but would not set the property.

config/config_store: intermediary commit to remove an indentation level

f0e8f8a

this is a non-functional change

config/config_store: optional to ptr

35ed706

non functional change, the optional was just used inside a code block

config/config_store property_aliases() property_names_and_aliases()

bebf4aa

property_aliases() mirrors property_names() and both get merged in property_names_and_aliases(), to be used instead of property_names when filtering/scanning over user input

config: use property_names_and_aliases()

b025e34

this allows to include prop aliases when validating external input

andijcr force-pushed the issue/15603/config_tests branch from c6f5348 to 1f12a5c Compare December 15, 2023 23:07

andijcr requested a review from andrwng December 15, 2023 23:11

andrwng reviewed Dec 16, 2023

View reviewed changes

andijcr mentioned this pull request Dec 16, 2023

Properties revert back to default value upon restart, upgrade related #15674

Closed

piyushredpanda added this to the v23.3.1-rc5 milestone Dec 16, 2023

andijcr changed the title ~~allow property aliases in redpanda.yaml~~ allow property aliases in redpanda.yaml, fix aliases not being applied and convert aliases to proper names in the controller.log Dec 18, 2023

andijcr mentioned this pull request Dec 18, 2023

allow property aliases in redpanda.yaml #15720

Closed

7 tasks

andijcr force-pushed the issue/15603/config_tests branch from 1f12a5c to b025e34 Compare December 18, 2023 17:08

andijcr requested a review from andrwng December 18, 2023 17:08

andijcr changed the title ~~allow property aliases in redpanda.yaml, fix aliases not being applied and convert aliases to proper names in the controller.log~~ allow property aliases in redpanda.yaml, fix aliases not being applied Dec 18, 2023

andrwng previously approved these changes Dec 18, 2023

View reviewed changes

src/v/config/config_store.h Outdated Show resolved Hide resolved

config/config_store_test: unit testing for aliases in yaml source

b76f908

andijcr dismissed andrwng’s stale review via b76f908 December 18, 2023 18:32

andijcr requested a review from andrwng December 18, 2023 18:33

andrwng approved these changes Dec 18, 2023

View reviewed changes

piyushredpanda merged commit 8132430 into redpanda-data:dev Dec 19, 2023
17 of 19 checks passed

This was referenced Dec 19, 2023

[v23.3.x] delete_retention_ms migration is not working when included in redpanda.yaml #15759

Closed

[v23.3.x] allow property aliases in redpanda.yaml, fix aliases not being applied #15760

Merged

andijcr mentioned this pull request Dec 19, 2023

follow-up: config/config_store_test: ignored_keys test #15765

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow property aliases in redpanda.yaml, fix aliases not being applied #15605

allow property aliases in redpanda.yaml, fix aliases not being applied #15605

andijcr commented Dec 14, 2023 •

edited

vbotbuildovich commented Dec 14, 2023 •

edited

dotnwat Dec 15, 2023

andijcr Dec 15, 2023

dotnwat left a comment

andijcr commented Dec 15, 2023

dotnwat commented Dec 15, 2023

andrwng left a comment

andijcr commented Dec 15, 2023

andrwng left a comment

andijcr commented Dec 18, 2023

andrwng left a comment

andrwng left a comment

vbotbuildovich commented Dec 18, 2023

piyushredpanda commented Dec 19, 2023

vbotbuildovich commented Dec 19, 2023

allow property aliases in redpanda.yaml, fix aliases not being applied #15605

allow property aliases in redpanda.yaml, fix aliases not being applied #15605

Conversation

andijcr commented Dec 14, 2023 • edited

Backports Required

Release Notes

Bug Fixes

vbotbuildovich commented Dec 14, 2023 • edited

dotnwat Dec 15, 2023

Choose a reason for hiding this comment

andijcr Dec 15, 2023

Choose a reason for hiding this comment

dotnwat left a comment

Choose a reason for hiding this comment

andijcr commented Dec 15, 2023

dotnwat commented Dec 15, 2023

andrwng left a comment

Choose a reason for hiding this comment

andijcr commented Dec 15, 2023

andrwng left a comment

Choose a reason for hiding this comment

andijcr commented Dec 18, 2023

andrwng left a comment

Choose a reason for hiding this comment

andrwng left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Dec 18, 2023

piyushredpanda commented Dec 19, 2023

vbotbuildovich commented Dec 19, 2023

andijcr commented Dec 14, 2023 •

edited

vbotbuildovich commented Dec 14, 2023 •

edited