fix: retrospective review fixes for merged PR #97#115
Merged
Conversation
kind: compose's ReadyCheck only offered tcp/redis/postgres/http as extras["compose.ready_probe"] values, with "tcp" (a bare dial+close) as the default and the ONLY option available for a wrapped mysql service. That reintroduces, for any compose-wrapped mysql, the exact race plugins/engine/mysql/docker.go's dockerReadyCheck already had to fix for the bundled Docker backend: a Docker host port's forwarding accepts the TCP handshake the instant the container starts, regardless of whether mysqld is listening yet, so a dial-only probe goes ready during the mysql:8.x image's --skip-networking "temporary server" bootstrap phase. Confirmed empirically against a real, freshly started mysql:8.4 container: the bare "tcp" probe reported ready for ~1.6s while mysqld was still bootstrapping and any real connection attempt got EOF. Adds a "mysql" ready_probe that reads the server's unsolicited Initial Handshake Packet and checks its protocol-version byte (0x0a since MySQL 4.1) instead of just dialing — mirroring the handshake check the mysql plugin's own regression test already relies on. Default behavior for existing tcp/redis/postgres/http users is unchanged; this only adds the missing, correct option for mysql.
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
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.
Retrospective
/code-reviewsweep of merged PR #97 (kind: compose— wrap an existing docker-compose.yml as an engine), which shipped without pre-merge review.Finding: the only mysql-usable readiness probe is a bare TCP dial
kind: compose'sReadyCheckpicks a probe viaextras["compose.ready_probe"], supportingtcp(default) /redis/postgres/http. There is nomysqloption — so a compose-wrapped mysql service (the exact motivating use case in the sibling, unmerged PR #96) has no way to get a real readiness check; it is stuck on the bare-dialtcpdefault.That reintroduces, for
kind: compose, the exact raceplugins/engine/mysql/docker.go'sdockerReadyCheckalready had to fix for the bundled Docker backend (see itsTestDockerReadyCheck_NoRaceWithTemporaryServerregression test): a Docker host port's forwarding accepts the TCP handshake the instant the container starts, regardless of whether the process inside is listening yet. The officialmysql:8.ximage runs a--skip-networking"temporary server" to bootstrap the datadir before restarting as the final, network-enabled server — a dial-only probe goes ready during that window.Confirmed empirically, not just by reasoning: brought up a fresh
mysql:8.4container viadocker compose up -dand polled the actualprobeOncefunction from this package every 100ms. Results:For ~1.6 seconds, the current
tcpprobe (bare dial+close) reported the service ready while mysqld was still bootstrapping and any real connection attempt gotEOF— i.e.,bough createwould declare a compose-wrapped mysql engine ready before it can actually serve a query, breaking anypost_createhook (e.g. a migration) that connects immediately afterUpreturns.Fix
Add a
mysqlcompose.ready_probeoption that reads the server's unsolicited Initial Handshake Packet and checks its protocol-version byte (0x0a, unchanged since MySQL 4.1) instead of just dialing — the same technique the mysql plugin's own conformance regression test already relies on (plugins/engine/mysql/conformance_test.go'smysqlHandshakeOnce).plugins/engine/compose/probe.go: newmysqlProbe, wired intoprobeOnce's"mysql"case, error message updated.plugins/engine/compose/compose.go: package doc updated to describe the option and why it matters for mysql specifically.README.md:compose.ready_probedocumentation updated to listmysqland call out the risk of leaving it unset for a wrapped mysql service.plugins/engine/compose/probe_test.go:TestProbeOnce_MySQL(valid handshake),TestProbeOnce_MySQL_WrongProtocolVersion, andTestProbeOnce_MySQL_AcceptThenClose(the regression guard for the accept-then-close docker-proxy race a bare TCP probe cannot see).Default behavior for existing
tcp/redis/postgres/httpusers is unchanged — this only adds the missing, correct option formysql.Test plan
go build ./...go test ./...(full repo green, including newplugins/engine/composetests)go test ./plugins/engine/compose/... -race -vgreengolangci-lint run ./...— 0 issuesmysql:8.4container viadocker compose up -d(see finding above) — not just unit tests against a fake TCP server