config: update bindings when properties are reset #16504

ztlpn · 2024-02-06T21:37:33Z

Previously we just set the value without notifying bindings. This led to ignoring property value updates when the property was reset to its default value as a result of a patch_cluster_config request with non-empty "remove" section.

Backports Required

Release Notes

Bug Fixes

Fix a bug that resulted in Redpanda ignoring until the next restart config values that were reset to their defaults.

dotnwat · 2024-02-06T22:59:34Z

src/v/config/property.h

-    property<T>& operator()(T v) {
-        _value = std::move(v);
-        return *this;


what happened to this operator?

I had a choice of using update_value there as well, but chose to delete it because, well, using operator() to set a value is a bit confusing IMO. Turned out it was still being used in a test. Fixed that and moved the change to a separate config.

dotnwat · 2024-02-06T23:02:16Z

src/v/config/property.h

    base_property& operator=(const base_property& pr) override {
-        _value = dynamic_cast<const property<T>&>(pr)._value;
+        auto v = dynamic_cast<const property<T>&>(pr)._value;
+        update_value(std::move(v));
        return *this;
    }


good catch. presumably this is fine, but it feels really error prone to be able to use operator= to assign a value like this, in which case, would only be changed on the current core and not all cores. i wonder how many users there are of this operator=.

It's used in patch_cluster_config (and hopefully nowhere else) to create a copy of the configuration to validate incoming upserts

redpanda/src/v/redpanda/admin/server.cc

Lines 1702 to 1707 in 0afd0bd

// Populate the temporary config object with existing values

config::shard_local_cfg().for_each(

[&cfg](const config::base_property& p) {

auto& tmp_p = cfg.get(p.name());

tmp_p = p;

});

given this use, update_value isn't strictly needed here (the bindings are empty in that function)

got it, thanks. then it seems ok. the operator= is still weird but we can address it in the future if/when we redesign the config system.

Yeah, using operator= here is a bit confusing as well. I chose to use update_value here as well so that all value-setting methods use it, but ultimately IMO we should use something more explicit here.

vbotbuildovich · 2024-02-14T11:17:51Z

new failures in https://buildkite.com/redpanda/redpanda/builds/45036#018da714-22dd-4c1a-8a14-8548725ac589:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_rpk_import_sparse.all=False"

new failures in https://buildkite.com/redpanda/redpanda/builds/45036#018da73b-a118-4070-98b6-365a9eb0ef66:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_rpk_import_sparse.all=True"

new failures in https://buildkite.com/redpanda/redpanda/builds/45036#018da73b-a115-4f6c-ae63-d398a3707264:

"rptest.tests.cluster_config_test.ClusterConfigTest.test_rpk_import_sparse.all=False"

vbotbuildovich · 2024-02-14T12:14:10Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45036#018da73b-a115-4f6c-ae63-d398a3707264

Previously, the max_connections_per_ip watch callback assumed that if there are overrides present, the main per-ip limit is there as well.

Remove an old way of setting the property value that was only used in tests.

Previously, if one of the binding watch callbacks threw an exception, the main property value didn't get updated, resulting in an inconsistency.

Previously we just set the value without notifying bindings. This lead to ignoring property value updates when the property was reset to its default value as a result of a patch_cluster_config request with non-empty "remove" section.

ztlpn · 2024-02-14T17:42:57Z

Ready for review!

andijcr · 2024-02-16T16:13:30Z

src/v/net/conn_quota.cc

+
+            if (_cfg.max_connections_overrides().empty()) {
+                ip_home.clear();
+            } else {
+                // Remove everything except the overrides.
+                std::vector<inet_address_key> to_delete;
+                for (const auto& i : ip_home) {
+                    if (!overrides.contains(i.first)) {
+                        to_delete.push_back(i.first);
+                    }
+                }
+                for (const auto& k : to_delete) {
+                    ip_home.erase(k);
+                }
+            }
+


was this the watcher throwing the exception?

nit: seems like you could use directly
std::erase_if

andijcr

lgtm

vbotbuildovich · 2024-02-19T11:22:39Z

/backport v23.3.x

vbotbuildovich · 2024-02-19T11:22:40Z

/backport v23.2.x

vbotbuildovich · 2024-02-19T11:22:41Z

/backport v23.1.x

vbotbuildovich · 2024-02-19T11:23:33Z

Failed to create a backport PR to v23.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-16504-v23.2.x-367 remotes/upstream/v23.2.x
git cherry-pick -x bde7db0e8185e0e9bd6e20783e065291f4c70525 30309ea5fb2c2521e6d5e42378ead610037d990c bb6e9b4ddc3f5c43cbd1c459143824f96f96a675 d58fe3db18ab7f13d531ffa11d1a836e8bb72db2 741158695070159595348cc766bfe51861d4166d 63eb446a94a5df3d7da64abd6e8f6e7bd26bb133

Workflow run logs.

vbotbuildovich · 2024-02-19T11:23:41Z

Failed to create a backport PR to v23.1.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-16504-v23.1.x-331 remotes/upstream/v23.1.x
git cherry-pick -x bde7db0e8185e0e9bd6e20783e065291f4c70525 30309ea5fb2c2521e6d5e42378ead610037d990c bb6e9b4ddc3f5c43cbd1c459143824f96f96a675 d58fe3db18ab7f13d531ffa11d1a836e8bb72db2 741158695070159595348cc766bfe51861d4166d 63eb446a94a5df3d7da64abd6e8f6e7bd26bb133

Workflow run logs.

ztlpn requested review from dotnwat and andijcr February 6, 2024 21:37

github-actions bot added the area/redpanda label Feb 6, 2024

dotnwat previously approved these changes Feb 6, 2024

View reviewed changes

dotnwat requested a review from BenPope February 6, 2024 23:02

piyushredpanda modified the milestones: v23.3.5, v23.3.x-next Feb 7, 2024

ztlpn force-pushed the fix-config-reset-notify branch from af44064 to ebe7a95 Compare February 13, 2024 17:22

ztlpn dismissed dotnwat’s stale review via 3f33c72 February 13, 2024 18:26

ztlpn force-pushed the fix-config-reset-notify branch 2 times, most recently from 3f33c72 to 337e935 Compare February 13, 2024 20:06

ztlpn added 3 commits February 14, 2024 15:27

c/config_manager: handle possible exceptions thrown by property resets

bde7db0

net/conn_quota: handle null per-ip limit and non-empty overrides case

30309ea

Previously, the max_connections_per_ip watch callback assumed that if there are overrides present, the main per-ip limit is there as well.

config: remove property::operator()(value)

bb6e9b4

Remove an old way of setting the property value that was only used in tests.

ztlpn force-pushed the fix-config-reset-notify branch from 337e935 to 7f4da21 Compare February 14, 2024 14:27

ztlpn added 3 commits February 14, 2024 15:41

config: update main property value before updating bindings

d58fe3d

Previously, if one of the binding watch callbacks threw an exception, the main property value didn't get updated, resulting in an inconsistency.

config: test that the bindings are updated after a property.reset() call

63eb446

ztlpn force-pushed the fix-config-reset-notify branch from 7f4da21 to 63eb446 Compare February 14, 2024 14:41

ztlpn requested a review from dotnwat February 14, 2024 17:42

andijcr reviewed Feb 16, 2024

View reviewed changes

andijcr approved these changes Feb 16, 2024

View reviewed changes

mmaslankaprv merged commit f1f0b96 into redpanda-data:dev Feb 19, 2024
16 checks passed

vbotbuildovich mentioned this pull request Feb 19, 2024

[v23.3.x] config: update bindings when properties are reset #16638

Merged

vbotbuildovich mentioned this pull request Feb 19, 2024

[v23.2.x] config: update bindings when properties are reset #16639

Closed

vbotbuildovich mentioned this pull request Feb 19, 2024

[v23.1.x] config: update bindings when properties are reset #16640

Closed

mmaslankaprv mentioned this pull request Feb 19, 2024

[v23.2.x] config: update bindings when properties are reset #16641

Merged

piyushredpanda modified the milestones: v23.3.x-next, v23.3.6 Feb 21, 2024

ztlpn deleted the fix-config-reset-notify branch March 4, 2024 11:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: update bindings when properties are reset #16504

config: update bindings when properties are reset #16504

ztlpn commented Feb 6, 2024 •

edited by piyushredpanda

dotnwat Feb 6, 2024

ztlpn Feb 13, 2024

dotnwat Feb 6, 2024

andijcr Feb 7, 2024

dotnwat Feb 7, 2024

ztlpn Feb 13, 2024

vbotbuildovich commented Feb 14, 2024 •

edited

vbotbuildovich commented Feb 14, 2024

ztlpn commented Feb 14, 2024

andijcr Feb 16, 2024

andijcr Feb 16, 2024

andijcr left a comment

vbotbuildovich commented Feb 19, 2024

vbotbuildovich commented Feb 19, 2024

vbotbuildovich commented Feb 19, 2024

vbotbuildovich commented Feb 19, 2024

vbotbuildovich commented Feb 19, 2024

	// Populate the temporary config object with existing values
	config::shard_local_cfg().for_each(
	[&cfg](const config::base_property& p) {
	auto& tmp_p = cfg.get(p.name());
	tmp_p = p;
	});

config: update bindings when properties are reset #16504

config: update bindings when properties are reset #16504

Conversation

ztlpn commented Feb 6, 2024 • edited by piyushredpanda

Backports Required

Release Notes

Bug Fixes

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

vbotbuildovich commented Feb 14, 2024 • edited

vbotbuildovich commented Feb 14, 2024

ztlpn commented Feb 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andijcr left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Feb 19, 2024

vbotbuildovich commented Feb 19, 2024

vbotbuildovich commented Feb 19, 2024

vbotbuildovich commented Feb 19, 2024

vbotbuildovich commented Feb 19, 2024

ztlpn commented Feb 6, 2024 •

edited by piyushredpanda

vbotbuildovich commented Feb 14, 2024 •

edited