feat(cassandra): install arbitrary versions/branches onto a live cluster without an AMI rebuild - #878
feat(cassandra): install arbitrary versions/branches onto a live cluster without an AMI rebuild#878rustyrazorblade wants to merge 20 commits into
Conversation
withHosts(parallel = true) fired a bare thread per host and only join()ed them, so an exception inside a host action died with its thread and the caller reported success. Collect each host's outcome and rethrow the first failure; add collectFromHosts() for callers that need to report per-host success and failure instead of aborting on the first one.
Review — spec-only PR (proposal/design/spec/tasks for issue #876)This PR contains no implementation code yet, just the OpenSpec artifacts ( What I verified
All of it checked out. This is a well-researched design — I want to flag that positively since it's not always the case. Findings1. The Related minor precision nit: 2. Shell-injection surface for the new CLI-flag path isn't addressed. 3. 4. Minor wording nit — 5. Minor — concurrent-install race on Test coverage
Overall this is a strong, well-grounded design doc — the factual rigor about the existing codebase is genuinely above average for a proposal at this stage. The findings above are all refinements, not blockers, for moving into implementation. |
…cript install_cassandra_version() was trapped inside install_cassandra.sh's bake-time loop. Move it, download_cassandra_version(), and the S3 cache sourcing into packer/cassandra/bin/install-cassandra-version, a flag-driven script baked onto the AMI by the existing bin/ catch-all provisioner, and rewrite the bake-time loop to call it so bake time and runtime share one install path. Two deliberate divergences from the inline original: JAVA_HOME for the ant build comes from the target version's own java field rather than the node's default alternative (switching that default would disturb the running version), and ~/.m2 cleanup stays a once-per-bake step in install_cassandra.sh. The loop also skips entries marked lazy: true, and cassandra.in.sh is now installed to a durable path since /tmp does not survive a reboot.
ln -vfns was unconditional, so pointing 'current' at a version that was never installed left a dangling symlink and surfaced as a confusing startup failure later. Check the version directory first and direct the operator to 'cassandra install'.
A lazy entry ships in the AMI's cassandra_versions.yaml so it is discoverable and installable at runtime, but the bake skips installing it. Serialized only when true so rewriting a node's file does not add a lazy: false line to every existing entry.
ReviewReviewed the diff (openspec docs + Findings1. 2. The bake-time refactor isn't exercised by any automated test 3. Minor: 4. Minor, defense-in-depth: Things done well
I didn't run the full build/test suite myself here (worth a |
Installs one additional version onto a running cluster without an AMI rebuild. Resolution takes CLI flags over a declared cassandra_versions.yaml entry field by field, defaulting python to 3.11.9 since use-cassandra hard-exits without it. Per host, the resolved entry is pushed into /etc/cassandra_versions.yaml via the typed CassandraVersion loader (an entry already there is the idempotency check), then install-cassandra-version runs remotely. Outcomes are reported per host and a failed install rolls the node's version list back so it can be retried. shellQuote moves out of ExecRun to a top-level function so the remote command is built with one quoting implementation, not two.
A lazy version is declared but never baked, so it is invisible until installed. List it alongside the node's installed versions, marked declared-but-not-installed, and share the declared-set lookup with 'cassandra install'.
Covers both resolution paths (declared entry and CLI options), --hosts targeting, the no-op and per-host failure behavior, and the cost of a branch build on a running node. Also points the build-cassandra-ref tarball workflow at the runtime install, which skips the AMI rebuild entirely.
…ean up a failed install Container-verified two flaws in the extracted script: the snippet check ran only after a full tarball download, and a failure after the move left a directory in /usr/local/cassandra that the already-installed guard would then trust, making every retry a silent no-op.
ReviewNice, well-scoped feature — the shared Two things worth a look before this lands: 1.
|
ReviewNicely scoped PR — extracting Bug:
|
MINA's executeRemoteCommand builds its Response only after the call returns, so a non-zero exit discarded both stdout and the captured stderr and left the caller with 'Remote command failed (1): <command>'. That hid use-cassandra's 'not installed, run cassandra install first' guidance entirely, and reduced every install failure to the command string instead of the ant/clone/download error that caused it. Own both streams and rethrow as RemoteCommandFailedException carrying what the command actually said. URL userinfo is stripped on the way out, since a git URL can embed a token and that message reaches logs, MCP and Redis. A non-zero exit is also excluded from the SSH retry policy: it is a deterministic failure, not a transport fault, and re-running it repeats minutes of build work.
…rsion list Every AMI ships the whole cassandra_versions.yaml, lazy entries included, so a lazy version is always already declared on a node that has never installed it. Reading that file as the idempotency signal made 'cassandra install' a no-op on the first attempt for exactly the versions the feature exists to install — and for any version declared after the AMI was baked. Check /usr/local/cassandra/<version> instead and treat the push-up as pure input to the install. A rollback that fails no longer replaces the install failure: it is recorded as a suppressed exception and the original is always rethrown. The failure reason is redacted before it reaches the event bus, an empty --hosts match now fails fast instead of installing nowhere and exiting 0, and the tests cover --hosts targeting and the declared git-branch path.
The one script both the AMI bake and 'cassandra install' depend on had no automated coverage. Add two suites in the repo's existing bash-test style: install-cassandra-version's decisions before it touches the network (argument handling, already-installed no-op, source-build guards, JDK selection, and that it never switches the node's default JDK), and the bake loop's yq-driven flag construction and lazy skip. To make the loop testable, install_cassandra.sh regains the safe-to-source function header it documents, with version resolution as functions above the INSTALL_CASSANDRA guard. install-cassandra-version's install directory becomes overridable so the script can run outside a node, and its ERROR lines now go to stderr where a failure handler can find them. Both suites are wired to Gradle tasks and to the packer-lint workflow.
Two hosts sharing an alias would overwrite each other's outcome and misattribute a failure. Also drops HostOperationsServiceTest's unused BaseKoinTest base — it resolves nothing from Koin.
…gnal Every AMI ships the whole cassandra_versions.yaml, so a lazy entry is always present on a node that has never installed it. Record why the implementation checks the filesystem instead, so the next reader doesn't restore the original design.
|
Review: PR #878 - cassandra install (runtime version install without AMI rebuild) Summary This is a well-designed, well-tested draft. The core idea of extracting install_cassandra_version() into a standalone install-cassandra-version script so bake-time and runtime share one install path is sound, and the extraction looks behaviorally faithful (dead scaffolding correctly dropped, lazy: skip logic correctly added to the bake loop). The final commit fix that makes the already-installed check disk-based (test -d /usr/local/cassandra/) rather than yaml-based is the right call and closes a real bug from an earlier round of this PR. The HostOperationsService fix (rethrow-after-join instead of silently swallowing per-thread exceptions) is a genuine, well-justified correctness fix with good test coverage. ShellQuoting and Redaction are both solid, no bypass found in either. The main thing worth resolving before merge is the git clone / --url argument-injection angle below, plus a --hosts fail-fast edge case and a UseCassandra state-loss interaction. Strengths
Bugs / Correctness Issues
This correctly fails fast when --hosts foo,bar matches nothing. But when hosts.hostList is blank (no --hosts flag given at all) and state.hosts[ServerType.Cassandra] is empty or missing, targeted is empty and the isBlank() disjunct makes require pass trivially. The command then emits InstallingVersion(version, 0, ""), the results loop does nothing, failed stays empty, and the command exits 0 having done nothing. That is the same silent-zero-hosts-success class of bug the commit set out to fix, just not fully closed. Worth either an unconditional require(targeted.isNotEmpty()), or a comment explaining why the blank case is intentionally exempt.
Before this PR, withHosts(parallel = true) swallowed per-host exceptions, so save() always ran. This PR fix correctly makes withHosts rethrow the first host failure after every host has run, but that means if any host fails (for example, the new use-cassandra "version not installed" guard this same PR adds), save() is never reached, and every host that did succeed in that batch has its state.versions update discarded from disk. A later cassandra start would then read stale state for a host that is actually running the new version fine. Not a bug in the diff itself since UseCassandra.kt is not touched, but a direct, real behavioral consequence of the HostOperationsService change, in a caller with exactly the mutate-then-save-after shape. Worth fixing UseCassandra alongside this PR, for example via collectFromHosts plus save per successful host. Checked SetupInstance, ExecList, ExecStop, and Start.kt: none of them have this shape, so UseCassandra appears to be the only affected caller. |
|
Review: PR #878 continued (part 2/2) Security Concerns
$URL is passed as a bare positional argument. A value beginning with -- (for example --upload-pack=) is parsed by git as another option, not as the repository URL - this is the well-known git argument-injection RCE pattern (--upload-pack / --upload-archive let you specify an arbitrary program git runs). ShellQuoting.kt shellQuote() only protects the outer SSH command line from shell metacharacter injection; it does nothing to stop git itself from reinterpreting a value that merely starts with -- as a flag, since such a string is entirely within SAFE_UNQUOTED and passes through unmodified as a single argument either way. This pattern already existed pre-PR (the old inline install_cassandra_version() had the identical git clone ... "$URL" "$version"), sourced only from a hand-curated cassandra_versions.yaml. What is new is that $URL is now also directly reachable via a live, operator-typed "cassandra install --url " CLI flag (CassandraInstall.kt:47-50 into installCommand() into the remote script), a meaningfully wider input surface than editing a YAML file ahead of an AMI bake. Given this repo single-operator / ephemeral-cluster trust model this is not cross-tenant severity, but it is a cheap standard fix worth taking: insert the -- end-of-options sentinel before the positional args, e.g. git clone --depth=1 --single-branch --branch "$BRANCH" -- "$URL" "$version". The same class of issue applies more mildly to the curl -fsSL --retry 3 "$1" -o "$3" fallback fetcher (install-cassandra-version:74) if $1/$URL begins with a dash - lower severity (curl option confusion vs git RCE-capable --upload-pack), but the same -- guard or an explicit case check on a leading dash would close it for both call sites.
Test Coverage Gaps
Minor / Nits
|
ReviewReviewed the diff for the 1. 2. 3. Empty 4. Minor: Nothing here looks blocking — mostly edge cases and a couple of gaps in the redaction/validation logic worth tightening given this touches credential handling and a command whose failures only surface much later ( |
Round 1 redacted only SSHClient's failure branch, and three sinks were missed: the raw command in SSHClient's own debug log, and — on a SUCCESSFUL command — the returned Response and the Ssh.CommandOutput event, which is @serializable and reaches every MCP and Redis subscriber. install-cassandra-version echoes the clone URL it was handed, so a normal successful install published the token. Redaction now happens once, inside SSHClient, covering every sink. The pattern also missed https://TOKEN@host — the single-field form GitHub documents for personal access tokens, and the likeliest real value. The password half is now optional. A secret command's captured output is replaced wholesale rather than attached: tailscale and the axonops setup script both echo the key back in their own failure output.
…oll back Three problems in one code path: The push-up was gated on the version being absent from the node's list, so a resolved --java 21 was silently dropped whenever the version was already declared — the normal state for any baked lazy entry, since the whole yaml ships in every AMI. The node kept saying java 11 and a later cassandra use picked the wrong JDK. It now pushes whenever the node's entry differs from what was resolved, replacing it in place. The pushed entry carried the resolved url — including any embedded token — and it stayed in /etc/cassandra_versions.yaml indefinitely after a successful install. use-cassandra reads only java/python from that file, so url and branch are stripped before it is written. Rollback on failure is gone. It could strip the declaration back out after the binary was already on disk, leaving a version installed but undeclared — and every retry short-circuits on the disk check before it can re-declare, so nothing could repair it. Declared-but-uninstalled is the harmless state; it is what a lazy entry looks like. Also: an empty target set now fails instead of reporting success for an install that never ran, distinguishing an unmatched --hosts from a cluster with no Cassandra nodes at all.
withHosts rethrew the first failure and dropped the rest, so an operator fixing a multi-node problem found the next one only on the following run. The remaining failures are attached as suppressed.
A failed git clone left the credentialed remote in <workdir>/.git/config, because cleanup only ran on the success path — and a failed build is the common case when testing an unmerged branch. An EXIT trap now removes it however the script ends. Also rejects --branch against a tarball --url instead of silently installing the tarball, and adds use-cassandra.test.sh: the guard against selecting a version the node never installed had no automated coverage at all, only a manual container run.
… suites packer-test.yml already runs resolve-build-plan/resolve-ref this way; my round-1 job was in packer-lint.yml, which only lints. Also pins that workflow's two third-party actions to release tags and gives it the contents: read block every other workflow has, and syncs tasks.md 5.3/8.7 to the on-disk idempotency check.
1 participant
Closes #876
Draft — implementation in progress. The unit tier runs locally; the full suite runs in CI on each push.