argoci: make scaffold scripts bug-for-bug identical to Semaphore#13153
argoci: make scaffold scripts bug-for-bug identical to Semaphore#13153lwr20 wants to merge 2 commits into
Conversation
The hand-ported .argoci/scripts diverged from .semaphore/end-to-end/scripts in ways that change what runs. Revert to match Semaphore (keeping only the behaviour-neutral platform adaptations): run_tests.sh: - drop the added '&& -n E2E_TEST_CONFIG' from the binary-acquire and make-e2e-run conditions. k8s-e2e jobs with an empty E2E_TEST_CONFIG now run the monorepo e2e binary via 'make e2e-run' (as Semaphore does) instead of falling through to 'bz tests' (the k8s-e2e image's [sig-calico] suite) -- a different test binary/suite. - restore the aws-iam-authenticator bind-mount (EKS make-e2e-run can't auth without it). global_prologue.sh: - INSTALLER default operator -> manual. - restore dropped defaults: K8S_E2E_EXTRA_FLAGS (calicoctl-opensource-image pin), K8S_VERSION, HELM_PATCH, CALICOCTL_INSTALL_TYPE. - restore GOOGLE_REGION randomisation + gcloud-derived GOOGLE_ZONE. global_epilogue.sh: - restore the 'always capture diags for ocp-cert' branch (code matched the existing comment). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Aligns the ArgoCI scaffold scripts under .argoci/scripts/ with the canonical Semaphore equivalents so ArgoCI runs the same test binaries and job behaviors (keeping only platform-neutral adaptations like secret materialization and artifact upload paths).
Changes:
- Restore Semaphore-equivalent test selection in
run_tests.shsoTEST_TYPE=k8s-e2econsistently runsmake e2e-run(even whenE2E_TEST_CONFIGis empty) and add anaws-iam-authenticatorbind-mount for EKS kubeconfigs. - Revert prologue defaults to match Semaphore (e.g.,
INSTALLER=manual, defaults forK8S_VERSION,K8S_E2E_EXTRA_FLAGS,HELM_PATCH,CALICOCTL_INSTALL_TYPE, and randomizedGOOGLE_REGION/GOOGLE_ZONEselection). - Always capture diagnostics for
ocp-certruns in the epilogue (in addition to capturing on failure).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.argoci/scripts/phases/run_tests.sh |
Restores Semaphore-equivalent selection of make e2e-run for k8s-e2e and adds optional EKS aws-iam-authenticator mount. |
.argoci/scripts/global_prologue.sh |
Reverts environment defaults and GCP region/zone selection behavior to match Semaphore. |
.argoci/scripts/global_epilogue.sh |
Captures diags for ocp-cert runs regardless of exit status. |
Restore the flannel/canal-migration body dropped in the initial port. Faithful copy of .semaphore/end-to-end/scripts/body_flannel-migration.sh; only the PHASES path is adapted to the absolute form (matching body_standard.sh). No Semaphore cache/artifact deps. Wired to the iptables/patch-verification flannel jobs in the crons PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| # e2e-run locates the binary itself). Take the structured path whenever a | ||
| # k8s-e2e binary was acquired above; non-e2e test types fall through to bz | ||
| # tests below. E2E_TEST_CONFIG may be empty (selects the default config). | ||
| if [[ -n "${E2E_BINARY:-}" ]]; then |
There was a problem hiding this comment.
This gate change (dropping && -n "${E2E_TEST_CONFIG:-}") correctly matches .semaphore/end-to-end/scripts/run_tests.sh — so as a parity change it's right. But it has two consequences worth confirming the e2e-nftables cron is migrated for in lockstep:
K8S_E2E_FLAGSbecomes dead config.make e2e-runnever receives--ginkgo.focus/--ginkgo.skip(rootMakefilee2e-run target passes only--calico.test-config), yet the cron still sets an elaborateK8S_E2E_FLAGSon 8/9 jobs (globalPrologue L13, plus the EKS-calico-cni per-job value at L122–123 with extra known-failure DNS/proxy skips). Those skips are now ignored, so previously-skipped tests will actually run and likely fail.- Empty
E2E_TEST_CONFIGoverrides the Makefile default. Those same 8 jobs set noE2E_TEST_CONFIG, and this path passesE2E_TEST_CONFIG=''unconditionally — which overrides the Makefile's?= e2e/config/kind.yamland hands the binary an empty--calico.test-config=rather than the kind.yaml default.
If historical Semaphore ran these nftables jobs through make e2e-run (not bz+K8S_E2E_FLAGS), then the fix belongs in the cron: drop the now-dead K8S_E2E_FLAGS and give each job a proper E2E_TEST_CONFIG. Flagging since the cron isn't in this PR.
| export GOOGLE_ZONE=${GOOGLE_ZONE:-us-central1-a} | ||
| export GOOGLE_REGIONS=("us-central1" "us-west1") | ||
| export GOOGLE_REGION=${GOOGLE_REGION:-${GOOGLE_REGIONS[RANDOM%${#GOOGLE_REGIONS[@]}]}} | ||
| export GOOGLE_ZONE=${GOOGLE_ZONE:-$(gcloud compute zones list --filter="region~'$GOOGLE_REGION'" --format="value(name)" | awk 'BEGIN {srand()} {a[NR]=$0} rand() * NR < 1 {zone=$0} END {print zone}')} |
There was a problem hiding this comment.
GOOGLE_ZONE is derived from gcloud … | awk inside ${GOOGLE_ZONE:-$(…)}. The prologue runs under set -o pipefail but deliberately not set -e (it's sourced), and this assignment returns rc=0 even when the substitution is empty. So if gcloud fails (auth/API/quota), GOOGLE_ZONE is silently set to "" and GCP provisioning proceeds with an empty zone instead of failing fast with a clear error. Consider validating the result is non-empty (and erroring if not).
Minor, related: this gcloud compute zones list runs on every job including the AWS-provisioner ones (aws-talos/aws-kubeadm/aws-eks) that never use GOOGLE_ZONE — worth gating on a GCP provisioner.
| export GOOGLE_REGION=${GOOGLE_REGION:-us-central1} | ||
| export GOOGLE_ZONE=${GOOGLE_ZONE:-us-central1-a} | ||
| export GOOGLE_REGIONS=("us-central1" "us-west1") | ||
| export GOOGLE_REGION=${GOOGLE_REGION:-${GOOGLE_REGIONS[RANDOM%${#GOOGLE_REGIONS[@]}]}} |
There was a problem hiding this comment.
GOOGLE_REGION is now randomly chosen from {us-central1, us-west1}, but GOOGLE_NETWORK stays the fixed semaphore-autotest VPC (L54). Jobs that don't pin a region (e.g. nftables-talos-native-v3-crds-gcp, kubevirt-e2e-tests) will land in us-west1 ~50% of the time — if semaphore-autotest is a custom-mode VPC with no us-west1 subnet, bz provision will fail to place the instance. Worth confirming a us-west1 subnet (and any region-specific machine types/images) exist before this rides.
|
|
||
| export CNI_VERSION=${CNI_VERSION:-"v1.1.1"} | ||
| export DOCS_BASE=${DOCS_BASE:-"https://github.com/projectcalico/calico"} | ||
| export DOWNLEVEL_MANIFEST=${DOWNLEVEL_MANIFEST:-"https://github.com/projectcalico/calico/raw/release-${RELEASE_STREAM}/manifests/canal.yaml"} |
There was a problem hiding this comment.
DOWNLEVEL_MANIFEST hardcodes release-${RELEASE_STREAM}. On master (the default stream, with USE_HASH_RELEASE=true also the default) this expands to …/raw/release-master/manifests/canal.yaml — there is no release-master branch, so wget -O flannel.yaml (L112) 404s (or truncates the file) and the flannel install breaks. The DOCS_URL guard above only exit 1s on master when USE_HASH_RELEASE != true, so the hashrelease path still reaches this. Only release-vX.Y streams work today.
| operator: Exists | ||
| - effect: NoExecute | ||
| operator: Exists | ||
| terminationGracePeriodSeconds: 0 |
There was a problem hiding this comment.
terminationGracePeriodSeconds: 0 is set twice in this pod spec (also at the line above the tolerations block). Duplicate YAML map key — harmless value-wise (both 0), but strict kubectl field validation (default since ~1.25) can reject a duplicate field, and at best it's confusing. Suggest deleting this one.
| terminationGracePeriodSeconds: 0 | |
| priorityClassName: system-node-critical |
| kubectl -n kube-system get jobs flannel-migration | ||
| kubectl -n kube-system describe jobs flannel-migration | ||
| kubectl get po -A -owide | ||
| kubectl wait --for=condition=complete --timeout=600s -n kube-system job/flannel-migration |
There was a problem hiding this comment.
Nit: kubectl wait --for=condition=complete only returns early when the Job reaches complete (or is deleted). A failed migration sets Failed=true, never complete=true, so this blocks the full --timeout=600s and only then errors — ~10 wasted minutes per broken run. Consider also racing a --for=condition=failed (or a jsonpath poll) to abort fast on failure.
| type: RuntimeDefault | ||
| initContainers: | ||
| - name: cni-installer | ||
| image: quay.io/dosmith/cni-plugins:gen4 |
There was a problem hiding this comment.
quay.io/dosmith/cni-plugins:gen4 is a personal quay namespace with a mutable tag baked into shared CI. If that account/repo/tag is deleted, made private, or repushed, every flannel-migration run silently breaks or changes behavior with no provenance. If this was copied verbatim from Semaphore for parity, fine to confirm as intended — otherwise worth moving to an org-owned registry (e.g. quay.io/calico/…) with a pinned digest.
|
|
||
| # Run a basic services test to check that flannel networking is working. | ||
| K8S_E2E_FLAGS='--ginkgo.focus=should.serve.a.basic.endpoint.from.pods' \ | ||
| ./bz.sh tests:run |& tee >(gzip --stdout > "${BZ_LOGS_DIR}/e2e-tests-pre.log") |
There was a problem hiding this comment.
Nit: these tee >(gzip --stdout > …) sinks write gzip'd bytes into files named .log (also e2e-tests.log at the end of the script), whereas phases/provision.sh uses .log.gz for the same pattern. Anyone/anything opening e2e-tests*.log gets binary garbage, and it defeats tooling keyed on .gz. Suggest e2e-tests-pre.log.gz / e2e-tests.log.gz.
| export RELEASE_STREAM=${RELEASE_STREAM:-$( _b="${CI_GIT_CLONED_BRANCH:-${BRANCH:-master}}"; [[ "${_b}" =~ ^release-(v[0-9]+\.[0-9]+)$ ]] && echo "${BASH_REMATCH[1]}" || echo "master" )} | ||
|
|
||
| export K8S_VERSION=${K8S_VERSION:-stable-3} | ||
| export K8S_E2E_EXTRA_FLAGS=${K8S_E2E_EXTRA_FLAGS:-" --e2ecfg.calicoctl-opensource-image=calico/ctl:release-${RELEASE_STREAM} "} |
There was a problem hiding this comment.
Latent trap: when RELEASE_STREAM=master (the default), this default becomes calico/ctl:release-master, which is almost certainly not a published tag (master builds are calico/ctl:master). It happens to be inert on the make e2e-run path (this var isn't forwarded into the container or onto the make line), so it'd only bite the bz tests (non-e2e) path — but worth fixing the master casing regardless.
The hand-ported
.argoci/scripts/*diverged from.semaphore/end-to-end/scripts/*in ways that change what runs. This reverts them to match Semaphore, keeping only behaviour-neutral platform adaptations (secrets viacreateLocalSecret,CI_*vars, GCS paths, no Semaphore cache CLI, bz-from-image).Most impactful:
run_tests.shhad added&& -n E2E_TEST_CONFIGgates, sok8s-e2ejobs with an emptyE2E_TEST_CONFIGfell through tobz tests(the k8s-e2e image[sig-calico]suite) instead ofmake e2e-run(the monorepo suite Semaphore runs) — a different test binary. Restored to key only onTEST_TYPE == k8s-e2e/E2E_BINARY, matching Semaphore.Also: restored the
aws-iam-authenticatormount (EKS auth);INSTALLERdefaultoperator→manual; restoredK8S_E2E_EXTRA_FLAGS/K8S_VERSION/HELM_PATCH/CALICOCTL_INSTALL_TYPEdefaults +GOOGLE_REGIONrandomisation; restored theocp-certalways-capture-diags branch.Not covered here (follow-up, touches #13133 crons): restoring
body_flannel-migration.sh/phases/hcp.shand wiring the flannel/HCP jobs to them.Release Note