e2e improvements - #987
Conversation
Host-side clients (jmp, curl) resolve <prefix>.jumpstarter.<IP>.nip.io against a public resolver on every invocation. A slow or rate-limited lookup burns the client's entire connect budget and surfaces as "Timeout connecting to grpc....:8082", which reads like a server fault. The IP is already embedded in the nip.io name, so serve the same answer locally. Mirrors the existing dex /etc/hosts handling. Non-nip.io base domains are left alone.
RunCmd folds stderr into the returned string. For a value polled inside
Eventually that is wrong: `-o jsonpath={.items[0].metadata.name}` against
an empty list exits non-zero and prints "array index out of bounds", so
Eventually(...).ShouldNot(BeEmpty()) accepts the error text as a result
and stops waiting on the first attempt. A five-minute wait satisfied
itself in 122ms, and the captured error string was then passed to
`kubectl wait exporters.jumpstarter.dev/<error text>`, turning a clean
RBAC denial into an unreadable failure.
Add KubectlQuery, which returns "" when kubectl fails, and use it for the
polled queries. Guard WaitForExporter against being handed a non-name.
The e2e suite talks to a real cluster over the network, so a spec can fail for reasons unrelated to the code under test. Honour E2E_FLAKE_ATTEMPTS (default 1 locally, 2 in CI) so a single infrastructure hiccup does not fail an unrelated PR. Retried specs are still reported as flaky in the ginkgo summary, so genuine instability stays visible.
A failing spec in an Ordered container skips every spec after it, so a run reports one problem instead of all of them. Add ContinueOnFailure to the six suites whose specs re-establish their own state in AfterEach. The compat suites and exporterset-qemu are genuinely sequential (a "creates resources" spec feeds later ones), so they keep the current behaviour.
fail_after(timeout) wrapped both resolution and connection, so a slow resolver consumed the whole budget and reported "Timeout connecting to <host>:<port>" with an empty cause list — blaming the server for a name that was never resolved. Give resolution its own budget and its own message, and name the resolved IPs in the connect error.
The client and the exporter race: the exporter ends the lease as soon as the hook fails, so which message the client prints depends on how far it got first. If the lease was torn down before the client's first RPC, the client legitimately reports the exporter as unreachable — an outcome the specs did not accept, causing spurious failures. Extract a shared beforeLeaseFailureOutput covering all the outcomes.
A beforeLease hook with onFailure=exit reports BEFORE_LEASE_HOOK_FAILED
and immediately overwrites it with OFFLINE before tearing the session
down. The client waits on [LEASE_READY, BEFORE_LEASE_HOOK_FAILED], so
whether it notices is a race against two back-to-back status writes: win
it and the shell fails in ~5s, lose it and the hook-failed status is
already gone and nothing else it waits for will ever arrive, so it sits
out the full 300s timeout.
In CI this cost 300s per affected spec, nondeterministically — the same
green run had B5 at 25s on arm64 and 306s on amd64, with C2 at 306s on
both. Together those two specs were 41% of a 25-minute suite.
Treat OFFLINE as the terminal outcome it is. The session's status starts
at AVAILABLE, so OFFLINE only ever appears as a deliberate shutdown
transition and cannot be observed as an initial value. The resulting
error carries the exporter's own message ("Exporter shutting down ...").
This leaves the indefinite UNAVAILABLE retry from jumpstarter-dev#606 untouched, so an
exporter restarting under a live lease still does not kill the session.
The two pagination specs each spawned ten jmp processes to create their fixtures, and the exporter spec spawned ten more to delete them. The resources only exist so the client has something to page through, so build a multi-doc manifest and apply it in a single call, and delete the exporters by label selector. Adds MustKubectlApply for piping a manifest to `kubectl apply -f -`.
Every hooks spec stopped the exporter, slept a second, rewrote the config and waited for it to come back, even when the overlay it needed was the one already running. Track the running overlay and skip the restart when it matches; exit-mode specs clear it because they leave the exporter deliberately dead.
Everything runs in one ginkgo process today, so the suite costs the sum of its containers. Add opt-in `--procs` via E2E_PROCS and mark the containers that cannot share a runner as Serial: core and the compat suites write the shared client config, dut-network owns host networking, and exporterset-qemu saturates the CPU under TCG. Default is unchanged (one process), so this only takes effect where it is asked for.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds E2E retries, concurrency controls, host pinning, serial execution, safer Kubernetes polling, graceful exporter shutdown, offline lease handling, separate DNS and connection timeout diagnostics, CI caching, and conditional Python coverage. ChangesE2E environment and execution controls
Kubernetes fixtures and exporter lifecycle
Hook exporter reuse and failure matching
Offline lease and gRPC failure reporting
CI caching and test optimization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant E2EWorkflow
participant Ginkgo
participant Kubernetes
participant Exporter
E2EWorkflow->>Ginkgo: configure retries and process count
Ginkgo->>Kubernetes: apply E2E fixtures
Ginkgo->>Exporter: poll exporter and pod status
Kubernetes-->>Ginkgo: return query output or empty result
Exporter-->>Ginkgo: return status and lease state
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/lib/common.sh`:
- Around line 82-98: Validate E2E_FLAKE_ATTEMPTS and E2E_PROCS as numeric
integer settings immediately after they are assigned and before constructing
flags in the surrounding runner setup. Fail early with a clear error for either
invalid value, while preserving the existing defaults and conditional --procs
behavior for valid values.
In `@python/packages/jumpstarter/jumpstarter/common/grpc.py`:
- Around line 131-135: Update try_with_ip() so when every resolved-address
attempt raises TimeoutError, it propagates the timeout-specific ConnectionError
containing the resolved IP list instead of the generic all-IP failure; retain
existing behavior for mixed or non-timeout failures. Add regression coverage for
both a single resolved address and multiple resolved addresses timing out.
- Around line 70-73: Update the DNS resolution flow around loop.getaddrinfo() to
use a bounded, dedicated executor (or an equivalent cancellable resolver)
instead of the event loop’s default ThreadPoolExecutor. Preserve the existing
timeout behavior while ensuring repeated slow lookups cannot exhaust or delay
unrelated executor work.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c5dc2d93-e8a1-4ad4-8583-6fd950cbe940
📒 Files selected for processing (15)
.github/workflows/e2e.yamle2e/lib/common.she2e/setup-e2e.she2e/test/auth_logging_test.goe2e/test/compat_old_client_test.goe2e/test/compat_old_controller_test.goe2e/test/direct_listener_test.goe2e/test/dut_network_test.goe2e/test/e2e_test.goe2e/test/exit_on_lease_end_test.goe2e/test/exporterset_qemu_test.goe2e/test/hooks_test.goe2e/test/utils.gopython/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter/jumpstarter/common/grpc.py
| # Retry a failed spec instead of failing the whole suite. The e2e suite talks | ||
| # to a real cluster over the network, so a spec can fail for reasons that have | ||
| # nothing to do with the code under test (a slow DNS answer, a pod scheduled | ||
| # late, a router connection dropped). A retried spec is still reported as | ||
| # flaky in the summary, so genuine instability stays visible. | ||
| local flake_attempts="${E2E_FLAKE_ATTEMPTS:-1}" | ||
|
|
||
| # Run top-level containers concurrently when asked. Off by default: the | ||
| # suite shares one cluster and one runner, so more processes is not free. | ||
| # Containers that touch host-global state or the shared client config are | ||
| # marked Serial and still run one at a time, after the parallel ones. | ||
| local procs="${E2E_PROCS:-1}" | ||
|
|
||
| local flags=(-v --show-node-events --trace --timeout "${timeout}" --flake-attempts "${flake_attempts}") | ||
| if [ "${procs}" -gt 1 ]; then | ||
| flags+=(--procs "${procs}") | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'E2E_(FLAKE_ATTEMPTS|PROCS)' .github e2e Makefile 2>/dev/null || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 1268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '70,115p' e2e/lib/common.sh
printf '\n--- behavioral probe ---\n'
(
set +e
E2E_FLAKE_ATTEMPTS=abc E2E_PROCS=0 bash -c '
trap '\''exit_status=$?; printf "exit_status=%d\n" "$exit_status"\'' EXIT
local flake_attempts="${E2E_FLAKE_ATTEMPTS:-1}"
local procs="${E2E_PROCS:-1}"
local flags=(-v --show-node-events --trace --timeout "${timeout}" --flake-attempts "${flake_attempts}")
if [ "${procs}" -gt 1 ]; then
flags+=(--procs "${procs}")
fi
printf "%s\n" "${flags[@]}"
'
)Repository: jumpstarter-dev/jumpstarter
Length of output: 1884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- E2E invocation / common.sh function reference ---'
rg -n 'run-e2e|common\.sh|E2E_FLAKE_ATTEMPTS|E2E_PROCS|ginkgo' e2e Makefile .github/workflows/e2e.yaml 2>/dev/null || true
printf '\n%s\n' '--- behavior probe with non-numeric E2E_FLAKE_ATTEMPTS and valid E2E_PROCS ---'
(
set +e
E2E_FLAKE_ATTEMPTS=abc E2E_PROCS=2 bash -c '
trap '\''exit_status=$?; printf "behavior_probe_exit_status=%d\n" "$exit_status"\'' EXIT
local flake_attempts="${E2E_FLAKE_ATTEMPTS:-1}"
local procs="${E2E_PROCS:-1}"
local flags=(-v --show-node-events --trace --timeout "${timeout}" --flake-attempts "${flake_attempts}")
if [ "${procs}" -gt 1 ]; then
flags+=(--procs "${procs}")
fi
printf "flags=%s\n" "${flags[*]}"
' timeout=30m label_filter=
)
printf '\n%s\n' '--- behavior probe with numeric E2E_FLAKE_ATTEMPTS and non-numeric E2E_PROCS ---'
(
set +e
E2E_FLAKE_ATTEMPTS=2 E2E_PROCS=bad bash -c '
trap '\''exit_status=$?; printf "behavior_probe_exit_status=%d\n" "$exit_status"\'' EXIT
local flake_attempts="${E2E_FLAKE_ATTEMPTS:-1}"
local procs="${E2E_PROCS:-1}"
local flags=(-v --show-node-events --trace --timeout "${timeout}" --flake-attempts "${flake_attempts}")
if [ "${procs}" -gt 1 ]; then
flags+=(--procs "${procs}")
fi
printf "flags=%s\n" "${flags[*]}"
' timeout=30m label_filter=
)
printf '%s\n' '--- behavior probe with E2E_PROCS=0 ---'
(
set +e
E2E_FLAKE_ATTEMPTS=2 E2E_PROCS=0 bash -c '
local flake_attempts="${E2E_FLAKE_ATTEMPTS:-1}"
local procs="${E2E_PROCS:-1}"
local flags=(-v --show-node-events --trace --timeout "${timeout}" --flake-attempts "${flake_attempts}")
if [ "${procs}" -gt 1 ]; then
flags+=(--procs "${procs}")
fi
printf "flags=%s\n" "${flags[*]}"
' timeout=30m label_filter=
)Repository: jumpstarter-dev/jumpstarter
Length of output: 2989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path("e2e/lib/common.sh").read_text()
print("common.txt exists:", Path("e2e/lib/common.sh").exists())
print(text[text.index("run_ginkgo()"):text.index("}", text.index("run_ginkgo()")+1):1200])
import subprocess
for e_flake in ["abc", "", None]: e_flake = f"E2E_FLAKE_ATTEMPTS={e_flake}" if e_flake is not None else ""
for e_procs in ["bad", "0", "2", ""]:
e_procs_full = f"E2E_PROCS={e_procs}" if e_procs else ""
cmd = ["bash","-c",
"local flake_attempts=\"${E2E_FLAKE_ATTEMPTS:-1}\"; local procs=\"${E2E_PROCS:-1}\";"
"local flags=(-v --show-node-events --trace --timeout \"30m\" --flake-attempts \"${flake_attempts}\");"
"if [ \"${procs}\" -gt 1 ]; then flags+=(--procs \"${procs}\"); fi;"
"printf '%s\\n' \"${flags[*]}\""
]
if e_flake: cmd = ["bash"]+cmd; env={}; proc_full=e_procs_full.split("=",1); env[proc_full[0]]=proc_full[1] if len(proc_full)>1 else ""
PYRepository: jumpstarter-dev/jumpstarter
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- run-ginkgo context ---'
sed -n '66,103p' e2e/lib/common.sh
printf '%s\n' '--- run-e2e call context ---'
sed -n '58,72p' e2e/run-e2e.sh
printf '%s\n' '--- make e2e targets ---'
sed -n '190,218p' Makefile
tmpdir="$(mktemp -d)"
cd "$tmpdir"
printf '%s\n' '--- shell probe wrapper ---'
trap 'rm -rf "$WD"' EXIT
run_probe() {
local name="$1"
printf '%s: ' "$name"
E2E_FLAKE_ATTEMPTS="$2" E2E_PROCS="$3" bash -c '
local flake_attempts="${E2E_FLAKE_ATTEMPTS:-1}"
local procs="${E2E_PROCS:-1}"
local flags=(-v --show-node-events --trace --timeout 30m --flake-attempts "${flake_attempts}")
if [ "${procs}" -gt 1 ]; then
flags+=(--procs "${procs}")
fi
printf "%s\n" "${flags[*]}"
' timeout=30m label_filter=
printf "exit=%s\n" "${PIPESTATUS[1]}"
}
run_probe "nonnumeric flake" abc 2
run_probe "empty flake" "" 2
run_probe "numeric zero" 2 0
run_probe "nonnumeric procs" 2 bad
run_probe "numeric zero procs" 2 0
run_probe "valid" 2 2Repository: jumpstarter-dev/jumpstarter
Length of output: 3380
🏁 Script executed:
#!/bin/bash
set -u
tmpdir="$(mktemp -d)"
cd "$tmpdir"
run_probe() {
local name="$1"
printf '%s: ' "$name"
(
E2E_FLAKE_ATTEMPTS="$2" E2E_PROCS="$3" bash -c '
local flaky="${E2E_FLAKE_ATTEMPTS:-1}"
local procs="${E2E_PROCS:-1}"
local flags=(-v --show-node-events --trace --timeout 30m --flake-attempts "${flaky}")
if [ "${procs}" -gt 1 ]; then
flags+=(--procs "${procs}")
fi
printf "%s\n" "${flags[*]}"
'
)
status="${PIPESTATUS[0]}"
if [ "$status" -ne 0 ]; then
printf "exit=%s\n" "$status"
fi
}
run_probe "nonnumeric flake" abc 2
run_probe "empty flake" "" 2
run_probe "numeric zero procs" 2 0
run_probe "nonnumeric procs" 2 bad
run_probe "numeric zero procs" 2 0
run_probe "valid" 2 2
printf '%s\n' '--- compatible run call ---'
sed -n '60,78p' e2e/compat/run.shRepository: jumpstarter-dev/jumpstarter
Length of output: 1940
🏁 Script executed:
#!/bin/bash
set -u
tmpdir="$(mktemp -d)"
cd "$tmpdir" || exit 1
cat > /tmp/runner.sh <<'BASH'
run_ginkgo() {
local test_dir="$1"
shift
local label_filter="${1:-}"
shift || true
local timeout="30m"
local flake_attempts="${E2E_FLAKE_ATTEMPTS:-1}"
local procs="${E2E_PROCS:-1}"
local flags=(-v --show-node-events --trace --timeout "${timeout}" --flake-attempts "${flake_attempts}")
if [ "${procs}" -gt 1 ]; then
flags+=(--procs "${procs}")
fi
if [ -n "$label_filter" ]; then
flags+=(--label-filter "$label_filter")
fi
printf 'flags=%s\n' "${flags[*]}"
}
run_ginkgo "$@"
BASH
run_probe() {
local name="$1"
printf '%s: ' "$name"
(
E2E_FLAKE_ATTEMPTS="$2" E2E_PROCS="$3" bash /tmp/runner.sh "$1"
)
status="${PIPESTATUS[0]}"
if [ "$status" -ne 0 ]; then
printf "exit=%s\n" "$status"
fi
}
run_probe "nonnumeric flake" abc 2
run_probe "empty flake" "" 2
run_probe "empty default flake" "unset" 2
run_probe "numeric zero procs" 2 0
run_probe "nonnumeric procs" 2 bad
run_probe "valid" 2 2
printf '%s\n' '--- compatible run call ---'
sed -n '60,78p' e2e/compat/run.shRepository: jumpstarter-dev/jumpstarter
Length of output: 872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '(^|/)e2e/compat/run\.sh$|(^|/)\.github/workflows/e2e\.yaml$'
git ls-files | rg '(^|/)e2e/run-e2e\.sh$|(^|/)Makefile$'Repository: jumpstarter-dev/jumpstarter
Length of output: 469
Validate E2E runner integer settings.
E2E_PROCS is passed through an integer comparison, so a non-numeric value emits [: integer expression expected and Ginkgo runs without --procs. Fail early for both E2E_FLAKE_ATTEMPTS and E2E_PROCS before constructing the Ginkgo command.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/lib/common.sh` around lines 82 - 98, Validate E2E_FLAKE_ATTEMPTS and
E2E_PROCS as numeric integer settings immediately after they are assigned and
before constructing flags in the surrounding runner setup. Fail early with a
clear error for either invalid value, while preserving the existing defaults and
conditional --procs behavior for valid values.
| loop = asyncio.get_running_loop() | ||
| try: | ||
| with fail_after(timeout): | ||
| ssl_context = ssl.create_default_context() | ||
| ssl_context.check_hostname = False | ||
| ssl_context.verify_mode = ssl.CERT_NONE | ||
|
|
||
| # Resolve all IP addresses for the hostname | ||
| loop = asyncio.get_running_loop() | ||
| addr_info = await loop.getaddrinfo( | ||
| parsed.hostname, port, family=socket.AF_UNSPEC, type=socket.SOCK_STREAM | ||
| ) | ||
|
|
||
| # Log resolved IPs | ||
| resolved_ips = [sockaddr[0] for _, _, _, _, sockaddr in addr_info] | ||
| logger.debug( | ||
| f"Resolved {parsed.hostname} to {len(resolved_ips)} IP(s): {', '.join(resolved_ips)}" | ||
| ) | ||
| addr_info = await loop.getaddrinfo(parsed.hostname, port, family=socket.AF_UNSPEC, type=socket.SOCK_STREAM) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo root files =="
git ls-files | sed -n '1,80p'
echo "== target file lines =="
if [ -f python/packages/jumpstarter/jumpstarter/common/grpc.py ]; then
nl -ba python/packages/jumpstarter/jumpstarter/common/grpc.py | sed -n '1,140p'
else
echo "target file missing"
fi
echo "== references to getaddrinfo/ssl_connect/grpc functions =="
rg -n "loop\.getaddrinfo|fail_after|getaddrinfo|grpc|ssl_connect" python/packages/jumpstarter -S || true
echo "== asyncio version docs from installed python? =="
uv run python3 - <<'PY'
import asyncio, sys
print(sys.version)
print(asyncio.__version__ if hasattr(asyncio, '__version__') else 'no __version__')
print(asyncio.get_running_loop().__class__.__module__)
PYRepository: jumpstarter-dev/jumpstarter
Length of output: 3032
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate grpc.py =="
find . -path './python/packages/jumpstarter/jumpstarter/common/grpc.py' -print
TARGET='python/packages/jumpstarter/jumpstarter/common/grpc.py'
if [ -f "$TARGET" ]; then
echo "== target file outline =="
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline "$TARGET" --view compact || true
fi
echo "== target file lines 1-160 =="
awk '{printf "%6d\t%s\n", NR, $0}' "$TARGET" | sed -n '1,160p'
echo "== target size =="
wc -l "$TARGET"
fi
echo "== references to getaddrinfo / fail_after / grpc =="
rg -n "loop\.getaddrinfo|fail_after|getaddrinfo|grpc|fail_after_async|event_loop|event loop|running_loop" python/packages/jumpstarter python -S || true
echo "== import fail_after candidates =="
rg -n "from .*fail_after|import .*fail_after|def fail_after" python/packages/jumpstarter python -S || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 50384
🌐 Web query:
Python asyncio loop.getaddrinfo cancelled while already running executor does not stop getaddrinfo future cancelling
💡 Result:
The behavior you are observing is expected because asyncio.getaddrinfo is not natively cancellable in the way many other asyncio tasks are [1][2]. Under the hood, asyncio.getaddrinfo is a wrapper around the synchronous socket.getaddrinfo call, which is executed using the loop's default ThreadPoolExecutor [3][4]. Because the underlying call is a blocking, synchronous function running in a separate thread, asyncio has no mechanism to force that specific thread to stop or abort the operation immediately upon cancellation of the asyncio.Future or Task [1][5]. When you cancel an asyncio task awaiting getaddrinfo: 1. The asyncio task itself is cancelled, and the await expression will raise a CancelledError in the coroutine [3]. 2. However, the synchronous socket.getaddrinfo call continues to run to completion within the ThreadPoolExecutor worker thread [1][2]. 3. The result of that background call is essentially discarded by asyncio once the task is cancelled, preventing it from trying to set a result on a cancelled Future [1][2]. This design is a limitation of Python's underlying thread model, which does not provide a safe way to forcefully terminate an arbitrary thread [5]. Consequently, the background thread remains occupied until the getaddrinfo call finishes naturally, even if the application has already "cancelled" the associated task [1][2]. If your application frequently cancels these operations or faces high load, this can lead to thread pool saturation, which may cause delays in other network operations [3][4]. To manage this, you can: - Be aware that getaddrinfo consumes a thread from the default executor, and heavy use can lead to resource contention [3][4]. - If you require more control, you may consider using a dedicated ThreadPoolExecutor with a controlled size for your tasks rather than relying solely on the default loop executor [3][4]. - Understand that closing the event loop (loop.close()) will shut down the executor, but it does not wait for already-running threads to complete, which is why you may sometimes encounter issues if you attempt to clean up immediately after cancellation [6][4].
Citations:
- 1: RuntimeError('Event loop is closed') after cancelling getaddrinfo and closing loop python/cpython#77329
- 2: https://bugs.python.org/issue33148
- 3:
asyncio.getaddrinfomanifests in misleading DNS timeouts when thread pool executor is saturated python/cpython#112169 - 4: https://docs.python.org/3/library/asyncio-eventloop.html
- 5: https://stackoverflow.com/questions/69760390/python-asyncio-cancel-run-in-executor-blocks-mainthread
- 6: https://github.com/python/cpython/blob/3.14/Lib/asyncio/base%5Fevents.py
🌐 Web query:
Python asyncio event loop default executor ThreadPoolExecutor loop.run_in_executor getaddrinfo cancellation future result
💡 Result:
In Python's asyncio, loop.run_in_executor allows you to execute blocking functions in a concurrent.futures.Executor, with the default executor being a ThreadPoolExecutor managed by the event loop [1][2][3]. When you call loop.run_in_executor, it returns an asyncio.Future [4]. If you call.cancel on this future, the effect is limited to the asyncio event loop [5][4]. Specifically, it informs the event loop that you no longer require the result of that operation [4]. Critically, it does not stop, interrupt, or kill the underlying thread or process where the function is running [5][6][4]. Because Python threads cannot be safely interrupted or terminated, the function will continue to execute until it naturally finishes [5][7][4]. This behavior directly impacts asyncio.getaddrinfo. Because getaddrinfo is inherently a synchronous, blocking network call, asyncio implements it by wrapping it in a call to loop.run_in_executor using the default executor [2][3][8]. Consequently, if you cancel a coroutine awaiting getaddrinfo, you are only canceling the asyncio-side wrapper [9][5]. The actual synchronous getaddrinfo call remains running in the background thread until it completes [9][4]. Key considerations regarding this mechanism include: 1. Cancellation Semantics: Calling.cancel on the returned future merely prevents the event loop from processing the result [4]. It does not stop the background work [5][6]. 2. Resource Management: Because the background task continues to run, it may still interact with the event loop (e.g., via call_soon_threadsafe) after you have cancelled the future [10][11]. This can lead to errors such as RuntimeError: Event loop is closed if the loop is shut down while the thread is still attempting to report its result [9][11]. 3. Executor Saturation: Since getaddrinfo uses the default thread pool, high volumes of DNS resolutions can saturate the pool [2][8]. If the pool is saturated, other tasks using run_in_executor(None,...) may experience delays or timeouts [2][8]. 4. Mitigation: If you require the ability to stop background tasks, you must implement your own cooperative cancellation mechanism (e.g., passing a threading.Event or a cancellation flag that the blocking function periodically checks) rather than relying on Future.cancel [10][5][7]. Alternatively, using a ProcessPoolExecutor may allow for termination if the design permits killing worker processes [5][4].
Citations:
- 1: https://docs.python.org/3.11/library/asyncio-eventloop.html
- 2: https://docs.python.org/3/library/asyncio-eventloop.html
- 3: https://github.com/python/cpython/blob/13ff396c019d548ba181cf22c6f39309a300723c/Lib/asyncio/base_events.py
- 4: https://www.pythontutorials.net/blog/asyncio-is-it-possible-to-cancel-a-future-been-run-by-an-executor/
- 5: https://stackoverflow.com/questions/26413613/asyncio-is-it-possible-to-cancel-a-future-been-run-by-an-executor
- 6: run_in_executor not stopping thread after task cancellation in asyncio (Python 3.11) python/cpython#107505
- 7: https://stackoverflow.com/questions/69760390/python-asyncio-cancel-run-in-executor-blocks-mainthread
- 8:
asyncio.getaddrinfomanifests in misleading DNS timeouts when thread pool executor is saturated python/cpython#112169 - 9: RuntimeError('Event loop is closed') after cancelling getaddrinfo and closing loop python/cpython#77329
- 10: https://stackoverflow.com/questions/72542334/cancel-run-in-executor-coroutine-from-the-main-thread-not-working
- 11: https://bugs.python.org/issue33469
Use a bounded executor for loop.getaddrinfo() timeouts.
loop.getaddrinfo() runs through the event loop’s default ThreadPoolExecutor, so cancelling the fail_after(timeout) await does not stop a resolver call already running in a worker. Repeated slow DNS lookups can occupy default executor threads and delay other run_in_executor() work; move DNS resolution to a bounded, dedicated executor or implement a cancellable resolver.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter/jumpstarter/common/grpc.py` around lines 70 - 73,
Update the DNS resolution flow around loop.getaddrinfo() to use a bounded,
dedicated executor (or an equivalent cancellable resolver) instead of the event
loop’s default ThreadPoolExecutor. Preserve the existing timeout behavior while
ensuring repeated slow lookups cannot exhaust or delay unrelated executor work.
| except TimeoutError as e: | ||
| raise ConnectionError(f"Timeout connecting to {parsed.hostname}:{port}") from e | ||
| raise ConnectionError( | ||
| f"Timeout connecting to {parsed.hostname}:{port} after {timeout}s " | ||
| f"(resolved to {', '.join(resolved_ips)})" | ||
| ) from e |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate grpc.py:"
fd -a 'grpc\.py$' . | sed 's#^\./##' | grep 'python/packages/jumpstarter/jumpstarter/common/grpc.py' || true
echo
echo "File size and relevant sections:"
wc -l python/packages/jumpstarter/jumpstarter/common/grpc.py
sed -n '1,220p' python/packages/jumpstarter/jumpstarter/common/grpc.py
echo
echo "Search related usages:"
rg -n "asyncio\.wait_for|fail_after|try_with_ip|_try_connect_and_extract_cert|TimeoutError|all IPs exhausted|resolved_ips" python/packages/jumpstarter/jumpstarter/common/grpc.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 8909
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant tests around grpc.py:"
fd -a 'test.*grpc.*\.py$|.*grpc.*test.*\.py$' test python packages | sed 's#^\./##' || true
echo
rg -n "ssl_channel_credentials|grpc\.py|all IPs exhausted|Timeout connecting|Timeout resolving" -S .
echo
echo "Behavioral probe: asyncio.wait_for timeout and exception type without importing package code"
python3 - <<'PY'
import asyncio
from collections import namedtuple
try:
from anyio import fail_after, TooSlowError
except Exception as e:
print(f"anyio import: {type(e).__name__}: {e}")
raise SystemExit(0)
async def wait_for_always():
await asyncio.sleep(1e9)
async def wait_for_cancelled():
await asyncio.wait_for(wait_for_always(), timeout=1e-3)
class FakeSSLError(Exception):
pass
async def try_with_ip(ip_address: str, timeout: float):
try:
await asyncio.wait_for(
asyncio.open_connection("127.0.0.1", 65535, timeout=timeout),
timeout=timeout,
)
return (ip_address, None, None)
except Exception as e:
return (ip_address, None, e)
async def main():
errs = {}
resolved_ips = ['127.0.0.1', '127.0.0.2']
tasks = [asyncio.create_task(try_with_ip(ip, timeout=1e-3)) for ip in resolved_ips]
try:
for future in asyncio.as_completed(tasks):
ip_address, _, error = await future
errs[ip_address] = error
finally:
for task in tasks:
if not task.done():
task.cancel()
for ip in resolved_ips:
err = errs[ip]
print(f"{ip}: type={type(err).__module__}.{type(err).__name__}, is TimeoutError={isinstance(err, asyncio.TimeoutError)}, is asyncio.CancelledError={isinstance(err, asyncio.CancelledError)}")
if errs:
fail_msg = f"Failed connecting example after 0.001s - all IPs exhausted. Errors: {errs}"
timeout_msg = f"Timeout connecting example after 0.001s (resolved to {', '.join(fail_msg)}]"
else:
print("no errors collected by snippet")
asyncio.run(main())
PYRepository: jumpstarter-dev/jumpstarter
Length of output: 4429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "common grpc tests:"
sed -n '1,260p' python/packages/jumpstarter/jumpstarter/common/grpc_test.py
echo
echo "client grpc tests:"
sed -n '1,260p' python/packages/jumpstarter/jumpstarter/client/grpc_test.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 10009
Preserve the timeout diagnostic for per-IP timeout failures.
try_with_ip() catches per-IP TimeoutErrors from asyncio.wait_for() and stores them in errors. If all attempts time out before the outer fail_after(timeout) expires, callers receive the all-IP failure instead of the timeout-specific message, so the resolved IP list is lost. Emit the timeout ConnectionError for all-timeout failures, or add a timeout failure subtype with that diagnostic. Add regression tests for one and multiple resolved addresses that timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter/jumpstarter/common/grpc.py` around lines 131 -
135, Update try_with_ip() so when every resolved-address attempt raises
TimeoutError, it propagates the timeout-specific ConnectionError containing the
resolved IP list instead of the generic all-IP failure; retain existing behavior
for mixed or non-timeout failures. Add regression coverage for both a single
resolved address and multiple resolved addresses timing out.
diff-cover flagged the split timeout change as uncovered. Exercise the four outcomes it distinguishes: a reachable IP, an unresolvable name, a resolver that stalls past the budget, and a server that never completes the handshake.
Same race as the beforeLease hook, on the other end of the session: an afterLease hook with onFailure=exit reports AFTER_LEASE_HOOK_FAILED and overwrites it with OFFLINE in the next breath, so a poll landing after the overwrite never sees a status the client is waiting for. The poll loop retries UNAVAILABLE indefinitely by design, so nothing else breaks the wait either and the client sits for the full 300s. This is what kept C2 at 306s in CI after B5 dropped to 6s.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/test/utils.go`:
- Around line 821-824: The ProcessTracker.argvMatches method must verify that
the tracked process is the jmp executable before matching run and exporter
arguments, preventing unrelated processes from being killed. Use /proc/<pid>/exe
or an equivalent canonical executable identity available to the tracker, then
retain the existing argument checks; run the requested package tests, type
checks, lint, and full test commands.
- Around line 478-486: Protect ProcessTracker lifecycle state with a single
mutex across track, TrackedPIDs, StopAll, sweepOrphans, and the process-start
path in StartExporterLoop. Mark cleanup as started under that lock, reject any
subsequent process starts, and ensure PID/spec registration and StopAll’s
snapshot-and-clear are serialized so no newly started process is missed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d7cbd01-1095-4402-863f-3ef260a4f73e
📒 Files selected for processing (6)
.github/workflows/build-images.yaml.github/workflows/e2e.yaml.github/workflows/lint.yaml.github/workflows/release-operator-installer.yamle2e/test/e2e_test.goe2e/test/utils.go
| // track records a started process and the argv identity that finds its forked | ||
| // child, so StopAll can sweep an orphan without touching exporters belonging to | ||
| // another ginkgo process. | ||
| func (pt *ProcessTracker) track(pid int, flag, value string) { | ||
| pt.pids = append(pt.pids, pid) | ||
| spec := procSpec{flag: flag, value: value} | ||
| if !slices.Contains(pt.specs, spec) { | ||
| pt.specs = append(pt.specs, spec) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)utils\.go$|(^|/)Makefile$|(^|/)go\.(mod|sum)$' || true
echo "== target context =="
fd -a utils.go . | sed 's#^\./##'
echo "== e2e/test/utils.go outline =="
ast-grep outline e2e/test/utils.go || true
echo "== relevant utils.go sections =="
sed -n '430,835p' e2e/test/utils.go | cat -n
echo "== exported tracker symbols usages =="
rg -n "ProcessTracker|track\\(|TrackedPIDs|StopAll|StartExporterLoop|restart|argvMatches" e2e/test/utils.go e2e || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 31145
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('e2e/test/utils.go')
text = p.read_text()
def occurrences(name):
out=[]
for i,line in enumerate(text.splitlines(),1):
if name in line:
out.append((i,line.strip()))
return out
for name in ['type ProcessTracker','func (pt *ProcessTracker) track','func (pt *ProcessTracker) TrackedPIDs','func (pt *ProcessTracker) StopAll']:
print(f'--- {name} ---')
for i,line in occurrences(name):
print(f'{i}: {line}')
PYRepository: jumpstarter-dev/jumpstarter
Length of output: 519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== e2e/test/utils.go tail sections =="
sed -n '855,1125p' e2e/test/utils.go | cat -n
echo "== StopAll export/usages in e2e =="
python3 - <<'PY'
from pathlib importPath
p=Path('e2e/test/utils.go')
for i,line in enumerate(p.read_text().splitlines(),1):
if 'TrackedPIDs()' in line or 'StopAll' in line:
print(f'{i}: {line}')
PY
echo "== Makefile package definitions =="
rg -n "pkg-test-|pkg-ty-|test:|lint-fix|e2e" Makefile | head -120Repository: jumpstarter-dev/jumpstarter
Length of output: 10310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== util functions around exporter loops/stops =="
sed -n '1220,1440p' e2e/test/utils.go | cat -n
echo "== StopAll/isProcessRunning usages with context =="
rg -n -C 4 "StartExporterLoop|StopAll|TrackedPIDs|IsProcessRunning|WaitForExporterOffline|WaitForExporterOffline" e2e/test e2e | head -220Repository: jumpstarter-dev/jumpstarter
Length of output: 13099
Guard ProcessTracker lifecycle state under a lock.
StopAll cancels the restart context, takes a PID snapshot, and clears pids/specs without a lock, while StartExporterLoop can still start a process and call track after cancellation. That new PID/argv can miss the termination sweep if the process stays alive through SIGTERM. Use one mutex for track, TrackedPIDs, StopAll, sweepOrphans, and the new process start; reject new starts when cleanup has begun.
Also applies to: 549, 577, 602, 633. Run package and full tests plus type and lint checks per the Make targets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/test/utils.go` around lines 478 - 486, Protect ProcessTracker lifecycle
state with a single mutex across track, TrackedPIDs, StopAll, sweepOrphans, and
the process-start path in StartExporterLoop. Mark cleanup as started under that
lock, reject any subsequent process starts, and ensure PID/spec registration and
StopAll’s snapshot-and-clear are serialized so no newly started process is
missed.
Source: Coding guidelines
| func (pt *ProcessTracker) argvMatches(argv []string) bool { | ||
| if len(argv) < 3 || !slices.Contains(argv, "run") { | ||
| return false | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Require jmp executable identity before killing a process.
argvMatches accepts any process that contains run and a tracked flag/value pair. A non-jmp process with matching arguments can receive SIGKILL during cleanup.
Verify /proc/<pid>/exe, or an equivalent canonical executable identity, before matching run and exporter arguments.
As per coding guidelines, run make pkg-test-<package_name>, make pkg-ty-<package_name>, make lint-fix, and make test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/test/utils.go` around lines 821 - 824, The ProcessTracker.argvMatches
method must verify that the tracked process is the jmp executable before
matching run and exporter arguments, preventing unrelated processes from being
killed. Use /proc/<pid>/exe or an equivalent canonical executable identity
available to the tracker, then retain the existing argument checks; run the
requested package tests, type checks, lint, and full test commands.
Source: Coding guidelines
29c12c3 to
346f2cc
Compare
`jmp run` forks and the child calls setsid(), so the PID the tracker holds is the parent. StopAll sent it SIGKILL, which left the exporter itself orphaned with its controller registration intact: the controller then had to time out the heartbeat before the exporter counted as gone, and the pkill fallback reaped the orphan without it ever unregistering. That is the 73s WaitForExporterOffline in the exit_on_lease_end AfterEach. SIGTERM instead, with SIGKILL only as a fallback, so the parent forwards to the child's process group and the child reports OFFLINE and unregisters. The grace period is an upper bound, not a fixed cost. That in turn removes the reason for the flat 2s sleep in WaitForExporter, which cost ~100s across the suite. It was guarding two stale reads, and both have exact conditions available now. After a lease ends, exporterStatus can still read Available from before the lease started, so the poll also requires status.leaseRef to be empty; the controller derives it from the active non-ended leases and the Exporter is reconciled on lease changes through the owner reference, so it clears as soon as the release is processed. Status and lease come from a single query so they cannot be read from different revisions. After a restart the conditions still describe the process that went away, which no status check can detect, so the hooks helper waits for the exporter to go offline where it used to sleep a second. WaitForExporterOffline moved off Kubectl for the same class of reason: an empty result is one of its accepted answers and Kubectl folds stderr into the output, so a failed query was indistinguishable from "offline" and would have ended the wait on the first attempt. Harmless while it sat in an AfterEach; not harmless now that a restart depends on it. Also drop the ExporterSet QEMU polls from 5s. Those conditions are reached by a controller reacting to an event, so the poll period is almost entirely overshoot once the condition holds.
setup-go has cache enabled by default, but every job logged Restore cache failed: Dependencies file is not found in /home/runner/work/jumpstarter/jumpstarter. Supported file pattern: go.mod The action looks for a dependency file next to the working directory to build its cache key, and there is no go.mod at the repo root -- the modules are controller/ and e2e/test/. So the cache never restored and never saved, in all nine setup-go usages across four workflows. Point cache-dependency-path at the go.sum files that do exist. In the e2e job this covers the ~36s spent re-downloading and rebuilding kind and grpcurl on every run.
StopAll ended with a global `pkill -9 -f "jmp run --exporter"`. Two problems, both blocking `ginkgo --procs`: - The pattern is not scoped to the caller, so one ginkgo process's cleanup would reap every other process's exporters. - Being a substring match, "jmp run --exporter" also matches "jmp run --exporter-config", so it reached the direct-listener and dut-network exporters it was never meant to touch. The sweep is still needed: `jmp run` forks (run.py:215) and the child calls setsid() (run.py:255), so the SIGKILL fallback on the tracked parent orphans the child. The child does not re-exec, so it carries the parent's argv and can be matched precisely. Record the flag/value pair each process was started with and sweep /proc for whole-argv matches against only those. This kills the same orphans without touching anything the tracker did not start.
The core suite was Serial for one reason: it selected the active client with `jmp config client use`. That writes `current-client` into the single shared config.yaml (user.py:52), which is process-global state, so core could not run alongside any container that reads it. Being both the largest non-Serial container and Serial, it made E2E_PROCS buy nothing. Name the client explicitly with --client at each jmp invocation instead, matching what the rest of the file already does for the legacy client. `jmp login` needs no such change: it writes per-client files under clients/<alias>.yaml (client.py:115), not the shared config. Nothing else in the parallel set collides: exporter names, lease selectors (oidc/sa/legacy vs hooks, exit-on-lease-end, authlog) and the direct listener's port are all distinct, and every lease deletion is client-scoped. compat_old_controller still uses `config client use`, but it is Serial and runs alone after the parallel batch. With that, turn on E2E_PROCS=2.
Cache controller/bin (kind, kustomize, grpcurl) across runs so the Makefile's go-install-tool guard skips compilation on cache hit. Load all container images into the kind cluster concurrently — each kind load is I/O bound so overlapping them cuts wall-clock to roughly the cost of the single largest image.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
346f2cc to
eec87c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/e2e.yaml (1)
346-346: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTie the controller-tool cache to its installation inputs.
If
controller/Makefilechanges a tool version or install rule without a matching workflow edit, an exact cache hit can restore the oldcontroller/binsymlink.makecan then skip installation and the E2E job can use stale tools. IncludehashFiles('controller/Makefile', '.go-version')in each key, or use one source of truth. (raw.githubusercontent.com)Suggested key update
- key: controller-tools-${{ matrix.arch }}-kind${{ env.KIND_VERSION }}-kustomize${{ env.KUSTOMIZE_VERSION }}-grpcurl${{ env.GRPCURL_VERSION }} + key: controller-tools-${{ matrix.arch }}-${{ hashFiles('controller/Makefile', '.go-version') }}-kind${{ env.KIND_VERSION }}-kustomize${{ env.KUSTOMIZE_VERSION }}-grpcurl${{ env.GRPCURL_VERSION }}Apply the same change to the two compatibility-job keys.
Also applies to: 405-405, 463-463
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e.yaml at line 346, Update the cache keys at the shown location and the two compatibility-job keys to include the hash of controller/Makefile and .go-version alongside the existing architecture and tool-version inputs. Ensure all controller-tool caches use this same installation-input hash so changes to tool versions or install rules invalidate stale controller/bin symlinks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/pyproject.toml`:
- Line 113: Update the pytest addopts setting in pyproject.toml to include
--doctest-modules alongside the existing --capture=no option, restoring doctest
execution for the supported driver packages.
---
Nitpick comments:
In @.github/workflows/e2e.yaml:
- Line 346: Update the cache keys at the shown location and the two
compatibility-job keys to include the hash of controller/Makefile and
.go-version alongside the existing architecture and tool-version inputs. Ensure
all controller-tool caches use this same installation-input hash so changes to
tool versions or install rules invalidate stale controller/bin symlinks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ce3254f2-3022-4337-a44e-f8f84724b165
📒 Files selected for processing (36)
.github/workflows/e2e.yaml.github/workflows/python-tests.yamlcontroller/hack/deploy_with_operator.shpython/Makefilepython/packages/jumpstarter-driver-adb/pyproject.tomlpython/packages/jumpstarter-driver-androidemulator/pyproject.tomlpython/packages/jumpstarter-driver-ble/pyproject.tomlpython/packages/jumpstarter-driver-doip/pyproject.tomlpython/packages/jumpstarter-driver-dut-network/pyproject.tomlpython/packages/jumpstarter-driver-esp32/pyproject.tomlpython/packages/jumpstarter-driver-flashers/pyproject.tomlpython/packages/jumpstarter-driver-http-power/pyproject.tomlpython/packages/jumpstarter-driver-http/pyproject.tomlpython/packages/jumpstarter-driver-mitmproxy/pyproject.tomlpython/packages/jumpstarter-driver-noyito-relay/pyproject.tomlpython/packages/jumpstarter-driver-obd/pyproject.tomlpython/packages/jumpstarter-driver-pi-pico/pyproject.tomlpython/packages/jumpstarter-driver-probe-rs/pyproject.tomlpython/packages/jumpstarter-driver-renode/pyproject.tomlpython/packages/jumpstarter-driver-ridesx/pyproject.tomlpython/packages/jumpstarter-driver-shell/pyproject.tomlpython/packages/jumpstarter-driver-someip/pyproject.tomlpython/packages/jumpstarter-driver-ssh-mitm/pyproject.tomlpython/packages/jumpstarter-driver-ssh-mount/pyproject.tomlpython/packages/jumpstarter-driver-ssh/pyproject.tomlpython/packages/jumpstarter-driver-stlink-msd/pyproject.tomlpython/packages/jumpstarter-driver-tasmota/pyproject.tomlpython/packages/jumpstarter-driver-tftp/pyproject.tomlpython/packages/jumpstarter-driver-tmt/pyproject.tomlpython/packages/jumpstarter-driver-uds-can/pyproject.tomlpython/packages/jumpstarter-driver-uds-doip/pyproject.tomlpython/packages/jumpstarter-driver-uds/pyproject.tomlpython/packages/jumpstarter-driver-vnc/pyproject.tomlpython/packages/jumpstarter-driver-xcp/pyproject.tomlpython/packages/jumpstarter-driver-yepkit/pyproject.tomlpython/pyproject.toml
💤 Files with no reviewable changes (30)
- python/packages/jumpstarter-driver-ssh-mitm/pyproject.toml
- python/packages/jumpstarter-driver-tasmota/pyproject.toml
- python/packages/jumpstarter-driver-noyito-relay/pyproject.toml
- python/packages/jumpstarter-driver-ble/pyproject.toml
- python/packages/jumpstarter-driver-adb/pyproject.toml
- python/packages/jumpstarter-driver-xcp/pyproject.toml
- python/packages/jumpstarter-driver-tftp/pyproject.toml
- python/packages/jumpstarter-driver-tmt/pyproject.toml
- python/packages/jumpstarter-driver-doip/pyproject.toml
- python/packages/jumpstarter-driver-uds/pyproject.toml
- python/packages/jumpstarter-driver-ssh/pyproject.toml
- python/packages/jumpstarter-driver-uds-can/pyproject.toml
- python/packages/jumpstarter-driver-uds-doip/pyproject.toml
- python/packages/jumpstarter-driver-vnc/pyproject.toml
- python/packages/jumpstarter-driver-stlink-msd/pyproject.toml
- python/packages/jumpstarter-driver-someip/pyproject.toml
- python/packages/jumpstarter-driver-http/pyproject.toml
- python/packages/jumpstarter-driver-yepkit/pyproject.toml
- python/packages/jumpstarter-driver-ssh-mount/pyproject.toml
- python/packages/jumpstarter-driver-probe-rs/pyproject.toml
- python/packages/jumpstarter-driver-renode/pyproject.toml
- python/packages/jumpstarter-driver-ridesx/pyproject.toml
- python/packages/jumpstarter-driver-obd/pyproject.toml
- python/packages/jumpstarter-driver-esp32/pyproject.toml
- python/packages/jumpstarter-driver-androidemulator/pyproject.toml
- python/packages/jumpstarter-driver-mitmproxy/pyproject.toml
- python/packages/jumpstarter-driver-http-power/pyproject.toml
- python/packages/jumpstarter-driver-flashers/pyproject.toml
- python/packages/jumpstarter-driver-shell/pyproject.toml
- python/packages/jumpstarter-driver-pi-pico/pyproject.toml
|
|
||
| [tool.pytest.ini_options] | ||
| addopts = "--capture=no --doctest-modules --cov=. --cov-report=html --cov-report=xml" | ||
| addopts = "--capture=no" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cd python
rg -n --glob '*.py' '^\s*>>>' packages examples || true
PYTEST_ADDOPTS="--doctest-modules" \
make test LOGS_DIR="${TMPDIR:-/tmp}/jumpstarter-doctest-logs"Repository: jumpstarter-dev/jumpstarter
Length of output: 7147
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== python/pyproject.toml test section =="
sed -n '100,130p' python/pyproject.toml || true
echo
echo "== doctest markers excluding skip/internal =="
python3 - <<'PY'
import pathlib, re
root = pathlib.Path("python")
files = list(root.rglob("*.py"))
pat = re.compile(r"^\s*>>>\s*")
matches = []
for p in files:
try:
text = p.read_text(encoding="utf-8", errors="replace")
except Exception:
continue
for i,line in enumerate(text.splitlines(), 1):
if pat.match(line):
prev = text.splitlines()[i-2] if i > 1 else ""
if "doctest: +SKIP" in prev or "doctest: +SKIP" in line:
cont = "SKIP"
else:
cont = "RAN"
matches.append((str(p), i, cont, line.strip()))
print(f"total doctest lines: {len(matches)}")
for path,line_no,status,line in matches:
print(f"{path}:{line_no} {status} {line}")
PY
echo
echo "== pytest/test files mentioning doctest or doctest-modules =="
rg -n --glob '*.toml' --glob '*.py' --glob 'Makefile' --glob 'Makefile.*' \
'doctest|doctest-modules|--doctest|pytest_addopts' python .github 2>/dev/null || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 8561
Restore workspace doctests.
This change removes pytest’s --doctest-modules option while the repository still contains supported doctests in python/packages/jumpstarter-driver-power, python/packages/jumpstarter-driver-network, and python/packages/jumpstarter-driver-opendal. Add --doctest-modules back to python/pyproject.toml so package tests continue executing these doc examples.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/pyproject.toml` at line 113, Update the pytest addopts setting in
pyproject.toml to include --doctest-modules alongside the existing --capture=no
option, restoring doctest execution for the supported driver packages.
passt (usermode network stack) fails on GH Actions runners. None of the image pre-configuration steps need network access. Co-authored-by: Cursor <cursoragent@cursor.com>
Class-scoped fixtures keep mitmdump running across tests within each class. Mocks are cleared and reconfigured between tests via hot-reload instead of full process restart (~3s savings per test). Co-authored-by: Cursor <cursoragent@cursor.com>
docker-build, docker-build-exporter-set-controller, build-operator, and cluster creation are independent targets. Running with -j3 lets them overlap, saving ~2-3 minutes of sequential container builds. Co-authored-by: Cursor <cursoragent@cursor.com>
…ntainers Adds a CI-optimized build path that: - Compiles Go binaries directly on the runner (with setup-go module cache) - Packages them into minimal runtime-only containers (no Go toolchain) - Eliminates pulling the ~1.5GB go-toolset image for each build - Eliminates re-downloading modules inside containers without cache New targets: docker-build-ci, build-operator-ci, deploy-operator-ci, test-operator-e2e-ci. The CI workflow now uses setup-go with cache and the -ci targets. Expected savings: 3-5 minutes on the e2e job (module download + compile now benefits from GitHub Actions Go cache across runs). Co-authored-by: Cursor <cursoragent@cursor.com>
Two bugs: 1. virt-customize disabled cloud-init but didn't generate SSH host keys or enable password auth, so `qemu.shell()` (Fabric/SSH) got "Error reading SSH protocol banner". Fix: ssh-keygen -A + PasswordAuthentication yes + enable sshd. 2. test-report ran before all tests finished due to missing Make dependency. With -j8, Make started test-report in parallel with pkg-test-all because test-report had no prerequisites. It printed "All package tests passed" 23 seconds before the QEMU test actually failed. Fix: test-report now depends on pkg-test-all. Bump image cache key to v2 to pick up the SSH changes. Co-authored-by: Cursor <cursoragent@cursor.com>
144d864 to
23e89f8
Compare
- Remove gha-cleanup step from python-tests: the runner has 88GB free, Python tests need ~5GB. The cleanup was wasting ~1.5 minutes. - Merge 3 separate apt-get update + install steps into one: saves ~15-20s of redundant package index downloads. - Apply setup-go + host-compiled CI targets to deploy-kind job too (was only on e2e-test-operator). Saves ~3-4 minutes there. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
No description provided.