Repository navigation
Conversation
CreateXidEntry (INSERT ... RETURNING Xid), FetchXidEntry, ListXidEntries via the shared query generator and xidschema view, and the transactional FetchXidEntry (FOR NO KEY UPDATE) and UpdateXidEntry. Integration tests run against PostgreSQL through postgrestest, including multi-page pagination and a lost-update concurrency test.
There was a problem hiding this comment.
Code Review
This pull request introduces support for a PostgreSQL backend for the experiment state server, including necessary schema migrations, infrastructure deployment scripts for AWS EKS, and the required repository implementation. The review identified several actionable improvements: a critical permission issue for PostgreSQL on EKS due to volume ownership, a performance optimization for environment variable parsing in the XID filter, a missing cleanup trap for temporary directories in the deployment script, and a more portable method for decoding secrets in kubectl.
| spec: | ||
| containers: | ||
| - name: postgres |
There was a problem hiding this comment.
On AWS EKS, EBS volumes mounted via the EBS CSI driver are owned by root:root by default. Since the official PostgreSQL Alpine image runs as a non-root user (postgres, UID/GID 999), the container will fail to initialize the database with a "permission denied" error when writing to /var/lib/postgresql/data/pgdata. To prevent this, configure securityContext.fsGroup to 999 on the Pod spec so Kubernetes automatically changes the ownership of the mounted volume.
spec:
securityContext:
fsGroup: 999
containers:
- name: postgres| func xidLookupFilter(xids []int64) string { | ||
| backend, err := dbconfig.ParseBackend(os.Getenv("DB_BACKEND")) | ||
| usePlainXid := err == nil && backend == dbconfig.BackendPostgres | ||
| terms := make([]string, 0, len(xids)) | ||
| for _, xid := range xids { | ||
| if usePlainXid { | ||
| terms = append(terms, fmt.Sprintf("xid=%d", xid)) | ||
| continue | ||
| } | ||
| terms = append(terms, fmt.Sprintf("bit_reversed_xid=%d", data.ReverseBits(xid))) | ||
| } | ||
| return strings.Join(terms, " OR ") | ||
| } |
There was a problem hiding this comment.
Reading the environment variable DB_BACKEND on every call to xidLookupFilter can introduce a performance bottleneck under high concurrency due to internal locking in os.Getenv. Since the database backend configuration is static for the lifetime of the process, this value should be parsed once and cached.
| func xidLookupFilter(xids []int64) string { | |
| backend, err := dbconfig.ParseBackend(os.Getenv("DB_BACKEND")) | |
| usePlainXid := err == nil && backend == dbconfig.BackendPostgres | |
| terms := make([]string, 0, len(xids)) | |
| for _, xid := range xids { | |
| if usePlainXid { | |
| terms = append(terms, fmt.Sprintf("xid=%d", xid)) | |
| continue | |
| } | |
| terms = append(terms, fmt.Sprintf("bit_reversed_xid=%d", data.ReverseBits(xid))) | |
| } | |
| return strings.Join(terms, " OR ") | |
| } | |
| var usePlainXid = func() bool { | |
| backend, err := dbconfig.ParseBackend(os.Getenv("DB_BACKEND")) | |
| return err == nil && backend == dbconfig.BackendPostgres | |
| }() | |
| func xidLookupFilter(xids []int64) string { | |
| terms := make([]string, 0, len(xids)) | |
| for _, xid := range xids { | |
| if usePlainXid { | |
| terms = append(terms, fmt.Sprintf("xid=%d", xid)) | |
| continue | |
| } | |
| terms = append(terms, fmt.Sprintf("bit_reversed_xid=%d", data.ReverseBits(xid))) | |
| } | |
| return strings.Join(terms, " OR ") | |
| } |
| 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 on exit. To prevent cluttering the host system with orphaned temporary directories, add a trap to clean up WORK_DIR on script exit.
| WORK_DIR="${WORK_DIR:-$(mktemp -d)}" | |
| WORK_DIR="${WORK_DIR:-$(mktemp -d)}" | |
| trap 'rm -rf "${WORK_DIR}"' EXIT |
| DEX_ISSUER="http://localhost:5556/dex" | ||
| fi | ||
| local pg_password | ||
| pg_password="$(kubectl get secret xmc-postgres-credentials -n "${NAMESPACE}" -o jsonpath='{.data.password}' | base64 -d)" |
There was a problem hiding this comment.
Using base64 -d is not portable across all operating systems (e.g., macOS uses base64 -D or base64 --decode). A more robust and native way to decode the secret value directly within kubectl is to use Go templates with the base64decode function. This avoids any external piping and ensures cross-platform compatibility.
| pg_password="$(kubectl get secret xmc-postgres-credentials -n "${NAMESPACE}" -o jsonpath='{.data.password}' | base64 -d)" | |
| pg_password="$(kubectl get secret xmc-postgres-credentials -n "${NAMESPACE}" -o go-template='{{.data.password | base64decode}}')" |
Summary
Completes the end-to-end PostgreSQL backend (
DB_BACKEND=postgres) and adds an in-cluster PostgreSQL 16 addon and AWS EKS "island-mode" deployment procedure (zero GCP dependencies).Changes Included
internal/storage/postgres/{xid,statushistory,pausedtask,artifact,experiment,dashboard,workunit}: Implements the remaining 7 PostgreSQL repositories andpostgrestestintegration suites on top oforigin/main(which already includespausestate,operation,clilogincode,statusmessage,dbconfig, andfactory).cmd/experiment_state_server/server/xidfilter.go: Selectsxid=instead ofbit_reversed_xid=whenDB_BACKEND=postgresinListExperimentsandListArtifactsACL resolution (bit_reversed_xidis a Spanner-only view column).deploy/addons/postgres/:manifests/postgres.yaml&setup_postgres_addon.sh: In-cluster PostgreSQL 16 StatefulSet (essandxiddatabases) + migration Job runner usingpostgres-migrator.deploy_aws_eks.sh&manifests/gateway-nlb.yaml: End-to-end deployment and optional IP-allowlisted AWS NLB gateway for running XMC on AWS EKS (DB_BACKEND=postgres) without any GCP dependencies.Verification
//internal/storage/postgres/...unit and integration tests and//cmd/experiment_state_server/server:server_testpass.eu-north-1, Kubernetes 1.34) with in-cluster PostgreSQL 16, Redis, Dex OIDC, JobSet0.12.0, and Kueue0.18.2: experiment creation, work unit scheduling oncpu-queue, state transitions (INITIALIZING -> STARTING -> RUNNING -> COMPLETED), and UI/REST/gRPC access via the NLB gateway.TAG=agy
CONV=9cd11755-1c0f-4576-8304-43044a6d7f9a