andygrove commented on PR #2244:
URL:
https://github.com/apache/datafusion-ballista/pull/2244#issuecomment-5226731477
This is good work, thanks for putting it together. RAII teardown on both
`K8sCluster` and a separate `NamespaceGuard`, `mem::forget` to hand over
ownership only once startup fully succeeds, deadline based polling instead of
fixed sleeps, `free_port()` rather than a hardcoded port, a `CHAOS_KEEP_NS`
escape hatch, and diagnostics dumped before teardown. The rejected alternatives
note about executor pod IPs versus the flight proxy is exactly the kind of
reasoning that makes a PR easy to review.
I checked the "no user facing changes" claim rather than taking it on faith,
and it holds. `SchedulerConfig::default()` already sets `external_host:
"localhost"`, so `env_or("CHAOS_EXTERNAL_HOST", "localhost")` preserves the
process harness behavior exactly, and every other new env var falls back to the
old hardcoded value.
## CI is blocked on ASF Infra, and I think it's worth routing around
`asf-allowlist-check` is failing because `helm/kind-action@v1` isn't on the
ASF allowlist. The check prints the remediation, which is to open a PR against
`apache/infrastructure-actions` adding it. That does work, but it's an external
dependency with unbounded lead time, and we've been bitten by exactly this on
the release workflow before.
The cheaper path is to not need the action at all. This workflow already
installs protoc with `apt-get` and Rust with `rustup` in plain `run:` steps
rather than reaching for actions, so installing kind the same way would be
consistent with what's already there:
```yaml
- name: Install kind
run: |
curl -Lo ./kind https://kind.sigs.k8s.io/dl/v0.30.0/kind-linux-amd64
chmod +x ./kind && sudo mv ./kind /usr/local/bin/kind
kind create cluster --name ballista-chaos --config
chaos-testing/k8s/kind-config.yaml
```
That has the side benefit of pinning the kind version explicitly, which
`@v1` doesn't. And `azure/setup-kubectl@v4` may be droppable outright, since I
believe kubectl ships on the GitHub hosted ubuntu runner images, though that's
worth confirming rather than taking from me.
## One thing that will bite the follow-ups
`delete_namespace` passes `--wait=false`, so teardown is fire and forget and
the namespace sits in `Terminating` for a while afterwards. Meanwhile the
namespace name is `format!("chaos-{}", std::process::id())`, which is constant
for the life of the test binary.
With a single scenario that's fine. But #2029 adds kill, graceful
decommission and total loss scenarios, and as soon as a second scenario in the
same binary calls `K8sCluster::start()`, the `kubectl apply` lands in a
namespace that's still terminating and fails with "unable to create new content
in namespace ... because it is being terminated". An atomic counter or a random
suffix on top of the PID would head that off now, while it's a one line change
rather than a flaky test investigation later.
## Smaller things
- Action pinning is inconsistent with the rest of the repo.
`Swatinem/rust-cache` is pinned to the same SHA we use elsewhere, which is
great, but `actions/[email protected]`, `azure/setup-kubectl@v4` and
`helm/kind-action@v1` are floating tags where most workflows here pin to a full
SHA. Also `swatinem` is lowercase here and `Swatinem` everywhere else, which is
purely cosmetic but easy to fix while you're in there.
- `free_port()` has the usual bind then release race before `kubectl
port-forward` claims the port. Standard practice and low risk, just noting it
exists.
- The empty string sentinel for `advertise_flight_sql_endpoint` is genuinely
confusing, since `Some(proxy) if proxy.is_empty()` at
`scheduler_process.rs:103` is what actually enables the proxy. That's a pre
existing Ballista wart rather than anything you introduced, and your inline
comment explaining it is a real service to the next reader. Might be worth a
separate issue to turn it into an explicit bool.
## Contract checks
No persona red flags. Nothing here touches `ballista-core`, the scheduler or
the executor as shipped code, the `k8s` feature is off by default and carries
no dependencies, and the test is gated behind both the feature and `--test k8s`
so the default `cargo test` is untouched. No public API changes, and no plan
shape or execution changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
