Repository navigation
Conversation
With componentsDeployment.enabled=true and no metrics store configured, dashboard-service never turns ready and `helm --wait` times out. At startup the data engine looks up the `clickhouse` Component and then the `bigquery` Component, but the dashboard-service-components-reader Role only grants get on `clickhouse`. isComponentExpected counts the Forbidden error as a configured store, so the dashboard picks BigQuery, cannot build it, and /readyz returns 503 forever. Grant get on `bigquery` too. The lookup now returns NotFound and the dashboard starts without a chart data engine. TAG=agy CONV=b6f14b72-3ac1-4d84-ae7e-f90e012997d5
Add deploy/addons/postgres/deploy_azure_aks.sh, the Azure counterpart of deploy_aws_eks.sh. It provisions an AKS cluster and deploys XMC with the PostgreSQL backend and no GCP dependencies: in-cluster PostgreSQL 16, Redis, Dex, JobSet 0.12.0, Kueue 0.18.2 and the chart with database.backend=postgres. Extra steps publish the gateway through an Azure load balancer (expose), run the Playwright e2e suite against it (e2e), and delete the resource group (teardown). Azure-specific choices: - Registry access: attach ACR when the caller may assign roles, otherwise use a pull-only ACR token as an imagePullSecret, added to every pod template with a Helm post-renderer and to the default service account for the migration Jobs. - Public endpoint: a Standard Load Balancer with loadBalancerSourceRanges and an azure-dns-label-name hostname, reusing gateway-nlb.yaml. - Credentials go to a separate kubeconfig file. - componentsDeployment is on, with gcpServiceAccount set to the Dex user that xmc-bridge signs in as, which CMS requires for component writes. TAG=agy CONV=b6f14b72-3ac1-4d84-ae7e-f90e012997d5
There was a problem hiding this comment.
Code Review
This pull request introduces a new deployment script deploy_azure_aks.sh to provision an AKS cluster and deploy XMC in "island mode" with a PostgreSQL backend. It also updates the dashboard-service RBAC rules to include the bigquery component, preventing startup readiness failures when neither ClickHouse nor BigQuery is configured, and adds corresponding unit tests. Feedback on the deployment script includes cleaning up the temporary directory on exit, separating variable declaration from assignment to avoid masking curl exit codes, and replacing seq with Bash brace expansion.
| JOBSET_CHART_VERSION="${JOBSET_CHART_VERSION:-0.12.0}" | ||
| KUEUE_CHART_VERSION="${KUEUE_CHART_VERSION:-0.18.2}" | ||
| NAMESPACE="${NAMESPACE:-xmc}" | ||
| WORK_DIR="${WORK_DIR:-$(mktemp -d)}" |
There was a problem hiding this comment.
The script creates a temporary directory using mktemp -d but does not clean it up upon exit. This can leave orphaned directories in /tmp after multiple runs.\n\nTo prevent this, register an EXIT trap to automatically remove the temporary directory when the script finishes, but only if it was automatically created by the script (i.e., WORK_DIR was not pre-defined by the user).
| WORK_DIR="${WORK_DIR:-$(mktemp -d)}" | |
| if [[ -z "${WORK_DIR:-}" ]]; then\n WORK_DIR="$(mktemp -d)"\n trap 'rm -rf "${WORK_DIR}"' EXIT\nfi |
| } | ||
|
|
||
| step_expose() { | ||
| local cidrs="${ALLOWED_CIDRS:-$(curl -4 -s --max-time 10 ifconfig.me)/32}" |
There was a problem hiding this comment.
Using local in the same line as a command substitution (e.g., local cidrs='${ALLOWED_CIDRS:-$(curl ...)}') masks the exit code of the command. If curl fails or times out, the script will silently continue with an invalid CIDR block (like /32), which will cause downstream Kubernetes or load balancer configurations to fail.\n\nSeparating the declaration from the assignment and explicitly handling the potential failure of the external curl command ensures the script fails gracefully with a clear error message.
| local cidrs="${ALLOWED_CIDRS:-$(curl -4 -s --max-time 10 ifconfig.me)/32}" | |
| local cidrs\n if [[ -n "${ALLOWED_CIDRS:-}" ]]; then\n cidrs="${ALLOWED_CIDRS}"\n else\n local public_ip\n public_ip="$(curl -4 -s --max-time 10 ifconfig.me)" || {\n echo "Error: Failed to detect public IP address via ifconfig.me. Please set ALLOWED_CIDRS manually." >&2\n exit 1\n }\n cidrs="${public_ip}/32"\n fi |
References
- SC2155: Declare and assign separately to avoid masking return values.
| PYEOF | ||
| kubectl rollout status deployment/gateway-proxy -n "${NAMESPACE}" --timeout=120s | ||
| local ip="" | ||
| for _ in $(seq 1 30); do |
There was a problem hiding this comment.
Using $(seq 1 30) spawns an external process in a subshell. In modern Bash, you can use the built-in brace expansion {1..30} to avoid spawning an external process, which is more efficient and idiomatic.
| for _ in $(seq 1 30); do | |
| for _ in {1..30}; do |
References
- Avoid spawning external processes like seq when bash built-in brace expansion is available.
| done | ||
| [[ -z "${ip}" ]] && { echo "load balancer IP not assigned" >&2; exit 1; } | ||
| local host="${label}.${AZURE_LOCATION}.cloudapp.azure.com" | ||
| for _ in $(seq 1 30); do |
There was a problem hiding this comment.
Using $(seq 1 30) spawns an external process in a subshell. In modern Bash, you can use the built-in brace expansion {1..30} to avoid spawning an external process, which is more efficient and idiomatic.
| for _ in $(seq 1 30); do | |
| for _ in {1..30}; do |
References
- Avoid spawning external processes like seq when bash built-in brace expansion is available.
Stacked on #1455 (base:
szlaski/postgres-island). Review only the last commit. The first commit is the dashboard RBAC fix from #1468. It is included so this branch deploys end to end, and it drops out on rebase once #1468 merges.What
deploy/addons/postgres/deploy_azure_aks.shdeploys XMC on AKS in island mode: in-cluster PostgreSQL 16, Redis, Dex, JobSet 0.12.0 and Kueue 0.18.2, with no GCP dependencies. It is the Azure counterpart ofdeploy_aws_eks.shand keeps the same step layout:cluster controllers images postgres support xmc verify.exposepublishes the gateway through an Azure Standard Load Balancer with an IP allowlist and a<label>.<region>.cloudapp.azure.comhostname, then re-points Dex and the chart at it.e2eruns the Playwright suite (test/e2e/test_e2e_flow.py) against the public URL.teardowndeletes the resource group.Azure-specific choices
--attach-acrneeds Owner, User Access Administrator or RBAC Administrator. WithACR_PULL=auto(the default), the script attaches the registry when the caller can assign roles. Otherwise it creates a pull-only ACR token (_repositories_pullscope map) as theacr-pullimagePullSecret. The chart has noimagePullSecretsvalue, so a Helm post-renderer adds the secret to every pod template, and the namespace's default service account carries it for the PostgreSQL migration Jobs.service.beta.kubernetes.io/azure-dns-label-nameannotation gives a stable hostname without a DNS zone, andloadBalancerSourceRangeslimits who can reach it. The script reusesmanifests/gateway-nlb.yamland swaps its AWS annotations for Azure ones.~/.kube/<cluster>.aks.yaml(KUBECONFIG_FILE), so the current context of~/.kube/configstays untouched.componentsDeployment.enabled=truewithgcpServiceAccount: admin@xmc.local, as in the e2e values. CMS accepts component writes only from the services' own identity, and xmc-bridge signs in as that Dex user. Without a Spanner client, CMS keeps components in its in-memory store.Testing
I ran this on AKS 1.36 (
swedencentral, 3xStandard_D4s_v5) in a subscription where I only have Contributor:cpu-queuereachedCOMPLETEDthrough the public URL (Dex sign-in, then REST).deploy_azure_aks.sh e2e: all 6 Playwright stages pass in about 60 s: admin flow, work unit lifecycle and preemption, viewer boundaries, unauthenticated access, unauthorized persona, and xmc-bridge to CMS reconciliation.bash -nandshellcheck -S warningare clean.CI note:
Code Quality & Lintingfails here because of the base branch, not this PR. golangci-lint flagsscanExperimentininternal/storage/postgres/experiment/repository.gofrom #1455 (gocyclo 17 > 15), and #1455 fails the same step. This PR doesn't touch Go code.Two issues I found along the way are not fixed here:
test_e2e_flow.pyrunskubectl delete componentwith the default--wait. When xmc-bridge can't reach CMS, the delete blocks forever on thexmc.dev/cms-cleanupfinalizer, so the suite hangs instead of failing.ListExperimentsreturns every experiment to every signed-in user, whileGET /v1/experiments/{id}enforces ACLs. As a result, private experiments show up in everyone's list and return 403 when opened.TAG=agy
CONV=b6f14b72-3ac1-4d84-ae7e-f90e012997d5