fix: bug-wave — 8 backlog bug fixes (trino P0, proxy teardown, k3s remount, stress/spark otel, start guards) - #852
Conversation
…738) Down.cleanupSocks5Proxy() resolved the proxy state file against the process cwd via a bare relative path, while ProcessSocksProxyService writes and reads it against context.workingDirectory. The two agree only when cwd happens to be the workspace directory. When workingDirectory is set explicitly rather than inherited from cwd (long-running Server/Repl processes, tests), down finds no state file, returns early, and the ssh -N -D tunnel process survives teardown as an orphan holding a local port. Resolve the state file against context.workingDirectory, matching the writer, so teardown reliably finds and kills the tunnel. Add a regression test that spawns a real process, records it in a state file under workingDirectory (never the process cwd), and asserts cleanup kills the process and removes the file.
…ed SOCKS tunnels (#741) Two narrow follow-ups from the #739 review: 1. Control-node SSH readiness was silently skipped under `up --hosts <alias>`. checkSshReady reused the --hosts filter for both the control and db checks. A scale-out run like `up --hosts db2` set the filter to {"db2"}, which never matches the control alias, so HostOperationsService.withHosts no-op'd and the control readiness check verified nothing — reopening the D10 gap the control check was added to close. The control node is never what --hosts scopes, so run its readiness check with an empty filter; only the db/app check honors hosts.hostList. 2. A superseded zombie SOCKS tunnel was never killed. ensureRunning correctly rejects a PID-alive/port-not-accepting zombie and starts a replacement, but never terminated the stale PID; startNewProxy then overwrote the state file with the new PID, leaving the old process unrecorded. Down reads only the current state file, so it could never kill the superseded process — it leaked and survived teardown. Terminate the stale PID before starting the replacement. The kill is best-effort: it never destroys this JVM and never throws, so cleanup can't break starting the replacement. Add regression tests: control readiness still covers control0 under a --hosts db0 filter, and a real superseded zombie process is killed when a replacement is started.
The k3s data dir (/var/lib/rancher/k3s) is a symlink onto the instance-store NVMe mounted at /mnt/db1, but the mount was only established at provision time with no fstab entry or mount unit. Instance-store data persists across a reboot, but the mount does not, so after any reboot (kernel update, spot rebalance, manual recovery) k3s could not find its data dir and crash-looped with "extracting data: no such file or directory", requiring a manual remount. Persist the mount and tie k3s to it: - setup_instance.sh: after mounting, write an /etc/fstab entry keyed on the stable XFS filesystem UUID (instance-store device names can change across reboots) with nofail + x-systemd.device-timeout so a wiped instance store (stop/terminate) never blocks boot. The entry is rewritten idempotently and a daemon-reload publishes the generated mnt-db1.mount unit. - start_k3s_server.sh / start_k3s_agent.sh: install a systemd drop-in with RequiresMountsFor=/mnt/db1 so k3s waits for the (nofail) mount before starting on every boot, closing the race the nofail option otherwise allows. Closes #823
The stress Job's otel-sidecar init container read CLUSTER_NAME via a non-optional configMapKeyRef to a `cluster-config` ConfigMap. That ConfigMap is created by the separate `grafana update-config` command and is not guaranteed to exist when a stress job starts, so the kubelet raised CreateContainerConfigError, the init container looped, and the Job was marked Failed (backoffLimit=0) — blocking all stress writes. Inject the value directly from cluster state (`clusterLabelName()`) instead, removing the fragile cross-command dependency. The value still matches the `cluster` label used across the observability stack, so stress metrics line up with Grafana dashboards.
…role The OTel collector already runs on every EMR node via the bootstrap action, but the collector config hardcoded node_role=spark for all nodes, so master and worker telemetry were indistinguishable. It looked like only the master was reporting because there was no way to tell the workers apart. Give each node's collector its own role, consistent with how the K8s OTel collector derives node_role per node type (db/app/control): - otel-collector-config.yaml: replace the hardcoded node_role value with an unresolved __NODE_ROLE__ placeholder (TemplateService leaves it intact at S3 upload time). - bootstrap-otel.sh: detect the node's role from EMR's /mnt/var/lib/info/instance.json (isMaster), resolve spark-master or spark-worker, and sed-patch __NODE_ROLE__ in the collector config before the collector starts. Every metric/log/trace forwarded from that node is now tagged with its role. - spark-emr spec: document the per-node role labeling scenario. - EMRProvisioningServiceTest: regression test asserting the uploaded bootstrap script resolves the control node IP but leaves __NODE_ROLE__ for per-node patching, and includes the role detection + sed.
…start `cassandra start` previously proceeded even when a targeted node had never been assigned a version via `cassandra use`. On such a node `/usr/local/cassandra/current` points at nothing, so the systemd start failed far downstream with a confusing error. Add a pre-flight check in Start.execute() that inspects every node targeted by the command (respecting the --hosts filter) and aborts up front with a clear, actionable message naming the node(s) missing a version, before touching any node. Extract the host-filter selection logic from HostOperationsService.withHosts into a reusable filteredHosts() so the check targets exactly the same set the start loop would act on.
Previously the restart-and-wait flow could not detect a failed startup. If an invalid JVM/config was pushed, the nodetool wait loop would break on timeout and report success, and the port 9042 wait loop had no timeout at all, so it looped forever. Either way the operator got no error. The script now polls the systemd unit state (systemctl is-active) on every iteration and fails fast the moment the cassandra unit enters a failed/exited state, dumping recent journalctl output so the operator sees the actual cause. Both wait loops are bounded and exit non-zero on timeout or detected failure, which the SSH layer surfaces as a RemoteException through CassandraService.restart.
Code Coverage
|
Review: bug-wave (#852)Reviewed commit-by-commit as requested. Overall solid — real regression tests (spawned processes, not mock-echo), fabric8/typed-object usage for the K8s env var change, and the SOCKS invariant from CLAUDE.md is respected (only 1. #823 fix likely doesn't take effect — wrong
|
…tart web-ui.authentication.type and web-ui.user were set under the shared additionalConfigProperties, which the trinodb/trino Helm chart applies to both coordinator and worker config.properties. web-ui.* are coordinator-only properties (the web UI runs only on the coordinator), so workers flagged them as unused and crashed on startup with exit 100 / CrashLoopBackOff. Move both properties into server.coordinatorExtraConfig, the chart's coordinator-only config key, which is appended solely to the coordinator's config.properties. The web-UI auth config is preserved; workers now start clean. Closes #827
rustyrazorblade
changed the title
Review of #852Reviewed commit-by-commit as requested. Solid set of fixes overall — good regression-test discipline (real spawned processes for the proxy/tunnel tests, real Koin-wired state for 🔴 #823 fix (NVMe remount) —
|
|
Review Went through this commit-by-commit as suggested in the description. Overall this is a well-executed bug-wave: each fix is scoped tightly, comes with a real test (not mock-echo), and the code comments consistently explain why rather than what — matches the repo's conventions well. Strengths worth calling out
Minor observations (non-blocking)
No security, backwards-compatibility, or storage-sizing concerns applicable here per this projects conventions (ephemeral clusters, local-storage PVCs). Didnt spot wildcard imports, |
…PollInterval Duration default applies (fixes stress + status)
|
Reviewed all 12 commits. This is a well-executed batch of bug fixes — good adherence to repo conventions (never touches Solid fixes, no concerns
Worth a follow-up (non-blocking)
Minor
No security or performance concerns. Test coverage is thorough throughout (real spawned processes for the process-lifecycle bugs, a real Koin container for the DI bug, targeted fixture updates rather than loosened assertions). |
ReviewReviewed commit-by-commit as requested. Overall this is a well-scoped bug-wave: each fix is narrow, has a root-cause explanation in the commit body, and (for the Kotlin fixes) comes with a real regression test that exercises actual process/state behavior rather than mock-echo assertions. Nice adherence to the project's "minimal mocking / real TestContainers-or-real-process" testing philosophy ( Correctness
Test coverage
Minor / style
SummarySolid, well-tested bug wave. The one thing I'd actually want addressed before merge is hardening |
1 participant
Backlog bugs fixed as one wave — one commit per fix, review commit-by-commit. Being live-validated end-to-end on a real cluster; fixes for anything surfaced during that test are added here.
Closes #827, closes #738, closes #741, closes #823, closes #733, closes #564, closes #162, closes #200, closes #854, closes #855.
Fixes
web-ui.*props scoped toserver.coordinatorExtraConfig(coordinator-only) so workers stop rejecting them and CrashLooping. Verified withhelm template.downresolves the SOCKS proxy state file againstcontext.workingDirectory(was cwd) → no orphaned ssh tunnel.--hosts; leaked zombie SOCKS ssh survivesdown#741 — control node included in the SSH readiness wait under--hosts; superseded zombie SOCKS tunnels killed before replacement./mnt/db1NVMe mount across reboots (fstab XFS-UUIDnofail+ k3sRequiresMountsFor) → no k3s crash-loop after reboot.CLUSTER_NAMEfrom cluster state, not the often-absentcluster-configConfigMap → noCreateContainerConfigError.spark-master/spark-workerrole tagging (collector was already on all nodes; hardcodednode_rolewas the bug).cassandra startfails fast naming any version-less node.restart-cassandra-and-waitdetects startup failures instead of hanging / false-success.Invariant
#738/#741 never touch
socksProxyHost/socksProxyPort— path resolution, readiness filtering, process lifecycle only (CLAUDE.md / #725).Validation
Unit + integration tests per fix; CI green (incl. detekt on JDK 21). Live-cluster validation in progress — reboot remount (#823), orphan-tunnel cleanup (#738/#741), stress pod start (#733), trino workers Ready + SELECT 1 (#827); EMR role tags (#564) need a separate spark cluster.
Scope notes
cassandra restarthas the same version exposure — possible follow-up.Live validation — 7/8 PASS, $0 (bug-wave @ 4429972)
End-to-end on a real cluster built from this branch. 7 of 8 fixes proven live; #564 (EMR role tagging) code-verified only (unit tests + review; separate EMR run to follow, non-gating).
SELECT 1) · bug: k3s data dir on instance-store NVMe is not remounted on reboot → k3s crash-loops #823 ✅ (UUID-keyed fstab survived a device rename nvme0n1→nvme1n1; k3s orderedAfter mnt-db1.mount) · Down.cleanupSocks5Proxy() resolves proxy state file against cwd, not context.workingDirectory — orphans the ssh tunnel #738/Two follow-ups from #739 review: control-node readiness gap under--hosts; leaked zombie SOCKS ssh survivesdown#741 ✅ (real orphan tunnel killed ondown)status) fail to construct — Koin singleOf ignores the jobPollInterval: Duration default #854 (stress/status Koin construction) and bug: tailscaled is not enabled for boot — control-node reboot kills operator/observability access until manual restart #855 (tailscaled not boot-enabled).