From 87ebad255a80de96e426eca1a45c68269263a63d Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Thu, 23 Jul 2026 00:07:34 -0400 Subject: [PATCH 01/17] feat(deployment): Deploy Presto with stock images and install the CLP connector as a plugin. Replace the connector-baked Presto images with the upstream `ghcr.io/y-scope/presto` and `ghcr.io/y-scope/presto-native` images, and install the CLP connector into them at startup from a dedicated connector image, so that only the connector artifact needs to be published. Docker Compose gains a one-shot `clp-plugin-presto-connector-init` service that populates shared plugin volumes; the Helm chart gains an equivalent `install-clp-plugin` init container backed by an `emptyDir`, selected via `image.clpConnector`. Both set the worker's `plugin.dir` and the coordinator's `use-connector-provided-serialization-codecs`, the latter being required for the worker to deserialize the connector's handles through its own codecs rather than through JSON, which the connector doesn't implement. The stock `presto-native` image is CentOS-based rather than Ubuntu, so the worker's config generation can no longer rely on `apt-get`, `jq`, `wget`, or `hostname`; use `curl` and `python3`, which the image already ships. Resolving the node identity via assignments also ensures a failure aborts the script instead of silently writing empty values. Mount the Compose config directory as tmpfs instead of a named volume so that it's rebuilt from the image and templates on every start, rather than retaining files seeded from a previously deployed image. Co-Authored-By: Claude Opus 4.8 --- .../deployment/package-helm/.set-up-common.sh | 11 ++++- .../set-up-multi-dedicated-test.sh | 3 +- .../package-helm/set-up-multi-shared-test.sh | 3 +- tools/deployment/package-helm/set-up-test.sh | 3 +- .../package-helm/templates/configmap.yaml | 2 + .../presto-coordinator-deployment.yaml | 18 ++++++++ .../templates/presto-worker-deployment.yaml | 18 ++++++++ tools/deployment/package-helm/values.yaml | 17 ++++++-- .../config-template/config.properties | 1 + .../coordinator/scripts/generate-configs.sh | 4 +- .../deployment/presto-clp/docker-compose.yaml | 41 +++++++++++++++---- .../worker/config-template/config.properties | 1 + .../worker/scripts/generate-configs.sh | 28 ++++++++----- 13 files changed, 122 insertions(+), 28 deletions(-) diff --git a/tools/deployment/package-helm/.set-up-common.sh b/tools/deployment/package-helm/.set-up-common.sh index 79e785888e..403bbbd1cb 100755 --- a/tools/deployment/package-helm/.set-up-common.sh +++ b/tools/deployment/package-helm/.set-up-common.sh @@ -75,11 +75,12 @@ get_image_helm_args() { } # Parses common arguments shared across set-up scripts. -# Sets CLP_PACKAGE_IMAGE and ENABLE_PRESTO global variables. +# Sets CLP_PACKAGE_IMAGE, CLP_CONNECTOR_IMAGE, and ENABLE_PRESTO global variables. # # @param {string[]} args Script arguments parse_common_args() { CLP_PACKAGE_IMAGE="" + CLP_CONNECTOR_IMAGE="" ENABLE_PRESTO="false" while [[ $# -gt 0 ]]; do case "$1" in @@ -91,6 +92,14 @@ parse_common_args() { CLP_PACKAGE_IMAGE="$2" shift 2 ;; + --clp-connector-image) + if [[ $# -lt 2 || "$2" == --* ]]; then + echo "Error: '--clp-connector-image' requires a value." >&2 + exit 1 + fi + CLP_CONNECTOR_IMAGE="$2" + shift 2 + ;; --presto) ENABLE_PRESTO="true" shift diff --git a/tools/deployment/package-helm/set-up-multi-dedicated-test.sh b/tools/deployment/package-helm/set-up-multi-dedicated-test.sh index b838b05196..cb9a2b27f4 100755 --- a/tools/deployment/package-helm/set-up-multi-dedicated-test.sh +++ b/tools/deployment/package-helm/set-up-multi-dedicated-test.sh @@ -162,6 +162,7 @@ helm install test "${script_dir}" \ --set "scheduling.mcpServer.nodeSelector.yscope\.io/nodeType=core" \ $(get_service_exposure_helm_args) \ $(get_presto_helm_args) \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") + $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") \ + $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_CONNECTOR_IMAGE}") wait_for_cluster_ready diff --git a/tools/deployment/package-helm/set-up-multi-shared-test.sh b/tools/deployment/package-helm/set-up-multi-shared-test.sh index 45a498d9a9..1a8461b200 100755 --- a/tools/deployment/package-helm/set-up-multi-shared-test.sh +++ b/tools/deployment/package-helm/set-up-multi-shared-test.sh @@ -52,6 +52,7 @@ helm install test "${script_dir}" \ --set "scheduling.prestoWorker.replicas=${PRESTO_WORKER_REPLICAS}" \ $(get_service_exposure_helm_args) \ $(get_presto_helm_args) \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") + $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") \ + $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_CONNECTOR_IMAGE}") wait_for_cluster_ready diff --git a/tools/deployment/package-helm/set-up-test.sh b/tools/deployment/package-helm/set-up-test.sh index de7238363a..50be5ac079 100755 --- a/tools/deployment/package-helm/set-up-test.sh +++ b/tools/deployment/package-helm/set-up-test.sh @@ -31,6 +31,7 @@ sleep 2 helm install test "${script_dir}" \ $(get_service_exposure_helm_args) \ $(get_presto_helm_args) \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") + $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") \ + $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_CONNECTOR_IMAGE}") wait_for_cluster_ready diff --git a/tools/deployment/package-helm/templates/configmap.yaml b/tools/deployment/package-helm/templates/configmap.yaml index ee49584a06..6880755589 100644 --- a/tools/deployment/package-helm/templates/configmap.yaml +++ b/tools/deployment/package-helm/templates/configmap.yaml @@ -391,6 +391,7 @@ data: inline-sql-functions=false nested-data-serialization-enabled=false native-execution-enabled=true + use-connector-provided-serialization-codecs=true presto-coordinator-config-jvm.config: | -server @@ -438,6 +439,7 @@ data: system-memory-gb={{ .worker.system_memory_gb }} register-test-functions=false runtime-metrics-collection-enabled=false + plugin.dir=/opt/presto-server/plugin/clp presto-worker-config-node.properties: | node.environment=production diff --git a/tools/deployment/package-helm/templates/presto-coordinator-deployment.yaml b/tools/deployment/package-helm/templates/presto-coordinator-deployment.yaml index 8b5e56e5c0..92ecbb7630 100644 --- a/tools/deployment/package-helm/templates/presto-coordinator-deployment.yaml +++ b/tools/deployment/package-helm/templates/presto-coordinator-deployment.yaml @@ -30,6 +30,19 @@ spec: "type" "job" "name" "db-table-creator" ) | nindent 10 }} + - name: "install-clp-plugin" + image: {{ include "clp.imageRef" (dict "root" . "component" "clpConnector") | quote }} + imagePullPolicy: {{ .Values.image.clpConnector.pullPolicy | quote }} + env: + - name: "COORDINATOR_PLUGIN_INSTALL_PATH" + value: "/install/coordinator" + {{- include "clp.createResourceLimits" (dict + "root" . + "component" "prestoCoordinatorInstallPlugin" + ) | nindent 10 }} + volumeMounts: + - name: "presto-plugin" + mountPath: "/install/coordinator" containers: - name: "presto-coordinator" image: "{{ .Values.image.prestoCoordinator.repository }}:{{ .Values.image.prestoCoordinator.tag }}" @@ -65,6 +78,9 @@ spec: mountPath: "/opt/presto-server/etc/split-filter.json" subPath: "presto-coordinator-config-split-filter.json" readOnly: true + - name: "presto-plugin" + mountPath: "/opt/presto-server/plugin/clp" + readOnly: true readinessProbe: {{- include "clp.readinessProbeTimings" . | nindent 12 }} httpGet: &presto-coordinator-health-check @@ -83,4 +99,6 @@ spec: - name: "presto-config" configMap: name: {{ include "clp.fullname" . }}-config + - name: "presto-plugin" + emptyDir: {} {{- end }} diff --git a/tools/deployment/package-helm/templates/presto-worker-deployment.yaml b/tools/deployment/package-helm/templates/presto-worker-deployment.yaml index 2c9337c04c..222bd048cf 100644 --- a/tools/deployment/package-helm/templates/presto-worker-deployment.yaml +++ b/tools/deployment/package-helm/templates/presto-worker-deployment.yaml @@ -30,6 +30,19 @@ spec: "type" "service" "name" "presto-coordinator" ) | nindent 10 }} + - name: "install-clp-plugin" + image: {{ include "clp.imageRef" (dict "root" . "component" "clpConnector") | quote }} + imagePullPolicy: {{ .Values.image.clpConnector.pullPolicy | quote }} + env: + - name: "WORKER_PLUGIN_INSTALL_PATH" + value: "/install/worker" + {{- include "clp.createResourceLimits" (dict + "root" . + "component" "prestoWorkerInstallPlugin" + ) | nindent 10 }} + volumeMounts: + - name: "presto-plugin" + mountPath: "/install/worker" - name: "setup-configs" image: {{ include "clp.imageRef" (dict "root" . "component" "kubectl") | quote }} imagePullPolicy: {{ .Values.image.kubectl.pullPolicy | quote }} @@ -68,6 +81,9 @@ spec: readOnly: true - name: "presto-etc" mountPath: "/opt/presto-server/etc" + - name: "presto-plugin" + mountPath: "/opt/presto-server/plugin/clp" + readOnly: true {{- if eq .Values.clpConfig.archive_output.storage.type "fs" }} - name: {{ include "clp.volumeName" (dict "component_category" "shared-data" @@ -95,6 +111,8 @@ spec: name: {{ include "clp.fullname" . }}-config - name: "presto-etc" emptyDir: {} + - name: "presto-plugin" + emptyDir: {} - name: "presto-scripts" configMap: name: {{ include "clp.fullname" . }}-config diff --git a/tools/deployment/package-helm/values.yaml b/tools/deployment/package-helm/values.yaml index 27412fdbb0..c5f4127847 100644 --- a/tools/deployment/package-helm/values.yaml +++ b/tools/deployment/package-helm/values.yaml @@ -5,6 +5,13 @@ fullnameOverride: "" allowHostAccessForSbinScripts: true image: + # Installs the CLP Presto connector plugin into the Presto coordinator and worker pods. Override + # the repository to use a different edition of the connector. + # TODO: default the tag to the released multi-arch tag before merging. + clpConnector: + repository: "ghcr.io/y-scope/clp-plugin-presto-connector" + pullPolicy: "IfNotPresent" + tag: "0.1.0-SNAPSHOT-arm64" clpPackage: repository: "ghcr.io/y-scope/clp/clp-package" pullPolicy: "Always" @@ -31,13 +38,13 @@ image: pullPolicy: "IfNotPresent" tag: "0.152.0" prestoCoordinator: - repository: "ghcr.io/y-scope/presto/coordinator" + repository: "ghcr.io/y-scope/presto" pullPolicy: "IfNotPresent" - tag: "clp-v0.10.0" + tag: "0.299" prestoWorker: - repository: "ghcr.io/y-scope/presto/prestissimo-worker" + repository: "ghcr.io/y-scope/presto-native" pullPolicy: "IfNotPresent" - tag: "clp-v0.10.0-fix.1" + tag: "0.299" queue: repository: "rabbitmq" pullPolicy: "Always" @@ -142,6 +149,8 @@ resources: # Jobs dbTableCreator: {} + prestoCoordinatorInstallPlugin: {} + prestoWorkerInstallPlugin: {} prestoWorkerSetupConfigs: {} resultsCacheIndicesCreator: {} topologyMetricsEmitter: {} diff --git a/tools/deployment/presto-clp/coordinator/config-template/config.properties b/tools/deployment/presto-clp/coordinator/config-template/config.properties index b9da2234f4..d0bd7bb8ad 100644 --- a/tools/deployment/presto-clp/coordinator/config-template/config.properties +++ b/tools/deployment/presto-clp/coordinator/config-template/config.properties @@ -11,3 +11,4 @@ use-alternative-function-signatures=true inline-sql-functions=false nested-data-serialization-enabled=false native-execution-enabled=true +use-connector-provided-serialization-codecs=true diff --git a/tools/deployment/presto-clp/coordinator/scripts/generate-configs.sh b/tools/deployment/presto-clp/coordinator/scripts/generate-configs.sh index d2d05cf041..802e5bf61f 100755 --- a/tools/deployment/presto-clp/coordinator/scripts/generate-configs.sh +++ b/tools/deployment/presto-clp/coordinator/scripts/generate-configs.sh @@ -15,6 +15,6 @@ find /configs -type f | while read -r f; do ) | sh >"${PRESTO_CONFIG_DIR}/$(basename "$f")" done -# Remove existing catalog files that exist in the image and add the CLP catalog -rm -f "${PRESTO_CONFIG_DIR}/catalog/"* +# Create the catalog directory and add the CLP catalog +mkdir -p "${PRESTO_CONFIG_DIR}/catalog" mv "${PRESTO_CONFIG_DIR}/clp.properties" "${PRESTO_CONFIG_DIR}/catalog" diff --git a/tools/deployment/presto-clp/docker-compose.yaml b/tools/deployment/presto-clp/docker-compose.yaml index fdea3c7294..53592db171 100644 --- a/tools/deployment/presto-clp/docker-compose.yaml +++ b/tools/deployment/presto-clp/docker-compose.yaml @@ -1,7 +1,27 @@ services: + clp-plugin-presto-connector-init: + # One-shot: installs the CLP Presto connector plugin into shared volumes that the + # coordinator and worker mount into their plugin dirs (replacing the connector-baked + # Presto images). The connector image's own entrypoint does the install; we only supply + # the target paths via *_PLUGIN_INSTALL_PATH. + # TODO: default CLP_CONNECTOR_TAG to the released multi-arch tag before merging. + image: "${CLP_CONNECTOR_IMAGE:-ghcr.io/y-scope/clp-plugin-presto-connector}:${CLP_CONNECTOR_TAG:-0.1.0-SNAPSHOT-arm64}" + restart: "no" + environment: + COORDINATOR_PLUGIN_INSTALL_PATH: "/install/coordinator" + WORKER_PLUGIN_INSTALL_PATH: "/install/worker" + volumes: + - "coordinator-plugin:/install/coordinator" + - "worker-plugin:/install/worker" + networks: + - "clp-package" + presto-coordinator: - image: "ghcr.io/y-scope/presto/coordinator:${CLP_PRESTO_COORDINATOR_IMAGE_TAG:-clp-v0.10.0}" + image: "ghcr.io/y-scope/presto:${CLP_PRESTO_COORDINATOR_IMAGE_TAG:-0.299}" entrypoint: ["/bin/bash", "-c", "/scripts/generate-configs.sh && /opt/entrypoint.sh"] + depends_on: + clp-plugin-presto-connector-init: + condition: "service_completed_successfully" env_file: - ".env" - "coordinator-common.env" @@ -9,7 +29,9 @@ services: volumes: - "./coordinator/config-template:/configs:ro" - "./coordinator/scripts:/scripts:ro" - - "coordinator-config:/opt/presto-server/etc" + - type: "tmpfs" + target: "/opt/presto-server/etc" + - "coordinator-plugin:/opt/presto-server/plugin/clp" networks: - "clp-package" ports: @@ -24,11 +46,12 @@ services: retries: 30 presto-worker: - image: >- - ghcr.io/y-scope/presto/prestissimo-worker:${CLP_PRESTO_WORKER_IMAGE_TAG:-clp-v0.10.0-fix.1} + image: "ghcr.io/y-scope/presto-native:${CLP_PRESTO_WORKER_IMAGE_TAG:-0.299}" depends_on: presto-coordinator: condition: "service_healthy" + clp-plugin-presto-connector-init: + condition: "service_completed_successfully" entrypoint: ["/bin/bash", "-c", "/scripts/generate-configs.sh && /opt/entrypoint.sh"] env_file: - ".env" @@ -39,7 +62,9 @@ services: - "${CLP_STAGED_ARCHIVES_DIR:-empty}:/var/data/staged-archives:ro" - "./worker/config-template:/configs:ro" - "./worker/scripts:/scripts:ro" - - "worker-config:/opt/presto-server/etc" + - type: "tmpfs" + target: "/opt/presto-server/etc" + - "worker-plugin:/opt/presto-server/plugin/clp" networks: - "clp-package" @@ -47,8 +72,10 @@ volumes: # Dummy volume to use when a bind mount is not desired. empty: - coordinator-config: - worker-config: + # Populated by clp-plugin-presto-connector-init; mounted into the coordinator/worker + # plugin directories. + coordinator-plugin: + worker-plugin: networks: clp-package: diff --git a/tools/deployment/presto-clp/worker/config-template/config.properties b/tools/deployment/presto-clp/worker/config-template/config.properties index 2b0d386b1b..2fbf419c84 100644 --- a/tools/deployment/presto-clp/worker/config-template/config.properties +++ b/tools/deployment/presto-clp/worker/config-template/config.properties @@ -5,3 +5,4 @@ shutdown-onset-sec=1 system-memory-gb=${PRESTO_WORKER_CONFIGPROPERTIES_SYSTEM_MEMORY_GB} register-test-functions=false runtime-metrics-collection-enabled=false +plugin.dir=/opt/presto-server/plugin/clp diff --git a/tools/deployment/presto-clp/worker/scripts/generate-configs.sh b/tools/deployment/presto-clp/worker/scripts/generate-configs.sh index a2205f5502..a9ab197a28 100755 --- a/tools/deployment/presto-clp/worker/scripts/generate-configs.sh +++ b/tools/deployment/presto-clp/worker/scripts/generate-configs.sh @@ -24,11 +24,13 @@ get_coordinator_version() { local discovery_uri discovery_uri=$(awk -F "=" '/^discovery.uri=/ {print $2}' "$config_properties_file") - if response=$( - wget --quiet --output-document - --timeout 10 "${discovery_uri}/v1/info" 2>/dev/null - ); then - version=$(echo "$response" | jq --raw-output '.nodeVersion.version') - if [[ "$version" = "null" ]]; then + if response=$(curl --fail --silent --max-time 10 "${discovery_uri}/v1/info"); then + if ! version=$( + echo "$response" \ + | python3 -c \ + "import json, sys; print(json.load(sys.stdin)['nodeVersion']['version'])" \ + 2>/dev/null + ); then log "ERROR" "Presto response is empty or doesn't contain version info." exit 1 fi @@ -58,8 +60,6 @@ update_config_file() { log "INFO" "Set ${key}=${value} in ${file_path}" } -apt-get update && apt-get install --assume-yes --no-install-recommends jq wget - readonly PRESTO_CONFIG_DIR="/opt/presto-server/etc" # Substitute environment variables in config template @@ -71,8 +71,8 @@ find /configs -type f | while read -r f; do ) | sh >"${PRESTO_CONFIG_DIR}/$(basename "$f")" done -# Remove existing catalog files that exist in the image and add the CLP catalog -rm -f "${PRESTO_CONFIG_DIR}/catalog/"* +# Create the catalog directory and add the CLP catalog +mkdir -p "${PRESTO_CONFIG_DIR}/catalog" mv "${PRESTO_CONFIG_DIR}/clp.properties" "${PRESTO_CONFIG_DIR}/catalog" # Update config.properties @@ -82,6 +82,12 @@ log "INFO" "Detected Presto version: $version" update_config_file "$CONFIG_PROPERTIES_FILE" "presto.version" "$version" # Update node.properties +# +# NOTE: These are resolved through Python rather than `hostname`, which the Presto worker image +# doesn't ship. Assigning them first ensures a resolution failure aborts the script instead of +# silently writing empty values. readonly NODE_PROPERTIES_FILE="/opt/presto-server/etc/node.properties" -update_config_file "$NODE_PROPERTIES_FILE" "node.internal-address" "$(hostname -i)" -update_config_file "$NODE_PROPERTIES_FILE" "node.id" "$(hostname)" +node_internal_address=$(python3 -c "import socket; print(socket.gethostbyname(socket.gethostname()))") +node_id=$(python3 -c "import socket; print(socket.gethostname())") +update_config_file "$NODE_PROPERTIES_FILE" "node.internal-address" "$node_internal_address" +update_config_file "$NODE_PROPERTIES_FILE" "node.id" "$node_id" From 134f7f5d40e639449be2be7317594d1c7708830e Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Thu, 30 Jul 2026 17:39:08 -0400 Subject: [PATCH 02/17] chore(helm): Bump chart version to 0.4.1-dev.4 for the connector-plugin deployment changes. --- tools/deployment/package-helm/Chart.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/deployment/package-helm/Chart.yaml b/tools/deployment/package-helm/Chart.yaml index 5e6fef4af4..82ef2f2b56 100644 --- a/tools/deployment/package-helm/Chart.yaml +++ b/tools/deployment/package-helm/Chart.yaml @@ -1,6 +1,6 @@ apiVersion: "v2" name: "clp" -version: "0.4.1-dev.3" +version: "0.4.1-dev.4" description: "A Helm chart for CLP's (Compressed Log Processor) package deployment" type: "application" appVersion: "0.13.1-dev" From b8fde9a72b9d551df9c4a87355e25987b8012d66 Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Thu, 30 Jul 2026 17:51:40 -0400 Subject: [PATCH 03/17] fix(presto-clp): Wrap the connector init image line to satisfy yamllint line-length. --- tools/deployment/presto-clp/docker-compose.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/deployment/presto-clp/docker-compose.yaml b/tools/deployment/presto-clp/docker-compose.yaml index 53592db171..c1202be29b 100644 --- a/tools/deployment/presto-clp/docker-compose.yaml +++ b/tools/deployment/presto-clp/docker-compose.yaml @@ -5,7 +5,9 @@ services: # Presto images). The connector image's own entrypoint does the install; we only supply # the target paths via *_PLUGIN_INSTALL_PATH. # TODO: default CLP_CONNECTOR_TAG to the released multi-arch tag before merging. - image: "${CLP_CONNECTOR_IMAGE:-ghcr.io/y-scope/clp-plugin-presto-connector}:${CLP_CONNECTOR_TAG:-0.1.0-SNAPSHOT-arm64}" + image: "${CLP_CONNECTOR_IMAGE:-\ + ghcr.io/y-scope/clp-plugin-presto-connector}:${CLP_CONNECTOR_TAG:-\ + 0.1.0-SNAPSHOT-arm64}" restart: "no" environment: COORDINATOR_PLUGIN_INSTALL_PATH: "/install/coordinator" From e0b406ede27be88f15eddd1fad8299fa023f4aaa Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Thu, 30 Jul 2026 18:34:37 -0400 Subject: [PATCH 04/17] feat(presto-clp): Auto-resolve the CLP connector image tag in Compose and document local testing. `init.py` now resolves `CLP_CONNECTOR_IMAGE`/`CLP_CONNECTOR_TAG` into `.env` instead of relying on the arm64-specific default in `docker-compose.yaml`. It tries, in order, a locally-built per-architecture tag (`-`, via `docker image inspect`), a published multi-architecture tag (``, via `docker manifest inspect`), and a published per-architecture tag, erroring with an actionable message if none is found. This lets a connector built locally via `task package` (which only loads the building machine's `:-` tag, since a multi-arch manifest can't be loaded into a local daemon) be picked up automatically, while published deployments use the multi-arch manifest. `CLP_CONNECTOR_PREFER_LOCAL=false` flips the order to prefer the published multi-architecture image, so an upstream release can be tested even when a local build of the same version is loaded; `CLP_CONNECTOR_TAG`/`CLP_CONNECTOR_IMAGE`/`CLP_CONNECTOR_VERSION` override the resolution directly. The inline default tag in `docker-compose.yaml` becomes the multi-arch `0.1.0-SNAPSHOT` (no arch suffix) since it's only used when `.env` is absent; `init.py`'s resolved tag is authoritative. Add a short README under `tools/deployment/presto-clp` covering setup, the resolution order and its overrides, building the connector locally, and Helm (kind) local testing via `set-up-test.sh --presto --clp-connector-image=...` (the local-image path the Helm set-up scripts already wire up). --- tools/deployment/presto-clp/README.md | 78 +++++++ .../deployment/presto-clp/docker-compose.yaml | 8 +- tools/deployment/presto-clp/scripts/init.py | 205 +++++++++++++++++- 3 files changed, 288 insertions(+), 3 deletions(-) create mode 100644 tools/deployment/presto-clp/README.md diff --git a/tools/deployment/presto-clp/README.md b/tools/deployment/presto-clp/README.md new file mode 100644 index 0000000000..937f015dd4 --- /dev/null +++ b/tools/deployment/presto-clp/README.md @@ -0,0 +1,78 @@ +# presto-clp + +Docker Compose deployment of Presto with the [CLP Presto connector][clp-connector] installed at +startup, for querying a running [CLP package][clp-package]'s archives from Presto. + +The coordinator and worker run the stock `ghcr.io/y-scope/presto` and +`ghcr.io/y-scope/presto-native` images (unmodified); the CLP connector plugin is installed into +shared volumes by a one-shot `clp-plugin-presto-connector-init` service before Presto starts. + +## Setup + +This stack runs alongside a CLP package and reads its config, so start the CLP package first, then +generate `.env` from the package's config: + +```sh +./scripts/set-up-config.sh +``` + +This creates a Python venv, renders the Presto config from `clp-config.yaml` / `credentials.yaml`, +and resolves the connector image tag (see below) into `.env`. Then start the stack: + +```sh +docker compose up -d +``` + +The coordinator UI is exposed on the host at `http://localhost:8889`. + +## Connector image resolution + +`init.py` writes `CLP_CONNECTOR_IMAGE` and `CLP_CONNECTOR_TAG` into `.env`, which +`docker-compose.yaml` consumes. By default the tag is resolved by trying, in order: + +1. A **locally-built per-architecture** tag `-` (via `docker image inspect`) — so a + connector built locally via `task package` is picked up with no network lookup. +2. A **published multi-architecture** tag `` (via `docker manifest inspect`); each node + pulls its own architecture from the manifest. +3. A **published per-architecture** tag `-`. + +If none is found, the setup errors out with the refs it tried and instructions to build the +connector or set `CLP_CONNECTOR_TAG` explicitly. + +To test the **published/upstream** image even when a local build of the same version is loaded, set +`CLP_CONNECTOR_PREFER_LOCAL=false` — this tries the published multi-architecture tag first. + +Override the resolution by exporting environment variables before running `set-up-config.sh`: + +| Variable | Default | Purpose | +| --------------------------- | ------------------------------------------------- | ------------------------------------------------- | +| `CLP_CONNECTOR_IMAGE` | `ghcr.io/y-scope/clp-plugin-presto-connector` | Connector image repository. | +| `CLP_CONNECTOR_TAG` | (resolved) | Exact tag; skips resolution when set. | +| `CLP_CONNECTOR_VERSION` | `0.1.0-SNAPSHOT` | Version to resolve (multi-arch tag / per-arch prefix). | +| `CLP_CONNECTOR_PREFER_LOCAL`| `true` | `false` prefers the published multi-arch image over a local build. | + +## Building the connector locally + +In the [`clp-plugin-presto-connector`][clp-connector] repo, `task package` builds and loads the +connector image into the local Docker daemon under the per-architecture tag, e.g. + +`ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT-arm64` + +A multi-architecture manifest can't be loaded into a local daemon, so the per-architecture tag is +what local builds produce and what Compose picks up in step 1 above — no manual retag needed. + +## Helm (kind) local testing + +The Helm chart's local-image path is wired into the set-up scripts in +[`tools/deployment/package-helm`](../package-helm), which load a local image into the kind cluster +and set `image.clpConnector.{repository,tag,pullPolicy=Never}` for you. Pass the connector image via +`--clp-connector-image`, using the per-architecture tag (for the same local-daemon reason): + +```sh +./package-helm/set-up-test.sh --presto --clp-connector-image=:- +``` + +For example, `ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT-arm64`. + +[clp-connector]: https://github.com/y-scope/clp-plugin-presto-connector +[clp-package]: https://github.com/y-scope/clp/tree/main/tools/deployment/package \ No newline at end of file diff --git a/tools/deployment/presto-clp/docker-compose.yaml b/tools/deployment/presto-clp/docker-compose.yaml index c1202be29b..a18f260ff4 100644 --- a/tools/deployment/presto-clp/docker-compose.yaml +++ b/tools/deployment/presto-clp/docker-compose.yaml @@ -4,10 +4,14 @@ services: # coordinator and worker mount into their plugin dirs (replacing the connector-baked # Presto images). The connector image's own entrypoint does the install; we only supply # the target paths via *_PLUGIN_INSTALL_PATH. - # TODO: default CLP_CONNECTOR_TAG to the released multi-arch tag before merging. + # init.py resolves CLP_CONNECTOR_TAG into .env (preferring a locally-built per-architecture + # tag, then a published multi-architecture tag, then a published per-architecture tag). The + # defaults below are only used when .env is absent, so the tag default is the multi-architecture + # form (not arch-specific). + # TODO: default the version to the released multi-arch tag before merging. image: "${CLP_CONNECTOR_IMAGE:-\ ghcr.io/y-scope/clp-plugin-presto-connector}:${CLP_CONNECTOR_TAG:-\ - 0.1.0-SNAPSHOT-arm64}" + 0.1.0-SNAPSHOT}" restart: "no" environment: COORDINATOR_PLUGIN_INSTALL_PATH: "/install/coordinator" diff --git a/tools/deployment/presto-clp/scripts/init.py b/tools/deployment/presto-clp/scripts/init.py index 4cd357931d..6c7366ee89 100755 --- a/tools/deployment/presto-clp/scripts/init.py +++ b/tools/deployment/presto-clp/scripts/init.py @@ -3,6 +3,10 @@ import argparse import logging +import os +import platform +import shutil +import subprocess import sys from pathlib import Path from typing import Any @@ -24,6 +28,22 @@ # S3 URL constant AWS_S3_DOMAIN = "amazonaws.com" +# Default CLP Presto connector image and version. The connector is installed into Presto at +# startup from this image (see docker-compose.yaml). `init.py` resolves the actual tag (per the +# order in `_add_connector_image_env_vars`) and writes `CLP_CONNECTOR_IMAGE`/`CLP_CONNECTOR_TAG` +# to `.env`; the defaults below are only used when `.env` is absent. +# TODO: default the version to the released multi-arch tag before merging. +DEFAULT_CONNECTOR_IMAGE = "ghcr.io/y-scope/clp-plugin-presto-connector" +DEFAULT_CONNECTOR_VERSION = "0.1.0-SNAPSHOT" + +# Maps `platform.machine()` to the architecture suffix used in per-architecture connector tags. +_ARCH_NAME_BY_PLATFORM_MACHINE = { + "aarch64": "arm64", + "arm64": "arm64", + "x86_64": "amd64", + "amd64": "amd64", +} + # Set up console logging logging_console_handler = logging.StreamHandler() logging_formatter = logging.Formatter( @@ -40,7 +60,7 @@ logger = logging.getLogger(__name__) -def main(argv: list[str] | None = None) -> int: +def main(argv: list[str] | None = None) -> int: # noqa: PLR0911 """Initializes Presto worker configuration based on CLP package settings.""" if argv is None: argv = sys.argv @@ -89,6 +109,9 @@ def main(argv: list[str] | None = None) -> int: ): return 1 + if not _add_connector_image_env_vars(env_vars): + return 1 + with output_file.open("w") as output_file_handle: output_file_handle.writelines(f"{key}={value}\n" for key, value in env_vars.items()) @@ -363,6 +386,186 @@ def _add_worker_env_vars(coordinator_common_env_file_path: Path, env_vars: dict[ return True +def _add_connector_image_env_vars(env_vars: dict[str, str]) -> bool: + """ + Resolves the CLP Presto connector image and adds `CLP_CONNECTOR_IMAGE` and `CLP_CONNECTOR_TAG` + to `env_vars`, which `docker-compose.yaml` consumes. + + An explicit `CLP_CONNECTOR_TAG` in the environment is used as-is and skips resolution. The + `CLP_CONNECTOR_IMAGE` (repository) and `CLP_CONNECTOR_VERSION` (version) env vars override + their respective defaults. + + Otherwise the tag is resolved by trying, in order: a locally-built per-architecture image, + a published multi-architecture tag, and a published per-architecture tag. By default a + locally-built image is preferred (so local development picks up `task package`'s + `:-` tag without a network lookup); set `CLP_CONNECTOR_PREFER_LOCAL=false` to + prefer the published multi-architecture image instead, e.g. to test an upstream release even + when a local build of the same version is loaded. If no image is found, an error is logged and + False is returned. + + :param env_vars: Dictionary to populate with the connector image environment variables. + :return: Whether the image and tag were successfully resolved. + """ + image = os.environ.get("CLP_CONNECTOR_IMAGE", DEFAULT_CONNECTOR_IMAGE) + + explicit_tag = os.environ.get("CLP_CONNECTOR_TAG") + if explicit_tag is not None: + logger.info( + "Using explicitly provided CLP_CONNECTOR_TAG='%s' for connector image '%s'.", + explicit_tag, + image, + ) + env_vars["CLP_CONNECTOR_IMAGE"] = image + env_vars["CLP_CONNECTOR_TAG"] = explicit_tag + return True + + prefer_local = _env_is_truthy("CLP_CONNECTOR_PREFER_LOCAL", True) + version = os.environ.get("CLP_CONNECTOR_VERSION", DEFAULT_CONNECTOR_VERSION) + tag = _resolve_connector_tag(image, version, prefer_local) + if tag is None: + return False + + env_vars["CLP_CONNECTOR_IMAGE"] = image + env_vars["CLP_CONNECTOR_TAG"] = tag + return True + + +def _resolve_connector_tag(image: str, version: str, prefer_local: bool) -> str | None: + """ + Resolves the connector image tag for `image` and `version` per the order documented in + `_add_connector_image_env_vars`. + + :param image: The connector image repository. + :param version: The connector version (the multi-architecture tag, and the prefix of the + per-architecture tag). + :param prefer_local: When True, a locally-built per-architecture image is tried before the + published tags; when False, the published multi-architecture tag is tried first. + :return: The resolved tag, or None if no matching image was found. + """ + # Silence Ruff S607: the absolute path of the Docker binary may vary depending on the + # installation method. + docker_executable = "docker" + + if shutil.which(docker_executable) is None: + logger.error( + "Docker isn't installed or isn't on PATH, so the CLP Presto connector image can't be" + " resolved automatically. Install Docker, or set CLP_CONNECTOR_TAG explicitly." + ) + return None + + machine = platform.machine() + arch = _ARCH_NAME_BY_PLATFORM_MACHINE.get(machine) + if arch is None: + logger.error( + "Unsupported host architecture '%s' for resolving the CLP Presto connector image.", + machine, + ) + return None + + per_arch_tag = f"{version}-{arch}" + multi_arch_tag = version + per_arch_ref = f"{image}:{per_arch_tag}" + multi_arch_ref = f"{image}:{multi_arch_tag}" + + # Each candidate: (label, existence check, image ref, tag to use on a match). + local_per_arch = ( + "a locally-built per-architecture", + _image_exists_locally, + per_arch_ref, + per_arch_tag, + ) + registry_multi_arch = ( + "a published multi-architecture", + _tag_exists_on_registry, + multi_arch_ref, + multi_arch_tag, + ) + registry_per_arch = ( + "a published per-architecture", + _tag_exists_on_registry, + per_arch_ref, + per_arch_tag, + ) + if prefer_local: + candidates = [local_per_arch, registry_multi_arch, registry_per_arch] + else: + candidates = [registry_multi_arch, local_per_arch, registry_per_arch] + + for label, check, ref, tag in candidates: + if check(docker_executable, ref): + logger.info("Resolved CLP connector image to %s tag '%s'.", label, ref) + return tag + + logger.error( + "Couldn't find a CLP Presto connector image for version '%s'. Looked for %s image '%s'," + " %s image '%s', and %s image '%s'. Build the connector image (e.g. via `task package` in" + " clp-plugin-presto-connector) or set CLP_CONNECTOR_TAG explicitly.", + version, + local_per_arch[0], + local_per_arch[2], + registry_multi_arch[0], + registry_multi_arch[2], + registry_per_arch[0], + registry_per_arch[2], + ) + return None + + +def _env_is_truthy(name: str, default_value: bool) -> bool: + """ + Returns whether the environment variable `name` is truthy, using `default_value` when unset. + + "false", "0", "no", and "off" (case-insensitive) are treated as falsy; any other non-empty + value is truthy. + + :param name: The environment variable name. + :param default_value: The value to return when the variable is unset. + :return: Whether the variable is truthy. + """ + value = os.environ.get(name) + if value is None: + return default_value + return value.strip().lower() not in {"0", "false", "no", "off"} + + +def _image_exists_locally(docker_executable: str, image_ref: str) -> bool: + """ + Returns whether `image_ref` is present in the local Docker daemon. + + :param docker_executable: The Docker executable to call. + :param image_ref: The image reference (repository:tag). + :return: True if the image exists locally. + """ + return ( + subprocess.run( + [docker_executable, "image", "inspect", image_ref], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ).returncode + == 0 + ) + + +def _tag_exists_on_registry(docker_executable: str, image_ref: str) -> bool: + """ + Returns whether `image_ref` exists on its registry, queried via `docker manifest inspect`. + + :param docker_executable: The Docker executable to call. + :param image_ref: The image reference (repository:tag). + :return: True if the tag exists on the registry. + """ + return ( + subprocess.run( + [docker_executable, "manifest", "inspect", image_ref], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ).returncode + == 0 + ) + + def _generate_worker_clp_properties( worker_config_template_path: Path, env_vars: dict[str, str] ) -> bool: From bd48358e98672fc0fd43951dac3872ab59e16f0c Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Thu, 30 Jul 2026 18:51:59 -0400 Subject: [PATCH 05/17] docs(presto): Document connector image resolution and local testing. Move the connector-image-resolution and local-testing documentation out of a code-adjacent README under tools/deployment/presto-clp and into the docs tree: the user-facing resolution/override notes and the Helm --clp-connector-image flag go into guides-using-presto.md, and the developer content (building the connector image locally, the CLP_CONNECTOR_PREFER_LOCAL toggle, and the Helm kind recipe) goes into a new dev-docs page. --- docs/src/dev-docs/index.md | 8 ++ docs/src/dev-docs/testing-presto-connector.md | 73 +++++++++++++++++ docs/src/user-docs/guides-using-presto.md | 17 ++++ tools/deployment/presto-clp/README.md | 78 ------------------- 4 files changed, 98 insertions(+), 78 deletions(-) create mode 100644 docs/src/dev-docs/testing-presto-connector.md delete mode 100644 tools/deployment/presto-clp/README.md diff --git a/docs/src/dev-docs/index.md b/docs/src/dev-docs/index.md index f22ae29f37..6036563ba9 100644 --- a/docs/src/dev-docs/index.md +++ b/docs/src/dev-docs/index.md @@ -15,6 +15,13 @@ Building Docs about building CLP. ::: +:::{grid-item-card} +:link: testing-presto-connector +Presto connector +^^^ +Building and testing the CLP Presto connector locally. +::: + :::{grid-item-card} :link: testing/index Testing @@ -55,6 +62,7 @@ Any design docs describing parts of this project. :hidden: building-package +testing-presto-connector ::: :::{toctree} diff --git a/docs/src/dev-docs/testing-presto-connector.md b/docs/src/dev-docs/testing-presto-connector.md new file mode 100644 index 0000000000..f97a59e769 --- /dev/null +++ b/docs/src/dev-docs/testing-presto-connector.md @@ -0,0 +1,73 @@ +# Building and testing the Presto connector + +The Presto integration installs the [CLP connector][clp-connector] at startup from the +`ghcr.io/y-scope/clp-plugin-presto-connector` image, so that the stock Presto images can be used +unmodified. This page covers building that connector image locally and pointing the +[Docker Compose](#docker-compose) and [Helm](#helm-kind) stacks at it for testing. + +The connector image is published as a multi-architecture manifest, but a multi-architecture manifest +can't be loaded into a local Docker daemon. Local builds therefore produce, and the stacks consume, +a per-architecture tag of the form `-` (e.g. `0.1.0-SNAPSHOT-arm64`). + +## Building the connector image + +In the [`clp-plugin-presto-connector`][clp-connector] repository, run: + +```shell +task package +``` + +This builds the connector image and loads it into the local Docker daemon under the +per-architecture tag, e.g. `ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT-arm64`. + +## Docker Compose + +The `presto-clp` Compose stack is in `tools/deployment/presto-clp`. `scripts/set-up-config.sh` +resolves the connector tag into `.env`, trying in order: + +1. A locally-built per-architecture tag (`-`, via `docker image inspect`). +2. A published multi-architecture tag (``, via `docker manifest inspect`). +3. A published per-architecture tag (`-`). + +If none is found, it errors with the refs it tried. Set `CLP_CONNECTOR_PREFER_LOCAL=false` to flip +the order and test the published multi-architecture image even when a local build of the same +version is loaded. Override the resolution by exporting environment variables before running +`set-up-config.sh`: + +* `CLP_CONNECTOR_IMAGE` (default `ghcr.io/y-scope/clp-plugin-presto-connector`): connector image + repository. +* `CLP_CONNECTOR_TAG`: exact tag; skips resolution when set. +* `CLP_CONNECTOR_VERSION` (default `0.1.0-SNAPSHOT`): version to resolve (multi-arch tag / per-arch + prefix). +* `CLP_CONNECTOR_PREFER_LOCAL` (default `true`): set to `false` to prefer the published multi-arch + image over a local build. + +Then start the stack: + +```shell +./scripts/set-up-config.sh +docker compose up -d +``` + +See the [Using Presto with CLP][using-presto] user guide for the full setup. + +## Helm (kind) + +The Helm chart's local-image path is wired into the set-up scripts in +`tools/deployment/package-helm`, which load a local image into the `kind` cluster and set +`image.clpConnector.{repository,tag,pullPolicy=Never}` for you. Pass the connector image via +`--clp-connector-image`, using the per-architecture tag: + +```shell +./package-helm/set-up-test.sh --presto --clp-connector-image=:- +``` + +For example: + +```shell +./package-helm/set-up-test.sh --presto \ + --clp-connector-image=ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT-arm64 +``` + +[clp-connector]: https://github.com/y-scope/clp-plugin-presto-connector +[using-presto]: ../user-docs/guides-using-presto.md diff --git a/docs/src/user-docs/guides-using-presto.md b/docs/src/user-docs/guides-using-presto.md index 0a66152356..ec95d629ff 100644 --- a/docs/src/user-docs/guides-using-presto.md +++ b/docs/src/user-docs/guides-using-presto.md @@ -102,6 +102,13 @@ When using Kubernetes, Presto worker scheduling can be configured using the Kubernetes deployment guide for details. ::: +:::{note} +The CLP connector is installed at startup from the image under `image.clpConnector` (default +`ghcr.io/y-scope/clp-plugin-presto-connector`). To test a locally-built connector image in a `kind` +cluster, use the set-up script's `--clp-connector-image` flag; see +[Testing the Presto connector locally][testing-presto-connector]. +::: + ## Docker Compose ### Requirements @@ -199,6 +206,15 @@ Using Presto with CLP via Docker Compose requires: * Replace `` with the location of the clp-json package you set up in the previous section. + :::{note} + `set-up-config.sh` also resolves the CLP connector image tag into `.env`. It prefers a + locally-built per-architecture image, then a published multi-architecture image, then + a published per-architecture image, and errors if none is found. Override the resolution + by exporting `CLP_CONNECTOR_IMAGE`, `CLP_CONNECTOR_TAG`, or `CLP_CONNECTOR_VERSION` before + running the script; see [Testing the Presto connector locally][testing-presto-connector] for + building the connector image yourself. + ::: + 4. Configure Presto to use CLP's metadata database as follows: * Open and edit `coordinator/config-template/split-filter.json`. @@ -332,3 +348,4 @@ These limitations will be addressed in a future release of the Presto integratio [Presto]: https://prestodb.io/ [y-scope/presto#8]: https://github.com/y-scope/presto/issues/8 [yscope-presto]: https://github.com/y-scope/presto +[testing-presto-connector]: ../dev-docs/testing-presto-connector.md diff --git a/tools/deployment/presto-clp/README.md b/tools/deployment/presto-clp/README.md deleted file mode 100644 index 937f015dd4..0000000000 --- a/tools/deployment/presto-clp/README.md +++ /dev/null @@ -1,78 +0,0 @@ -# presto-clp - -Docker Compose deployment of Presto with the [CLP Presto connector][clp-connector] installed at -startup, for querying a running [CLP package][clp-package]'s archives from Presto. - -The coordinator and worker run the stock `ghcr.io/y-scope/presto` and -`ghcr.io/y-scope/presto-native` images (unmodified); the CLP connector plugin is installed into -shared volumes by a one-shot `clp-plugin-presto-connector-init` service before Presto starts. - -## Setup - -This stack runs alongside a CLP package and reads its config, so start the CLP package first, then -generate `.env` from the package's config: - -```sh -./scripts/set-up-config.sh -``` - -This creates a Python venv, renders the Presto config from `clp-config.yaml` / `credentials.yaml`, -and resolves the connector image tag (see below) into `.env`. Then start the stack: - -```sh -docker compose up -d -``` - -The coordinator UI is exposed on the host at `http://localhost:8889`. - -## Connector image resolution - -`init.py` writes `CLP_CONNECTOR_IMAGE` and `CLP_CONNECTOR_TAG` into `.env`, which -`docker-compose.yaml` consumes. By default the tag is resolved by trying, in order: - -1. A **locally-built per-architecture** tag `-` (via `docker image inspect`) — so a - connector built locally via `task package` is picked up with no network lookup. -2. A **published multi-architecture** tag `` (via `docker manifest inspect`); each node - pulls its own architecture from the manifest. -3. A **published per-architecture** tag `-`. - -If none is found, the setup errors out with the refs it tried and instructions to build the -connector or set `CLP_CONNECTOR_TAG` explicitly. - -To test the **published/upstream** image even when a local build of the same version is loaded, set -`CLP_CONNECTOR_PREFER_LOCAL=false` — this tries the published multi-architecture tag first. - -Override the resolution by exporting environment variables before running `set-up-config.sh`: - -| Variable | Default | Purpose | -| --------------------------- | ------------------------------------------------- | ------------------------------------------------- | -| `CLP_CONNECTOR_IMAGE` | `ghcr.io/y-scope/clp-plugin-presto-connector` | Connector image repository. | -| `CLP_CONNECTOR_TAG` | (resolved) | Exact tag; skips resolution when set. | -| `CLP_CONNECTOR_VERSION` | `0.1.0-SNAPSHOT` | Version to resolve (multi-arch tag / per-arch prefix). | -| `CLP_CONNECTOR_PREFER_LOCAL`| `true` | `false` prefers the published multi-arch image over a local build. | - -## Building the connector locally - -In the [`clp-plugin-presto-connector`][clp-connector] repo, `task package` builds and loads the -connector image into the local Docker daemon under the per-architecture tag, e.g. - -`ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT-arm64` - -A multi-architecture manifest can't be loaded into a local daemon, so the per-architecture tag is -what local builds produce and what Compose picks up in step 1 above — no manual retag needed. - -## Helm (kind) local testing - -The Helm chart's local-image path is wired into the set-up scripts in -[`tools/deployment/package-helm`](../package-helm), which load a local image into the kind cluster -and set `image.clpConnector.{repository,tag,pullPolicy=Never}` for you. Pass the connector image via -`--clp-connector-image`, using the per-architecture tag (for the same local-daemon reason): - -```sh -./package-helm/set-up-test.sh --presto --clp-connector-image=:- -``` - -For example, `ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT-arm64`. - -[clp-connector]: https://github.com/y-scope/clp-plugin-presto-connector -[clp-package]: https://github.com/y-scope/clp/tree/main/tools/deployment/package \ No newline at end of file From 63cc8b77d94911575bc431704c821b3cbf6ff99a Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Fri, 31 Jul 2026 20:34:48 -0400 Subject: [PATCH 06/17] presto-clp: share one : tag between local and published connector images Drop the :- suffix scheme for locally-built connector images in favor of the conventional Docker pattern: local builds and the published multi-arch image share the same : tag, and whatever is in the local daemon wins (docker pull to go back to the published image). Matches the corresponding change in clp-plugin-presto-connector. init.py's three-candidate tag resolution (local per-arch, published multi-arch, published per-arch) and CLP_CONNECTOR_PREFER_LOCAL collapse into a single existence check: error if the tag is in neither the local daemon nor the registry. The Helm default tag also loses its arm64 suffix, which was wrong on amd64 hosts. Docs updated; the --clp-connector-image examples also switch to the space-separated form the parser actually accepts. --- docs/src/dev-docs/testing-presto-connector.md | 38 ++--- docs/src/user-docs/guides-using-presto.md | 12 +- tools/deployment/package-helm/values.yaml | 4 +- .../deployment/presto-clp/docker-compose.yaml | 10 +- tools/deployment/presto-clp/scripts/init.py | 139 +++++------------- 5 files changed, 61 insertions(+), 142 deletions(-) diff --git a/docs/src/dev-docs/testing-presto-connector.md b/docs/src/dev-docs/testing-presto-connector.md index f97a59e769..d88ca23f2e 100644 --- a/docs/src/dev-docs/testing-presto-connector.md +++ b/docs/src/dev-docs/testing-presto-connector.md @@ -5,9 +5,11 @@ The Presto integration installs the [CLP connector][clp-connector] at startup fr unmodified. This page covers building that connector image locally and pointing the [Docker Compose](#docker-compose) and [Helm](#helm-kind) stacks at it for testing. -The connector image is published as a multi-architecture manifest, but a multi-architecture manifest -can't be loaded into a local Docker daemon. Local builds therefore produce, and the stacks consume, -a per-architecture tag of the form `-` (e.g. `0.1.0-SNAPSHOT-arm64`). +Locally-built and published connector images share the same `:` tag (e.g. +`0.1.0-SNAPSHOT`), following the conventional Docker pattern: whatever is in your local Docker +daemon wins, and Docker pulls the published image when nothing is loaded locally. To go back to +the published image after building locally, `docker pull` the tag (or `docker rmi` your local +build). ## Building the connector image @@ -17,30 +19,20 @@ In the [`clp-plugin-presto-connector`][clp-connector] repository, run: task package ``` -This builds the connector image and loads it into the local Docker daemon under the -per-architecture tag, e.g. `ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT-arm64`. +This builds the connector image and loads it into the local Docker daemon, e.g. +`ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT`. ## Docker Compose The `presto-clp` Compose stack is in `tools/deployment/presto-clp`. `scripts/set-up-config.sh` -resolves the connector tag into `.env`, trying in order: - -1. A locally-built per-architecture tag (`-`, via `docker image inspect`). -2. A published multi-architecture tag (``, via `docker manifest inspect`). -3. A published per-architecture tag (`-`). - -If none is found, it errors with the refs it tried. Set `CLP_CONNECTOR_PREFER_LOCAL=false` to flip -the order and test the published multi-architecture image even when a local build of the same -version is loaded. Override the resolution by exporting environment variables before running -`set-up-config.sh`: +verifies the connector image exists (locally or on the registry) and writes it into `.env`, +erroring with the ref it tried if neither exists. Override the image by exporting environment +variables before running `set-up-config.sh`: * `CLP_CONNECTOR_IMAGE` (default `ghcr.io/y-scope/clp-plugin-presto-connector`): connector image repository. -* `CLP_CONNECTOR_TAG`: exact tag; skips resolution when set. -* `CLP_CONNECTOR_VERSION` (default `0.1.0-SNAPSHOT`): version to resolve (multi-arch tag / per-arch - prefix). -* `CLP_CONNECTOR_PREFER_LOCAL` (default `true`): set to `false` to prefer the published multi-arch - image over a local build. +* `CLP_CONNECTOR_TAG`: exact tag; skips the existence check when set. +* `CLP_CONNECTOR_VERSION` (default `0.1.0-SNAPSHOT`): tag to verify and use. Then start the stack: @@ -56,17 +48,17 @@ See the [Using Presto with CLP][using-presto] user guide for the full setup. The Helm chart's local-image path is wired into the set-up scripts in `tools/deployment/package-helm`, which load a local image into the `kind` cluster and set `image.clpConnector.{repository,tag,pullPolicy=Never}` for you. Pass the connector image via -`--clp-connector-image`, using the per-architecture tag: +`--clp-connector-image`: ```shell -./package-helm/set-up-test.sh --presto --clp-connector-image=:- +./package-helm/set-up-test.sh --presto --clp-connector-image : ``` For example: ```shell ./package-helm/set-up-test.sh --presto \ - --clp-connector-image=ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT-arm64 + --clp-connector-image ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT ``` [clp-connector]: https://github.com/y-scope/clp-plugin-presto-connector diff --git a/docs/src/user-docs/guides-using-presto.md b/docs/src/user-docs/guides-using-presto.md index ec95d629ff..52f479f022 100644 --- a/docs/src/user-docs/guides-using-presto.md +++ b/docs/src/user-docs/guides-using-presto.md @@ -207,12 +207,12 @@ Using Presto with CLP via Docker Compose requires: section. :::{note} - `set-up-config.sh` also resolves the CLP connector image tag into `.env`. It prefers a - locally-built per-architecture image, then a published multi-architecture image, then - a published per-architecture image, and errors if none is found. Override the resolution - by exporting `CLP_CONNECTOR_IMAGE`, `CLP_CONNECTOR_TAG`, or `CLP_CONNECTOR_VERSION` before - running the script; see [Testing the Presto connector locally][testing-presto-connector] for - building the connector image yourself. + `set-up-config.sh` also writes the CLP connector image into `.env`, after verifying it + exists locally or on the registry (a locally-built image and the published one share the + same tag; whatever is in your Docker daemon wins). Override the image by exporting + `CLP_CONNECTOR_IMAGE`, `CLP_CONNECTOR_TAG`, or `CLP_CONNECTOR_VERSION` before running the + script; see [Testing the Presto connector locally][testing-presto-connector] for building + the connector image yourself. ::: 4. Configure Presto to use CLP's metadata database as follows: diff --git a/tools/deployment/package-helm/values.yaml b/tools/deployment/package-helm/values.yaml index c5f4127847..db19d0f936 100644 --- a/tools/deployment/package-helm/values.yaml +++ b/tools/deployment/package-helm/values.yaml @@ -7,11 +7,11 @@ allowHostAccessForSbinScripts: true image: # Installs the CLP Presto connector plugin into the Presto coordinator and worker pods. Override # the repository to use a different edition of the connector. - # TODO: default the tag to the released multi-arch tag before merging. + # TODO: default the tag to the released tag before merging. clpConnector: repository: "ghcr.io/y-scope/clp-plugin-presto-connector" pullPolicy: "IfNotPresent" - tag: "0.1.0-SNAPSHOT-arm64" + tag: "0.1.0-SNAPSHOT" clpPackage: repository: "ghcr.io/y-scope/clp/clp-package" pullPolicy: "Always" diff --git a/tools/deployment/presto-clp/docker-compose.yaml b/tools/deployment/presto-clp/docker-compose.yaml index a18f260ff4..5b77c0235e 100644 --- a/tools/deployment/presto-clp/docker-compose.yaml +++ b/tools/deployment/presto-clp/docker-compose.yaml @@ -4,11 +4,11 @@ services: # coordinator and worker mount into their plugin dirs (replacing the connector-baked # Presto images). The connector image's own entrypoint does the install; we only supply # the target paths via *_PLUGIN_INSTALL_PATH. - # init.py resolves CLP_CONNECTOR_TAG into .env (preferring a locally-built per-architecture - # tag, then a published multi-architecture tag, then a published per-architecture tag). The - # defaults below are only used when .env is absent, so the tag default is the multi-architecture - # form (not arch-specific). - # TODO: default the version to the released multi-arch tag before merging. + # init.py verifies the image exists (locally or on the registry) and writes + # CLP_CONNECTOR_IMAGE/CLP_CONNECTOR_TAG into .env. Locally-built and published images share + # the same : tag; whatever is in the local daemon wins. The defaults below are only + # used when .env is absent. + # TODO: default the version to the released tag before merging. image: "${CLP_CONNECTOR_IMAGE:-\ ghcr.io/y-scope/clp-plugin-presto-connector}:${CLP_CONNECTOR_TAG:-\ 0.1.0-SNAPSHOT}" diff --git a/tools/deployment/presto-clp/scripts/init.py b/tools/deployment/presto-clp/scripts/init.py index 6c7366ee89..801c0a9176 100755 --- a/tools/deployment/presto-clp/scripts/init.py +++ b/tools/deployment/presto-clp/scripts/init.py @@ -4,7 +4,6 @@ import argparse import logging import os -import platform import shutil import subprocess import sys @@ -29,21 +28,13 @@ AWS_S3_DOMAIN = "amazonaws.com" # Default CLP Presto connector image and version. The connector is installed into Presto at -# startup from this image (see docker-compose.yaml). `init.py` resolves the actual tag (per the -# order in `_add_connector_image_env_vars`) and writes `CLP_CONNECTOR_IMAGE`/`CLP_CONNECTOR_TAG` -# to `.env`; the defaults below are only used when `.env` is absent. +# startup from this image (see docker-compose.yaml). `init.py` verifies the image exists (per +# `_add_connector_image_env_vars`) and writes `CLP_CONNECTOR_IMAGE`/`CLP_CONNECTOR_TAG` to +# `.env`; the defaults below are only used when `.env` is absent. # TODO: default the version to the released multi-arch tag before merging. DEFAULT_CONNECTOR_IMAGE = "ghcr.io/y-scope/clp-plugin-presto-connector" DEFAULT_CONNECTOR_VERSION = "0.1.0-SNAPSHOT" -# Maps `platform.machine()` to the architecture suffix used in per-architecture connector tags. -_ARCH_NAME_BY_PLATFORM_MACHINE = { - "aarch64": "arm64", - "arm64": "arm64", - "x86_64": "amd64", - "amd64": "amd64", -} - # Set up console logging logging_console_handler = logging.StreamHandler() logging_formatter = logging.Formatter( @@ -391,17 +382,15 @@ def _add_connector_image_env_vars(env_vars: dict[str, str]) -> bool: Resolves the CLP Presto connector image and adds `CLP_CONNECTOR_IMAGE` and `CLP_CONNECTOR_TAG` to `env_vars`, which `docker-compose.yaml` consumes. - An explicit `CLP_CONNECTOR_TAG` in the environment is used as-is and skips resolution. The - `CLP_CONNECTOR_IMAGE` (repository) and `CLP_CONNECTOR_VERSION` (version) env vars override - their respective defaults. + An explicit `CLP_CONNECTOR_TAG` in the environment is used as-is and skips the existence + check. The `CLP_CONNECTOR_IMAGE` (repository) and `CLP_CONNECTOR_VERSION` (tag) env vars + override their respective defaults. - Otherwise the tag is resolved by trying, in order: a locally-built per-architecture image, - a published multi-architecture tag, and a published per-architecture tag. By default a - locally-built image is preferred (so local development picks up `task package`'s - `:-` tag without a network lookup); set `CLP_CONNECTOR_PREFER_LOCAL=false` to - prefer the published multi-architecture image instead, e.g. to test an upstream release even - when a local build of the same version is loaded. If no image is found, an error is logged and - False is returned. + Locally-built and published images share the `:` tag (the conventional Docker + pattern): whatever is in the local daemon is used, and Docker pulls the published image + when nothing is loaded locally. To test the published image over a stale local build, run + `docker pull :` first (or `docker rmi` the local one). This function only + verifies the image is available locally or on the registry, and errors otherwise. :param env_vars: Dictionary to populate with the connector image environment variables. :return: Whether the image and tag were successfully resolved. @@ -419,10 +408,8 @@ def _add_connector_image_env_vars(env_vars: dict[str, str]) -> bool: env_vars["CLP_CONNECTOR_TAG"] = explicit_tag return True - prefer_local = _env_is_truthy("CLP_CONNECTOR_PREFER_LOCAL", True) - version = os.environ.get("CLP_CONNECTOR_VERSION", DEFAULT_CONNECTOR_VERSION) - tag = _resolve_connector_tag(image, version, prefer_local) - if tag is None: + tag = os.environ.get("CLP_CONNECTOR_VERSION", DEFAULT_CONNECTOR_VERSION) + if not _connector_image_available(image, tag): return False env_vars["CLP_CONNECTOR_IMAGE"] = image @@ -430,17 +417,15 @@ def _add_connector_image_env_vars(env_vars: dict[str, str]) -> bool: return True -def _resolve_connector_tag(image: str, version: str, prefer_local: bool) -> str | None: +def _connector_image_available(image: str, tag: str) -> bool: """ - Resolves the connector image tag for `image` and `version` per the order documented in - `_add_connector_image_env_vars`. + Returns whether `image`:`tag` exists in the local Docker daemon or on its registry, logging + which one was found (a local image takes precedence at `docker compose up`) or an error if + neither exists. :param image: The connector image repository. - :param version: The connector version (the multi-architecture tag, and the prefix of the - per-architecture tag). - :param prefer_local: When True, a locally-built per-architecture image is tried before the - published tags; when False, the published multi-architecture tag is tried first. - :return: The resolved tag, or None if no matching image was found. + :param tag: The image tag. + :return: Whether the image is available. """ # Silence Ruff S607: the absolute path of the Docker binary may vary depending on the # installation method. @@ -449,83 +434,25 @@ def _resolve_connector_tag(image: str, version: str, prefer_local: bool) -> str if shutil.which(docker_executable) is None: logger.error( "Docker isn't installed or isn't on PATH, so the CLP Presto connector image can't be" - " resolved automatically. Install Docker, or set CLP_CONNECTOR_TAG explicitly." - ) - return None - - machine = platform.machine() - arch = _ARCH_NAME_BY_PLATFORM_MACHINE.get(machine) - if arch is None: - logger.error( - "Unsupported host architecture '%s' for resolving the CLP Presto connector image.", - machine, + " checked. Install Docker, or set CLP_CONNECTOR_TAG explicitly." ) - return None - - per_arch_tag = f"{version}-{arch}" - multi_arch_tag = version - per_arch_ref = f"{image}:{per_arch_tag}" - multi_arch_ref = f"{image}:{multi_arch_tag}" - - # Each candidate: (label, existence check, image ref, tag to use on a match). - local_per_arch = ( - "a locally-built per-architecture", - _image_exists_locally, - per_arch_ref, - per_arch_tag, - ) - registry_multi_arch = ( - "a published multi-architecture", - _tag_exists_on_registry, - multi_arch_ref, - multi_arch_tag, - ) - registry_per_arch = ( - "a published per-architecture", - _tag_exists_on_registry, - per_arch_ref, - per_arch_tag, - ) - if prefer_local: - candidates = [local_per_arch, registry_multi_arch, registry_per_arch] - else: - candidates = [registry_multi_arch, local_per_arch, registry_per_arch] + return False - for label, check, ref, tag in candidates: - if check(docker_executable, ref): - logger.info("Resolved CLP connector image to %s tag '%s'.", label, ref) - return tag + ref = f"{image}:{tag}" + if _image_exists_locally(docker_executable, ref): + logger.info("Found CLP connector image '%s' in the local Docker daemon.", ref) + return True + if _tag_exists_on_registry(docker_executable, ref): + logger.info("Found CLP connector image '%s' on the registry.", ref) + return True logger.error( - "Couldn't find a CLP Presto connector image for version '%s'. Looked for %s image '%s'," - " %s image '%s', and %s image '%s'. Build the connector image (e.g. via `task package` in" - " clp-plugin-presto-connector) or set CLP_CONNECTOR_TAG explicitly.", - version, - local_per_arch[0], - local_per_arch[2], - registry_multi_arch[0], - registry_multi_arch[2], - registry_per_arch[0], - registry_per_arch[2], + "Couldn't find CLP Presto connector image '%s' locally or on the registry. Build it" + " (e.g. via `task package` in clp-plugin-presto-connector) or set CLP_CONNECTOR_TAG" + " explicitly.", + ref, ) - return None - - -def _env_is_truthy(name: str, default_value: bool) -> bool: - """ - Returns whether the environment variable `name` is truthy, using `default_value` when unset. - - "false", "0", "no", and "off" (case-insensitive) are treated as falsy; any other non-empty - value is truthy. - - :param name: The environment variable name. - :param default_value: The value to return when the variable is unset. - :return: Whether the variable is truthy. - """ - value = os.environ.get(name) - if value is None: - return default_value - return value.strip().lower() not in {"0", "false", "no", "off"} + return False def _image_exists_locally(docker_executable: str, image_ref: str) -> bool: From 7b352386414f6e182d3f2cfdf84943193b771af2 Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Fri, 31 Jul 2026 20:56:55 -0400 Subject: [PATCH 07/17] presto-clp: rename CLP_CONNECTOR_* env vars to CLP_PRESTO_CONNECTOR_* CLP_CONNECTOR_IMAGE/TAG/VERSION were ambiguous (connector for what?); rename to CLP_PRESTO_CONNECTOR_* to match the image repo (clp-plugin-presto-connector) and the existing PRESTO_COORDINATOR_*/PRESTO_WORKER_* env var family. The vars are new in this PR so there's no backward-compat to preserve. Also renames the same-named internal shell variable in the Helm set-up scripts for consistency; the --clp-connector-image CLI flag is unchanged. --- docs/src/dev-docs/testing-presto-connector.md | 6 ++-- docs/src/user-docs/guides-using-presto.md | 2 +- .../deployment/package-helm/.set-up-common.sh | 6 ++-- .../set-up-multi-dedicated-test.sh | 2 +- .../package-helm/set-up-multi-shared-test.sh | 2 +- tools/deployment/package-helm/set-up-test.sh | 2 +- .../deployment/presto-clp/docker-compose.yaml | 10 +++--- tools/deployment/presto-clp/scripts/init.py | 34 +++++++++---------- 8 files changed, 32 insertions(+), 32 deletions(-) diff --git a/docs/src/dev-docs/testing-presto-connector.md b/docs/src/dev-docs/testing-presto-connector.md index d88ca23f2e..5c5e252935 100644 --- a/docs/src/dev-docs/testing-presto-connector.md +++ b/docs/src/dev-docs/testing-presto-connector.md @@ -29,10 +29,10 @@ verifies the connector image exists (locally or on the registry) and writes it i erroring with the ref it tried if neither exists. Override the image by exporting environment variables before running `set-up-config.sh`: -* `CLP_CONNECTOR_IMAGE` (default `ghcr.io/y-scope/clp-plugin-presto-connector`): connector image +* `CLP_PRESTO_CONNECTOR_IMAGE` (default `ghcr.io/y-scope/clp-plugin-presto-connector`): connector image repository. -* `CLP_CONNECTOR_TAG`: exact tag; skips the existence check when set. -* `CLP_CONNECTOR_VERSION` (default `0.1.0-SNAPSHOT`): tag to verify and use. +* `CLP_PRESTO_CONNECTOR_TAG`: exact tag; skips the existence check when set. +* `CLP_PRESTO_CONNECTOR_VERSION` (default `0.1.0-SNAPSHOT`): tag to verify and use. Then start the stack: diff --git a/docs/src/user-docs/guides-using-presto.md b/docs/src/user-docs/guides-using-presto.md index 52f479f022..51c8b38a40 100644 --- a/docs/src/user-docs/guides-using-presto.md +++ b/docs/src/user-docs/guides-using-presto.md @@ -210,7 +210,7 @@ Using Presto with CLP via Docker Compose requires: `set-up-config.sh` also writes the CLP connector image into `.env`, after verifying it exists locally or on the registry (a locally-built image and the published one share the same tag; whatever is in your Docker daemon wins). Override the image by exporting - `CLP_CONNECTOR_IMAGE`, `CLP_CONNECTOR_TAG`, or `CLP_CONNECTOR_VERSION` before running the + `CLP_PRESTO_CONNECTOR_IMAGE`, `CLP_PRESTO_CONNECTOR_TAG`, or `CLP_PRESTO_CONNECTOR_VERSION` before running the script; see [Testing the Presto connector locally][testing-presto-connector] for building the connector image yourself. ::: diff --git a/tools/deployment/package-helm/.set-up-common.sh b/tools/deployment/package-helm/.set-up-common.sh index 403bbbd1cb..72b9a5cbe5 100755 --- a/tools/deployment/package-helm/.set-up-common.sh +++ b/tools/deployment/package-helm/.set-up-common.sh @@ -75,12 +75,12 @@ get_image_helm_args() { } # Parses common arguments shared across set-up scripts. -# Sets CLP_PACKAGE_IMAGE, CLP_CONNECTOR_IMAGE, and ENABLE_PRESTO global variables. +# Sets CLP_PACKAGE_IMAGE, CLP_PRESTO_CONNECTOR_IMAGE, and ENABLE_PRESTO global variables. # # @param {string[]} args Script arguments parse_common_args() { CLP_PACKAGE_IMAGE="" - CLP_CONNECTOR_IMAGE="" + CLP_PRESTO_CONNECTOR_IMAGE="" ENABLE_PRESTO="false" while [[ $# -gt 0 ]]; do case "$1" in @@ -97,7 +97,7 @@ parse_common_args() { echo "Error: '--clp-connector-image' requires a value." >&2 exit 1 fi - CLP_CONNECTOR_IMAGE="$2" + CLP_PRESTO_CONNECTOR_IMAGE="$2" shift 2 ;; --presto) diff --git a/tools/deployment/package-helm/set-up-multi-dedicated-test.sh b/tools/deployment/package-helm/set-up-multi-dedicated-test.sh index cb9a2b27f4..6270655d39 100755 --- a/tools/deployment/package-helm/set-up-multi-dedicated-test.sh +++ b/tools/deployment/package-helm/set-up-multi-dedicated-test.sh @@ -163,6 +163,6 @@ helm install test "${script_dir}" \ $(get_service_exposure_helm_args) \ $(get_presto_helm_args) \ $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_CONNECTOR_IMAGE}") + $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") wait_for_cluster_ready diff --git a/tools/deployment/package-helm/set-up-multi-shared-test.sh b/tools/deployment/package-helm/set-up-multi-shared-test.sh index 1a8461b200..fd30d9c322 100755 --- a/tools/deployment/package-helm/set-up-multi-shared-test.sh +++ b/tools/deployment/package-helm/set-up-multi-shared-test.sh @@ -53,6 +53,6 @@ helm install test "${script_dir}" \ $(get_service_exposure_helm_args) \ $(get_presto_helm_args) \ $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_CONNECTOR_IMAGE}") + $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") wait_for_cluster_ready diff --git a/tools/deployment/package-helm/set-up-test.sh b/tools/deployment/package-helm/set-up-test.sh index 50be5ac079..33c5d8a44d 100755 --- a/tools/deployment/package-helm/set-up-test.sh +++ b/tools/deployment/package-helm/set-up-test.sh @@ -32,6 +32,6 @@ helm install test "${script_dir}" \ $(get_service_exposure_helm_args) \ $(get_presto_helm_args) \ $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_CONNECTOR_IMAGE}") + $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") wait_for_cluster_ready diff --git a/tools/deployment/presto-clp/docker-compose.yaml b/tools/deployment/presto-clp/docker-compose.yaml index 5b77c0235e..51ef28dbd1 100644 --- a/tools/deployment/presto-clp/docker-compose.yaml +++ b/tools/deployment/presto-clp/docker-compose.yaml @@ -5,12 +5,12 @@ services: # Presto images). The connector image's own entrypoint does the install; we only supply # the target paths via *_PLUGIN_INSTALL_PATH. # init.py verifies the image exists (locally or on the registry) and writes - # CLP_CONNECTOR_IMAGE/CLP_CONNECTOR_TAG into .env. Locally-built and published images share - # the same : tag; whatever is in the local daemon wins. The defaults below are only - # used when .env is absent. + # CLP_PRESTO_CONNECTOR_IMAGE/CLP_PRESTO_CONNECTOR_TAG into .env. Locally-built and published + # images share the same : tag; whatever is in the local daemon wins. The defaults + # below are only used when .env is absent. # TODO: default the version to the released tag before merging. - image: "${CLP_CONNECTOR_IMAGE:-\ - ghcr.io/y-scope/clp-plugin-presto-connector}:${CLP_CONNECTOR_TAG:-\ + image: "${CLP_PRESTO_CONNECTOR_IMAGE:-\ + ghcr.io/y-scope/clp-plugin-presto-connector}:${CLP_PRESTO_CONNECTOR_TAG:-\ 0.1.0-SNAPSHOT}" restart: "no" environment: diff --git a/tools/deployment/presto-clp/scripts/init.py b/tools/deployment/presto-clp/scripts/init.py index 801c0a9176..b61035e067 100755 --- a/tools/deployment/presto-clp/scripts/init.py +++ b/tools/deployment/presto-clp/scripts/init.py @@ -29,8 +29,8 @@ # Default CLP Presto connector image and version. The connector is installed into Presto at # startup from this image (see docker-compose.yaml). `init.py` verifies the image exists (per -# `_add_connector_image_env_vars`) and writes `CLP_CONNECTOR_IMAGE`/`CLP_CONNECTOR_TAG` to -# `.env`; the defaults below are only used when `.env` is absent. +# `_add_connector_image_env_vars`) and writes `CLP_PRESTO_CONNECTOR_IMAGE`/ +# `CLP_PRESTO_CONNECTOR_TAG` to `.env`; the defaults below are only used when `.env` is absent. # TODO: default the version to the released multi-arch tag before merging. DEFAULT_CONNECTOR_IMAGE = "ghcr.io/y-scope/clp-plugin-presto-connector" DEFAULT_CONNECTOR_VERSION = "0.1.0-SNAPSHOT" @@ -379,12 +379,12 @@ def _add_worker_env_vars(coordinator_common_env_file_path: Path, env_vars: dict[ def _add_connector_image_env_vars(env_vars: dict[str, str]) -> bool: """ - Resolves the CLP Presto connector image and adds `CLP_CONNECTOR_IMAGE` and `CLP_CONNECTOR_TAG` - to `env_vars`, which `docker-compose.yaml` consumes. + Resolves the CLP Presto connector image and adds `CLP_PRESTO_CONNECTOR_IMAGE` and + `CLP_PRESTO_CONNECTOR_TAG` to `env_vars`, which `docker-compose.yaml` consumes. - An explicit `CLP_CONNECTOR_TAG` in the environment is used as-is and skips the existence - check. The `CLP_CONNECTOR_IMAGE` (repository) and `CLP_CONNECTOR_VERSION` (tag) env vars - override their respective defaults. + An explicit `CLP_PRESTO_CONNECTOR_TAG` in the environment is used as-is and skips the + existence check. The `CLP_PRESTO_CONNECTOR_IMAGE` (repository) and + `CLP_PRESTO_CONNECTOR_VERSION` (tag) env vars override their respective defaults. Locally-built and published images share the `:` tag (the conventional Docker pattern): whatever is in the local daemon is used, and Docker pulls the published image @@ -395,25 +395,25 @@ def _add_connector_image_env_vars(env_vars: dict[str, str]) -> bool: :param env_vars: Dictionary to populate with the connector image environment variables. :return: Whether the image and tag were successfully resolved. """ - image = os.environ.get("CLP_CONNECTOR_IMAGE", DEFAULT_CONNECTOR_IMAGE) + image = os.environ.get("CLP_PRESTO_CONNECTOR_IMAGE", DEFAULT_CONNECTOR_IMAGE) - explicit_tag = os.environ.get("CLP_CONNECTOR_TAG") + explicit_tag = os.environ.get("CLP_PRESTO_CONNECTOR_TAG") if explicit_tag is not None: logger.info( - "Using explicitly provided CLP_CONNECTOR_TAG='%s' for connector image '%s'.", + "Using explicitly provided CLP_PRESTO_CONNECTOR_TAG='%s' for connector image '%s'.", explicit_tag, image, ) - env_vars["CLP_CONNECTOR_IMAGE"] = image - env_vars["CLP_CONNECTOR_TAG"] = explicit_tag + env_vars["CLP_PRESTO_CONNECTOR_IMAGE"] = image + env_vars["CLP_PRESTO_CONNECTOR_TAG"] = explicit_tag return True - tag = os.environ.get("CLP_CONNECTOR_VERSION", DEFAULT_CONNECTOR_VERSION) + tag = os.environ.get("CLP_PRESTO_CONNECTOR_VERSION", DEFAULT_CONNECTOR_VERSION) if not _connector_image_available(image, tag): return False - env_vars["CLP_CONNECTOR_IMAGE"] = image - env_vars["CLP_CONNECTOR_TAG"] = tag + env_vars["CLP_PRESTO_CONNECTOR_IMAGE"] = image + env_vars["CLP_PRESTO_CONNECTOR_TAG"] = tag return True @@ -434,7 +434,7 @@ def _connector_image_available(image: str, tag: str) -> bool: if shutil.which(docker_executable) is None: logger.error( "Docker isn't installed or isn't on PATH, so the CLP Presto connector image can't be" - " checked. Install Docker, or set CLP_CONNECTOR_TAG explicitly." + " checked. Install Docker, or set CLP_PRESTO_CONNECTOR_TAG explicitly." ) return False @@ -448,7 +448,7 @@ def _connector_image_available(image: str, tag: str) -> bool: logger.error( "Couldn't find CLP Presto connector image '%s' locally or on the registry. Build it" - " (e.g. via `task package` in clp-plugin-presto-connector) or set CLP_CONNECTOR_TAG" + " (e.g. via `task package` in clp-plugin-presto-connector) or set CLP_PRESTO_CONNECTOR_TAG" " explicitly.", ref, ) From 5d8e9abc3cf8203b3621f65eac669f287b606895 Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Fri, 31 Jul 2026 23:48:03 -0400 Subject: [PATCH 08/17] docs(presto): invoke set-up-test.sh from the repo root in the Helm example MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Helm testing example used ./package-helm/set-up-test.sh, which only works from tools/deployment/. The script resolves its own directory via BASH_SOURCE, so use the repo-root-relative path tools/deployment/package-helm/set-up-test.sh instead — copy-pasteable from the repository root. Arguments and image examples unchanged. --- docs/src/dev-docs/testing-presto-connector.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/src/dev-docs/testing-presto-connector.md b/docs/src/dev-docs/testing-presto-connector.md index 5c5e252935..d3644a4d55 100644 --- a/docs/src/dev-docs/testing-presto-connector.md +++ b/docs/src/dev-docs/testing-presto-connector.md @@ -51,13 +51,13 @@ The Helm chart's local-image path is wired into the set-up scripts in `--clp-connector-image`: ```shell -./package-helm/set-up-test.sh --presto --clp-connector-image : +tools/deployment/package-helm/set-up-test.sh --presto --clp-connector-image : ``` For example: ```shell -./package-helm/set-up-test.sh --presto \ +tools/deployment/package-helm/set-up-test.sh --presto \ --clp-connector-image ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT ``` From 0349aef5a96dddebe2e5eef443d143d260463d95 Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Fri, 31 Jul 2026 23:49:56 -0400 Subject: [PATCH 09/17] docs(presto): clarify CLP_PRESTO_CONNECTOR_TAG skips image validation The connector-image note in the user guide said set-up-config.sh writes the image into .env "after verifying it exists," which only holds for CLP_PRESTO_CONNECTOR_IMAGE/VERSION. An explicit CLP_PRESTO_CONNECTOR_TAG is written as-is and skips the existence check (init.py returns early without calling _connector_image_available), so an invalid tag surfaces later during docker compose up. Distinguish the two cases so users know when validation happens and when it does not. --- docs/src/user-docs/guides-using-presto.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/src/user-docs/guides-using-presto.md b/docs/src/user-docs/guides-using-presto.md index 51c8b38a40..ebcfa87dab 100644 --- a/docs/src/user-docs/guides-using-presto.md +++ b/docs/src/user-docs/guides-using-presto.md @@ -210,9 +210,12 @@ Using Presto with CLP via Docker Compose requires: `set-up-config.sh` also writes the CLP connector image into `.env`, after verifying it exists locally or on the registry (a locally-built image and the published one share the same tag; whatever is in your Docker daemon wins). Override the image by exporting - `CLP_PRESTO_CONNECTOR_IMAGE`, `CLP_PRESTO_CONNECTOR_TAG`, or `CLP_PRESTO_CONNECTOR_VERSION` before running the - script; see [Testing the Presto connector locally][testing-presto-connector] for building - the connector image yourself. + `CLP_PRESTO_CONNECTOR_IMAGE`, `CLP_PRESTO_CONNECTOR_VERSION`, or `CLP_PRESTO_CONNECTOR_TAG` + before running the script. An explicit `CLP_PRESTO_CONNECTOR_TAG` skips the existence check, + so an invalid tag is written to `.env` as-is and surfaces later during `docker compose up`; + `CLP_PRESTO_CONNECTOR_IMAGE` and `CLP_PRESTO_CONNECTOR_VERSION` are still validated before + `.env` is written. See [Testing the Presto connector locally][testing-presto-connector] for + building the connector image yourself. ::: 4. Configure Presto to use CLP's metadata database as follows: From fed5c4c047b0ebf978e7b196310bab464ea308ad Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Fri, 31 Jul 2026 23:55:48 -0400 Subject: [PATCH 10/17] fix(helm): fail loudly on invalid connector image overrides get_image_helm_args was invoked inline as $(...) in the helm install command, so its non-zero exit (invalid image ref, or image absent from the local Docker daemon) was silently swallowed: helm install fell back to the chart-default image with no error. Resolve the overrides into Helm --set flags up front and exit 1 on failure in set-up-test.sh and set-up-multi-dedicated-test.sh. Also make get_image_helm_args check `kind load` explicitly. A bare `kind load ...` relied on errexit, but the caller's `$(...) ... || exit 1` pattern suspends errexit inside the command substitution, so a kind-load failure would be ignored and we would proceed with pullPolicy=Never for an image never loaded. Return 1 explicitly so every failure mode propagates. --- tools/deployment/package-helm/.set-up-common.sh | 10 +++++++++- .../package-helm/set-up-multi-dedicated-test.sh | 15 ++++++++++++--- tools/deployment/package-helm/set-up-test.sh | 15 ++++++++++++--- 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/tools/deployment/package-helm/.set-up-common.sh b/tools/deployment/package-helm/.set-up-common.sh index 72b9a5cbe5..b242574efe 100755 --- a/tools/deployment/package-helm/.set-up-common.sh +++ b/tools/deployment/package-helm/.set-up-common.sh @@ -58,7 +58,15 @@ get_image_helm_args() { fi echo "Loading local image '${image}' into kind cluster..." >&2 - kind load docker-image "${image}" --name "${cluster_name}" >&2 + # Check explicitly rather than relying on `errexit`: callers capture this + # function's output via `$(...) ... || exit 1`, and the `||` suspends errexit + # inside the command substitution — so an unchecked `kind load` failure would + # be silently ignored and we'd proceed with `pullPolicy=Never` for an image + # that was never loaded. + if ! kind load docker-image "${image}" --name "${cluster_name}" >&2; then + echo "Error: failed to load local image '${image}' into kind cluster '${cluster_name}'." >&2 + return 1 + fi # Split "repo:tag" on the last colon whose right-hand side contains no '/' # (so registry ports like localhost:5000/repo are not mistaken for tags). diff --git a/tools/deployment/package-helm/set-up-multi-dedicated-test.sh b/tools/deployment/package-helm/set-up-multi-dedicated-test.sh index 6270655d39..09fe1ed124 100755 --- a/tools/deployment/package-helm/set-up-multi-dedicated-test.sh +++ b/tools/deployment/package-helm/set-up-multi-dedicated-test.sh @@ -136,8 +136,17 @@ EOF echo "Installing Helm chart..." helm uninstall test --ignore-not-found sleep 2 + +# Resolve the local-image overrides into Helm --set flags up front so a failure +# (an invalid image ref, or an image absent from the local Docker daemon) exits +# loudly instead of being silently dropped — which would make `helm install` +# fall back to the chart-default image. An empty override is intentional (no +# --clp-*-image passed) and resolves to empty flags. +clp_package_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") || exit 1 +clp_connector_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") || exit 1 + # Word splitting is intentional: helper functions return multiple --set flags. -# shellcheck disable=SC2046 +# shellcheck disable=SC2086,SC2046 helm install test "${script_dir}" \ --set "distributedDeployment=true" \ --set "scheduling.compressionWorker.replicas=${COMPRESSION_WORKER_REPLICAS}" \ @@ -162,7 +171,7 @@ helm install test "${script_dir}" \ --set "scheduling.mcpServer.nodeSelector.yscope\.io/nodeType=core" \ $(get_service_exposure_helm_args) \ $(get_presto_helm_args) \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") + ${clp_package_args} \ + ${clp_connector_args} wait_for_cluster_ready diff --git a/tools/deployment/package-helm/set-up-test.sh b/tools/deployment/package-helm/set-up-test.sh index 33c5d8a44d..a9b64faec8 100755 --- a/tools/deployment/package-helm/set-up-test.sh +++ b/tools/deployment/package-helm/set-up-test.sh @@ -26,12 +26,21 @@ generate_kind_config 0 | kind create cluster --name "${CLUSTER_NAME}" --config=- echo "Installing Helm chart..." helm uninstall test --ignore-not-found sleep 2 + +# Resolve the local-image overrides into Helm --set flags up front so a failure +# (an invalid image ref, or an image absent from the local Docker daemon) exits +# loudly instead of being silently dropped — which would make `helm install` +# fall back to the chart-default image. An empty override is intentional (no +# --clp-*-image passed) and resolves to empty flags. +clp_package_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") || exit 1 +clp_connector_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") || exit 1 + # Word splitting is intentional: helper functions return multiple --set flags. -# shellcheck disable=SC2046 +# shellcheck disable=SC2086,SC2046 helm install test "${script_dir}" \ $(get_service_exposure_helm_args) \ $(get_presto_helm_args) \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") + ${clp_package_args} \ + ${clp_connector_args} wait_for_cluster_ready From d8a3aa2aefd2b95b459bd6a9fb99335e3d6551bf Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Sat, 1 Aug 2026 00:02:15 -0400 Subject: [PATCH 11/17] refactor(presto): share a timed Docker probe for image availability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add _run_docker_probe: a shared runner for the Docker CLI probes used to check the connector image. It applies a finite timeout and catches OSError and subprocess.TimeoutExpired, logging the failure and returning False so a hung Docker daemon or an unresponsive registry does not stall set-up-config.sh indefinitely. Inline the two one-line wrappers (_image_exists_locally and _tag_exists_on_registry) into _connector_image_available — each was called once and the call site already distinguishes local daemon vs registry in its log messages. Command arguments and boolean success behavior are unchanged. --- tools/deployment/presto-clp/scripts/init.py | 59 +++++++++++---------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/tools/deployment/presto-clp/scripts/init.py b/tools/deployment/presto-clp/scripts/init.py index b61035e067..ac4fda3ba5 100755 --- a/tools/deployment/presto-clp/scripts/init.py +++ b/tools/deployment/presto-clp/scripts/init.py @@ -439,10 +439,12 @@ def _connector_image_available(image: str, tag: str) -> bool: return False ref = f"{image}:{tag}" - if _image_exists_locally(docker_executable, ref): + # `image inspect` queries the local Docker daemon; `manifest inspect` queries the registry. + # A local image takes precedence at `docker compose up`. + if _run_docker_probe(docker_executable, ["image", "inspect", ref]): logger.info("Found CLP connector image '%s' in the local Docker daemon.", ref) return True - if _tag_exists_on_registry(docker_executable, ref): + if _run_docker_probe(docker_executable, ["manifest", "inspect", ref]): logger.info("Found CLP connector image '%s' on the registry.", ref) return True @@ -455,42 +457,41 @@ def _connector_image_available(image: str, tag: str) -> bool: return False -def _image_exists_locally(docker_executable: str, image_ref: str) -> bool: - """ - Returns whether `image_ref` is present in the local Docker daemon. - - :param docker_executable: The Docker executable to call. - :param image_ref: The image reference (repository:tag). - :return: True if the image exists locally. - """ - return ( - subprocess.run( - [docker_executable, "image", "inspect", image_ref], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - check=False, - ).returncode - == 0 - ) +# Timeout for Docker CLI probes (local `image inspect` or registry `manifest inspect`). A +# hung Docker daemon or an unresponsive registry would otherwise stall `set-up-config.sh` +# indefinitely; this is ample for a small manifest request even on a slow link. +_DOCKER_PROBE_TIMEOUT_SECONDS = 30 -def _tag_exists_on_registry(docker_executable: str, image_ref: str) -> bool: +def _run_docker_probe(docker_executable: str, args: list[str]) -> bool: """ - Returns whether `image_ref` exists on its registry, queried via `docker manifest inspect`. + Runs ``docker `` and returns whether it exited 0, suppressing stdout/stderr. + + Catches ``OSError`` (e.g. the Docker binary vanished or isn't executable) and + ``subprocess.TimeoutExpired``, logging the failure and returning False so a hung or + broken Docker/registry doesn't stall setup. :param docker_executable: The Docker executable to call. - :param image_ref: The image reference (repository:tag). - :return: True if the tag exists on the registry. + :param args: Arguments to pass to Docker (e.g. ``["image", "inspect", ref]``). + :return: True if the command exited 0. """ - return ( - subprocess.run( - [docker_executable, "manifest", "inspect", image_ref], + try: + completed_process = subprocess.run( + [docker_executable, *args], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False, - ).returncode - == 0 - ) + timeout=_DOCKER_PROBE_TIMEOUT_SECONDS, + ) + except (OSError, subprocess.TimeoutExpired) as e: + logger.error( + "Docker command '%s %s' failed: %s", + docker_executable, + " ".join(args), + e, + ) + return False + return completed_process.returncode == 0 def _generate_worker_clp_properties( From 883e93ae705208684899a9f96df7bef6d5933bbf Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Sat, 1 Aug 2026 00:21:54 -0400 Subject: [PATCH 12/17] fix(presto): use logger.exception for Docker probe failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ruff TRY400 (stable under the pinned ruff<0.16) flags `logger.error` inside an except clause. Switch the _run_docker_probe failure log to `logger.exception(..., exc_info=e)`, matching the existing idiom in _add_worker_env_vars. Behavior is unchanged: the probe still returns False on OSError and subprocess.TimeoutExpired. The CPY001 copyright-header notices surfaced by newer local ruff builds are not failing CI — the repo caps ruff<0.16 (commit 40bee805, issue #2422) to defer them, so they belong in a separate PR. --- tools/deployment/presto-clp/scripts/init.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/deployment/presto-clp/scripts/init.py b/tools/deployment/presto-clp/scripts/init.py index ac4fda3ba5..1d1b4a17d3 100755 --- a/tools/deployment/presto-clp/scripts/init.py +++ b/tools/deployment/presto-clp/scripts/init.py @@ -484,11 +484,11 @@ def _run_docker_probe(docker_executable: str, args: list[str]) -> bool: timeout=_DOCKER_PROBE_TIMEOUT_SECONDS, ) except (OSError, subprocess.TimeoutExpired) as e: - logger.error( - "Docker command '%s %s' failed: %s", + logger.exception( + "Docker command '%s %s' failed.", docker_executable, " ".join(args), - e, + exc_info=e, ) return False return completed_process.returncode == 0 From 8f95c55e48bd5fc0186d649db933d91cae609b7b Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Sun, 2 Aug 2026 00:56:14 -0400 Subject: [PATCH 13/17] feat(presto): Default to the published multi-arch connector image. `ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT` is now published as a multi-arch (linux/amd64 + linux/arm64) tag, so drop the three pre-merge TODOs asking for the defaults to point at a released tag. The default values in `init.py`, `docker-compose.yaml`, and `values.yaml` already named that tag and are unchanged. Also convert `set-up-multi-shared-test.sh` to resolve the image overrides into Helm flags before `helm install`, matching `set-up-test.sh` and `set-up-multi-dedicated-test.sh`. fed5c4c04 made an invalid or unloadable image override fail loudly in those two scripts but missed this one, where the override was still expanded inline inside the `helm install` command and a failure was swallowed, silently falling back to the chart-default image. Co-Authored-By: Claude --- .../package-helm/set-up-multi-shared-test.sh | 14 +++++++++++--- tools/deployment/package-helm/values.yaml | 1 - tools/deployment/presto-clp/docker-compose.yaml | 1 - tools/deployment/presto-clp/scripts/init.py | 1 - 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/tools/deployment/package-helm/set-up-multi-shared-test.sh b/tools/deployment/package-helm/set-up-multi-shared-test.sh index fd30d9c322..2fea48943b 100755 --- a/tools/deployment/package-helm/set-up-multi-shared-test.sh +++ b/tools/deployment/package-helm/set-up-multi-shared-test.sh @@ -42,8 +42,16 @@ generate_kind_config "${NUM_WORKER_NODES}" | kind create cluster --name "${CLUST echo "Installing Helm chart..." helm uninstall test --ignore-not-found sleep 2 +# Resolve the local-image overrides into Helm --set flags up front so a failure +# (an invalid image ref, or an image absent from the local Docker daemon) exits +# loudly instead of being silently dropped — which would make `helm install` +# fall back to the chart-default image. An empty override is intentional (no +# --clp-*-image passed) and resolves to empty flags. +clp_package_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") || exit 1 +clp_connector_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") || exit 1 + # Word splitting is intentional: helper functions return multiple --set flags. -# shellcheck disable=SC2046 +# shellcheck disable=SC2086,SC2046 helm install test "${script_dir}" \ --set "distributedDeployment=true" \ --set "scheduling.compressionWorker.replicas=${COMPRESSION_WORKER_REPLICAS}" \ @@ -52,7 +60,7 @@ helm install test "${script_dir}" \ --set "scheduling.prestoWorker.replicas=${PRESTO_WORKER_REPLICAS}" \ $(get_service_exposure_helm_args) \ $(get_presto_helm_args) \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") \ - $(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") + ${clp_package_args} \ + ${clp_connector_args} wait_for_cluster_ready diff --git a/tools/deployment/package-helm/values.yaml b/tools/deployment/package-helm/values.yaml index db19d0f936..9a3a09b0b8 100644 --- a/tools/deployment/package-helm/values.yaml +++ b/tools/deployment/package-helm/values.yaml @@ -7,7 +7,6 @@ allowHostAccessForSbinScripts: true image: # Installs the CLP Presto connector plugin into the Presto coordinator and worker pods. Override # the repository to use a different edition of the connector. - # TODO: default the tag to the released tag before merging. clpConnector: repository: "ghcr.io/y-scope/clp-plugin-presto-connector" pullPolicy: "IfNotPresent" diff --git a/tools/deployment/presto-clp/docker-compose.yaml b/tools/deployment/presto-clp/docker-compose.yaml index 51ef28dbd1..d3af2eb942 100644 --- a/tools/deployment/presto-clp/docker-compose.yaml +++ b/tools/deployment/presto-clp/docker-compose.yaml @@ -8,7 +8,6 @@ services: # CLP_PRESTO_CONNECTOR_IMAGE/CLP_PRESTO_CONNECTOR_TAG into .env. Locally-built and published # images share the same : tag; whatever is in the local daemon wins. The defaults # below are only used when .env is absent. - # TODO: default the version to the released tag before merging. image: "${CLP_PRESTO_CONNECTOR_IMAGE:-\ ghcr.io/y-scope/clp-plugin-presto-connector}:${CLP_PRESTO_CONNECTOR_TAG:-\ 0.1.0-SNAPSHOT}" diff --git a/tools/deployment/presto-clp/scripts/init.py b/tools/deployment/presto-clp/scripts/init.py index 1d1b4a17d3..ec1917d38d 100755 --- a/tools/deployment/presto-clp/scripts/init.py +++ b/tools/deployment/presto-clp/scripts/init.py @@ -31,7 +31,6 @@ # startup from this image (see docker-compose.yaml). `init.py` verifies the image exists (per # `_add_connector_image_env_vars`) and writes `CLP_PRESTO_CONNECTOR_IMAGE`/ # `CLP_PRESTO_CONNECTOR_TAG` to `.env`; the defaults below are only used when `.env` is absent. -# TODO: default the version to the released multi-arch tag before merging. DEFAULT_CONNECTOR_IMAGE = "ghcr.io/y-scope/clp-plugin-presto-connector" DEFAULT_CONNECTOR_VERSION = "0.1.0-SNAPSHOT" From 6bf710bd500a5d16ddf111be4b16658787d0ede6 Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Sun, 2 Aug 2026 01:03:28 -0400 Subject: [PATCH 14/17] docs(presto): Tighten the connector image docs. Both the dev page and the Compose set-up note in the user guide restated the same image-resolution rules at length. Trim them to what a reader needs and let the user guide defer to the dev page for the details. `testing-presto-connector.md`: fold the two Helm examples into one concrete command, and shorten the local-vs-published explanation. `guides-using-presto.md`: shorten the `set-up-config.sh` note, and match the surrounding notes' one-idea-per-sentence phrasing rather than chaining clauses. Both files are added by this PR, so neither change reflows text that exists on main. Co-Authored-By: Claude --- docs/src/dev-docs/testing-presto-connector.md | 43 +++++++------------ docs/src/user-docs/guides-using-presto.md | 15 +++---- 2 files changed, 21 insertions(+), 37 deletions(-) diff --git a/docs/src/dev-docs/testing-presto-connector.md b/docs/src/dev-docs/testing-presto-connector.md index d3644a4d55..84cf1a7c97 100644 --- a/docs/src/dev-docs/testing-presto-connector.md +++ b/docs/src/dev-docs/testing-presto-connector.md @@ -1,15 +1,12 @@ # Building and testing the Presto connector The Presto integration installs the [CLP connector][clp-connector] at startup from the -`ghcr.io/y-scope/clp-plugin-presto-connector` image, so that the stock Presto images can be used -unmodified. This page covers building that connector image locally and pointing the -[Docker Compose](#docker-compose) and [Helm](#helm-kind) stacks at it for testing. +`ghcr.io/y-scope/clp-plugin-presto-connector` image, so the stock Presto images stay unmodified. +This page covers building that image locally and pointing the [Docker Compose](#docker-compose) and +[Helm](#helm-kind) stacks at it. -Locally-built and published connector images share the same `:` tag (e.g. -`0.1.0-SNAPSHOT`), following the conventional Docker pattern: whatever is in your local Docker -daemon wins, and Docker pulls the published image when nothing is loaded locally. To go back to -the published image after building locally, `docker pull` the tag (or `docker rmi` your local -build). +Local and published images share the same `:` tag (e.g. `0.1.0-SNAPSHOT`), so whatever is +in your Docker daemon wins. `docker rmi` your local build to go back to the published image. ## Building the connector image @@ -19,20 +16,18 @@ In the [`clp-plugin-presto-connector`][clp-connector] repository, run: task package ``` -This builds the connector image and loads it into the local Docker daemon, e.g. -`ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT`. +This builds `ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT` and loads it into your +local Docker daemon. ## Docker Compose -The `presto-clp` Compose stack is in `tools/deployment/presto-clp`. `scripts/set-up-config.sh` -verifies the connector image exists (locally or on the registry) and writes it into `.env`, -erroring with the ref it tried if neither exists. Override the image by exporting environment -variables before running `set-up-config.sh`: +The `presto-clp` stack is in `tools/deployment/presto-clp`. `scripts/set-up-config.sh` verifies the +connector image exists (locally or on the registry) and writes it into `.env`, erroring with the +ref it tried if neither exists. Export any of these before running it: -* `CLP_PRESTO_CONNECTOR_IMAGE` (default `ghcr.io/y-scope/clp-plugin-presto-connector`): connector image - repository. -* `CLP_PRESTO_CONNECTOR_TAG`: exact tag; skips the existence check when set. -* `CLP_PRESTO_CONNECTOR_VERSION` (default `0.1.0-SNAPSHOT`): tag to verify and use. +* `CLP_PRESTO_CONNECTOR_IMAGE`: repository (default `ghcr.io/y-scope/clp-plugin-presto-connector`). +* `CLP_PRESTO_CONNECTOR_VERSION`: tag to verify and use (default `0.1.0-SNAPSHOT`). +* `CLP_PRESTO_CONNECTOR_TAG`: exact tag, skipping the existence check. Then start the stack: @@ -45,16 +40,8 @@ See the [Using Presto with CLP][using-presto] user guide for the full setup. ## Helm (kind) -The Helm chart's local-image path is wired into the set-up scripts in -`tools/deployment/package-helm`, which load a local image into the `kind` cluster and set -`image.clpConnector.{repository,tag,pullPolicy=Never}` for you. Pass the connector image via -`--clp-connector-image`: - -```shell -tools/deployment/package-helm/set-up-test.sh --presto --clp-connector-image : -``` - -For example: +`--clp-connector-image` loads a local image into the `kind` cluster and sets +`image.clpConnector.{repository,tag,pullPolicy=Never}` for you: ```shell tools/deployment/package-helm/set-up-test.sh --presto \ diff --git a/docs/src/user-docs/guides-using-presto.md b/docs/src/user-docs/guides-using-presto.md index ebcfa87dab..4a61d580d6 100644 --- a/docs/src/user-docs/guides-using-presto.md +++ b/docs/src/user-docs/guides-using-presto.md @@ -207,15 +207,12 @@ Using Presto with CLP via Docker Compose requires: section. :::{note} - `set-up-config.sh` also writes the CLP connector image into `.env`, after verifying it - exists locally or on the registry (a locally-built image and the published one share the - same tag; whatever is in your Docker daemon wins). Override the image by exporting - `CLP_PRESTO_CONNECTOR_IMAGE`, `CLP_PRESTO_CONNECTOR_VERSION`, or `CLP_PRESTO_CONNECTOR_TAG` - before running the script. An explicit `CLP_PRESTO_CONNECTOR_TAG` skips the existence check, - so an invalid tag is written to `.env` as-is and surfaces later during `docker compose up`; - `CLP_PRESTO_CONNECTOR_IMAGE` and `CLP_PRESTO_CONNECTOR_VERSION` are still validated before - `.env` is written. See [Testing the Presto connector locally][testing-presto-connector] for - building the connector image yourself. + `set-up-config.sh` also verifies that the CLP connector image exists, locally or on the + registry, and writes it into `.env`. To override the image, export `CLP_PRESTO_CONNECTOR_IMAGE` + or `CLP_PRESTO_CONNECTOR_VERSION` before running the script. `CLP_PRESTO_CONNECTOR_TAG` sets an + exact tag and skips the check, so an invalid tag surfaces later during `docker compose up`. See + [Testing the Presto connector locally][testing-presto-connector] for building the image + yourself. ::: 4. Configure Presto to use CLP's metadata database as follows: From 490148f52b00fcecb0aa11f90a64b588fb131b89 Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Sun, 2 Aug 2026 01:09:54 -0400 Subject: [PATCH 15/17] docs(helm): Condense the image-override comments in the set-up scripts. The rationale comment for resolving image overrides before `helm install` was five lines repeated verbatim in all three set-up scripts, and the `kind load` check carried a five-line walkthrough of the `errexit` interaction. Keep the non-obvious part of each (that `errexit` is suspended inside the caller's command substitution, and that the failure mode is a silent fall back to the chart default) and drop the restatement. Also restore the blank line after `sleep 2` in `set-up-multi-shared-test.sh` so it matches its two sibling scripts. Co-Authored-By: Claude --- tools/deployment/package-helm/.set-up-common.sh | 7 ++----- .../package-helm/set-up-multi-dedicated-test.sh | 7 ++----- tools/deployment/package-helm/set-up-multi-shared-test.sh | 8 +++----- tools/deployment/package-helm/set-up-test.sh | 7 ++----- 4 files changed, 9 insertions(+), 20 deletions(-) diff --git a/tools/deployment/package-helm/.set-up-common.sh b/tools/deployment/package-helm/.set-up-common.sh index b242574efe..fc80e5f067 100755 --- a/tools/deployment/package-helm/.set-up-common.sh +++ b/tools/deployment/package-helm/.set-up-common.sh @@ -58,11 +58,8 @@ get_image_helm_args() { fi echo "Loading local image '${image}' into kind cluster..." >&2 - # Check explicitly rather than relying on `errexit`: callers capture this - # function's output via `$(...) ... || exit 1`, and the `||` suspends errexit - # inside the command substitution — so an unchecked `kind load` failure would - # be silently ignored and we'd proceed with `pullPolicy=Never` for an image - # that was never loaded. + # `errexit` is suspended inside the caller's `$(...) || exit 1`, so check explicitly; + # otherwise a failed load silently yields `pullPolicy=Never` for an unloaded image. if ! kind load docker-image "${image}" --name "${cluster_name}" >&2; then echo "Error: failed to load local image '${image}' into kind cluster '${cluster_name}'." >&2 return 1 diff --git a/tools/deployment/package-helm/set-up-multi-dedicated-test.sh b/tools/deployment/package-helm/set-up-multi-dedicated-test.sh index 09fe1ed124..2e31ae4a1c 100755 --- a/tools/deployment/package-helm/set-up-multi-dedicated-test.sh +++ b/tools/deployment/package-helm/set-up-multi-dedicated-test.sh @@ -137,11 +137,8 @@ echo "Installing Helm chart..." helm uninstall test --ignore-not-found sleep 2 -# Resolve the local-image overrides into Helm --set flags up front so a failure -# (an invalid image ref, or an image absent from the local Docker daemon) exits -# loudly instead of being silently dropped — which would make `helm install` -# fall back to the chart-default image. An empty override is intentional (no -# --clp-*-image passed) and resolves to empty flags. +# Resolve image overrides up front so an invalid or unloadable image exits loudly instead +# of silently falling back to the chart default. An empty override resolves to no flags. clp_package_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") || exit 1 clp_connector_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") || exit 1 diff --git a/tools/deployment/package-helm/set-up-multi-shared-test.sh b/tools/deployment/package-helm/set-up-multi-shared-test.sh index 2fea48943b..69a7fda473 100755 --- a/tools/deployment/package-helm/set-up-multi-shared-test.sh +++ b/tools/deployment/package-helm/set-up-multi-shared-test.sh @@ -42,11 +42,9 @@ generate_kind_config "${NUM_WORKER_NODES}" | kind create cluster --name "${CLUST echo "Installing Helm chart..." helm uninstall test --ignore-not-found sleep 2 -# Resolve the local-image overrides into Helm --set flags up front so a failure -# (an invalid image ref, or an image absent from the local Docker daemon) exits -# loudly instead of being silently dropped — which would make `helm install` -# fall back to the chart-default image. An empty override is intentional (no -# --clp-*-image passed) and resolves to empty flags. + +# Resolve image overrides up front so an invalid or unloadable image exits loudly instead +# of silently falling back to the chart default. An empty override resolves to no flags. clp_package_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") || exit 1 clp_connector_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") || exit 1 diff --git a/tools/deployment/package-helm/set-up-test.sh b/tools/deployment/package-helm/set-up-test.sh index a9b64faec8..f3e1a3be49 100755 --- a/tools/deployment/package-helm/set-up-test.sh +++ b/tools/deployment/package-helm/set-up-test.sh @@ -27,11 +27,8 @@ echo "Installing Helm chart..." helm uninstall test --ignore-not-found sleep 2 -# Resolve the local-image overrides into Helm --set flags up front so a failure -# (an invalid image ref, or an image absent from the local Docker daemon) exits -# loudly instead of being silently dropped — which would make `helm install` -# fall back to the chart-default image. An empty override is intentional (no -# --clp-*-image passed) and resolves to empty flags. +# Resolve image overrides up front so an invalid or unloadable image exits loudly instead +# of silently falling back to the chart default. An empty override resolves to no flags. clp_package_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpPackage" "${CLP_PACKAGE_IMAGE}") || exit 1 clp_connector_args=$(get_image_helm_args "${CLUSTER_NAME}" "clpConnector" "${CLP_PRESTO_CONNECTOR_IMAGE}") || exit 1 From 4e089ec6c280c87d9a1a79e5eafb4095105f5312 Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Sun, 2 Aug 2026 01:15:28 -0400 Subject: [PATCH 16/17] refactor(presto): Simplify the connector image resolution in `init.py`. Collapse the two exit paths in `_add_connector_image_env_vars` into one: both wrote the same pair of env vars, so resolve the tag first and assign once. Drop the `docker_executable` parameter from `_run_docker_probe`. It was always the same literal, threaded through two functions; hoist it to `_DOCKER_EXECUTABLE` alongside the timeout constant. Trim the comments to what isn't already evident from the code. The module constant comment restated the function's docstring, the docstring restated the local-vs-published behaviour already covered in `testing-presto-connector.md`, and the probe call sites were annotated with what their own log lines say. No behaviour change: defaults resolve, an explicit `CLP_PRESTO_CONNECTOR_TAG` still bypasses the check, an unavailable image still fails without writing a partial env var set, and the local and registry probes are unchanged. Co-Authored-By: Claude --- tools/deployment/presto-clp/scripts/init.py | 88 +++++++-------------- 1 file changed, 29 insertions(+), 59 deletions(-) diff --git a/tools/deployment/presto-clp/scripts/init.py b/tools/deployment/presto-clp/scripts/init.py index ec1917d38d..0d8edda8a3 100755 --- a/tools/deployment/presto-clp/scripts/init.py +++ b/tools/deployment/presto-clp/scripts/init.py @@ -27,13 +27,17 @@ # S3 URL constant AWS_S3_DOMAIN = "amazonaws.com" -# Default CLP Presto connector image and version. The connector is installed into Presto at -# startup from this image (see docker-compose.yaml). `init.py` verifies the image exists (per -# `_add_connector_image_env_vars`) and writes `CLP_PRESTO_CONNECTOR_IMAGE`/ -# `CLP_PRESTO_CONNECTOR_TAG` to `.env`; the defaults below are only used when `.env` is absent. +# Default CLP Presto connector image and tag, used when the corresponding env vars are unset. DEFAULT_CONNECTOR_IMAGE = "ghcr.io/y-scope/clp-plugin-presto-connector" DEFAULT_CONNECTOR_VERSION = "0.1.0-SNAPSHOT" +# Silence Ruff S607: the absolute path of the Docker binary may vary depending on the installation +# method. +_DOCKER_EXECUTABLE = "docker" + +# Bounds each Docker probe so a hung daemon or an unresponsive registry can't stall setup. +_DOCKER_PROBE_TIMEOUT_SECONDS = 30 + # Set up console logging logging_console_handler = logging.StreamHandler() logging_formatter = logging.Formatter( @@ -381,35 +385,22 @@ def _add_connector_image_env_vars(env_vars: dict[str, str]) -> bool: Resolves the CLP Presto connector image and adds `CLP_PRESTO_CONNECTOR_IMAGE` and `CLP_PRESTO_CONNECTOR_TAG` to `env_vars`, which `docker-compose.yaml` consumes. - An explicit `CLP_PRESTO_CONNECTOR_TAG` in the environment is used as-is and skips the - existence check. The `CLP_PRESTO_CONNECTOR_IMAGE` (repository) and - `CLP_PRESTO_CONNECTOR_VERSION` (tag) env vars override their respective defaults. - - Locally-built and published images share the `:` tag (the conventional Docker - pattern): whatever is in the local daemon is used, and Docker pulls the published image - when nothing is loaded locally. To test the published image over a stale local build, run - `docker pull :` first (or `docker rmi` the local one). This function only - verifies the image is available locally or on the registry, and errors otherwise. + `CLP_PRESTO_CONNECTOR_IMAGE` (repository) and `CLP_PRESTO_CONNECTOR_VERSION` (tag) override + the defaults. An explicit `CLP_PRESTO_CONNECTOR_TAG` is used as-is and skips the existence + check. :param env_vars: Dictionary to populate with the connector image environment variables. :return: Whether the image and tag were successfully resolved. """ image = os.environ.get("CLP_PRESTO_CONNECTOR_IMAGE", DEFAULT_CONNECTOR_IMAGE) - explicit_tag = os.environ.get("CLP_PRESTO_CONNECTOR_TAG") - if explicit_tag is not None: - logger.info( - "Using explicitly provided CLP_PRESTO_CONNECTOR_TAG='%s' for connector image '%s'.", - explicit_tag, - image, - ) - env_vars["CLP_PRESTO_CONNECTOR_IMAGE"] = image - env_vars["CLP_PRESTO_CONNECTOR_TAG"] = explicit_tag - return True - - tag = os.environ.get("CLP_PRESTO_CONNECTOR_VERSION", DEFAULT_CONNECTOR_VERSION) - if not _connector_image_available(image, tag): - return False + tag = os.environ.get("CLP_PRESTO_CONNECTOR_TAG") + if tag is None: + tag = os.environ.get("CLP_PRESTO_CONNECTOR_VERSION", DEFAULT_CONNECTOR_VERSION) + if not _connector_image_available(image, tag): + return False + else: + logger.info("Using CLP_PRESTO_CONNECTOR_TAG='%s'; skipping the existence check.", tag) env_vars["CLP_PRESTO_CONNECTOR_IMAGE"] = image env_vars["CLP_PRESTO_CONNECTOR_TAG"] = tag @@ -418,19 +409,14 @@ def _add_connector_image_env_vars(env_vars: dict[str, str]) -> bool: def _connector_image_available(image: str, tag: str) -> bool: """ - Returns whether `image`:`tag` exists in the local Docker daemon or on its registry, logging - which one was found (a local image takes precedence at `docker compose up`) or an error if - neither exists. + Returns whether `image`:`tag` exists in the local Docker daemon or on its registry. A local + image takes precedence at `docker compose up`. :param image: The connector image repository. :param tag: The image tag. :return: Whether the image is available. """ - # Silence Ruff S607: the absolute path of the Docker binary may vary depending on the - # installation method. - docker_executable = "docker" - - if shutil.which(docker_executable) is None: + if shutil.which(_DOCKER_EXECUTABLE) is None: logger.error( "Docker isn't installed or isn't on PATH, so the CLP Presto connector image can't be" " checked. Install Docker, or set CLP_PRESTO_CONNECTOR_TAG explicitly." @@ -438,12 +424,10 @@ def _connector_image_available(image: str, tag: str) -> bool: return False ref = f"{image}:{tag}" - # `image inspect` queries the local Docker daemon; `manifest inspect` queries the registry. - # A local image takes precedence at `docker compose up`. - if _run_docker_probe(docker_executable, ["image", "inspect", ref]): + if _run_docker_probe(["image", "inspect", ref]): logger.info("Found CLP connector image '%s' in the local Docker daemon.", ref) return True - if _run_docker_probe(docker_executable, ["manifest", "inspect", ref]): + if _run_docker_probe(["manifest", "inspect", ref]): logger.info("Found CLP connector image '%s' on the registry.", ref) return True @@ -456,27 +440,16 @@ def _connector_image_available(image: str, tag: str) -> bool: return False -# Timeout for Docker CLI probes (local `image inspect` or registry `manifest inspect`). A -# hung Docker daemon or an unresponsive registry would otherwise stall `set-up-config.sh` -# indefinitely; this is ample for a small manifest request even on a slow link. -_DOCKER_PROBE_TIMEOUT_SECONDS = 30 - - -def _run_docker_probe(docker_executable: str, args: list[str]) -> bool: +def _run_docker_probe(args: list[str]) -> bool: """ - Runs ``docker `` and returns whether it exited 0, suppressing stdout/stderr. - - Catches ``OSError`` (e.g. the Docker binary vanished or isn't executable) and - ``subprocess.TimeoutExpired``, logging the failure and returning False so a hung or - broken Docker/registry doesn't stall setup. + Runs `docker `, suppressing its output. - :param docker_executable: The Docker executable to call. - :param args: Arguments to pass to Docker (e.g. ``["image", "inspect", ref]``). - :return: True if the command exited 0. + :param args: Arguments to pass to Docker (e.g. `["image", "inspect", ref]`). + :return: Whether the command exited 0. False if Docker couldn't be run or timed out. """ try: completed_process = subprocess.run( - [docker_executable, *args], + [_DOCKER_EXECUTABLE, *args], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False, @@ -484,10 +457,7 @@ def _run_docker_probe(docker_executable: str, args: list[str]) -> bool: ) except (OSError, subprocess.TimeoutExpired) as e: logger.exception( - "Docker command '%s %s' failed.", - docker_executable, - " ".join(args), - exc_info=e, + "Docker command '%s %s' failed.", _DOCKER_EXECUTABLE, " ".join(args), exc_info=e ) return False return completed_process.returncode == 0 From cf9e2fab8e307a90a1009364c7a21d7a7b0a1727 Mon Sep 17 00:00:00 2001 From: Jack Luo Date: Mon, 3 Aug 2026 20:29:23 -0400 Subject: [PATCH 17/17] feat(deployment): Pin the Presto and CLP connector images by digest, and select each with a single image reference. The connector image was selected through three environment variables (`CLP_PRESTO_CONNECTOR_IMAGE`, `_VERSION`, and `_TAG`) that were concatenated with a hardcoded ":", so a digest couldn't be expressed at all. Each image is now chosen with one variable holding a complete reference, used as-is, so `repository:tag`, `repository@digest`, and `repository:tag@digest` all work: * `CLP_PRESTO_CONNECTOR_REF` * `CLP_PRESTO_COORDINATOR_REF` * `CLP_PRESTO_WORKER_REF` All three now default to `tag@digest`: the digest pins what's actually pulled, while the tag stays as a human-readable label. The digests are those of the multi-arch manifest lists, so both linux/amd64 and linux/arm64 continue to resolve. On the Helm side, `clp.imageRef` previously discarded the tag whenever a digest was set, so it could render only two of the three forms; it now renders all three and fails if neither is given. The Presto coordinator and worker templates were building their references by hand instead of calling it, which meant `image.presto{Coordinator,Worker}.digest` was silently ignored; both now go through the helper. --- docs/src/dev-docs/testing-presto-connector.md | 37 ++++++++--- docs/src/user-docs/guides-using-presto.md | 10 +-- .../package-helm/templates/_helpers.tpl | 30 +++++---- .../presto-coordinator-deployment.yaml | 2 +- .../templates/presto-worker-deployment.yaml | 2 +- tools/deployment/package-helm/values.yaml | 3 + .../deployment/presto-clp/docker-compose.yaml | 23 ++++--- tools/deployment/presto-clp/scripts/init.py | 64 +++++++++---------- 8 files changed, 99 insertions(+), 72 deletions(-) diff --git a/docs/src/dev-docs/testing-presto-connector.md b/docs/src/dev-docs/testing-presto-connector.md index 84cf1a7c97..4406e71fdb 100644 --- a/docs/src/dev-docs/testing-presto-connector.md +++ b/docs/src/dev-docs/testing-presto-connector.md @@ -5,8 +5,8 @@ The Presto integration installs the [CLP connector][clp-connector] at startup fr This page covers building that image locally and pointing the [Docker Compose](#docker-compose) and [Helm](#helm-kind) stacks at it. -Local and published images share the same `:` tag (e.g. `0.1.0-SNAPSHOT`), so whatever is -in your Docker daemon wins. `docker rmi` your local build to go back to the published image. +The default connector image is pinned by digest, so a local build of the same tag won't be picked +up on its own — point `CLP_PRESTO_CONNECTOR_REF` at your image to use it. ## Building the connector image @@ -23,19 +23,27 @@ local Docker daemon. The `presto-clp` stack is in `tools/deployment/presto-clp`. `scripts/set-up-config.sh` verifies the connector image exists (locally or on the registry) and writes it into `.env`, erroring with the -ref it tried if neither exists. Export any of these before running it: +ref it tried if neither exists. -* `CLP_PRESTO_CONNECTOR_IMAGE`: repository (default `ghcr.io/y-scope/clp-plugin-presto-connector`). -* `CLP_PRESTO_CONNECTOR_VERSION`: tag to verify and use (default `0.1.0-SNAPSHOT`). -* `CLP_PRESTO_CONNECTOR_TAG`: exact tag, skipping the existence check. +Each image is selected by a single environment variable holding a complete reference, which is used +as-is. Any of `repository:tag`, `repository@digest`, or `repository:tag@digest` works; unset +variables fall back to the pinned defaults. -Then start the stack: +* `CLP_PRESTO_CONNECTOR_REF`: the CLP connector. +* `CLP_PRESTO_COORDINATOR_REF`: the Presto coordinator. +* `CLP_PRESTO_WORKER_REF`: the Presto worker. + +To run against the image you just built: ```shell -./scripts/set-up-config.sh +CLP_PRESTO_CONNECTOR_REF=ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT \ + ./scripts/set-up-config.sh docker compose up -d ``` +Omitting the digest is what makes the local image win: Docker resolves a tag against the local +daemon first, but a digest only ever matches the exact published image. + See the [Using Presto with CLP][using-presto] user guide for the full setup. ## Helm (kind) @@ -48,5 +56,18 @@ tools/deployment/package-helm/set-up-test.sh --presto \ --clp-connector-image ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT ``` +## Pinning a new default + +When bumping a pinned default, take the digest of the *manifest list*, not of a per-architecture +manifest — the connector is published for both `linux/amd64` and `linux/arm64`, and a per-arch +digest would break the other architecture: + +```shell +docker buildx imagetools inspect --format '{{.Manifest.Digest}}' +``` + +`docker inspect` reports the digest of the single-platform image you happen to have pulled, so it's +the wrong source here. + [clp-connector]: https://github.com/y-scope/clp-plugin-presto-connector [using-presto]: ../user-docs/guides-using-presto.md diff --git a/docs/src/user-docs/guides-using-presto.md b/docs/src/user-docs/guides-using-presto.md index 4a61d580d6..fe897e0777 100644 --- a/docs/src/user-docs/guides-using-presto.md +++ b/docs/src/user-docs/guides-using-presto.md @@ -208,11 +208,11 @@ Using Presto with CLP via Docker Compose requires: :::{note} `set-up-config.sh` also verifies that the CLP connector image exists, locally or on the - registry, and writes it into `.env`. To override the image, export `CLP_PRESTO_CONNECTOR_IMAGE` - or `CLP_PRESTO_CONNECTOR_VERSION` before running the script. `CLP_PRESTO_CONNECTOR_TAG` sets an - exact tag and skips the check, so an invalid tag surfaces later during `docker compose up`. See - [Testing the Presto connector locally][testing-presto-connector] for building the image - yourself. + registry, and writes it into `.env`. The default is pinned by digest, so the image can't change + underneath you. To use a different one, export `CLP_PRESTO_CONNECTOR_REF` before running the + script; it's used as-is, so it may be `repository:tag`, `repository@digest`, or + `repository:tag@digest`. See [Testing the Presto connector locally][testing-presto-connector] + for building the image yourself. ::: 4. Configure Presto to use CLP's metadata database as follows: diff --git a/tools/deployment/package-helm/templates/_helpers.tpl b/tools/deployment/package-helm/templates/_helpers.tpl index 26d03d8523..308365fb69 100644 --- a/tools/deployment/package-helm/templates/_helpers.tpl +++ b/tools/deployment/package-helm/templates/_helpers.tpl @@ -156,28 +156,32 @@ app.kubernetes.io/instance: {{ .Release.Name }} {{/* Creates a container image reference from .Values.image. -Renders repository@digest when "digest" is set; otherwise, renders repository:tag. clpPackage -defaults to Chart.AppVersion when "tag" is omitted; other components require "tag". +Renders repository:tag, repository@digest, or repository:tag@digest, depending on which of "tag" +and "digest" are set. Setting both keeps the tag as a human-readable label while the digest is what +actually gets pulled. clpPackage defaults to Chart.AppVersion when "tag" is omitted; other +components require at least one of "tag" or "digest". @param {object} root Root template context (required) @param {string} component Key under .Values.image (e.g., "clpPackage", "redis") -@return {string} Full image reference (repository@digest or repository:tag) +@return {string} Full image reference */}} {{- define "clp.imageRef" -}} {{- $img := index .root.Values.image .component -}} -{{- if $img.digest -}} -{{- printf "%s@%s" $img.repository $img.digest -}} -{{- else -}} {{- $tag := $img.tag -}} -{{- if not $tag -}} - {{- if eq .component "clpPackage" -}} - {{- $tag = .root.Chart.AppVersion -}} - {{- else -}} - {{- fail (printf "image.%s.tag is required" .component) -}} - {{- end -}} +{{- if and (not $tag) (eq .component "clpPackage") -}} + {{- $tag = .root.Chart.AppVersion -}} +{{- end -}} +{{- if not (or $tag $img.digest) -}} + {{- fail (printf "image.%s requires \"tag\" or \"digest\"" .component) -}} {{- end -}} -{{- printf "%s:%s" $img.repository $tag -}} +{{- $ref := $img.repository -}} +{{- if $tag -}} + {{- $ref = printf "%s:%s" $ref $tag -}} +{{- end -}} +{{- if $img.digest -}} + {{- $ref = printf "%s@%s" $ref $img.digest -}} {{- end -}} +{{- $ref -}} {{- end }} {{/* diff --git a/tools/deployment/package-helm/templates/presto-coordinator-deployment.yaml b/tools/deployment/package-helm/templates/presto-coordinator-deployment.yaml index 92ecbb7630..adaf67f7b9 100644 --- a/tools/deployment/package-helm/templates/presto-coordinator-deployment.yaml +++ b/tools/deployment/package-helm/templates/presto-coordinator-deployment.yaml @@ -45,7 +45,7 @@ spec: mountPath: "/install/coordinator" containers: - name: "presto-coordinator" - image: "{{ .Values.image.prestoCoordinator.repository }}:{{ .Values.image.prestoCoordinator.tag }}" + image: {{ include "clp.imageRef" (dict "root" . "component" "prestoCoordinator") | quote }} imagePullPolicy: "{{ .Values.image.prestoCoordinator.pullPolicy }}" {{- include "clp.createResourceLimits" (dict "root" . diff --git a/tools/deployment/package-helm/templates/presto-worker-deployment.yaml b/tools/deployment/package-helm/templates/presto-worker-deployment.yaml index 222bd048cf..6688669c22 100644 --- a/tools/deployment/package-helm/templates/presto-worker-deployment.yaml +++ b/tools/deployment/package-helm/templates/presto-worker-deployment.yaml @@ -62,7 +62,7 @@ spec: readOnly: true containers: - name: "presto-worker" - image: "{{ .Values.image.prestoWorker.repository }}:{{ .Values.image.prestoWorker.tag }}" + image: {{ include "clp.imageRef" (dict "root" . "component" "prestoWorker") | quote }} imagePullPolicy: "{{ .Values.image.prestoWorker.pullPolicy }}" {{- include "clp.createResourceLimits" (dict "root" . diff --git a/tools/deployment/package-helm/values.yaml b/tools/deployment/package-helm/values.yaml index 9a3a09b0b8..ca7dff0c4e 100644 --- a/tools/deployment/package-helm/values.yaml +++ b/tools/deployment/package-helm/values.yaml @@ -11,6 +11,7 @@ image: repository: "ghcr.io/y-scope/clp-plugin-presto-connector" pullPolicy: "IfNotPresent" tag: "0.1.0-SNAPSHOT" + digest: "sha256:d006b0ce7830b6932eea66f1edc8dedbc54dbd661943eb38147e62c847fe0c32" clpPackage: repository: "ghcr.io/y-scope/clp/clp-package" pullPolicy: "Always" @@ -40,10 +41,12 @@ image: repository: "ghcr.io/y-scope/presto" pullPolicy: "IfNotPresent" tag: "0.299" + digest: "sha256:87e9ce10a1184f1b5ce0dd74591ef753c3afe763d860ef412a331684ec653753" prestoWorker: repository: "ghcr.io/y-scope/presto-native" pullPolicy: "IfNotPresent" tag: "0.299" + digest: "sha256:8f9ca4ba7dd844385b85e7e3bd9568128096e2a7ad09432c6a982ea11279a13b" queue: repository: "rabbitmq" pullPolicy: "Always" diff --git a/tools/deployment/presto-clp/docker-compose.yaml b/tools/deployment/presto-clp/docker-compose.yaml index d3af2eb942..cb156e8651 100644 --- a/tools/deployment/presto-clp/docker-compose.yaml +++ b/tools/deployment/presto-clp/docker-compose.yaml @@ -4,13 +4,13 @@ services: # coordinator and worker mount into their plugin dirs (replacing the connector-baked # Presto images). The connector image's own entrypoint does the install; we only supply # the target paths via *_PLUGIN_INSTALL_PATH. - # init.py verifies the image exists (locally or on the registry) and writes - # CLP_PRESTO_CONNECTOR_IMAGE/CLP_PRESTO_CONNECTOR_TAG into .env. Locally-built and published - # images share the same : tag; whatever is in the local daemon wins. The defaults - # below are only used when .env is absent. - image: "${CLP_PRESTO_CONNECTOR_IMAGE:-\ - ghcr.io/y-scope/clp-plugin-presto-connector}:${CLP_PRESTO_CONNECTOR_TAG:-\ - 0.1.0-SNAPSHOT}" + # init.py verifies the image exists (locally or on the registry) and writes the resolved + # reference into .env as CLP_PRESTO_CONNECTOR_REF, which may be repository:tag, + # repository@digest, or repository:tag@digest. Locally-built and published images share the + # same : tag; whatever is in the local daemon wins. The default below is only used + # when .env is absent. + image: "${CLP_PRESTO_CONNECTOR_REF:-\ + ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT@sha256:d006b0ce7830b6932eea66f1edc8dedbc54dbd661943eb38147e62c847fe0c32}" restart: "no" environment: COORDINATOR_PLUGIN_INSTALL_PATH: "/install/coordinator" @@ -22,7 +22,10 @@ services: - "clp-package" presto-coordinator: - image: "ghcr.io/y-scope/presto:${CLP_PRESTO_COORDINATOR_IMAGE_TAG:-0.299}" + # Pinned by digest so the image can't change under us; the tag is kept as a + # human-readable label. Override with a full reference. + image: "${CLP_PRESTO_COORDINATOR_REF:-\ + ghcr.io/y-scope/presto:0.299@sha256:87e9ce10a1184f1b5ce0dd74591ef753c3afe763d860ef412a331684ec653753}" entrypoint: ["/bin/bash", "-c", "/scripts/generate-configs.sh && /opt/entrypoint.sh"] depends_on: clp-plugin-presto-connector-init: @@ -51,7 +54,9 @@ services: retries: 30 presto-worker: - image: "ghcr.io/y-scope/presto-native:${CLP_PRESTO_WORKER_IMAGE_TAG:-0.299}" + # Pinned by digest; see the coordinator's image above. + image: "${CLP_PRESTO_WORKER_REF:-\ + ghcr.io/y-scope/presto-native:0.299@sha256:8f9ca4ba7dd844385b85e7e3bd9568128096e2a7ad09432c6a982ea11279a13b}" depends_on: presto-coordinator: condition: "service_healthy" diff --git a/tools/deployment/presto-clp/scripts/init.py b/tools/deployment/presto-clp/scripts/init.py index 0d8edda8a3..04c51910d6 100755 --- a/tools/deployment/presto-clp/scripts/init.py +++ b/tools/deployment/presto-clp/scripts/init.py @@ -27,9 +27,12 @@ # S3 URL constant AWS_S3_DOMAIN = "amazonaws.com" -# Default CLP Presto connector image and tag, used when the corresponding env vars are unset. -DEFAULT_CONNECTOR_IMAGE = "ghcr.io/y-scope/clp-plugin-presto-connector" -DEFAULT_CONNECTOR_VERSION = "0.1.0-SNAPSHOT" +# Default CLP Presto connector image. Pinned by digest so the image can't change under us; +# the tag is kept as a human-readable label and must be updated alongside the digest. +DEFAULT_CONNECTOR_REF = ( + "ghcr.io/y-scope/clp-plugin-presto-connector:0.1.0-SNAPSHOT" + "@sha256:d006b0ce7830b6932eea66f1edc8dedbc54dbd661943eb38147e62c847fe0c32" +) # Silence Ruff S607: the absolute path of the Docker binary may vary depending on the installation # method. @@ -382,59 +385,50 @@ def _add_worker_env_vars(coordinator_common_env_file_path: Path, env_vars: dict[ def _add_connector_image_env_vars(env_vars: dict[str, str]) -> bool: """ - Resolves the CLP Presto connector image and adds `CLP_PRESTO_CONNECTOR_IMAGE` and - `CLP_PRESTO_CONNECTOR_TAG` to `env_vars`, which `docker-compose.yaml` consumes. + Resolves the CLP Presto connector image and adds `CLP_PRESTO_CONNECTOR_REF` to `env_vars`, + which `docker-compose.yaml` consumes. - `CLP_PRESTO_CONNECTOR_IMAGE` (repository) and `CLP_PRESTO_CONNECTOR_VERSION` (tag) override - the defaults. An explicit `CLP_PRESTO_CONNECTOR_TAG` is used as-is and skips the existence - check. + `CLP_PRESTO_CONNECTOR_REF` overrides the default and is used as-is, so it may be + `repository:tag`, `repository@digest`, or `repository:tag@digest`. - :param env_vars: Dictionary to populate with the connector image environment variables. - :return: Whether the image and tag were successfully resolved. + :param env_vars: Dictionary to populate with the connector image environment variable. + :return: Whether the reference was successfully resolved. """ - image = os.environ.get("CLP_PRESTO_CONNECTOR_IMAGE", DEFAULT_CONNECTOR_IMAGE) - - tag = os.environ.get("CLP_PRESTO_CONNECTOR_TAG") - if tag is None: - tag = os.environ.get("CLP_PRESTO_CONNECTOR_VERSION", DEFAULT_CONNECTOR_VERSION) - if not _connector_image_available(image, tag): - return False - else: - logger.info("Using CLP_PRESTO_CONNECTOR_TAG='%s'; skipping the existence check.", tag) + ref = os.environ.get("CLP_PRESTO_CONNECTOR_REF", DEFAULT_CONNECTOR_REF) + if not _connector_image_available(ref): + return False - env_vars["CLP_PRESTO_CONNECTOR_IMAGE"] = image - env_vars["CLP_PRESTO_CONNECTOR_TAG"] = tag + env_vars["CLP_PRESTO_CONNECTOR_REF"] = ref return True -def _connector_image_available(image: str, tag: str) -> bool: +def _connector_image_available(ref: str) -> bool: """ - Returns whether `image`:`tag` exists in the local Docker daemon or on its registry. A local - image takes precedence at `docker compose up`. + Returns whether `ref` exists in the local Docker daemon or on its registry. A local image takes + precedence at `docker compose up`. - :param image: The connector image repository. - :param tag: The image tag. + :param ref: The full image reference. :return: Whether the image is available. """ if shutil.which(_DOCKER_EXECUTABLE) is None: logger.error( "Docker isn't installed or isn't on PATH, so the CLP Presto connector image can't be" - " checked. Install Docker, or set CLP_PRESTO_CONNECTOR_TAG explicitly." + " checked. Install Docker to continue." ) return False - ref = f"{image}:{tag}" - if _run_docker_probe(["image", "inspect", ref]): - logger.info("Found CLP connector image '%s' in the local Docker daemon.", ref) - return True - if _run_docker_probe(["manifest", "inspect", ref]): - logger.info("Found CLP connector image '%s' on the registry.", ref) + # Check the local daemon before the registry so a locally-built image is accepted without a + # network round-trip. + if _run_docker_probe(["image", "inspect", ref]) or _run_docker_probe( + ["manifest", "inspect", ref] + ): + logger.info("Found CLP connector image '%s'.", ref) return True logger.error( "Couldn't find CLP Presto connector image '%s' locally or on the registry. Build it" - " (e.g. via `task package` in clp-plugin-presto-connector) or set CLP_PRESTO_CONNECTOR_TAG" - " explicitly.", + " (e.g. via `task package` in clp-plugin-presto-connector) or set" + " CLP_PRESTO_CONNECTOR_REF to an image that exists.", ref, ) return False