From 60ae66680dd4f2fb9640bf5bce841d0344e962d0 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Mon, 10 Aug 2026 08:26:34 +0000 Subject: [PATCH 1/2] docs: design optional cluster state comments --- ...8-10-cluster-simulator-optional-comment.md | 69 +++++++++++++++++++ ...uster-simulator-optional-comment-design.md | 65 +++++++++++++++++ 2 files changed, 134 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-10-cluster-simulator-optional-comment.md create mode 100644 docs/superpowers/specs/2026-08-10-cluster-simulator-optional-comment-design.md diff --git a/docs/superpowers/plans/2026-08-10-cluster-simulator-optional-comment.md b/docs/superpowers/plans/2026-08-10-cluster-simulator-optional-comment.md new file mode 100644 index 0000000000..dfd27de36a --- /dev/null +++ b/docs/superpowers/plans/2026-08-10-cluster-simulator-optional-comment.md @@ -0,0 +1,69 @@ +# Cluster Simulator Optional Comment Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make omitted expected cluster-state comments behave as wildcards while preserving exact matching for explicitly supplied comments and improving mismatch diagnostics. + +**Architecture:** Add a presence bit to the shared `server_status` tuple, populate it while parsing expected and runtime JSON, and consume it in comparison and JSON projection. Exercise the shared functions with a focused TAP unit binary and then validate the existing simulator end to end. + +**Tech Stack:** C++11-compatible tuple-based state model, nlohmann JSON, ProxySQL TAP test framework, GNU Make, isolated cluster-simulator CI infrastructure. + +## Global Constraints + +- Base the dedicated branch on the latest `origin/v3.0`. +- Omitted comments are wildcards; explicit comments, including empty strings, remain exact. +- Do not change checksum inputs or ProxySQL runtime behavior. +- Do not modify or include PR #6017 changes. + +--- + +### Task 1: Add the focused failing regression + +**Files:** +- Create: `test/deps/cluster_simulator/tests/common_utils_unit-t.cpp` +- Modify: `test/deps/cluster_simulator/Makefile` + +**Interfaces:** +- Consumes: `server_status`, `matching_server_status()`, and `cluster_status_to_json()` from `lib/common_utils.h`. +- Produces: `common_utils_unit-t`, a focused TAP binary runnable with `make common_utils_unit-t && ./common_utils_unit-t`. + +- [ ] **Step 1: Add assertions for omitted, explicit, different, and explicitly empty comments plus JSON projection.** +- [ ] **Step 2: Build and run the focused binary.** + + Run: `make -C test/deps/cluster_simulator common_utils_unit-t -j8 && test/deps/cluster_simulator/common_utils_unit-t` + + Expected: FAIL because the current tuple cannot distinguish omitted from explicitly empty comments and diagnostics omit comments. + +### Task 2: Preserve comment presence and apply the semantic contract + +**Files:** +- Modify: `test/deps/cluster_simulator/lib/common_utils.h` +- Modify: `test/deps/cluster_simulator/lib/common_utils.cpp` + +**Interfaces:** +- Produces: `MYSQL_SERVER_STATUS_T::COMMENT_IS_SET` and a nine-element `server_status` whose final boolean records comment presence. +- Preserves: all existing public helper names and checksum behavior. + +- [ ] **Step 1: Extend `server_status` and its index enum with the presence bit.** +- [ ] **Step 2: Set the bit in `extract_cluster_status()` using `m_mysql_server.contains("comment")`.** +- [ ] **Step 3: Compare comments only when the expected presence bit is true.** +- [ ] **Step 4: Serialize comments only when their presence bit is true.** +- [ ] **Step 5: Rebuild and rerun `common_utils_unit-t`.** + + Expected: all focused TAP assertions pass. + +### Task 3: Document and verify the shared behavior + +**Files:** +- Modify: `test/deps/cluster_simulator/README.md` + +**Interfaces:** +- Documents: optional expected-state fields and omission-as-wildcard semantics. + +- [ ] **Step 1: Update the payload-format documentation with the exact optional-field contract.** +- [ ] **Step 2: Build the cluster simulator with bounded parallelism.** +- [ ] **Step 3: Run the Galera simulator group repeatedly in a unique isolated infrastructure.** +- [ ] **Step 4: Run at least one additional shared-comparator simulator family.** +- [ ] **Step 5: Run `git diff --check`, inspect the full diff, and verify the worktree contains only this fix.** +- [ ] **Step 6: Commit the implementation with a focused message.** +- [ ] **Step 7: Push the dedicated branch and open a draft PR against `v3.0` describing the CI evidence and independence from PR #6017.** diff --git a/docs/superpowers/specs/2026-08-10-cluster-simulator-optional-comment-design.md b/docs/superpowers/specs/2026-08-10-cluster-simulator-optional-comment-design.md new file mode 100644 index 0000000000..8e5bdcdd40 --- /dev/null +++ b/docs/superpowers/specs/2026-08-10-cluster-simulator-optional-comment-design.md @@ -0,0 +1,65 @@ +# Cluster Simulator Optional Comment Design + +## Problem + +The cluster simulator accepts `comment` in `proxysql_init_state` and +`proxysql_final_state`, but it currently turns an omitted comment into an empty +string and compares that string exactly. This differs from the documented +four-field expected-state shape and from the wildcard behavior already used +for omitted `weight`, `max_connections`, and `use_ssl` values. + +The `cluster_sim_galera-g1` failure in GitHub Actions exposed the mismatch. +The initial runtime transition retained non-empty comments for only some +servers. All visible expected and actual fields matched, but the hidden comment +values caused `check_cluster_status()` to fail. The diagnostic JSON also +discarded comments, so the failure did not identify the differing field. + +PR #6017 does not modify the cluster simulator or its Galera wrapper. The fix +therefore belongs in a dedicated PR based on `v3.0`. + +## Contract + +- `hostgroup_id`, `hostname`, `port`, and `status` remain required expected + state fields. +- Omitted `weight`, `max_connections`, `use_ssl`, and `comment` fields are + wildcards. +- An explicitly supplied comment, including `""`, is compared exactly. +- Runtime rows always have a specified comment because the value is selected + from `runtime_mysql_servers`. +- Diagnostic and simulation JSON include a comment whenever the corresponding + state carries an explicitly specified comment. +- Checksums remain diagnostic. Their existing inputs and meaning do not change. + +## Representation + +Extend `server_status` with a boolean recording whether `comment` was present +in the source JSON. This preserves the distinction between an omitted comment +and an explicitly empty comment without requiring C++17 `std::optional`; the +project can still compile in its C++11 fallback mode. + +`extract_cluster_status()` sets the flag from `contains("comment")`. +`matching_server_status()` compares comment values only when the expected +flag is true. `cluster_status_to_json()` emits the comment only when the flag +is true. + +## Testing + +Add focused TAP unit coverage for the shared comparator and JSON projection: + +- omitted expected comment matches a non-empty actual comment; +- explicit matching comments pass; +- explicit different comments fail; +- explicitly empty expected comment does not match a non-empty actual comment; +- JSON omits an unspecified comment and emits a specified one. + +Run the focused test before implementation to capture the intended failure, +then after implementation for GREEN. Build the cluster simulator and run the +Galera simulator group in isolated infrastructure. Also run shared-comparator +coverage for another simulator family to guard against cross-family regressions. + +## Non-goals + +- Do not change ProxySQL runtime comment propagation. +- Do not add sleeps or hardcode infrastructure-specific comments. +- Do not change PR #6017 or its pending RSA authentication work. +- Do not redefine the cluster-state checksum as semantic equality. From 778db5f5f0994c5388b6a9897b9685c6186650f4 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Mon, 10 Aug 2026 08:52:15 +0000 Subject: [PATCH 2/2] test: make omitted cluster comments optional --- ...8-10-cluster-simulator-optional-comment.md | 8 +- test/deps/cluster_simulator/Makefile | 9 +- test/deps/cluster_simulator/README.md | 4 +- .../cluster_simulator/lib/common_utils.cpp | 58 ++++++---- .../deps/cluster_simulator/lib/common_utils.h | 19 ++-- .../tests/common_utils_unit-t.cpp | 107 ++++++++++++++++++ test/infra/control/cluster-simulator-ci.bash | 1 + 7 files changed, 175 insertions(+), 31 deletions(-) create mode 100644 test/deps/cluster_simulator/tests/common_utils_unit-t.cpp diff --git a/docs/superpowers/plans/2026-08-10-cluster-simulator-optional-comment.md b/docs/superpowers/plans/2026-08-10-cluster-simulator-optional-comment.md index dfd27de36a..bf4f9498a7 100644 --- a/docs/superpowers/plans/2026-08-10-cluster-simulator-optional-comment.md +++ b/docs/superpowers/plans/2026-08-10-cluster-simulator-optional-comment.md @@ -22,15 +22,17 @@ **Files:** - Create: `test/deps/cluster_simulator/tests/common_utils_unit-t.cpp` - Modify: `test/deps/cluster_simulator/Makefile` +- Modify: `test/infra/control/cluster-simulator-ci.bash` **Interfaces:** - Consumes: `server_status`, `matching_server_status()`, and `cluster_status_to_json()` from `lib/common_utils.h`. -- Produces: `common_utils_unit-t`, a focused TAP binary runnable with `make common_utils_unit-t && ./common_utils_unit-t`. +- Produces: `common_utils_unit-t`, a focused TAP binary runnable through `make check` and executed by the cluster-simulator CI build. - [ ] **Step 1: Add assertions for omitted, explicit, different, and explicitly empty comments plus JSON projection.** -- [ ] **Step 2: Build and run the focused binary.** +- [ ] **Step 2: Add `make check` to the cluster-simulator CI build path.** +- [ ] **Step 3: Build and run the focused binary.** - Run: `make -C test/deps/cluster_simulator common_utils_unit-t -j8 && test/deps/cluster_simulator/common_utils_unit-t` + Run: `make -C test/deps/cluster_simulator check -j"$(nproc)"` Expected: FAIL because the current tuple cannot distinguish omitted from explicitly empty comments and diagnostics omit comments. diff --git a/test/deps/cluster_simulator/Makefile b/test/deps/cluster_simulator/Makefile index eabc559248..3e0e84989d 100644 --- a/test/deps/cluster_simulator/Makefile +++ b/test/deps/cluster_simulator/Makefile @@ -90,7 +90,7 @@ debug: cluster_simulator .PHONY: clean clean: - rm -f cluster_simulator + rm -f cluster_simulator common_utils_unit-t rm -rf $(UTILSLDIR) @@ -130,3 +130,10 @@ $(TAP_LDIR)/libtap.so: cluster_simulator: cluster_simulator.cpp $(UTILSLDIR)/cluster_simulator_lib.a $(TAP_LDIR)/libtap.so $(CXX) -o $@ $(OBJ) cluster_simulator.cpp $(INCLUDEDIRS) $(LDIRS) $(OPT) $(MYLIBSJEMALLOC) $(MYLIBS) $(STATIC_LIBS) $(UTILSLDIR)/cluster_simulator_lib.a + +common_utils_unit-t: tests/common_utils_unit-t.cpp $(UTILSLDIR)/common_utils.o $(TAP_LDIR)/libtap.so + $(CXX) -o $@ $(OBJ) tests/common_utils_unit-t.cpp $(UTILSLDIR)/common_utils.o $(INCLUDEDIRS) $(LDIRS) $(OPT) $(MYLIBSJEMALLOC) $(MYLIBS) $(STATIC_LIBS) + +.PHONY: check +check: common_utils_unit-t + LD_LIBRARY_PATH="$(TAP_LDIR):$${LD_LIBRARY_PATH:-}" ./common_utils_unit-t diff --git a/test/deps/cluster_simulator/README.md b/test/deps/cluster_simulator/README.md index 8959557521..f4a2c1395f 100644 --- a/test/deps/cluster_simulator/README.md +++ b/test/deps/cluster_simulator/README.md @@ -135,7 +135,9 @@ Simulator payload is a JSON object holding the following members: the initial configuration provided. Objects in the array are required to be JSON objects with the following members: `{hostgroup_id, - hostname, port, status}`. + hostname, port, status}`. They can also include `weight`, `max_connections`, `use_ssl`, and + `comment`. An omitted optional member is not compared; an explicitly supplied member, including + an empty `comment`, must match exactly. Status member should be an `string` of one of the following values: "SHUNNED|ONLINE|OFFLINE_SOFT|OFFLINE_HARD". NOTE: Since 'OFFLINE_HARD' status is a transitory diff --git a/test/deps/cluster_simulator/lib/common_utils.cpp b/test/deps/cluster_simulator/lib/common_utils.cpp index 29596b4934..10735fc367 100644 --- a/test/deps/cluster_simulator/lib/common_utils.cpp +++ b/test/deps/cluster_simulator/lib/common_utils.cpp @@ -353,36 +353,40 @@ bool check_present_and_type(const json& j, const std::vector& path, return res; } -bool matching_server_status(const server_status& srv_st1, const server_status& srv_st2) { +bool matching_server_status(const server_status& exp_srv_st, const server_status& act_srv_st) { bool res = false; bool same_hid_host = - std::get<0>(srv_st1) == std::get<0>(srv_st2) && - std::get<1>(srv_st1) == std::get<1>(srv_st2) && - std::get<2>(srv_st1) == std::get<2>(srv_st2); + std::get<0>(exp_srv_st) == std::get<0>(act_srv_st) && + std::get<1>(exp_srv_st) == std::get<1>(act_srv_st) && + std::get<2>(exp_srv_st) == std::get<2>(act_srv_st); if (same_hid_host) { std::vector allowed_sts { - str_split(std::get<3>(srv_st1), '|') + str_split(std::get<3>(exp_srv_st), '|') }; - if (std::find(allowed_sts.begin(), allowed_sts.end(), std::get<3>(srv_st2)) != allowed_sts.end()) { - const int64_t exp_weight = std::get(srv_st1); - const int64_t exp_max_conns = std::get(srv_st1); - const int32_t exp_use_ssl = std::get(srv_st1); - const std::string exp_comment = std::get(srv_st1); + if (std::find(allowed_sts.begin(), allowed_sts.end(), std::get<3>(act_srv_st)) != allowed_sts.end()) { + const int64_t exp_weight = std::get(exp_srv_st); + const int64_t exp_max_conns = std::get(exp_srv_st); + const int32_t exp_use_ssl = std::get(exp_srv_st); + const std::string& exp_comment = std::get(exp_srv_st); + const bool exp_comment_is_set = + std::get(exp_srv_st); bool match = true; if (exp_weight != -1) { - match = match && exp_weight == std::get(srv_st2); + match = match && exp_weight == std::get(act_srv_st); } if (exp_max_conns != -1) { - match = match && exp_max_conns == std::get(srv_st2); + match = match && exp_max_conns == std::get(act_srv_st); } if (exp_use_ssl != -1) { - match = match && exp_use_ssl == std::get(srv_st2); + match = match && exp_use_ssl == std::get(act_srv_st); + } + if (exp_comment_is_set) { + match = match && exp_comment == std::get(act_srv_st); } - match = match && exp_comment == std::get(srv_st2); res = match; } @@ -990,6 +994,7 @@ std::pair extract_cluster_status( int64_t max_conns = -1; int32_t use_ssl = -1; std::string comment {}; + bool comment_is_set = false; try { hostgroup_id = m_mysql_server.at("hostgroup_id"); @@ -1008,13 +1013,24 @@ std::pair extract_cluster_status( } if (m_mysql_server.contains("comment")) { comment = m_mysql_server.at("comment"); + comment_is_set = true; } } catch(const std::exception& e) { return { EXIT_FAILURE, e.what() }; } cluster_status.push_back( - std::make_tuple(hostgroup_id, hostname, port, status, weight, max_conns, use_ssl, comment) + std::make_tuple( + hostgroup_id, + hostname, + port, + status, + weight, + max_conns, + use_ssl, + comment, + comment_is_set + ) ); } @@ -1088,10 +1104,11 @@ ordered_json cluster_status_to_json(const std::vector& cluster_st { "status", std::get<3>(server_status) }, }; - int64_t weight = std::get(server_status); - int64_t max_conns = std::get(server_status); - int64_t use_ssl = std::get(server_status); - string comment = std::get(server_status); + const int64_t weight = std::get(server_status); + const int64_t max_conns = std::get(server_status); + const int64_t use_ssl = std::get(server_status); + const string& comment = std::get(server_status); + const bool comment_is_set = std::get(server_status); if (weight != -1) { srv_result["weight"] = weight; @@ -1102,6 +1119,9 @@ ordered_json cluster_status_to_json(const std::vector& cluster_st if (use_ssl != -1) { srv_result["use_ssl"] = use_ssl; } + if (comment_is_set) { + srv_result["comment"] = comment; + } result.push_back(srv_result); } diff --git a/test/deps/cluster_simulator/lib/common_utils.h b/test/deps/cluster_simulator/lib/common_utils.h index aec11a88cf..c36c2fa2fd 100644 --- a/test/deps/cluster_simulator/lib/common_utils.h +++ b/test/deps/cluster_simulator/lib/common_utils.h @@ -20,7 +20,8 @@ using status = std::string; using comment = std::string; using mysql_server_def = std::tuple; -using server_status = std::tuple; +using server_status = + std::tuple; struct MYSQL_SERVER_STATUS_T { enum { HG_ID, @@ -30,7 +31,8 @@ struct MYSQL_SERVER_STATUS_T { WEIGHT, MAX_CONNS, USE_SSL, - COMMENT + COMMENT, + COMMENT_IS_SET }; }; @@ -246,14 +248,17 @@ std::pair gen_invalid_keys_err( */ bool check_present_and_type(const json& j, const std::vector& path, const json::value_t& type); /** - * @brief Check that the supplied 'server_status' are both equal. + * @brief Check whether an actual server status satisfies an expected status. * - * @param srv_st1 The first server status to check. - * @param srv_st2 The second server status to check. + * Optional fields omitted from the expected status are wildcards. Fields that + * are present, including an explicitly empty comment, are compared exactly. * - * @return 'true' if both server status are equal, 'false' otherwise. + * @param exp_srv_st The expected server status. + * @param act_srv_st The actual server status. + * + * @return 'true' if the actual status satisfies every expectation. */ -bool matching_server_status(const server_status& srv_st1, const server_status& srv_st2); +bool matching_server_status(const server_status& exp_srv_st, const server_status& act_srv_st); /** * @brief Check that the both supplied cluster states are equal, or equivalent. * diff --git a/test/deps/cluster_simulator/tests/common_utils_unit-t.cpp b/test/deps/cluster_simulator/tests/common_utils_unit-t.cpp new file mode 100644 index 0000000000..caa73760b3 --- /dev/null +++ b/test/deps/cluster_simulator/tests/common_utils_unit-t.cpp @@ -0,0 +1,107 @@ +#include +#include +#include + +#include "common_utils.h" +#include "tap.h" + +namespace { + +server_status make_status_with_comment(const std::string& comment) { + return std::make_tuple( + 10U, + "127.0.0.1", + 3306U, + "ONLINE", + 1, + 1000, + 0, + comment, + true + ); +} + +std::vector extract_status( + const ordered_json& server, + const char* description +) { + const ordered_json test_definition { + { "proxysql_init_state", ordered_json::array({ server }) } + }; + std::vector result {}; + const auto extraction_result = extract_cluster_status( + cluster_state::init_state, + test_definition, + result + ); + + ok( + extraction_result.first == EXIT_SUCCESS, + "%s", + description + ); + return result; +} + +} // namespace + +int main() { + plan(9); + + const server_status actual { make_status_with_comment("runtime comment") }; + const ordered_json base_server { + { "hostgroup_id", 10 }, + { "hostname", "127.0.0.1" }, + { "port", 3306 }, + { "status", "ONLINE" } + }; + const std::vector omitted_status = extract_status( + base_server, + "a payload with an omitted comment is extracted" + ); + ordered_json explicit_server = base_server; + explicit_server["comment"] = "runtime comment"; + const std::vector explicit_status = extract_status( + explicit_server, + "a payload with a nonempty comment is extracted" + ); + ordered_json empty_server = base_server; + empty_server["comment"] = ""; + const std::vector empty_status = extract_status( + empty_server, + "a payload with an empty comment is extracted" + ); + + ok( + matching_server_status(omitted_status.at(0), actual), + "an omitted expected comment matches a runtime comment" + ); + ok( + matching_server_status(explicit_status.at(0), actual), + "an explicit matching comment is accepted" + ); + ok( + !matching_server_status(make_status_with_comment("different"), actual), + "an explicit different comment is rejected" + ); + ok( + !matching_server_status(empty_status.at(0), actual), + "an explicitly empty comment is compared exactly" + ); + + const ordered_json omitted_json = + cluster_status_to_json(omitted_status); + ok( + !omitted_json.at(0).contains("comment"), + "an omitted comment stays omitted in diagnostic JSON" + ); + + const ordered_json specified_json = + cluster_status_to_json(explicit_status); + ok( + specified_json.at(0).at("comment") == "runtime comment", + "a specified comment is included in diagnostic JSON" + ); + + return exit_status(); +} diff --git a/test/infra/control/cluster-simulator-ci.bash b/test/infra/control/cluster-simulator-ci.bash index a55f0d30e2..1258e20d6d 100755 --- a/test/infra/control/cluster-simulator-ci.bash +++ b/test/infra/control/cluster-simulator-ci.bash @@ -206,6 +206,7 @@ handle_internal_build() { cd "${REPO_ROOT}" make -j"$(nproc)" GIT_VERSION_BASE="${GIT_VERSION_BASE}" testall make -j"$(nproc)" GIT_VERSION_BASE="${GIT_VERSION_BASE}" build_cluster_simulator + make -C test/deps/cluster_simulator -j"$(nproc)" check make -C test/tap -j"$(nproc)" GIT_VERSION="${GIT_VERSION_BASE}" tap make -C test/tap/tests -j"$(nproc)" \ GIT_VERSION="${GIT_VERSION_BASE}" "${SIMULATOR_BINARIES[@]}"