Skip to content

fix(mysql): bound ORC roleProbe client calls#3117

Draft
weicao wants to merge 10 commits into
helios/mysql-fixes-integrationfrom
henry/task82-orc-roleprobe-budget
Draft

fix(mysql): bound ORC roleProbe client calls#3117
weicao wants to merge 10 commits into
helios/mysql-fixes-integrationfrom
henry/task82-orc-roleprobe-budget

Conversation

@weicao

@weicao weicao commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Problem

Task #82 is handling the ORC side of the task #81 acceptance line. The focused #3078 evidence showed repeated roleProbe / kbagent timedOut fingerprints around orchestrator-client calls before switchover. That is a readiness/discovery instability signal, not enough by itself to call MySQL switchover product fail.

Change

  • Scope the longer probe budget to ORC ComponentDefinitions only.
  • Render ORC roleProbe as 5s period / 5s timeout.
  • Bound each orchestrator-client call inside the probe with a 4s client budget.
  • On client timeout/error, return empty output with rc=0 so kbagent sees unknown/transient instead of turning the probe into a timed-out execution.
  • Keep the existing non-ORC syncerctl getrole roleProbe at 1s / 1s.

Static validation

  • helm lint addons/mysql PASS.
  • helm template mysql addons/mysql shows ORC roleProbe periodSeconds: 5, timeoutSeconds: 5, ORC_ROLE_PROBE_CLIENT_TIMEOUT_SECONDS=4.
  • Same render shows regular syncerctl getrole roleProbe remains periodSeconds: 1, timeoutSeconds: 1.

Boundary

Draft until focused #3078 runtime replay proves the ORC roleProbe clean-window behavior. This PR alone is not a fullsuite PASS, not release-ready, and not a product conclusion.

@codecov-commenter

codecov-commenter commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 201 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (58cba07) to head (2957aa3).

Files with missing lines Patch % Lines
...ddons/mysql/scripts-ut-spec/orc_switchover_spec.sh 0.00% 137 Missing ⚠️
...ddons/mysql/scripts-ut-spec/orc_role_probe_spec.sh 0.00% 64 Missing ⚠️
Additional details and impacted files
@@                       Coverage Diff                       @@
##           helios/mysql-fixes-integration   #3117    +/-   ##
===============================================================
  Coverage                            0.00%   0.00%            
===============================================================
  Files                                 145     147     +2     
  Lines                               23126   23327   +201     
===============================================================
- Misses                              23126   23327   +201     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@weicao

weicao commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed head a0fa6ec8 (delta vs base helios/mysql-fixes-integration). Pre-flight gates + 8-class design-contract walk. Overall: the switchover verify-while-client-runs design is right and matches the field evidence (takeover succeeded at Orchestrator while Ops timed out on convergence — positive state verification instead of trusting client rc is exactly the correct fail-closed shape). Structured retry-safe stderr diagnostics + mirrored /tmp/switchover.log also close the evidence gap every focused #3078 run has hit. ShellSpec coverage is meaningful (stderr-noise-tolerant parsing, non-zero-client-but-converged case). Two blockers, two verify-flags, minor notes:

Blocker 1 — apps.kubeblocks.io/skip-immutable-check: "true" in mysql.annotations (commit e893b1a) is out of scope and contract-weakening as a permanent default. mysql.annotations is included by all 8 CMPDs, so this silently disables ComponentDefinition immutability protection for the entire addon, forever — any future chart upgrade can mutate immutable CMPD fields that running clusters reference. It is an official KB annotation, but shipping it unconditionally is a hack-shaped bypass of the API's protection (review principle #2). If it exists to allow in-place CMPD iteration during this test loop, gate it ({{- if .Values.dev.skipImmutableCheck }}, default false) or keep it out of the shippable chart; either way it deserves its own PR + justification, not a ride-along in a roleProbe fix.

Blocker 2 (verify-before-merge) — kbagent contract for empty roleProbe output. The probe now returns "" + rc=0 on client timeout/error, assuming kbagent treats empty as "unknown/transient, keep last role". If kbagent instead publishes the empty result and the controller clears the pod's role label, a transient Orchestrator hiccup would de-role the primary → RW Service loses its endpoint → outage window. This must be confirmed against kbagent source or a focused test (stop Orchestrator briefly; assert role labels persist and Service endpoints don't flap) before this trades probe-timeouts for role-flapping. The PR's own boundary note says runtime replay is pending — this is the specific assertion that replay must include.

Verify-flag 1 — action budget fit. switchover.timeoutSeconds templated + client 40s + verify window 40s (parallel) + 2×3s prechecks ≈ 46s worst-case. Confirm the rendered timeoutSeconds ≥ that and stays under the kbagent action clamp; also confirm MYSQL_ORC_SWITCHOVER_VERIFY_WINDOW_SECONDS defaulting to the client budget is intended coupling.

Verify-flag 2 — ${$}-${RANDOM} tmp names in the no-timeout fallback. Two subshells forked from the same parent (the parallel candidate/current readback) inherit identical RNG state on bash < 5.1, so their first $RANDOM expansions can collide; $$ is also identical in subshells. The outer files are prefix-disambiguated, but the inner run_command_with_budget files are not. Only reachable when timeout is absent from the image — but mktemp removes the whole class.

Minor: probe path discards orchestrator-client stderr entirely (2>/dev/null) — acceptable for probe quietness, but consider logging to a file for debuggability; tee /dev/stderr in the action wrapper assumes /dev/stderr is writable in the runtime image (usually true, worth one render-run check).

Classes walked: 1 (no silent fallback hiding root cause — the converged-despite-rc!=0 path is positively verified, good), 2/3 OK, 4 flagged (empty-output sentinel meaning must be confirmed at the consumer — Blocker 2), 5/6/7 N/A-or-clean, 8 OK (rc captured immediately after substitutions). Scope: everything except commit e893b1a serves the stated ORC stability line; e893b1a should be split out.

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.

2 participants