chore(systest): sync Grafana dashboards in run_systest.sh#10494
Open
basvandijk wants to merge 8 commits into
Open
chore(systest): sync Grafana dashboards in run_systest.sh#10494basvandijk wants to merge 8 commits into
basvandijk wants to merge 8 commits into
Conversation
Move the dfinity-ops/k8s Grafana dashboard checkout out of the `ict` Go tool and into rs/tests/run_systest.sh, gated by the new IC_DASHBOARDS_BRANCH environment variable. Doing the checkout in run_systest.sh makes it work uniformly for both plain testnets (`bazel run //rs/tests/testnets:...`) and colocated tests: the resulting directory is exported as IC_DASHBOARDS_DIR which is already read by the test driver (prometheus_vm.rs) and forwarded into the colocated UVM by colocate_test.rs. A pre-set IC_DASHBOARDS_DIR (e.g. a local clone) is honored and skips the checkout. - run_systest.sh: sparse-checkout the dashboards when IC_DASHBOARDS_BRANCH is set and IC_DASHBOARDS_DIR is not; non-fatal on failure. - ict testnet create: set IC_DASHBOARDS_BRANCH from --k8s-branch instead of checking out the repo itself (dashboards stay on by default). - ict test: drop the dashboard checkout (the driver runs in a sandbox without SSH/network so a runtime checkout cannot work there). - helpers.go: remove the now-unused sparse_checkout helper. - README_NEW.md: document the IC_DASHBOARDS_BRANCH workflow.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR relocates the Grafana dashboard checkout logic from the ict Go CLI into rs/tests/run_systest.sh so dashboards can be provisioned consistently for both plain testnet runs and colocated system tests via IC_DASHBOARDS_DIR.
Changes:
- Add optional, best-effort sparse checkout of
bases/apps/ic-dashboardsfromdfinity-ops/k8sinrun_systest.sh, controlled byIC_DASHBOARDS_BRANCH, exportingIC_DASHBOARDS_DIR. - Update
ict testnet createto setIC_DASHBOARDS_BRANCH(instead of performing the checkout itself) and remove dashboard syncing fromict test. - Remove the now-unused Go helper used for sparse checkouts and document the new workflow in
README_NEW.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
rs/tests/run_systest.sh |
Adds gated dashboards sync + exports IC_DASHBOARDS_DIR from a sparse checkout. |
rs/tests/README_NEW.md |
Documents how to enable dashboards via IC_DASHBOARDS_BRANCH / IC_DASHBOARDS_DIR. |
rs/tests/ict/cmd/testnetCreateCmd.go |
Switches to passing IC_DASHBOARDS_BRANCH for run_systest.sh to handle syncing. |
rs/tests/ict/cmd/testCmd.go |
Removes dashboard checkout logic and the associated flag from ict test. |
rs/tests/ict/cmd/helpers.go |
Removes the unused sparse_checkout helper and related imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- run_systest.sh: make the previous-checkout cleanup non-fatal so a failed `rm` can't abort the test run under `set -e`. - run_systest.sh: run `git clone` with `GIT_SSH_COMMAND="ssh -oBatchMode=yes"` so a missing SSH key / unknown host key fails fast instead of hanging on an interactive prompt; also clone shallowly (`--depth 1`). - README_NEW.md: clarify that the `ict testnet create` dashboard sync is best-effort and the testnet is still deployed if it fails.
- run_systest.sh: keep the `git clone` fully non-interactive on first contact with github.com by setting `StrictHostKeyChecking=accept-new` and an isolated `UserKnownHostsFile` under `$TEST_TMPDIR` (BatchMode alone does not handle an unknown host key). This avoids hanging a Bazel run and avoids touching the user's ~/.ssh/known_hosts. - ict testnet create: when `IC_DASHBOARDS_DIR` is already set (e.g. a local clone), `run_systest.sh` uses it and skips the checkout; log that accurately instead of always claiming a sync from the k8s branch.
Add `ConnectTimeout=15` and `ConnectionAttempts=1` to the git clone's SSH options so the best-effort dashboard sync fails fast on network/DNS issues instead of stalling a Bazel run.
`os.LookupEnv` reports an exported-but-empty `IC_DASHBOARDS_DIR` as set, which would skip setting `IC_DASHBOARDS_BRANCH` even though `run_systest.sh` treats an empty value as unset and would then sync no dashboards. Use `os.Getenv` and check for a non-empty value so both sides agree.
git runs GIT_SSH_COMMAND through a shell, so an unquoted `-oUserKnownHostsFile=$TEST_TMPDIR/k8s_known_hosts` would be split into multiple arguments if $TEST_TMPDIR contained spaces, failing the clone. Single-quote the path value so it survives the shell word-splitting.
- run_systest.sh: when IC_DASHBOARDS_DIR is provided directly, normalize it to an absolute path. The driver is later run with `env -C "$TEST_TMPDIR"`, so a relative value would otherwise resolve against $TEST_TMPDIR and the dashboards wouldn't be found. Falls back to the original value if realpath fails. - ict testnet create: only set/log IC_DASHBOARDS_BRANCH when --k8s-branch is non-empty; warn otherwise. run_systest.sh treats an empty branch as unset and skips syncing, so claiming a sync would be misleading.
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.
We're planning to deprecate the
icttool. The last remaining use case forictwas that it would automatically checkout the Grafana dashboards from thedfinity-ops/k8srepository.This functionality has been moved into
rs/tests/run_systest.sh, gated by a newIC_DASHBOARDS_BRANCHenvironment variable.run_systest.shis the single chokepoint for both the plain testnet driver and the colocated wrapper, so exportingIC_DASHBOARDS_DIRthere makes dashboards work uniformly with no Rust changes:IC_DASHBOARDS_DIRas before (prometheus_vm.rs).colocate_test.rs) already tarsIC_DASHBOARDS_DIR, ships it to the UVM and re-exports it into the inner docker container.A pre-set
IC_DASHBOARDS_DIR(e.g. a local clone) is honored and skips the checkout.Changes
run_systest.sh: sparse-checkoutbases/apps/ic-dashboardswhenIC_DASHBOARDS_BRANCHis set andIC_DASHBOARDS_DIRis not; non-fatal on failure.ict testnet create: setsIC_DASHBOARDS_BRANCHfrom--k8s-branchinstead of doing the checkout itself (dashboards remain on by default).ict test: drops its dashboard checkout (the driver runs in a sandbox without SSH/network, so a runtime checkout can't work there). Users are encouraged to usebazel runto run a test with dashboards.helpers.go: removes the now-unusedsparse_checkouthelper.README_NEW.md: documents theIC_DASHBOARDS_BRANCHworkflow.Usage
Future Work
Start emitting deprecation warnings from
ictand eventually remove it.