diff --git a/.github/actions/read-tool-versions/action.yaml b/.github/actions/read-tool-versions/action.yaml new file mode 100644 index 000000000..7a464f736 --- /dev/null +++ b/.github/actions/read-tool-versions/action.yaml @@ -0,0 +1,7 @@ +name: Read tool versions from Makefile +description: Export KIND_VERSION, KUSTOMIZE_VERSION, and GRPCURL_VERSION from controller/Makefile into $GITHUB_ENV +runs: + using: composite + steps: + - run: sed -nE 's/^(KIND_VERSION|KUSTOMIZE_VERSION|GRPCURL_VERSION) \?= (.*)/\1=\2/p' controller/Makefile >> "$GITHUB_ENV" + shell: bash diff --git a/.github/workflows/build-images.yaml b/.github/workflows/build-images.yaml index 73ed93bc1..6f27c79bd 100644 --- a/.github/workflows/build-images.yaml +++ b/.github/workflows/build-images.yaml @@ -135,6 +135,7 @@ jobs: uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 with: go-version-file: .go-version + cache-dependency-path: "**/go.sum" - name: Build operator installer manifest if: ${{ matrix.generate_installer && steps.check.outputs.skip != 'true' }} diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index f7e2ccf46..b10f0b998 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -10,6 +10,14 @@ permissions: env: CONTAINER_TOOL: docker + # Retry a failed spec once before failing the job. Retries are reported as + # flakes in the ginkgo summary, so this hides nothing; it only stops a single + # infrastructure hiccup from failing an unrelated PR. + E2E_FLAKE_ATTEMPTS: "2" + # Run the non-Serial containers two at a time. The runner has 4 CPUs and the + # suite spends most of its time waiting on the cluster, so a second process + # overlaps the two long containers (hooks, core) rather than competing for CPU. + E2E_PROCS: "2" jobs: changes: @@ -68,6 +76,7 @@ jobs: uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 with: go-version-file: .go-version + cache-dependency-path: "**/go.sum" - name: Cache controller image id: cache @@ -107,6 +116,7 @@ jobs: uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 with: go-version-file: .go-version + cache-dependency-path: "**/go.sum" - name: Cache operator artifacts id: cache @@ -151,6 +161,7 @@ jobs: uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 with: go-version-file: .go-version + cache-dependency-path: "**/go.sum" - name: Cache exporterset-controller image id: cache @@ -310,6 +321,7 @@ jobs: uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 with: go-version-file: .go-version + cache-dependency-path: "**/go.sum" - name: Load e2e artifacts uses: ./.github/actions/load-e2e-artifacts @@ -327,6 +339,15 @@ jobs: sudo modprobe "$mod" 2>/dev/null || true done + - name: Read tool versions from Makefile + uses: ./.github/actions/read-tool-versions + + - name: Cache controller tools + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6 + with: + path: controller/bin + key: controller-tools-${{ matrix.arch }}-kind${{ env.KIND_VERSION }}-kustomize${{ env.KUSTOMIZE_VERSION }}-grpcurl${{ env.GRPCURL_VERSION }} + - name: Setup e2e test environment run: make e2e-setup env: @@ -374,6 +395,16 @@ jobs: uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 with: go-version-file: .go-version + cache-dependency-path: "**/go.sum" + + - name: Read tool versions from Makefile + uses: ./.github/actions/read-tool-versions + + - name: Cache controller tools + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6 + with: + path: controller/bin + key: controller-tools-amd64-kind${{ env.KIND_VERSION }}-kustomize${{ env.KUSTOMIZE_VERSION }}-grpcurl${{ env.GRPCURL_VERSION }} - name: Setup compat environment (old controller v0.8.1) run: make e2e-compat-setup COMPAT_SCENARIO=old-controller @@ -416,12 +447,22 @@ jobs: uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 with: go-version-file: .go-version + cache-dependency-path: "**/go.sum" - name: Load e2e artifacts uses: ./.github/actions/load-e2e-artifacts with: arch: amd64 + - name: Read tool versions from Makefile + uses: ./.github/actions/read-tool-versions + + - name: Cache controller tools + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6 + with: + path: controller/bin + key: controller-tools-amd64-kind${{ env.KIND_VERSION }}-kustomize${{ env.KUSTOMIZE_VERSION }}-grpcurl${{ env.GRPCURL_VERSION }} + - name: Setup compat environment (old client v0.7.4) run: make e2e-compat-setup COMPAT_SCENARIO=old-client env: diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml index 79b0a1a91..83270e72a 100644 --- a/.github/workflows/lint.yaml +++ b/.github/workflows/lint.yaml @@ -55,6 +55,7 @@ jobs: uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 with: go-version-file: .go-version + cache-dependency-path: "**/go.sum" - name: Run go linter working-directory: controller diff --git a/.github/workflows/release-operator-installer.yaml b/.github/workflows/release-operator-installer.yaml index 1cef2616d..3f93341ff 100644 --- a/.github/workflows/release-operator-installer.yaml +++ b/.github/workflows/release-operator-installer.yaml @@ -20,6 +20,7 @@ jobs: uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 with: go-version-file: .go-version + cache-dependency-path: "**/go.sum" - name: Build operator installer manifest env: diff --git a/controller/hack/deploy_with_operator.sh b/controller/hack/deploy_with_operator.sh index 308737b6f..b8ec7a281 100755 --- a/controller/hack/deploy_with_operator.sh +++ b/controller/hack/deploy_with_operator.sh @@ -29,23 +29,40 @@ if [ "${USE_CERTMANAGER}" = "true" ]; then fi fi -# load the container images into the cluster -load_image "${IMG}" -load_image "${OPERATOR_IMG}" -load_image "${EXPORTER_SET_CONTROLLER_IMG}" -# Exporter + QEMU runtime images are required for ExporterSet QEMU e2e / samples. -# Missing images are skipped so plain controller deploys still work. +# Load container images into the cluster in parallel. Each `kind load` is I/O +# bound (piping a tarball into the node's containerd), so overlapping them cuts +# wall-clock to roughly the cost of the single largest image. +_load_pids=() +_load_failed=0 + +load_image "${IMG}" & +_load_pids+=($!) +load_image "${OPERATOR_IMG}" & +_load_pids+=($!) +load_image "${EXPORTER_SET_CONTROLLER_IMG}" & +_load_pids+=($!) + if container_image_exists "${EXPORTER_IMG}"; then - load_image "${EXPORTER_IMG}" + load_image "${EXPORTER_IMG}" & + _load_pids+=($!) else echo -e "${YELLOW}Skipping load of exporter image (not present locally): ${EXPORTER_IMG}${NC}" fi if container_image_exists "${QEMU_RUNTIME_IMG}"; then - load_image "${QEMU_RUNTIME_IMG}" + load_image "${QEMU_RUNTIME_IMG}" & + _load_pids+=($!) else echo -e "${YELLOW}Skipping load of qemu-runtime image (not present locally): ${QEMU_RUNTIME_IMG}${NC}" fi +for pid in "${_load_pids[@]}"; do + wait "${pid}" || _load_failed=1 +done +if [ "${_load_failed}" -eq 1 ]; then + echo -e "${RED}One or more images failed to load${NC}" + exit 1 +fi + # Deploy the operator echo -e "${GREEN}Deploying Jumpstarter operator ...${NC}" kubectl apply -f deploy/operator/dist/install.yaml diff --git a/e2e/lib/common.sh b/e2e/lib/common.sh index 9ee3b6a42..6a15a7f70 100644 --- a/e2e/lib/common.sh +++ b/e2e/lib/common.sh @@ -79,7 +79,23 @@ run_ginkgo() { timeout="60m" fi - local flags=(-v --show-node-events --trace --timeout "${timeout}") + # 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 if [ -n "$label_filter" ]; then flags+=(--label-filter "$label_filter") fi diff --git a/e2e/setup-e2e.sh b/e2e/setup-e2e.sh index 702cefaef..5291b8df5 100755 --- a/e2e/setup-e2e.sh +++ b/e2e/setup-e2e.sh @@ -275,6 +275,35 @@ deploy_controller() { # shellcheck source=lib/install.sh source "$SCRIPT_DIR/lib/install.sh" +# Pin the ingress hostnames in /etc/hosts so that host-side clients (jmp, curl) +# never depend on public DNS. +# +# baseDomain is a nip.io wildcard name of the form jumpstarter..nip.io, so +# every jmp invocation would otherwise resolve .jumpstarter..nip.io +# against a public resolver. A slow or rate-limited lookup burns the client's +# whole connect budget and surfaces as "Timeout connecting to grpc....:8082". +# The IP is already embedded in the name, so we can serve the same answer +# locally. Non-nip.io base domains are left alone. +pin_basedomain_hosts_entries() { + local basedomain="$1" + local ip + + ip=$(echo "${basedomain}" | sed -nE 's/^.*\.([0-9]+\.[0-9]+\.[0-9]+\.[0-9]+)\.nip\.io$/\1/p') + if [ -z "${ip}" ]; then + log_info "baseDomain ${basedomain} is not a nip.io name, leaving DNS resolution alone" + return 0 + fi + + if grep -Fq "grpc.${basedomain}" /etc/hosts 2>/dev/null; then + log_info "✓ ${basedomain} entries already in /etc/hosts" + return 0 + fi + + log_warn "About to add ${basedomain} entries to /etc/hosts (requires sudo)" + echo "${ip} ${basedomain} grpc.${basedomain} router.${basedomain} login.${basedomain}" | sudo tee -a /etc/hosts + log_info "✓ Pinned ${basedomain} to ${ip} in /etc/hosts" +} + # Step 6: Setup test environment setup_test_environment() { log_info "Setting up test environment..." @@ -290,6 +319,7 @@ setup_test_environment() { log_error "Failed to get baseDomain from Jumpstarter CR in namespace ${JS_NAMESPACE}" exit 1 fi + pin_basedomain_hosts_entries "${BASEDOMAIN}" export ENDPOINT="grpc.${BASEDOMAIN}:8082" export LOGIN_ENDPOINT="login.${BASEDOMAIN}:8086" log_info "Controller endpoint: $ENDPOINT" diff --git a/e2e/test/auth_logging_test.go b/e2e/test/auth_logging_test.go index 26c96c794..1cd8969e6 100644 --- a/e2e/test/auth_logging_test.go +++ b/e2e/test/auth_logging_test.go @@ -35,7 +35,7 @@ import ( // with legacy (controller-issued) tokens so the token lives in a local config // file where the test can corrupt it. It is intentionally NOT labelled for the // compat suites — old controller images do not have auth-failure logging. -var _ = Describe("Auth Failure Logging E2E Tests", Label("auth-logging"), Ordered, func() { +var _ = Describe("Auth Failure Logging E2E Tests", Label("auth-logging"), Ordered, ContinueOnFailure, func() { const ( clientName = "test-client-authlog" exporterName = "test-exporter-authlog" diff --git a/e2e/test/compat_old_client_test.go b/e2e/test/compat_old_client_test.go index 5aaf902a8..f3586c18f 100644 --- a/e2e/test/compat_old_client_test.go +++ b/e2e/test/compat_old_client_test.go @@ -26,7 +26,8 @@ import ( . "github.com/onsi/gomega" //nolint:revive ) -var _ = Describe("Compat: Old Client E2E Tests", Label("compat", "old-client"), Ordered, func() { +// Serial: installs an older client into the shared environment. +var _ = Describe("Compat: Old Client E2E Tests", Label("compat", "old-client"), Ordered, Serial, func() { var ( tracker *ProcessTracker ns string diff --git a/e2e/test/compat_old_controller_test.go b/e2e/test/compat_old_controller_test.go index f3ce3bc8e..3c10271cc 100644 --- a/e2e/test/compat_old_controller_test.go +++ b/e2e/test/compat_old_controller_test.go @@ -26,7 +26,8 @@ import ( . "github.com/onsi/gomega" //nolint:revive ) -var _ = Describe("Compat: Old Controller E2E Tests", Label("compat", "old-controller"), Ordered, func() { +// Serial: replaces the running controller and switches the active client. +var _ = Describe("Compat: Old Controller E2E Tests", Label("compat", "old-controller"), Ordered, Serial, func() { var ( tracker *ProcessTracker ns string diff --git a/e2e/test/direct_listener_test.go b/e2e/test/direct_listener_test.go index 0b301c7a8..7f082e485 100644 --- a/e2e/test/direct_listener_test.go +++ b/e2e/test/direct_listener_test.go @@ -26,7 +26,7 @@ import ( . "github.com/onsi/gomega" //nolint:revive ) -var _ = Describe("Direct Listener E2E Tests", Label("direct-listener"), Ordered, func() { +var _ = Describe("Direct Listener E2E Tests", Label("direct-listener"), Ordered, ContinueOnFailure, func() { var ( tracker *ProcessTracker listenerPort = 19090 diff --git a/e2e/test/dut_network_test.go b/e2e/test/dut_network_test.go index 1b3fe5cda..4cb426da4 100644 --- a/e2e/test/dut_network_test.go +++ b/e2e/test/dut_network_test.go @@ -51,7 +51,9 @@ func sudoArgs(args ...string) (string, []string) { return args[0], args[1:] } -var _ = Describe("DUT Network E2E Tests", Label("dut-network"), Ordered, func() { +// Serial: builds veth pairs, bridges and nftables rules in the host network +// namespace, and drives dnsmasq. There is only one host to share. +var _ = Describe("DUT Network E2E Tests", Label("dut-network"), Ordered, ContinueOnFailure, Serial, func() { var ( tracker *ProcessTracker listenerPort = 19091 diff --git a/e2e/test/e2e_test.go b/e2e/test/e2e_test.go index ddb4a8f38..bb4b80ee2 100644 --- a/e2e/test/e2e_test.go +++ b/e2e/test/e2e_test.go @@ -26,7 +26,11 @@ import ( . "github.com/onsi/gomega" //nolint:revive ) -var _ = Describe("Core E2E Tests", Label("core"), Ordered, func() { +// Every jmp invocation here names its client explicitly with --client rather +// than selecting one with `jmp config client use`. That call writes the shared +// client config, which is process-global state: it would make these specs +// unsafe to run alongside any other container that reads it. +var _ = Describe("Core E2E Tests", Label("core"), Ordered, ContinueOnFailure, func() { var tracker *ProcessTracker BeforeAll(func() { @@ -361,55 +365,63 @@ var _ = Describe("Core E2E Tests", Label("core"), Ordered, func() { Context("Lease operations", func() { It("can operate on leases", func() { WaitForExporters("test-exporter-oidc", "test-exporter-sa", "test-exporter-legacy") - MustJmp("config", "client", "use", "test-client-oidc") - MustJmp("create", "lease", "--selector", "example.com/board=oidc", "--duration", "1d") - MustJmp("get", "leases") - MustJmp("get", "exporters") + MustJmp("create", "lease", "--client", "test-client-oidc", + "--selector", "example.com/board=oidc", "--duration", "1d") + MustJmp("get", "leases", "--client", "test-client-oidc") + MustJmp("get", "exporters", "--client", "test-client-oidc") // Verify label selector filtering (regression test for #36) - out, err := Jmp("get", "leases", "--selector", "example.com/board=oidc", "-o", "yaml") + out, err := Jmp("get", "leases", "--client", "test-client-oidc", + "--selector", "example.com/board=oidc", "-o", "yaml") Expect(err).NotTo(HaveOccurred(), out) Expect(out).To(ContainSubstring("example.com/board=oidc")) - out, err = Jmp("get", "leases", "--selector", "example.com/board=doesnotexist") + out, err = Jmp("get", "leases", "--client", "test-client-oidc", + "--selector", "example.com/board=doesnotexist") Expect(err).NotTo(HaveOccurred(), out) Expect(out).To(Equal("No resources found.")) // Test complex selectors with matchExpressions - MustJmp("create", "lease", "--selector", "example.com/board=sa,!nonexistent", "--duration", "1d") + MustJmp("create", "lease", "--client", "test-client-oidc", + "--selector", "example.com/board=sa,!nonexistent", "--duration", "1d") - out, err = Jmp("get", "leases", "--selector", "example.com/board=sa", "-o", "yaml") + out, err = Jmp("get", "leases", "--client", "test-client-oidc", + "--selector", "example.com/board=sa", "-o", "yaml") Expect(err).NotTo(HaveOccurred(), out) Expect(out).To(ContainSubstring("example.com/board=sa")) - out, err = Jmp("get", "leases", "--selector", "!nonexistent", "-o", "yaml") + out, err = Jmp("get", "leases", "--client", "test-client-oidc", + "--selector", "!nonexistent", "-o", "yaml") Expect(err).NotTo(HaveOccurred(), out) Expect(out).To(ContainSubstring("!nonexistent")) - out, err = Jmp("get", "leases", "--selector", "example.com/board=sa,!production", "-o", "yaml") + out, err = Jmp("get", "leases", "--client", "test-client-oidc", + "--selector", "example.com/board=sa,!production", "-o", "yaml") Expect(err).NotTo(HaveOccurred(), out) Expect(out).To(ContainSubstring("example.com/board=sa")) - out, err = Jmp("get", "leases", "--selector", "example.com/board=sa,!example.com/board") + out, err = Jmp("get", "leases", "--client", "test-client-oidc", + "--selector", "example.com/board=sa,!example.com/board") Expect(err).NotTo(HaveOccurred(), out) Expect(out).To(Equal("No resources found.")) - out, err = Jmp("get", "leases", "--selector", "example.com/board=sa,!nonexistent,region=us") + out, err = Jmp("get", "leases", "--client", "test-client-oidc", + "--selector", "example.com/board=sa,!nonexistent,region=us") Expect(err).NotTo(HaveOccurred(), out) Expect(out).To(Equal("No resources found.")) - MustJmp("delete", "leases", "--all") + MustJmp("delete", "leases", "--client", "test-client-oidc", "--all") }) It("can create a lease with context metadata", func() { WaitForExporters("test-exporter-oidc", "test-exporter-sa", "test-exporter-legacy") - MustJmp("config", "client", "use", "test-client-oidc") DeferCleanup(func() { - MustJmp("delete", "leases", "--all") + MustJmp("delete", "leases", "--client", "test-client-oidc", "--all") }) out := MustJmp("create", "lease", + "--client", "test-client-oidc", "--selector", "example.com/board=oidc", "--duration", "1d", "--context", "build_id=nightly-42", @@ -426,64 +438,85 @@ var _ = Describe("Core E2E Tests", Label("core"), Ordered, func() { It("paginated lease listing returns all leases", func() { WaitForExporters("test-exporter-oidc", "test-exporter-sa", "test-exporter-legacy") - MustJmp("config", "client", "use", "test-client-oidc") + // As with the exporter pagination spec, the leases are fixtures for + // the client's pagination, so create them in a single apply. + var manifest strings.Builder for i := 1; i <= 10; i++ { - out, err := Jmp("create", "lease", "--selector", "example.com/board=oidc", "--duration", "1d") - Expect(err).NotTo(HaveOccurred(), out) + fmt.Fprintf(&manifest, `--- +apiVersion: jumpstarter.dev/v1alpha1 +kind: Lease +metadata: + name: pagination-lease-%d +spec: + clientRef: + name: test-client-oidc + duration: 24h + selector: + matchLabels: + example.com/board: oidc +`, i) } + MustKubectlApply(manifest.String()) - out, err := Jmp("get", "leases", "--page-size", "5", "-o", "name") + out, err := Jmp("get", "leases", "--client", "test-client-oidc", + "--page-size", "5", "-o", "name") Expect(err).NotTo(HaveOccurred(), out) lines := strings.Split(strings.TrimSpace(out), "\n") Expect(lines).To(HaveLen(10)) - MustJmp("delete", "leases", "--all") + MustJmp("delete", "leases", "--client", "test-client-oidc", "--all") }) It("paginated exporter listing returns all exporters", func() { WaitForExporters("test-exporter-oidc", "test-exporter-sa", "test-exporter-legacy") - MustJmp("config", "client", "use", "test-client-oidc") - ns := Namespace() + // The exporters are fixtures for the client's pagination, so + // create them in a single apply rather than one jmp process each. + var manifest strings.Builder for i := 1; i <= 10; i++ { name := fmt.Sprintf("pagination-exp-%d", i) - out, err := Jmp("admin", "create", "exporter", "-n", ns, name, - "--nointeractive", "-l", "pagination=true", - "--oidc-username", fmt.Sprintf("dex:%s", name)) - Expect(err).NotTo(HaveOccurred(), out) + fmt.Fprintf(&manifest, `--- +apiVersion: jumpstarter.dev/v1alpha1 +kind: Exporter +metadata: + name: %s + labels: + pagination: "true" +spec: + username: dex:%s +`, name, name) } + MustKubectlApply(manifest.String()) - out, err := Jmp("get", "exporters", "--selector", "pagination=true", "--page-size", "5", "-o", "name") + out, err := Jmp("get", "exporters", "--client", "test-client-oidc", + "--selector", "pagination=true", "--page-size", "5", "-o", "name") Expect(err).NotTo(HaveOccurred(), out) lines := strings.Split(strings.TrimSpace(out), "\n") Expect(lines).To(HaveLen(10)) - for i := 1; i <= 10; i++ { - MustJmp("admin", "delete", "exporter", "--namespace", ns, fmt.Sprintf("pagination-exp-%d", i), "--delete") - } + MustKubectl("-n", Namespace(), "delete", "exporters.jumpstarter.dev", + "-l", "pagination=true", "--wait=false") }) It("lease listing shows expires at and remaining columns", func() { WaitForExporters("test-exporter-oidc", "test-exporter-sa", "test-exporter-legacy") - MustJmp("config", "client", "use", "test-client-oidc") - - MustJmp("create", "lease", "--selector", "example.com/board=oidc", "--duration", "1d") + MustJmp("create", "lease", "--client", "test-client-oidc", + "--selector", "example.com/board=oidc", "--duration", "1d") out, err := RunCmdWithEnv(map[string]string{"COLUMNS": "200"}, - "jmp", "get", "leases") + "jmp", "get", "leases", "--client", "test-client-oidc") Expect(err).NotTo(HaveOccurred(), out) Expect(out).To(ContainSubstring("EXPIRES AT")) Expect(out).To(ContainSubstring("REMAINING")) - MustJmp("delete", "leases", "--all") + MustJmp("delete", "leases", "--client", "test-client-oidc", "--all") }) It("can transfer lease to another client", func() { WaitForExporters("test-exporter-oidc", "test-exporter-sa", "test-exporter-legacy") - MustJmp("config", "client", "use", "test-client-oidc") - - out := MustJmp("create", "lease", "--selector", "example.com/board=oidc", + out := MustJmp("create", "lease", "--client", "test-client-oidc", + "--selector", "example.com/board=oidc", "--duration", "1d", "-o", "yaml") // Parse the lease YAML to extract the lease name. @@ -499,7 +532,8 @@ var _ = Describe("Core E2E Tests", Label("core"), Ordered, func() { MustKubectl("-n", ns, "wait", "--timeout", "60s", "--for=condition=Ready", fmt.Sprintf("leases.jumpstarter.dev/%s", leaseName)) - out, err := Jmp("update", "lease", leaseName, "--to-client", "test-client-legacy", "-o", "yaml") + out, err := Jmp("update", "lease", leaseName, "--client", "test-client-oidc", + "--to-client", "test-client-legacy", "-o", "yaml") Expect(err).NotTo(HaveOccurred(), out) Expect(out).To(ContainSubstring("test-client-legacy")) diff --git a/e2e/test/exit_on_lease_end_test.go b/e2e/test/exit_on_lease_end_test.go index 9a0aa914f..219e703dd 100644 --- a/e2e/test/exit_on_lease_end_test.go +++ b/e2e/test/exit_on_lease_end_test.go @@ -24,7 +24,7 @@ import ( . "github.com/onsi/gomega" //nolint:revive ) -var _ = Describe("Exit On Lease End E2E Tests", Label("exit-on-lease-end"), Ordered, func() { +var _ = Describe("Exit On Lease End E2E Tests", Label("exit-on-lease-end"), Ordered, ContinueOnFailure, func() { var ( tracker *ProcessTracker exporterConfigPath string diff --git a/e2e/test/exporterset_qemu_test.go b/e2e/test/exporterset_qemu_test.go index eaecb1a46..36975de64 100644 --- a/e2e/test/exporterset_qemu_test.go +++ b/e2e/test/exporterset_qemu_test.go @@ -29,6 +29,17 @@ import ( const exporterSetQemuClientName = "test-client-exporterset-qemu" +// Poll periods for the waits in this file. The conditions here are reached by a +// controller reacting to an event rather than by anything on a fixed schedule, +// so the poll period is almost entirely overshoot once the condition holds. +const ( + // qemuPollPeriod is for waits that run a single kubectl query per attempt. + qemuPollPeriod = time.Second + // qemuComposePollPeriod is for waits that run several queries per attempt, + // where the attempt itself already costs a good fraction of a second. + qemuComposePollPeriod = 2 * time.Second +) + // qemuGuestArch holds native ExporterSet QEMU e2e identifiers for the host. type qemuGuestArch struct { Arch string @@ -62,7 +73,9 @@ func loadQemuGuestArch() qemuGuestArch { } } -var _ = Describe("ExporterSet QEMU E2E Tests", Label("exporterset-qemu"), Ordered, func() { +// Serial: boots VMs under TCG emulation, which will starve every other spec +// on the runner if it shares the CPU. +var _ = Describe("ExporterSet QEMU E2E Tests", Label("exporterset-qemu"), Ordered, Serial, func() { var ( ns string manifest string @@ -117,28 +130,25 @@ var _ = Describe("ExporterSet QEMU E2E Tests", Label("exporterset-qemu"), Ordere By("waiting for ExporterSet to create an exporter") var exporterName string Eventually(func() string { - out, _ := Kubectl("-n", ns, "get", "exporter", + exporterName = KubectlQuery("-n", ns, "get", "exporter", "-l", guest.Selector, "-o", "jsonpath={.items[0].metadata.name}") - exporterName = out - return out - }, 5*time.Minute, 5*time.Second).ShouldNot(BeEmpty()) + return exporterName + }, 5*time.Minute, qemuPollPeriod).ShouldNot(BeEmpty()) By(fmt.Sprintf("waiting for exporter %s Online/Registered/Available", exporterName)) WaitForExporter(exporterName) By("waiting for Pod Ready") Eventually(func() string { - out, _ := Kubectl("-n", ns, "get", "pod", exporterName, + return KubectlQuery("-n", ns, "get", "pod", exporterName, "-o", "jsonpath={.status.phase}") - return out - }, 5*time.Minute, 5*time.Second).Should(Equal("Running")) + }, 5*time.Minute, qemuPollPeriod).Should(Equal("Running")) Eventually(func() string { - out, _ := Kubectl("-n", ns, "get", "pod", exporterName, + return KubectlQuery("-n", ns, "get", "pod", exporterName, "-o", "jsonpath={.status.containerStatuses[*].ready}") - return out - }, 5*time.Minute, 5*time.Second).Should(ContainSubstring("true")) + }, 5*time.Minute, qemuPollPeriod).Should(ContainSubstring("true")) By(fmt.Sprintf("verifying runtime image provides %s", guest.QemuBinary)) // fedora-minimal has no `which`; use a shell builtin. @@ -151,14 +161,13 @@ var _ = Describe("ExporterSet QEMU E2E Tests", Label("exporterset-qemu"), Ordere It("leases, flashes Alpine, and boots to a console login marker", func() { By("waiting for a Running pod so we can read shared volume SizeLimit") Eventually(func() string { - out, _ := Kubectl("-n", ns, "get", "pod", + return KubectlQuery("-n", ns, "get", "pod", "-l", guest.Selector, "--field-selector=status.phase=Running", "-o", "jsonpath={.items[0].metadata.name}") - return out - }, 2*time.Minute, 5*time.Second).ShouldNot(BeEmpty()) + }, 2*time.Minute, qemuPollPeriod).ShouldNot(BeEmpty()) - sizeLimit, _ := Kubectl("-n", ns, "get", "pod", + sizeLimit := KubectlQuery("-n", ns, "get", "pod", "-l", guest.Selector, "--field-selector=status.phase=Running", "-o", "jsonpath={.items[0].spec.volumes[?(@.name==\"shared\")].emptyDir.sizeLimit}") @@ -204,7 +213,7 @@ var _ = Describe("ExporterSet QEMU E2E Tests", Label("exporterset-qemu"), Ordere g.Expect(err).NotTo(HaveOccurred()) g.Expect(uid).NotTo(BeEmpty()) oldUID = uid - }, 2*time.Minute, 5*time.Second).Should(Succeed()) + }, 2*time.Minute, qemuComposePollPeriod).Should(Succeed()) By(fmt.Sprintf("power on, assert %s is running, then power off", guest.QemuBinary)) // One lease: start QEMU via the runtime sidecar, confirm the expected @@ -257,17 +266,16 @@ j qemu power off g.Expect(err).NotTo(HaveOccurred()) g.Expect(strings.Fields(strings.TrimSpace(exporters))).To(HaveLen(1), "expected exactly one Exporter after recycle, got %q", exporters) - }, 5*time.Minute, 5*time.Second).Should(Succeed()) + }, 5*time.Minute, qemuComposePollPeriod).Should(Succeed()) By("waiting for the replacement exporter to become Available") var exporterName string Eventually(func() string { - out, _ := Kubectl("-n", ns, "get", "exporter", + exporterName = KubectlQuery("-n", ns, "get", "exporter", "-l", guest.Selector, "-o", "jsonpath={.items[0].metadata.name}") - exporterName = out - return out - }, 2*time.Minute, 5*time.Second).ShouldNot(BeEmpty()) + return exporterName + }, 2*time.Minute, qemuPollPeriod).ShouldNot(BeEmpty()) WaitForExporter(exporterName) By("verifying the replacement still responds to qemu power on/off") diff --git a/e2e/test/hooks_test.go b/e2e/test/hooks_test.go index 66340d831..f88e292ce 100644 --- a/e2e/test/hooks_test.go +++ b/e2e/test/hooks_test.go @@ -26,10 +26,13 @@ import ( . "github.com/onsi/gomega" //nolint:revive ) -var _ = Describe("Hooks E2E Tests", Label("hooks"), Ordered, func() { +var _ = Describe("Hooks E2E Tests", Label("hooks"), Ordered, ContinueOnFailure, func() { var ( tracker *ProcessTracker exporterConfigPath string + // runningConfig is the overlay the exporter currently running in loop + // mode was started with, or "" when no reusable exporter is running. + runningConfig string ) exporterOverlay := func(configFile string) string { @@ -38,21 +41,38 @@ var _ = Describe("Hooks E2E Tests", Label("hooks"), Ordered, func() { // startHooksExporter stops the previous exporter, applies the config overlay, // and starts the exporter in a restart loop. + // + // Restarting costs several seconds per spec, so an exporter already running + // in loop mode with the same overlay is reused. The specs leave no state + // behind that outlives a lease, so the only thing that has to match is the + // config. Exit-mode specs clear runningConfig because they deliberately + // leave the exporter dead. startHooksExporter := func(configFile string) { + if runningConfig == configFile { + WaitForExporter("test-exporter-hooks") + return + } + + runningConfig = "" tracker.StopAll() - time.Sleep(time.Second) + // The controller must observe the old exporter leave before we wait for + // the new one; otherwise WaitForExporter is satisfied by the conditions + // the dead process left behind. + WaitForExporterOffline("test-exporter-hooks") ClearHooksConfig(exporterConfigPath) MergeExporterConfig(exporterConfigPath, exporterOverlay(configFile)) tracker.StartExporterLoop("test-exporter-hooks") WaitForExporter("test-exporter-hooks") + runningConfig = configFile } // startHooksExporterSingle starts without a restart loop (for exit-mode tests). startHooksExporterSingle := func(configFile string) { tracker.StopAll() - time.Sleep(time.Second) + WaitForExporterOffline("test-exporter-hooks") + runningConfig = "" ClearHooksConfig(exporterConfigPath) MergeExporterConfig(exporterConfigPath, exporterOverlay(configFile)) @@ -127,6 +147,20 @@ var _ = Describe("Hooks E2E Tests", Label("hooks"), Ordered, func() { // ==================================================================== // Group B: beforeLease Failure Modes // ==================================================================== + + // beforeLeaseFailureOutput matches every client-visible outcome of a failing + // beforeLease hook. + // + // The client and the exporter race here: the exporter ends the lease the + // moment the hook fails, so which message the client prints depends on how + // far it got first. It may have seen the hook's own output, the shutdown + // notice, a dropped connection, or — if the exporter tore the lease down + // before the client's very first RPC — nothing at all, in which case the + // client reports the exporter as unreachable. All of these mean the hook + // failed and the lease ended, which is what these specs assert. + const beforeLeaseFailureOutput = `(beforeLease hook fail|Exporter shutting down|Connection to exporter lost|` + + `did not respond to initial status check|unreachable after)` + Context("Group B: beforeLease Failure Modes", func() { It("B1: beforeLease onFailure=warn allows shell to proceed", func() { startHooksExporter("exporter-hooks-before-fail-warn.yaml") @@ -146,7 +180,7 @@ var _ = Describe("Hooks E2E Tests", Label("hooks"), Ordered, func() { "--retry-timeout", "0", "--selector", "example.com/board=hooks", "j", "power", "on") Expect(err).To(HaveOccurred()) - Expect(out).To(MatchRegexp(`(beforeLease hook fail|Exporter shutting down|Connection to exporter lost)`)) + Expect(out).To(MatchRegexp(beforeLeaseFailureOutput)) WaitForExporter("test-exporter-hooks") }) @@ -159,7 +193,7 @@ var _ = Describe("Hooks E2E Tests", Label("hooks"), Ordered, func() { "--retry-timeout", "0", "--selector", "example.com/board=hooks", "j", "power", "on") Expect(err).To(HaveOccurred()) - Expect(out).To(MatchRegexp(`(beforeLease hook fail|Connection to exporter lost)`)) + Expect(out).To(MatchRegexp(beforeLeaseFailureOutput)) // The exporter should release the lease and return to Available WaitForExporter("test-exporter-hooks") @@ -170,7 +204,7 @@ var _ = Describe("Hooks E2E Tests", Label("hooks"), Ordered, func() { "--retry-timeout", "0", "--selector", "example.com/board=hooks", "j", "power", "on") Expect(err2).To(HaveOccurred()) - Expect(out2).To(MatchRegexp(`(beforeLease hook fail|Connection to exporter lost)`)) + Expect(out2).To(MatchRegexp(beforeLeaseFailureOutput)) // Exporter should recover again WaitForExporter("test-exporter-hooks") @@ -196,7 +230,7 @@ var _ = Describe("Hooks E2E Tests", Label("hooks"), Ordered, func() { "--retry-timeout", "0", "--selector", "example.com/board=hooks", "j", "power", "on") Expect(err).To(HaveOccurred()) - Expect(out).To(MatchRegexp(`(beforeLease hook fail|Exporter shutting down|Connection to exporter lost)`)) + Expect(out).To(MatchRegexp(beforeLeaseFailureOutput)) // Exporter process should have exited (allow extra time on slower runners like ARM) Eventually(func() bool { diff --git a/e2e/test/utils.go b/e2e/test/utils.go index 4beefae5b..38a8d0499 100644 --- a/e2e/test/utils.go +++ b/e2e/test/utils.go @@ -24,6 +24,8 @@ import ( "os" "os/exec" "path/filepath" + "regexp" + "slices" "strconv" "strings" "sync" @@ -39,9 +41,18 @@ const ( defaultNamespace = "jumpstarter-lab" defaultWaitTimeout = 5 * time.Minute exporterPollPeriod = 500 * time.Millisecond - exporterPostDelay = 2 * time.Second exporterProcessWait = 2 * time.Second + // stopGracePeriod is how long StopAll gives a SIGTERMed exporter to + // unregister before falling back to SIGKILL. It matches the exporter's own + // unregistration timeout (exporter.py, _unregister_with_controller). It is + // an upper bound, not a fixed cost: StopAll polls and returns as soon as + // the process is gone, which is well under a second in the normal case. + stopGracePeriod = 10 * time.Second + // stopKillTimeout is how long StopAll waits after the SIGKILL fallback. + stopKillTimeout = 10 * time.Second + stopPollPeriod = 50 * time.Millisecond + // DexIssuer is the in-cluster Dex OIDC issuer used by e2e login helpers. DexIssuer = "https://dex.dex.svc.cluster.local:5556" ) @@ -335,6 +346,38 @@ func MustKubectl(args ...string) string { return out } +// MustKubectlApply pipes a manifest to `kubectl apply -f -` and fails the test +// on error. Use it to create a batch of fixture resources in one call; the jmp +// admin CLI creates them one process at a time, which is far slower than the +// test needs when the resources are only there to be listed. +func MustKubectlApply(manifest string) string { + cmd := exec.Command("kubectl", "-n", Namespace(), "apply", "-f", "-") + cmd.Stdin = strings.NewReader(manifest) + var out bytes.Buffer + cmd.Stdout = &out + cmd.Stderr = &out + err := cmd.Run() + ExpectWithOffset(1, err).NotTo(HaveOccurred(), "kubectl apply failed: %s", out.String()) + return strings.TrimSpace(out.String()) +} + +// KubectlQuery runs a kubectl query and returns its stdout, or "" if kubectl +// failed. Use it for values polled inside Eventually. +// +// Kubectl folds stderr into the returned string, which is wrong for a polled +// query: `-o jsonpath={.items[0].metadata.name}` against an empty list exits +// non-zero and prints "array index out of bounds", so a poll written as +// Eventually(...).ShouldNot(BeEmpty()) accepts that error text as a result and +// stops waiting on the very first attempt. Returning "" keeps the poll running +// until the resource actually appears. +func KubectlQuery(args ...string) string { + stdout, _, err := RunCmdSplit("kubectl", args...) + if err != nil { + return "" + } + return stdout +} + // ReadYAMLField reads a top-level field from a YAML file and returns its // string value. For scalar values the string representation is returned; // for nested structures the re-marshalled YAML is returned. @@ -415,13 +458,37 @@ func (lb *logBuffer) Close() { } } +// procSpec identifies an exporter by the flag/value pair it was started with, +// e.g. {"--exporter", "hooks-exporter"} or {"--exporter-config", "/tmp/x.yaml"}. +// `jmp run` forks and the child calls setsid() without re-execing, so the child +// carries the same argv as the tracked parent and can be found by matching it. +type procSpec struct { + flag string + value string +} + // ProcessTracker manages background exporter processes. type ProcessTracker struct { + mu sync.Mutex pids []int + specs []procSpec logs map[string]*logBuffer cancels []context.CancelFunc } +// 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.mu.Lock() + defer pt.mu.Unlock() + pt.pids = append(pt.pids, pid) + spec := procSpec{flag: flag, value: value} + if !slices.Contains(pt.specs, spec) { + pt.specs = append(pt.specs, spec) + } +} + // NewProcessTracker creates a new ProcessTracker. func NewProcessTracker() *ProcessTracker { return &ProcessTracker{ @@ -479,10 +546,7 @@ func (pt *ProcessTracker) StartExporterLoop(exporterName string, jmpBin ...strin } pid := cmd.Process.Pid - // Track the PID under the parent lock-free path; this is safe - // because StopAll first cancels the context so this goroutine - // will not spawn new processes concurrently. - pt.pids = append(pt.pids, pid) + pt.track(pid, "--exporter", exporterName) if restartCount > 0 { GinkgoWriter.Printf("Restarted exporter %s (PID %d, restart #%d)\n", exporterName, pid, restartCount) @@ -510,7 +574,7 @@ func (pt *ProcessTracker) StartExporterSingle(exporterName string) *exec.Cmd { cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} err := cmd.Start() ExpectWithOffset(1, err).NotTo(HaveOccurred(), "failed to start exporter %s", exporterName) - pt.pids = append(pt.pids, cmd.Process.Pid) + pt.track(cmd.Process.Pid, "--exporter", exporterName) GinkgoWriter.Printf("Started exporter %s (PID %d)\n", exporterName, cmd.Process.Pid) // Reap the child process in the background so it doesn't become a zombie. @@ -535,7 +599,7 @@ func (pt *ProcessTracker) StartExporterWithConfig(name, configPath string) *exec err := cmd.Start() ExpectWithOffset(1, err).NotTo(HaveOccurred(), "failed to start exporter with config %s", configPath) - pt.pids = append(pt.pids, cmd.Process.Pid) + pt.track(cmd.Process.Pid, "--exporter-config", configPath) GinkgoWriter.Printf("Started exporter %s (PID %d) with config %s\n", name, cmd.Process.Pid, configPath) // Reap the child process in the background so it doesn't become a zombie. @@ -566,7 +630,7 @@ func (pt *ProcessTracker) StartDirectExporter(configFile string, port int, passp err := cmd.Start() ExpectWithOffset(1, err).NotTo(HaveOccurred(), "failed to start direct exporter with config %s", configFile) - pt.pids = append(pt.pids, cmd.Process.Pid) + pt.track(cmd.Process.Pid, "--exporter-config", configFile) GinkgoWriter.Printf("Started direct exporter (PID %d) on port %d\n", cmd.Process.Pid, port) return cmd, stderrBuf } @@ -618,6 +682,8 @@ func (pt *ProcessTracker) DumpLogs(_ int) { // TrackedPIDs returns a copy of currently tracked process IDs. func (pt *ProcessTracker) TrackedPIDs() []int { + pt.mu.Lock() + defer pt.mu.Unlock() out := make([]int, len(pt.pids)) copy(out, pt.pids) return out @@ -642,36 +708,139 @@ func AnyPIDAlive(pids []int) bool { return false } -// StopAll cancels all restart loops, kills all tracked processes, waits +// signalPIDs best-effort sends sig to each of the given PIDs. +func signalPIDs(pids []int, sig syscall.Signal) { + for _, pid := range pids { + proc, err := os.FindProcess(pid) + if err != nil { + continue + } + _ = proc.Signal(sig) + } +} + +// waitPIDsGone reports whether all the given PIDs have exited within timeout. +func waitPIDsGone(pids []int, timeout time.Duration) bool { + deadline := time.Now().Add(timeout) + for time.Now().Before(deadline) { + if !AnyPIDAlive(pids) { + return true + } + time.Sleep(stopPollPeriod) + } + return !AnyPIDAlive(pids) +} + +// StopAll cancels all restart loops, terminates all tracked processes, waits // until those PIDs are gone, then clears the tracker and kills orphans. +// +// Termination is SIGTERM first, SIGKILL only as a fallback. `jmp run` forks and +// the child calls setsid(), so the tracked PID is the parent: a SIGKILL there +// leaves the exporter itself orphaned with its controller registration intact, +// and the controller then has to time out the heartbeat before the exporter +// counts as gone. On SIGTERM the parent forwards the signal to the child's +// process group, the child reports OFFLINE and unregisters, and the parent +// exits only once it has reaped the child — so the parent being gone means the +// exporter really has left the controller. func (pt *ProcessTracker) StopAll() { - // Cancel all restart-loop goroutines first + // Cancel all restart-loop goroutines first so track() won't be called + // concurrently from this point on. for _, cancel := range pt.cancels { cancel() } pt.cancels = nil pids := pt.TrackedPIDs() + signalPIDs(pids, syscall.SIGTERM) + + // Reap in the background. A zombie still answers signal 0, so a child that + // nobody waited on would look alive for the whole grace period. Processes + // started through StartExporter* already have a Wait goroutine; the extra + // waiter just loses the race and gets an error, which is harmless. for _, pid := range pids { - proc, err := os.FindProcess(pid) - if err != nil { - continue + if proc, err := os.FindProcess(pid); err == nil { + go func() { _, _ = proc.Wait() }() } - _ = proc.Signal(syscall.SIGKILL) - _, _ = proc.Wait() } // Wait until tracked PIDs are actually gone before clearing the list, // so callers that snapshot PIDs (or poll IsProcessRunning) observe a // real termination rather than an emptied tracker. - deadline := time.Now().Add(10 * time.Second) - for time.Now().Before(deadline) && AnyPIDAlive(pids) { - time.Sleep(50 * time.Millisecond) + if !waitPIDsGone(pids, stopGracePeriod) { + GinkgoWriter.Printf("Exporter PIDs %v did not exit on SIGTERM, sending SIGKILL\n", pids) + signalPIDs(pids, syscall.SIGKILL) + waitPIDsGone(pids, stopKillTimeout) } + + pt.mu.Lock() pt.pids = nil + pt.mu.Unlock() + + pt.sweepOrphans() +} + +// sweepOrphans SIGKILLs any surviving `jmp run` process that matches one of the +// argv identities this tracker started. It is a safety net for the SIGKILL +// fallback above: killing the parent orphans the forked child, which keeps the +// parent's argv. +// +// This is deliberately not a `pkill -f "jmp run --exporter"`. That pattern is +// global, so under `ginkgo --procs` one process's cleanup would reap every other +// process's exporters, and being a substring match it also matched the +// `--exporter-config` runs it was never meant to touch. Matching whole argv +// elements against the specs this tracker recorded avoids both. +func (pt *ProcessTracker) sweepOrphans() { + pt.mu.Lock() + specs := make([]procSpec, len(pt.specs)) + copy(specs, pt.specs) + pt.mu.Unlock() + + if len(specs) == 0 { + return + } + + entries, err := os.ReadDir("/proc") + if err != nil { + return + } + + self := os.Getpid() + for _, entry := range entries { + pid, err := strconv.Atoi(entry.Name()) + if err != nil || pid == self { + continue + } + + raw, err := os.ReadFile(filepath.Join("/proc", entry.Name(), "cmdline")) + if err != nil { + continue // process exited, or not ours to read + } + argv := strings.Split(strings.TrimSuffix(string(raw), "\x00"), "\x00") + if !argvMatchesSpecs(argv, specs) { + continue + } - // Kill orphaned jmp exporter processes - _ = exec.Command("pkill", "-9", "-f", "jmp run --exporter").Run() + GinkgoWriter.Printf("Killing orphaned exporter process %d (%s)\n", pid, strings.Join(argv, " ")) + if proc, err := os.FindProcess(pid); err == nil { + _ = proc.Signal(syscall.SIGKILL) + } + } +} + +// argvMatches reports whether argv is a `jmp run` invocation carrying one of the +// tracked flag/value pairs as adjacent, whole arguments. +func argvMatchesSpecs(argv []string, specs []procSpec) bool { + if len(argv) < 3 || !slices.Contains(argv, "run") { + return false + } + for _, spec := range specs { + for i := 0; i < len(argv)-1; i++ { + if argv[i] == spec.flag && argv[i+1] == spec.value { + return true + } + } + } + return false } // Cleanup stops all processes and closes log files. @@ -684,30 +853,58 @@ func (pt *ProcessTracker) Cleanup() { // IsProcessRunning checks if any tracked process is still running. func (pt *ProcessTracker) IsProcessRunning() bool { - return AnyPIDAlive(pt.pids) + pids := pt.TrackedPIDs() + return AnyPIDAlive(pids) } // --- Exporter wait helpers --- -// WaitForExporter waits for an exporter to become Online, Registered, and Available. +// validExporterName matches a Kubernetes resource name, so that a caller +// passing a captured kubectl error string produces a clear failure here rather +// than an unreadable `kubectl wait exporters.jumpstarter.dev/error: ...`. +var validExporterName = regexp.MustCompile(`^[a-z0-9]([-.a-z0-9]*[a-z0-9])?$`) + +// exporterFree is the value exporterState returns for an exporter that is +// Available with no lease outstanding. +const exporterFree = "Available|" + +// exporterState reads an exporter's status and its outstanding lease in a +// single query, so the two can never be read from different revisions. +func exporterState(ns, exporterRef string) string { + return KubectlQuery("-n", ns, "get", exporterRef, + "-o", "jsonpath={.status.exporterStatus}|{.status.leaseRef.name}") +} + +// WaitForExporter waits for an exporter to become Online, Registered, and +// Available with no lease outstanding. +// +// Waiting for the lease to clear is what makes this safe to call right after a +// `jmp shell` returns. The controller has not necessarily processed the release +// yet at that point, so exporterStatus can still read Available from before the +// lease ever started, and a wait that only looked at the status would be +// satisfied by that stale value. status.leaseRef is derived from the active, +// non-ended leases (exporter_controller.go, reconcileStatusLeaseRef), so it +// only empties once the release has actually been reconciled. +// +// A caller that just stopped an exporter has the same problem one step earlier +// — the conditions still describe the process that went away — and must wait +// for WaitForExporterOffline before calling this. func WaitForExporter(name string) { ns := Namespace() + ExpectWithOffset(1, validExporterName.MatchString(name)).To(BeTrue(), + "WaitForExporter called with an invalid exporter name %q", name) exporterRef := fmt.Sprintf("exporters.jumpstarter.dev/%s", name) - // Brief delay to avoid catching pre-disconnect state - time.Sleep(exporterPostDelay) - // Wait for Online + Registered conditions MustRunCmd("kubectl", "-n", ns, "wait", "--timeout", "5m", "--for=condition=Online", "--for=condition=Registered", exporterRef) - // Poll until exporterStatus is Available - Eventually(func() string { - out, _ := Kubectl("-n", ns, "get", exporterRef, - "-o", "jsonpath={.status.exporterStatus}") - return out - }, defaultWaitTimeout, exporterPollPeriod).Should(Equal("Available"), - "timed out waiting for %s to reach Available status", name) + // Poll until the exporter is Available and holds no lease + EventuallyWithOffset(1, func() string { + return exporterState(ns, exporterRef) + }, defaultWaitTimeout, exporterPollPeriod).Should(Equal(exporterFree), + "timed out waiting for %s to be Available with no outstanding lease "+ + "(reported as |)", name) } // WaitForExporters waits for multiple exporters in parallel. @@ -724,16 +921,25 @@ func WaitForExporters(names ...string) { wg.Wait() } -// WaitForExporterOffline waits for an exporter to go offline. +// WaitForExporterOffline waits for an exporter to stop reporting itself Online. +// +// The query goes through RunCmdSplit rather than Kubectl because an empty +// result is one of the accepted answers (the Online condition may not be set +// yet) and Kubectl folds stderr into its output: a failed query would otherwise +// be indistinguishable from "the exporter is offline" and end the wait on the +// first attempt. func WaitForExporterOffline(name string) { ns := Namespace() exporterRef := fmt.Sprintf("exporters.jumpstarter.dev/%s", name) - Eventually(func() bool { - out, _ := Kubectl("-n", ns, "get", exporterRef, + EventuallyWithOffset(1, func() bool { + out, _, err := RunCmdSplit("kubectl", "-n", ns, "get", exporterRef, "-o", `jsonpath={.status.conditions[?(@.type=="Online")].status}`) + if err != nil { + return false + } return out == "False" || out == "Unknown" || out == "" - }, 200*time.Second, time.Second).Should(BeTrue(), + }, 200*time.Second, exporterPollPeriod).Should(BeTrue(), "timed out waiting for %s to go offline", name) }