cluster: invoke config_frontend methods on controller shard

[pgellert]

Various places in the code were calling do_patch directly without regard
to the requirement that do_patch has to be called on the controller
shard.

This caused a fixture test to fail because it tried to invoke do_patch
on all shards and this violates the assertion in do_patch of
config_frontend.cc, causing it to fail with the error message "Must be
called on version_shard".

This fixes it by changing config_fronter::patch() to invoke do_patch on
the controller shard, and moving all calls of do_patch to call patch
instead.

Backports Required

• none - not a bug fix
• none - this is a backport
• none - issue does not exist in previous branches
• none - papercut/not impactful enough to backport
• v23.3.x
• v23.2.x

Release Notes

Bug Fixes

• Fixes a bug of config_frontend methods getting called on shards other than the controller shard.

[pgellert]

I am not sure why this test started failing now, because I can't see any recent changes that would explain it. But based on the code it seems to me that this is how we should fix the failing test.

[vbotbuildovich]

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46174#018e3ca9-0fed-49d5-bd21-1f96d0cc2ed3

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/46190#018e3d59-3145-421a-9c59-388bd910662d

[BenPope]

LGTM

Maybe @dotnwat has an opinion, too?

It might be worth backporting this, the change in the metrics reporter might not be inconsequential.

[pgellert]

Makes sense. I've updated the description now to make this a backport + added a bug-fix description.

[dotnwat]

why are we switching cores here to invoke do_patch rather than requiring the caller to invoke patch on the correct core, like in cluster::service::config_update?

to be clear, i'm not saying it should be one way or another. but i do think it should be consistent. i'd probably choose whichever pattern aligned with a majority of the callers, and then add a comment expressing the expectation on callers (if there are any), and an assertion to check it.

[BenPope]

It clearly has been used incorrectly; I can't imagine a downside of making do_patch private and dispatching to the correct core within patch, but I may have missed something.

The primary advantage is that it makes the API harder to misuse.

As far as I can tell, requiring the caller to know which core to call it on is error prone and has no advantage.

I may be missing something, though.

[pgellert]

Agree with what Ben said above.

This also improves consistency locally, by making all of the public methods of config_frontend callable from any shard, not just set_status.

Globally across *_frontend.h public methods, there is an inconsistency where some methods are callable from any shard while some aren't. I would think that since invoke_on seems cheap, we should standardise making it the *_frontend classes' responsibility to delegate work to the correct core. But that's a larger undertaking.

[dotnwat]

Yes, allowing the method to be invoked on any core is great.

Internally we should also be consistent:

    auto leader = _leaders.local().get_leader(model::controller_ntp);
    if (!leader) {
        co_return patch_result{
          .errc = errc::no_leader_controller, .version = config_version_unset};
    }
    if (leader == _self) {
        co_return co_await do_patch(std::move(update), timeout);
        co_return co_await container().invoke_on(

here we are combining state from different cores. it's benign in this case, but in general, it should be consistent.

[vbotbuildovich]

/backport v23.3.x

[vbotbuildovich]

/backport v23.2.x

[vbotbuildovich]

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-17088-v23.2.x-498 remotes/upstream/v23.2.x
git cherry-pick -x b7202f11b98f555a5403c77ff65f91e5a2f24f67 ed04fc3eba2b2e66ed68861d84ecb0a26bc2055a

Workflow run logs.