Skip to content

refactor: collapse the fourteen AMQP client config writers into one shared helper - #1499

Open
SekiXu wants to merge 5 commits into
developfrom
refactor/1426-shared-amqp-config-helper
Open

SekiXu wants to merge 5 commits into
developfrom
refactor/1426-shared-amqp-config-helper

Conversation

@SekiXu

@SekiXu SekiXu commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Closes #1426.

Prerequisite refactor for the AMQP TLS work in #1167. Twelve config_*.cpp modules each carried their
own copy of the code that writes a service's transport_url and its [oslo_messaging_rabbit] keys —
fourteen call sites once neutron's two and ironic's two are counted. Adding ssl = true /
ssl_ca_file for TLS would mean fourteen edits, and missing one is silent: that service keeps working
until the broker stops accepting the connection shape it still asks for.

This changes no generated config. It is a behaviour-preserving refactor so that the TLS change
later lands in one place, with the HA-gate decision made once.

What changed

SetMqClientConfig() in core/cube_sdk_library/src/cluster.{hpp,cpp} — one writer, taking the config
by reference the way the three SetWorkerQueue() copies already did. Its body keeps the unconditional
keys and the HA-gated keys as separate blocks, which is where #1167's ssl parameters go without
restructuring again.

  • glance / cinder / manila carried byte-identical SetWorkerQueue() copies, comment included, and
    their signature is the one the shared writer adopted — so those three functions are deleted and the
    call sites changed in name only.
  • nova / octavia / cyborg / heat / masakari / designate / watcher / ironic keep their
    UpdateMqConn() wrapper; only the body becomes a call. ironic writes twice, to its own config and to
    the inspector's, so it makes two calls.
  • neutron has two asymmetric call sites. The control node's config gets the full key set; the
    compute node's VPN agent config has never carried rpc_response_timeout — confirmed with
    git log -S back to the initial commit, never present. That asymmetry is preserved explicitly via
    withRpcTimeout = false, because the acceptance criterion is byte-identical output. The
    unconditional RabbitMqServers() call at the top of the function is gone with it: it built a string
    that a node which is neither control nor compute then discarded.

