fix(proxy): route every container-creating path through the stage-3c bridge - #169
Merged
Merged
Conversation
…bridge `Dash::Cli::Proxy::Reboot`, `Dash::Cli::Proxy::LoadbalancerReboot` and `dash proxy loadbalancer start` all create the renamed container without ever running the stage-3c bridge. `docker run --volume dash-loadbalancer-config:...` auto-creates the named volume empty when it does not exist, so a `dash proxy reboot` against a host that has never been through `dash proxy boot` brings the new volume into existence before the bridge has had any chance to copy the legacy routing table and ACME cache into it. The next boot then finds the new volume already there and skips the copy for good, via `copy_legacy_config_volume`'s own guard - silently. Since #167 that state also writes the `.legacy-renamed` marker, so recovery needs the marker deleted as well as the volume fixed. Fix it at the source rather than making the marker's heuristic smarter: every path that can create the container, the volume or the network now runs `prepare_boot` first. On both reboot paths this is round-trip neutral - they already spent a round trip on `ensure_apps_config_directory`, which `prepare_boot` carries. Refs #168
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…is live Both cubic and a human reader will ask whether `cp -a` over a volume the legacy container still mounts can capture a half-written routing table or certificate. It cannot: dash-proxy renames into place on every writer - the routing table via writeFileAtomic, the dynamic domain and redirect state via their own temp + rename, the response cache via CreateTemp + Rename, and the ACME cache via autocert.DirCache. Written at the shared copy rather than at one caller, since `boot` and both reboots all reach it. Refs #168
… output `dash app stale_containers --quiet` in test/cli/app_test.rb leaves :error on both the DASH singleton and SSHKit's global output_verbosity: Cli::Base#initialize_commander sets the commander's verbosity and Commander#configure_sshkit_with mirrors it into SSHKit. Nothing restores either between tests — Commander#reset would, but only `dash alias` calls it — so from that point on every SSHKit.config.output.info in the process is dropped. Whether that mattered depended on the seed. CI run 34709935073 put the quiet test ahead of test/cli/healthcheck/progress_reporter_test.rb on Ruby 3.2 (seed 36230) and three of its assertions saw "", while Ruby 3.3, 3.4 and 4.0 drew seeds that passed the same commit. Reproduced locally with `bin/test --seed 36230`, three failures, same three tests. Pin both to :info in the suite's global setup, beside the Docker pins that answer the same class of problem — a test that wants another verbosity still sets it itself. Refs #166
…t order `on` runs the proxy hosts in parallel threads, and the recorder behind "boot issues no round trip beyond the pinned per-host sequence" appended from both. The pin then spelled out host 1's sequence followed by host 2's, which held only while the two threads happened not to overlap. CI seed 59404 interleaved them (login, bridge, login, bridge, inspect, ...) - same commands, same count per host, different scheduling - and the test failed on a run that issued exactly what it pins. It reproduces standalone here too: 1 in 150. Tag every recorded round trip with the host it went to (the Printer command carries it; a capture reads it off SSHKit::Backend.current, the thread-local the backend sets for its run) and assert each host's own sequence. That is the claim the test was making - the count and the order the gem chooses - minus the one it never meant to: which thread the scheduler ran first. 0 in 300 after. Refs #167
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
docker run --volume dash-loadbalancer-config:...auto-creates the named volume, empty, when it does not exist. Three paths reached thatdocker runwithout ever running the stage-3c bridge first:Dash::Cli::Proxy::Reboot#run—dash proxy reboot, and the drift-detected reboot insidedash proxy bootDash::Cli::Proxy::LoadbalancerReboot#run— same, for the loadbalancer hostDash::Cli::Proxy#loadbalancer("start")—start_or_runfalls through todocker runon a host with no containerSo
dash proxy rebootagainst a dedicated load-balancer host that has never been throughdash proxy bootcreatesdash-loadbalancer-configbefore the bridge has had any chance to copykamal-loadbalancer-config's routing table, dynamic domains and ACME cache into it. The nextdash proxy bootfinds the new volume already there and skips the copy for good viacopy_legacy_config_volume's own guard — silently, every deploy. Since #167 that state also writes.legacy-renamed, so recovery needs the marker removed as well as the volume fixed.Fixed at the source, as the issue asks, rather than by making the marker's heuristic smarter: every path that can create the new container, volume or network now runs
prepare_boot(the bridge + the apps-config mkdir) first.Closes #168
Round trips
On both reboot paths this is round-trip neutral: they already executed
ensure_apps_config_directoryas a round trip of its own, andprepare_bootislegacy_rename && ensure_apps_config_directory— the same fold #167 gaveboot. A migrated or fresh-on-4.x host therefore runs onetest -fmore than before and nothing else.dash proxy loadbalancer startdoes gain one round trip; it had no apps-config mkdir to fold into. It also stops leaving that bind-mount source directory for docker to create root-owned.Placement
Reboot: afterpull_image— the volume copy borrows the proxy image, and the pull is the cheapest guarantee it is local — and beforestop_and_replace, so the bridge decides before the running proxy comes down.LoadbalancerReboot: afterensure_network— the bridge's first step attaches the legacy network's containers todash, which has to exist — and before the stop, matching the proxy bridge's own documented order (copy the volume before replacing the container).Test plan
reboot routes a host through the stage-3c bridge before replacing the container— bridge lands before both the stop and thedocker runreboot carries the bridge in the apps-config round trip it already paid— asserts the fold, so a future split shows up as a failurereboot routes the loadbalancer host through the stage-3c bridge before creating the container— network create < bridge <docker run --name load-balancerreboot carries the loadbalancer bridge in the apps-config round trip it already paidloadbalancer start routes through the stage-3c bridge before it can runbundle exec rubocop --parallel— cleanbin/test(integration) — running locally at the time of opening; CI'sbin/testjob is the gateDeviations & judgment calls
rebootthrough the bridge "adds a round trip torebootthat it doesn't pay today". It does not — both reboot paths already spent one onensure_apps_config_directory, whichprepare_bootcarries. Swapping one for the other is free on those paths.dash proxy loadbalancer startreachesstart_or_run, which is the identical volume-creating path; leaving it would have left the marker foolable by exactly the mechanism this closes. That one does cost a round trip.Dash::Commands::Loadbalancer#mark_legacy_renamedcomment rather than leaving it — it documented this gap as open and named Loadbalancer reboot never routes through the stage-3c bridge, so it can create the new volume before it is copied #168 as the tracking issue. It now says which paths run the bridge, and keeps the manual recovery note for a host that got into that state some other way (an operator's owndocker run, a volume created by hand).Summary by cubic
Fixes the stage-3c bridge being skipped on three container-creating paths so
dash-loadbalancer-configis never created empty before the legacy config is copied.dash proxy reboot, the loadbalancer reboot, anddash proxy loadbalancer startnow runprepare_bootbefore anything can create the new container or volume.loadbalancer startgains one round trip but no longer leaves a root-owned mount source.cp -areads a complete file either way.:infoso a--quiettest can't silence SSHKit output for later tests, and the proxy boot round-trip pin is now per host since hosts run in parallel.Closes #168.
Written for commit d5945bc. Summary will update on new commits.