Role gating deliberately stays with the caller. Which roles run a given service is a property of
that service's deployment, not of the message queue, and the nine UpdateMqConn() copies disagree
about it three different ways (IsControl only ×5, IsControl || IsCompute ×3, neutron's two-stage).
Folding that into the helper would have produced one function with a role flag, which is harder to
change, not easier.

config_barbican.cpp is untouched — it writes kafka://…:9095, and the RabbitMqServers occurrence
at config_barbican.cpp:228 is comment text, not a call. The comment already there is more complete
than anything this PR would add.

Why cluster.cpp rather than a new core/rabbitmq/rabbitmq_util.{h,cpp}

The mysql_util precedent would need a new set of .mk link settings, and it would split the two
things #1167 has to touch — scheme and port live in RabbitMqServers() in cluster.cpp, the ssl
parameters would live in the new file. That is the opposite of what this issue is buying.

Evidence

Taken against this branch's final commit 762ed5c4, on a 1cc dev node with cubesys.ha = false.

Both sides were built in the x86 jail container from the same 25bbcebd base — stock, and with the
five patches — so the only variable is this change:

ref sha256 strings | grep -c SetMqClientConfig
stock 25bbcebd 6d158ff0… 0
patched 762ed5c4 80bfdf9d… 4

Each binary then committed the same settings file, and the generated tree was snapshotted:

stock   (PID 2352543)   12/12 modules commit(o)   145 files
patched (PID 2443459)   12/12 modules commit(o)   145 files

diff -r cfg-stock cfg-patched   →  rc=0, empty output
md5sum table, file by file      →  identical, 145 vs 145

On the fourteen AMQP client configs the patched binary produced, /etc/neutron/vpn_agent.ini carries
transport_url but no rpc_response_timeout, while the other thirteen carry both — the asymmetry
described above, reproduced on a real node rather than argued from the diff.

Offline unit tests (new): core/cube_sdk_library/src/tests/test_mq_config_01.cpp, six cases
pinning the exact key set and values for each shape the fourteen call sites produce — including the
absence of the HA section on a single-node cluster, and neutron's missing rpc_response_timeout.
Verified non-vacuous by a negative control: changing the expected rpc_response_timeout to 9999
turns two assertions red and fails the target; reverting restores green. Note that make all does not
build that directory — the existing utils/tests is not built either. They run under make test.

These tests exist because the real-node evidence is expensive and taken rarely: hex_config's dry run
returns at the top of every module's Commit() and produces no config at all, and a module-scoped
commit rewrites cluster-wide settings. Both cheap routes are closed.

Three traps are worth recording for anyone reproducing the comparison, since each one produces a
confident-looking wrong answer:

  • hex_config commit /etc/settings.txt does nothing. Current and new are then the same file, the
    digest matches, and every module reports commit(x) — a skip, not a pass. The settings file passed
    in has to differ from /etc/settings.txt.
  • The "before" side cannot be the binary already on the node. That one carries unrelated develop
    drift, which shows up as a diff that has nothing to do with this change.
  • An empty /var/log/hex_config.log does not mean the commit did not run. rsyslog's imjournal
    rate limit (20000 messages / 600s) was tripped by another service mid-run and dropped 12098
    messages, silently blanking the whole window. journalctl -t hex_config had all 1594 lines,
    including every module's commit(o).

Not verified here: the refactored writer on an HA node

The HA branch is where all four [oslo_messaging_rabbit] keys live, so the non-HA run above says
nothing about them. Two things narrow that gap; one remains open.

Offline cases 2 and 4 pin the HA key set for the new writer. Those expectations were then checked
against a live cubesys.ha = true cluster: across all fourteen config files the six keys and their
exact values match what the pre-refactor code produces today — including vpn_agent.ini carrying the
four HA keys but no rpc_response_timeout. The test expectations are therefore grounded in production
output, not derived from the diff they are meant to check.

What remains open is that the refactored binary has not itself run on an HA node. Whether each of the
fourteen call sites passes the right arguments is read from the diff, not executed. That needs a
cluster installed from a build containing this change — QA's environment, not a hot-patch. The ticket
carries the rows for it.

Scope

No port change, no scheme change, no ssl parameters — those are #1167's implementation issue. Only
*.cpp / *.hpp and one tests Makefile change, so nothing enters the rootfs and no ISO rebuild is
required.

🤖 Generated with Claude Code

SekiXu and others added 5 commits September 15, 2026 15:30
… services

Twelve config modules each carry their own copy of the code that writes a
service's transport_url and its [oslo_messaging_rabbit] keys, across fourteen
call sites in total. Adding a key means fourteen edits, and missing one is
silent: the service keeps working until the broker stops accepting the shape
it still asks for.

SetMqClientConfig() takes the config by reference, the way the three
SetWorkerQueue() copies already did, and keeps the unconditional keys and the
HA-gated keys as separate blocks so a later change can add to either one
without restructuring.

Role gating stays with the caller. Which roles run a given service is a
property of that service's deployment, not of the message queue, and the nine
UpdateMqConn() copies disagree about it three different ways.

withRpcTimeout exists for neutron's VPN agent, the one call site that has
never carried rpc_response_timeout.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Seki Xu <seki.xu@bigstack.co>
…d writer

These three carried byte-identical SetWorkerQueue() copies, comment included,
and their signature is the one the shared writer adopted, so the call sites
change in name only.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Seki Xu <seki.xu@bigstack.co>
… shared writer

The role guards are left exactly as they were -- nova, octavia and cyborg
admit compute nodes, the other five are control-only -- because that
difference is real and belongs to each service.

ironic writes twice, to its own config and to the inspector's, so it makes two
calls.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Seki Xu <seki.xu@bigstack.co>
…d writer

neutron is the only module with two asymmetric call sites: the control node's
own config gets the full key set, while the compute node's VPN agent config
has never been given rpc_response_timeout. That asymmetry is preserved as-is
-- this change is meant to alter no generated config.

The unconditional RabbitMqServers() call at the top of the function is gone
with it. It built a string that a node which is neither control nor compute
then discarded.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Seki Xu <seki.xu@bigstack.co>
…call sites

The acceptance evidence for this refactor -- generated configs identical
before and after -- can only come off a real cluster: hex_config's dry run
returns at the top of every module's Commit() and produces no config at all,
and a module-scoped commit corrupts cluster-wide globals. Both of the cheap
routes are closed, so the evidence is expensive and taken rarely.

These tests hold the line offline in between. They assert the exact key set
and values for each shape the fourteen call sites produce, including the
absence of the HA section on a single-node cluster and neutron's missing
rpc_response_timeout, so that a later edit to the writer cannot quietly drop
a key for one service.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Seki Xu <seki.xu@bigstack.co>
@SekiXu
SekiXu requested a review from a team as a code owner September 16, 2026 05:33
@SekiXu
SekiXu marked this pull request as draft September 16, 2026 05:37
@SekiXu
SekiXu marked this pull request as ready for review September 16, 2026 06:21

@Eandalf-Bigstack Eandalf-Bigstack left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we would be merging this PR after v3.1.20, into v3.2.0 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task] Collapse the twelve copy-pasted AMQP client config writers into one shared helper

2 participants