From 1cc4ee3d0da0af358a72d2523496d7dabaa9ee6b Mon Sep 17 00:00:00 2001 From: Joaquin Rivero Date: Mon, 27 Jul 2026 10:39:15 -0300 Subject: [PATCH] fix(batch): sync Batch application settings across code and deploy paths `hastegeo` reads `AZURE_BATCH_REGISTRY_SERVER`, but every deploy path emitted `AZURE_BATCH_REGISTRY_SERVER_URL`, which no code read. The setting fell back to its `.azurecr.io` placeholder, so image preprocessing failed with `InvalidPropertyValue: The specified registry is an invalid docker registry server name` before any task reached Batch. Separately, `deploy_apps.sh` never emitted the shared-pool settings added with the multi-tenant Batch pools (`AZURE_BATCH_*_POOL_IDS`, `AZURE_BATCH_USE_SAS`, `AZURE_BATCH_MANAGE_POOLS`) or `EMBEDDING_QUEUE_NAME`, so environments deployed through it silently ran the legacy single-pool path against renamed pools. - config: resolve `AZURE_BATCH_REGISTRY_SERVER`, falling back to the deprecated `_URL` name and stripping any scheme, so environments provisioned before the rename keep working while operators migrate. - new `utils/batch_config.py`: fail fast naming the unset application setting, instead of surfacing an opaque Azure error from inside pool creation. Runs at the submission boundary so the message reaches the image layer status. - runner: rebind a reused Batch job whose pool binding is stale. Job ids default to the pool id, and `create_job` only re-enabled an existing job, so spillover was silently ineffective and tasks queued into jobs bound to deleted pools. - runner: whitelist both training and imageryprep images on created pools, matching `infra/modules/batchPool.bicep`. - deploy: emit the full setting set from `deploy_apps.sh` and `functions.bicep`, with per-environment pool overrides that default to legacy behavior. - new `check_env_drift.py` + `config-drift.yml`: fail any PR where a required variable is missing from a deploy path, or a deploy path emits a setting no code reads. Verified to fail on the pre-fix tree and pass after. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dd98b384-7e26-47a9-8c03-6f9594eb611c --- .github/scripts/check_env_drift.py | 237 ++++++++++++++++++ .github/scripts/deploy_apps.sh | 26 +- .github/workflows/config-drift.yml | 43 ++++ .github/workflows/deploy-apps.yml | 10 + CHANGELOG.md | 9 + api/hastefuncqueues/README.md | 11 +- docs/api/hastefuncqueues.md | 11 +- docs/configuration.md | 28 +++ hastelib/src/hastegeo/core/config.py | 43 +++- .../src/hastegeo/core/runners/azure_batch.py | 54 +++- .../src/hastegeo/core/utils/batch_config.py | 77 ++++++ .../tests/core/utils/test_batch_config.py | 171 +++++++++++++ infra/main.json | 91 ++++--- infra/modules/functions.bicep | 3 +- local.settings.example.jsonc | 14 +- spec/features/batch-config-drift/README.md | 72 ++++++ spec/features/batch-config-drift/design.md | 96 +++++++ 17 files changed, 939 insertions(+), 57 deletions(-) create mode 100644 .github/scripts/check_env_drift.py create mode 100644 .github/workflows/config-drift.yml create mode 100644 hastelib/src/hastegeo/core/utils/batch_config.py create mode 100644 hastelib/tests/core/utils/test_batch_config.py create mode 100644 spec/features/batch-config-drift/README.md create mode 100644 spec/features/batch-config-drift/design.md diff --git a/.github/scripts/check_env_drift.py b/.github/scripts/check_env_drift.py new file mode 100644 index 0000000..fdc0eba --- /dev/null +++ b/.github/scripts/check_env_drift.py @@ -0,0 +1,237 @@ +#!/usr/bin/env python3 +"""Fail when an application setting the code requires is not deployed. + +`hastegeo` reads its configuration from environment variables, and the Function +Apps receive those as application settings from two deploy paths: + + .github/scripts/deploy_apps.sh (used by .github/workflows/deploy-apps.yml) + infra/modules/functions.bicep (used by the Bicep/IaC path) + +Renaming a variable in code without renaming it in *both* deploy paths leaves +the setting silently unset, and the code falls back to a `` default +that only fails once Azure rejects it -- far from the cause. That is exactly how +`AZURE_BATCH_REGISTRY_SERVER` broke image preprocessing. + +A variable is treated as REQUIRED when the code either reads it with no default +at all, or defaults it to a value still containing a ``. Anything +with a real, working default is optional and ignored. + +Usage: python .github/scripts/check_env_drift.py +Exit 0 when in sync, 1 otherwise. +""" + +from __future__ import annotations + +import ast +import re +import sys +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] + +CODE_ROOTS = ( + REPO / "hastelib" / "src", + REPO / "api" / "hastefuncapi", + REPO / "api" / "hastefuncqueues", + REPO / "api" / "titilerfuncapi", +) + +DEPLOY_SH = REPO / ".github" / "scripts" / "deploy_apps.sh" +FUNCTIONS_BICEP = REPO / "infra" / "modules" / "functions.bicep" + +PLACEHOLDER = re.compile(r"<[^<>]+>") + +# Variables that are genuinely optional for an Azure deployment, with the reason +# each one is exempt. Anything not listed here that the code marks required must +# be emitted by both deploy paths. +ALLOWLIST = { + # Alternative metadata/storage backends. Azure deployments use blob storage + # (METADATA_STORAGE_TYPE=blob), so these are never read there. + "COSMOS_ENDPOINT": "cosmos backend only", + "COSMOS_DATABASE": "cosmos backend only", + "COSMOS_CONTAINER": "cosmos backend only", + "DATALAKE_ACCOUNT_URL": "datalake backend only", + "DATALAKE_FILESYSTEM": "datalake backend only", + "POSTGRES_HOST": "postgres backend only", + "POSTGRES_PORT": "postgres backend only", + "POSTGRES_USER": "postgres backend only", + # pragma: allowlist nextline secret + "POSTGRES_PASSWORD": "postgres backend only", # pragma: allowlist secret + "POSTGRES_DATABASE": "postgres backend only", + "POSTGRES_TABLE": "postgres backend only", + # Local docker-compose runner (RUNNER_TYPE=local); unused on Azure Batch. + "AZURE_STORAGE_CONNECTION_STRING": "local runner only", + "AZURE_STORAGE_ACCOUNT": "local runner only", + "HASTE_DOCKER_NETWORK": "local runner only", + "HASTE_DOCKER_MEM_LIMIT": "local runner only", + "HASTE_DOCKER_SHM_SIZE": "local runner only", + "HASTE_DOCKER_AZURITE_VOLUME": "local runner only", + "HASTE_ENABLE_GPU": "local runner only", + "HASTE_GPU_DEVICES": "local runner only", + "HASTE_DATALOADER_WORKERS": "local runner only", + "HASTE_DEBUG_VERBOSE": "local runner only", + "CLEANUP_CONTAINERS": "local runner only", + "PRESERVE_LOCAL_TASK_DIRS": "local runner only", + "FAIL_ON_EMPTY_OUTPUT_LOG": "local runner only", + # Set by the Batch node agent / supplied inside the task container. + "WORKDIR": "set inside the task container", + "INPUT_DIR": "set inside the task container", + "OUTPUT_TRAINING_ZIP_NAME": "set inside the task container", + "OUTPUT_INFERENCE_ZIP_NAME": "set inside the task container", + # Azure Functions / platform-provided. + "AzureWebJobsStorage": "provided by the Functions runtime", + "WEBSITE_HOSTNAME": "provided by the Functions runtime", + "FUNCTIONS_WORKER_RUNTIME": "provided by the Functions runtime", + # Deprecated legacy name, still read as a fallback so environments + # provisioned before the rename keep working. Deliberately not emitted. + "AZURE_BATCH_REGISTRY_SERVER_URL": "deprecated legacy fallback", + # GDAL tuning read inside the imageryprep/training container, where the + # workflow supplies them; not Function App settings. + "GDAL_SKIP": "read inside the task container", + "GDAL_WARP_PARAMS": "read inside the task container", + "GDAL_TRANSLATE_PARAMS": "read inside the task container", +} + + +def _literal(node: ast.AST): + try: + return ast.literal_eval(node) + except (ValueError, SyntaxError): + return None + + +def scan_code() -> tuple[dict[str, set[Path]], set[str]]: + """Return ({required var: files}, every var the code reads).""" + required: dict[str, set[Path]] = {} + seen: set[str] = set() + + for root in CODE_ROOTS: + if not root.exists(): + continue + for path in root.rglob("*.py"): + if "__pycache__" in path.parts or "tests" in path.parts: + continue + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except SyntaxError: + continue + + for node in ast.walk(tree): + name, default, has_default = None, None, False + + if isinstance(node, ast.Call): + func = node.func + is_getenv = ( + isinstance(func, ast.Attribute) + and func.attr in ("getenv", "get") + and node.args + ) + if is_getenv: + target = ast.unparse(func) + if target.endswith( + ("os.getenv", "os.environ.get", "environ.get") + ): + name = _literal(node.args[0]) + has_default = len(node.args) > 1 + if has_default: + default = _literal(node.args[1]) + + elif isinstance(node, ast.Subscript): + value = node.value + if isinstance(value, ast.Attribute) and ast.unparse( + value + ).endswith("os.environ"): + name = _literal(node.slice) + + if not isinstance(name, str): + continue + seen.add(name) + + # Required = no default, or a default that is still a + # the operator was meant to replace. + unresolved = isinstance(default, str) and PLACEHOLDER.search( + default + ) + if has_default and not unresolved: + continue + required.setdefault(name, set()).add(path.relative_to(REPO)) + + return required, seen + + +def settings_in_deploy_sh() -> set[str]: + """Extract only the keys inside the `appsettings set --settings` block. + + Scanning the whole file would also pick up resource tags such as + `project=haste`, which are not application settings. + """ + names: set[str] = set() + in_block = False + for line in DEPLOY_SH.read_text(encoding="utf-8").splitlines(): + if "appsettings set" in line: + in_block = True + continue + if in_block: + names.update(re.findall(r'"([A-Za-z_][A-Za-z0-9_]*)=', line)) + if not line.rstrip().endswith("\\"): + in_block = False + return names + + +def settings_in_bicep() -> set[str]: + text = FUNCTIONS_BICEP.read_text(encoding="utf-8") + return set(re.findall(r"\{\s*name:\s*'([A-Za-z_][A-Za-z0-9_]*)'", text)) + + +def main() -> int: + required, all_read = scan_code() + surfaces = { + str(DEPLOY_SH.relative_to(REPO)): settings_in_deploy_sh(), + str(FUNCTIONS_BICEP.relative_to(REPO)): settings_in_bicep(), + } + + failures: list[str] = [] + for var in sorted(required): + if var in ALLOWLIST: + continue + missing = [name for name, s in surfaces.items() if var not in s] + if missing: + readers = ", ".join(sorted(str(p) for p in required[var])) + failures.append( + f" {var}\n" + f" read by: {readers}\n" + f" NOT set by: {', '.join(missing)}" + ) + + # Settings a deploy path emits that no code reads -- dead config, and a + # strong signal that a rename was applied on only one side. + dead: list[str] = [] + for surface, names in surfaces.items(): + for var in sorted(names): + if var not in all_read and var not in ALLOWLIST: + dead.append(f" {var} (set by {surface}, read by no code)") + + if failures: + print( + "Required application settings are not emitted by every " + "deploy path:\n" + ) + print("\n".join(failures)) + if dead: + print("\nDead application settings (set but never read):\n") + print("\n".join(dead)) + + if failures or dead: + print( + "\nFix by updating .github/scripts/deploy_apps.sh and " + "infra/modules/functions.bicep together, or add a documented " + "entry to ALLOWLIST in this script." + ) + return 1 + + print("Application settings are in sync across code and deploy paths.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/deploy_apps.sh b/.github/scripts/deploy_apps.sh index 972863f..9804d70 100644 --- a/.github/scripts/deploy_apps.sh +++ b/.github/scripts/deploy_apps.sh @@ -42,6 +42,20 @@ USER_MANAGED_IDENTITY="${RESOURCE_PREFIX}-haste-${RANDOM_SUFFIX}-umi" TRAINING_DOCKER_IMAGE="hastetraining:${TRAINING_IMAGE_TAG}" IMAGEPREP_DOCKER_IMAGE="hasteimageryprep:${IMAGEPREP_IMAGE_TAG}" BATCH_POOL_ID="${RESOURCE_PREFIX}-haste-${RANDOM_SUFFIX}-pool" +# Batch pool wiring. Defaults reproduce the legacy single-pool behavior; set the +# corresponding GitHub Environment variables to point an environment at +# pre-created shared pools (see docs/configuration.md). +# *_POOL_ID - the pool used when no candidate list is supplied. It is also +# the default Batch *job* id, so it must be identical across the +# api and queues apps or status lookups miss the job. +# *_POOL_IDS - ordered candidate lists for capacity-aware routing. +BATCH_TRAINING_POOL_ID="${BATCH_TRAINING_POOL_ID:-$BATCH_POOL_ID}" +BATCH_IMAGERYPREP_POOL_ID="${BATCH_IMAGERYPREP_POOL_ID:-$BATCH_POOL_ID}" +BATCH_TRAINING_POOL_IDS="${BATCH_TRAINING_POOL_IDS:-}" +BATCH_INFERENCE_POOL_IDS="${BATCH_INFERENCE_POOL_IDS:-}" +BATCH_IMAGERYPREP_POOL_IDS="${BATCH_IMAGERYPREP_POOL_IDS:-}" +BATCH_USE_SAS="${BATCH_USE_SAS:-false}" +BATCH_MANAGE_POOLS="${BATCH_MANAGE_POOLS:-true}" MAPS_ACCOUNT="${RESOURCE_PREFIX}haste${RANDOM_SUFFIX}maps" API_MANAGEMENT="${RESOURCE_PREFIX}-haste-${RANDOM_SUFFIX}-apim" FIXED_TAGS="project=haste created_by=deploy_apps" @@ -87,6 +101,7 @@ deploy_function() { "STATS_QUEUE_NAME=stats-queue" \ "TRAIN_QUEUE_NAME=train-queue" \ "ZIP_QUEUE_NAME=zip-queue" \ + "EMBEDDING_QUEUE_NAME=embedding-queue" \ "IMAGERY_STORAGE_TYPE=blob" \ "METADATA_STORAGE_TYPE=blob" \ "ARTIFACT_STORAGE_TYPE=blob" \ @@ -104,9 +119,14 @@ deploy_function() { "AZURE_BATCH_IMAGERYPREP_DOCKER_IMAGE=${ACR_NAME}.azurecr.io/${IMAGEPREP_DOCKER_IMAGE}" \ "AZURE_BATCH_DOCKER_IMAGE=${ACR_NAME}.azurecr.io/${TRAINING_DOCKER_IMAGE}" \ "AZURE_BATCH_OUTPUT_CONTAINER_URL=https://${STORAGE_ACCOUNT}.blob.core.windows.net/data" \ - "AZURE_BATCH_TRAINING_POOL_ID=${BATCH_POOL_ID}" \ - "AZURE_BATCH_IMAGERYPREP_POOL_ID=${BATCH_POOL_ID}" \ - "AZURE_BATCH_REGISTRY_SERVER_URL=https://${ACR_NAME}.azurecr.io" \ + "AZURE_BATCH_TRAINING_POOL_ID=${BATCH_TRAINING_POOL_ID}" \ + "AZURE_BATCH_IMAGERYPREP_POOL_ID=${BATCH_IMAGERYPREP_POOL_ID}" \ + "AZURE_BATCH_TRAINING_POOL_IDS=${BATCH_TRAINING_POOL_IDS}" \ + "AZURE_BATCH_INFERENCE_POOL_IDS=${BATCH_INFERENCE_POOL_IDS}" \ + "AZURE_BATCH_IMAGERYPREP_POOL_IDS=${BATCH_IMAGERYPREP_POOL_IDS}" \ + "AZURE_BATCH_USE_SAS=${BATCH_USE_SAS}" \ + "AZURE_BATCH_MANAGE_POOLS=${BATCH_MANAGE_POOLS}" \ + "AZURE_BATCH_REGISTRY_SERVER=${ACR_NAME}.azurecr.io" \ "AZURE_BATCH_REGISTRY_IMAGE=${ACR_NAME}.azurecr.io/${TRAINING_DOCKER_IMAGE}" \ "AZURE_BATCH_REGISTRY_IDENTITY_RESOURCE_ID=/subscriptions/$SUBSCRIPTION_ID/resourceGroups/$RESOURCE_GROUP/providers/Microsoft.ManagedIdentity/userAssignedIdentities/$USER_MANAGED_IDENTITY" \ "STATIC_APP_SUBSCRIPTION_ID=$SUBSCRIPTION_ID" \ diff --git a/.github/workflows/config-drift.yml b/.github/workflows/config-drift.yml new file mode 100644 index 0000000..2805bcb --- /dev/null +++ b/.github/workflows/config-drift.yml @@ -0,0 +1,43 @@ +name: Config drift + +# A variable renamed in code but not in every deploy path leaves the setting +# silently unset in Azure, and the app falls back to a default +# that only fails once Azure rejects it. Catch that here instead of in an +# environment. +on: + pull_request: + branches: + - main + paths: + - "hastelib/src/**" + - "api/**" + - "infra/modules/functions.bicep" + - ".github/scripts/deploy_apps.sh" + - ".github/scripts/check_env_drift.py" + - ".github/workflows/config-drift.yml" + push: + branches: + - main + paths: + - "hastelib/src/**" + - "api/**" + - "infra/modules/functions.bicep" + - ".github/scripts/deploy_apps.sh" + +permissions: + contents: read + +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + with: + persist-credentials: false + + - uses: actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38 # v5 + with: + python-version: "3.11" + + - name: Check application-setting drift + run: python .github/scripts/check_env_drift.py diff --git a/.github/workflows/deploy-apps.yml b/.github/workflows/deploy-apps.yml index 374bf9a..5348363 100644 --- a/.github/workflows/deploy-apps.yml +++ b/.github/workflows/deploy-apps.yml @@ -115,6 +115,16 @@ jobs: # `func publish` (deploy_apps.sh); the editable default can't resolve # on Azure's remote build. HASTEGEO_WHEEL_URL: ${{ steps.hastegeo.outputs.url }} + # Batch pool wiring. All optional: unset reproduces the legacy + # single-pool behavior. Set these as GitHub Environment variables to + # point an environment at pre-created shared pools. + BATCH_TRAINING_POOL_ID: ${{ vars.BATCH_TRAINING_POOL_ID }} + BATCH_IMAGERYPREP_POOL_ID: ${{ vars.BATCH_IMAGERYPREP_POOL_ID }} + BATCH_TRAINING_POOL_IDS: ${{ vars.BATCH_TRAINING_POOL_IDS }} + BATCH_INFERENCE_POOL_IDS: ${{ vars.BATCH_INFERENCE_POOL_IDS }} + BATCH_IMAGERYPREP_POOL_IDS: ${{ vars.BATCH_IMAGERYPREP_POOL_IDS }} + BATCH_USE_SAS: ${{ vars.BATCH_USE_SAS }} + BATCH_MANAGE_POOLS: ${{ vars.BATCH_MANAGE_POOLS }} run: | chmod +x .github/scripts/deploy_apps.sh .github/scripts/deploy_apps.sh \ diff --git a/CHANGELOG.md b/CHANGELOG.md index 510dcad..24fc087 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,15 @@ Versioning follows the Docker image tags defined in the CI workflows (see [.gith ### Added - **Shared multi-tenant GPU Batch pools** — For deployments running many environments against scarce GPU quota, HASTE can now share a small set of multi-tenant Batch pools (H100 for training, T4 for inference/imageryprep + spillover) instead of one pool per environment. Data isolation is enforced at the credential boundary: each job mints a short-lived **user-delegation SAS** scoped to its own storage container (the pool identity is used only for ACR pull and holds no storage access), so a tenant's task can never read another tenant's data. Pools autoscale on low-priority nodes and scale to zero when idle. New `hastelib` routing picks a pool from an ordered candidate list **at submit time** (first with an idle node, else the preferred pool). Provisioned by the standalone [`infra/shared-pools.bicep`](infra/shared-pools.bicep) + [`shared-pools.bicepparam`](infra/shared-pools.bicepparam); opted into per environment via `AZURE_BATCH_*_POOL_IDS`, `AZURE_BATCH_USE_SAS`, and `AZURE_BATCH_MANAGE_POOLS` (all default to the legacy single-pool behavior). Full design in [`spec/features/batch-compute-expansion/`](spec/features/batch-compute-expansion). See [docs/configuration.md](docs/configuration.md#shared-multi-tenant-gpu-pools). +### Fixed +- **Batch application settings were out of sync with the code that reads them** — `hastegeo` reads `AZURE_BATCH_REGISTRY_SERVER`, but every deploy path still emitted `AZURE_BATCH_REGISTRY_SERVER_URL`, which nothing read. The setting therefore fell back to its `.azurecr.io` placeholder and image preprocessing failed with `InvalidPropertyValue: The specified registry is an invalid docker registry server name` before any task reached Batch. Separately, [`deploy_apps.sh`](.github/scripts/deploy_apps.sh) never emitted the shared-pool settings added alongside the multi-tenant pools (`AZURE_BATCH_*_POOL_IDS`, `AZURE_BATCH_USE_SAS`, `AZURE_BATCH_MANAGE_POOLS`) or `EMBEDDING_QUEUE_NAME`, so environments deployed through it silently ran the legacy single-pool path. Both deploy paths now emit the full set, the legacy `AZURE_BATCH_REGISTRY_SERVER_URL` is still honored as a fallback (with any `https://` prefix stripped) so existing deployments keep working, and a new `Config drift` workflow fails any PR that reintroduces this class of drift. + + > **Operator action:** rename the `AZURE_BATCH_REGISTRY_SERVER_URL` application setting to `AZURE_BATCH_REGISTRY_SERVER` (value: the bare login server, e.g. `myacr.azurecr.io`) on the `api` and `queues` Function Apps. Confirm `AZURE_BATCH_TRAINING_POOL_ID` / `AZURE_BATCH_IMAGERYPREP_POOL_ID` name pools that still exist and are **identical across both apps** — they double as the default Batch job ids. + +- **A reused Batch job stayed pinned to the pool that created it** — job ids default to the configured pool id, and `create_job` only re-enabled an existing job, never re-pointing it. Capacity-aware spillover was therefore silently ineffective, and once a pool was renamed or deleted every task queued into a job bound to a pool that no longer existed. The runner now rebinds the job to the pool selected for the submission, and fails loudly when Batch refuses. +- **Missing Batch settings now fail fast** — the runner validates its configuration before the first Batch call and names the specific application setting that is unset, instead of surfacing an opaque Azure error from deep inside pool creation. +- **Pool creation whitelisted only the training image** — pools created by the runner now list both the training and imageryprep images, matching [`infra/modules/batchPool.bicep`](infra/modules/batchPool.bicep). + ### Changed - **`infra/modules/batchPool.bicep` parameterized** — one module now serves fixed-dedicated (dev/prod) and autoscale-low-priority (shared) pools via `scaleMode` / `nodeType` / `minNodes` params, with optional VNet injection. Backward-compatible defaults. - **Generic-default IaC for reuse by other partners** — `HASTE_RESOURCE_PREFIX` now defaults to the neutral `haste` (overridable per deployment); the shared-pools template keeps its account/ACR as bring-your-own params. The `api`/`queues` Function App identity is granted **Storage Blob Delegator** (in `functionApp.bicep`) so it can mint user-delegation SAS. diff --git a/api/hastefuncqueues/README.md b/api/hastefuncqueues/README.md index 5e7e3ae..d5e4d69 100644 --- a/api/hastefuncqueues/README.md +++ b/api/hastefuncqueues/README.md @@ -127,8 +127,13 @@ Handles image layer messages that exceeded the max dequeue count (`maxDequeueCou | `AZURE_BATCH_ACCOUNT_NAME` | Batch account name | — | | `AZURE_BATCH_ACCOUNT_KEY` | Batch account key (if not using managed identity) | — | | `AZURE_BATCH_ACCOUNT_URL` | Batch account endpoint URL | — | -| `AZURE_BATCH_TRAINING_POOL_ID` | Pool for training and inference | `training-pool` | -| `AZURE_BATCH_IMAGERYPREP_POOL_ID` | Pool for imagery preprocessing | `imageryprep-pool` | +| `AZURE_BATCH_TRAINING_POOL_ID` | Pool for training and inference; also the default training/inference job id | `training-pool` | +| `AZURE_BATCH_IMAGERYPREP_POOL_ID` | Pool for imagery preprocessing; also the default imageryprep/artifact job id | `imageryprep-pool` | +| `AZURE_BATCH_TRAINING_POOL_IDS` | Ordered candidate training pools (comma-separated) | `AZURE_BATCH_TRAINING_POOL_ID` | +| `AZURE_BATCH_INFERENCE_POOL_IDS` | Ordered candidate inference/embedding pools | training pool | +| `AZURE_BATCH_IMAGERYPREP_POOL_IDS` | Ordered candidate imageryprep/artifact pools | `AZURE_BATCH_IMAGERYPREP_POOL_ID` | +| `AZURE_BATCH_USE_SAS` | Per-job user-delegation SAS for blob I/O instead of the pool identity. Required for shared pools. | `false` | +| `AZURE_BATCH_MANAGE_POOLS` | Runner auto-creates/resizes its pool. Set `false` for pre-created autoscale pools. | `true` | | `AZURE_BATCH_TARGET_DEDICATED_NODES` | Number of dedicated nodes per pool | `1` | | `AZURE_BATCH_TARGET_LOW_PRIORITY_NODES` | Number of spot nodes per pool | `0` | | `AZURE_BATCH_TASK_RETENTION_TIME` | ISO 8601 retention for task files | `P2D` | @@ -141,7 +146,7 @@ Handles image layer messages that exceeded the max dequeue count (`maxDequeueCou | Environment Variable | Description | |----------------------|-------------| -| `AZURE_BATCH_REGISTRY_SERVER` | ACR login server (e.g. `myregistry.azurecr.io`) | +| `AZURE_BATCH_REGISTRY_SERVER` | ACR login server (e.g. `myregistry.azurecr.io`). Falls back to the deprecated `AZURE_BATCH_REGISTRY_SERVER_URL`; a `https://` prefix is stripped. Only read when `AZURE_BATCH_MANAGE_POOLS` is true. | | `AZURE_BATCH_DOCKER_IMAGE` | Training/inference Docker image (full tag) | | `AZURE_BATCH_IMAGERYPREP_DOCKER_IMAGE` | Imagery preprocessing Docker image (full tag) | | `AZURE_BATCH_REGISTRY_IDENTITY_RESOURCE_ID` | Managed identity resource ID for ACR pull | diff --git a/docs/api/hastefuncqueues.md b/docs/api/hastefuncqueues.md index 453b369..336dc3e 100644 --- a/docs/api/hastefuncqueues.md +++ b/docs/api/hastefuncqueues.md @@ -126,8 +126,13 @@ Handles image layer messages that exceeded the max dequeue count (`maxDequeueCou | `AZURE_BATCH_ACCOUNT_NAME` | Batch account name | — | | `AZURE_BATCH_ACCOUNT_KEY` | Batch account key (if not using managed identity) | — | | `AZURE_BATCH_ACCOUNT_URL` | Batch account endpoint URL | — | -| `AZURE_BATCH_TRAINING_POOL_ID` | Pool for training and inference | `training-pool` | -| `AZURE_BATCH_IMAGERYPREP_POOL_ID` | Pool for imagery preprocessing | `imageryprep-pool` | +| `AZURE_BATCH_TRAINING_POOL_ID` | Pool for training and inference; also the default training/inference job id | `training-pool` | +| `AZURE_BATCH_IMAGERYPREP_POOL_ID` | Pool for imagery preprocessing; also the default imageryprep/artifact job id | `imageryprep-pool` | +| `AZURE_BATCH_TRAINING_POOL_IDS` | Ordered candidate training pools (comma-separated) | `AZURE_BATCH_TRAINING_POOL_ID` | +| `AZURE_BATCH_INFERENCE_POOL_IDS` | Ordered candidate inference/embedding pools | training pool | +| `AZURE_BATCH_IMAGERYPREP_POOL_IDS` | Ordered candidate imageryprep/artifact pools | `AZURE_BATCH_IMAGERYPREP_POOL_ID` | +| `AZURE_BATCH_USE_SAS` | Per-job user-delegation SAS for blob I/O instead of the pool identity. Required for shared pools. | `false` | +| `AZURE_BATCH_MANAGE_POOLS` | Runner auto-creates/resizes its pool. Set `false` for pre-created autoscale pools. | `true` | | `AZURE_BATCH_TARGET_DEDICATED_NODES` | Number of dedicated nodes per pool | `1` | | `AZURE_BATCH_TARGET_LOW_PRIORITY_NODES` | Number of spot nodes per pool | `0` | | `AZURE_BATCH_TASK_RETENTION_TIME` | ISO 8601 retention for task files | `P2D` | @@ -140,7 +145,7 @@ Handles image layer messages that exceeded the max dequeue count (`maxDequeueCou | Environment Variable | Description | |----------------------|-------------| -| `AZURE_BATCH_REGISTRY_SERVER` | ACR login server (e.g. `myregistry.azurecr.io`) | +| `AZURE_BATCH_REGISTRY_SERVER` | ACR login server (e.g. `myregistry.azurecr.io`). Falls back to the deprecated `AZURE_BATCH_REGISTRY_SERVER_URL`; a `https://` prefix is stripped. Only read when `AZURE_BATCH_MANAGE_POOLS` is true. | | `AZURE_BATCH_DOCKER_IMAGE` | Training/inference Docker image (full tag) | | `AZURE_BATCH_IMAGERYPREP_DOCKER_IMAGE` | Imagery preprocessing Docker image (full tag) | | `AZURE_BATCH_REGISTRY_IDENTITY_RESOURCE_ID` | Managed identity resource ID for ACR pull | diff --git a/docs/configuration.md b/docs/configuration.md index 3c879b7..e278cb1 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -175,6 +175,34 @@ pool-identity behavior, so existing environments are unaffected until opted in): The runner picks a pool from the candidate list **at submit time** — the first with an idle node, otherwise the preferred (first) pool, which scales up / queues. +These are emitted by both deploy paths: set them as GitHub Environment +variables (`BATCH_TRAINING_POOL_IDS`, `BATCH_INFERENCE_POOL_IDS`, +`BATCH_IMAGERYPREP_POOL_IDS`, `BATCH_USE_SAS`, `BATCH_MANAGE_POOLS`, +`BATCH_TRAINING_POOL_ID`, `BATCH_IMAGERYPREP_POOL_ID`) for +[`deploy-apps.yml`](../.github/workflows/deploy-apps.yml), or as the +corresponding `infra/main.bicepparam` values for the Bicep path. + +> **`*_POOL_ID` (singular) also names the Batch job.** `TRAINING_BATCH_JOB_ID` +> and friends default to the matching pool id, so the value must be **identical +> on the api and queues apps** — the queues app submits under that job id and +> the api app reads status back from it. Pointing it at a pool that no longer +> exists leaves a job permanently bound to a deleted pool; the runner now +> rebinds such a job to the pool it selected, but the setting should still be +> corrected. + +### Keeping settings in sync + +`AZURE_BATCH_REGISTRY_SERVER` was previously emitted as +`AZURE_BATCH_REGISTRY_SERVER_URL`. The legacy name is still read as a fallback +(and any `https://` prefix is stripped), so environments provisioned before the +rename keep working — but operators should rename the application setting. + +Any variable the code requires must be emitted by **both** +[`deploy_apps.sh`](../.github/scripts/deploy_apps.sh) and +[`functions.bicep`](../infra/modules/functions.bicep). +[`check_env_drift.py`](../.github/scripts/check_env_drift.py) enforces this on +every PR via the `Config drift` workflow. + ## Email sender domain The email backend (Azure Communication Services) is provisioned in-IaC, so its diff --git a/hastelib/src/hastegeo/core/config.py b/hastelib/src/hastegeo/core/config.py index 32ffa1c..30a64fb 100644 --- a/hastelib/src/hastegeo/core/config.py +++ b/hastelib/src/hastegeo/core/config.py @@ -1,11 +1,49 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License. +import logging import os +import re import tempfile from enum import Enum from string import Template from typing import NamedTuple +_logger = logging.getLogger(__name__) + +REGISTRY_SERVER_PLACEHOLDER = ".azurecr.io" + +_SCHEME_RE = re.compile(r"^https?://", re.IGNORECASE) + + +def _strip_scheme(value): + """Reduce a registry URL to the bare login server. + + Azure Batch's ``ContainerRegistry.registry_server`` expects + ``myacr.azurecr.io``, but the app setting is frequently supplied as a URL. + """ + return _SCHEME_RE.sub("", value.strip()).rstrip("/") + + +def _resolve_registry_server(): + """Resolve the ACR login server used for Batch pool creation. + + ``AZURE_BATCH_REGISTRY_SERVER`` is canonical. Environments provisioned + before the rename set ``AZURE_BATCH_REGISTRY_SERVER_URL`` instead, so it is + still honored to keep those deployments working across the upgrade. + """ + server = os.getenv("AZURE_BATCH_REGISTRY_SERVER") + if not server: + legacy = os.getenv("AZURE_BATCH_REGISTRY_SERVER_URL") + if not legacy: + return REGISTRY_SERVER_PLACEHOLDER + _logger.warning( + "AZURE_BATCH_REGISTRY_SERVER_URL is deprecated and will be " + "removed in a future release; rename this application setting " + "to AZURE_BATCH_REGISTRY_SERVER." + ) + server = legacy + return _strip_scheme(server) + class StorageType(Enum): """Enumeration of supported storage backend types. @@ -472,10 +510,7 @@ def _split_ids(raw, fallback): "AZURE_BATCH_MANAGE_POOLS", "true" ).lower() == "true", - "registry_server": os.getenv( - "AZURE_BATCH_REGISTRY_SERVER", - ".azurecr.io", - ), + "registry_server": _resolve_registry_server(), "registry_image": os.getenv( "AZURE_BATCH_REGISTRY_IMAGE", ".azurecr.io/:latest", diff --git a/hastelib/src/hastegeo/core/runners/azure_batch.py b/hastelib/src/hastegeo/core/runners/azure_batch.py index 0cb7686..f87a52c 100644 --- a/hastelib/src/hastegeo/core/runners/azure_batch.py +++ b/hastelib/src/hastegeo/core/runners/azure_batch.py @@ -22,6 +22,7 @@ EnvironmentSetting, ImageReference, JobAddParameter, + JobPatchParameter, JobState, OSDisk, OutputFile, @@ -47,6 +48,10 @@ generate_container_sas, ) from hastegeo.core.config import Config +from hastegeo.core.utils.batch_config import ( + BatchJobPoolMismatchError, + validate_batch_config, +) from hastegeo.core.utils.logs import Logger from tenacity import ( retry, @@ -141,6 +146,11 @@ def add_task( # NOTE: test workdir option to eliminate the cd into /app here command = command + # Validate before the first Batch call so a missing application + # setting is reported as itself, rather than as an opaque Azure error + # raised from deep inside pool creation. + validate_batch_config(self.batch_config, self.manage_pools) + # Capacity-aware routing (v2.1.0): pick the pool at submit time from the # ordered candidates (preference-first, spillover-second), then bind the # job to it. @@ -159,6 +169,16 @@ def add_task( self.logger.info( "Creating pool for job_id: %s and task_id: %s", job_id, task_id ) + # Both workload images must be whitelisted on the pool, matching + # infra/modules/batchPool.bicep; a pool created with only the + # training image cannot start imageryprep tasks. + registry_images = [] + for candidate in ( + self.batch_config["registry_image"], + self.batch_config["imageprep_docker_image"], + ): + if candidate and candidate not in registry_images: + registry_images.append(candidate) self.batch_cluster.create_pool_if_not_exists( self.batch_config["vm_size"], self.batch_config["vm_publisher"], @@ -168,7 +188,7 @@ def add_task( self.batch_config["target_dedicated_nodes"], self.batch_config["target_low_priority_nodes"], self.batch_config["registry_server"], - [self.batch_config["registry_image"]], + registry_images, self.batch_config["node_agent_sku_id"], ) self.logger.info( @@ -438,6 +458,7 @@ def create_job(self, job_id): self.wait_for_pool_to_be_ready() try: job = self.batch_client.job.get(job_id) + self._rebind_job_pool(job) if job.state != JobState.active: self.batch_client.job.enable(job_id) self.logger.info(f"Job {job_id} activated.") @@ -449,6 +470,37 @@ def create_job(self, job_id): self.batch_client.job.add(job) self.logger.info(f"Job {job_id} created.") + def _rebind_job_pool(self, job): + # A job keeps the pool it was first created against, so reusing a job id + # would pin every later task to that original pool -- defeating + # capacity-aware routing, and failing outright once that pool is renamed + # or deleted. Re-point the job at the pool selected for this submission. + bound_pool = getattr(job.pool_info, "pool_id", None) + if not bound_pool or bound_pool == self.pool_id: + return + self.logger.info( + "Job %s is bound to pool %s; rebinding to selected pool %s.", + job.id, + bound_pool, + self.pool_id, + ) + try: + self.batch_client.job.patch( + job.id, + JobPatchParameter( + pool_info=PoolInformation(pool_id=self.pool_id) + ), + ) + except BatchErrorException as e: + # Batch refuses the change while tasks are active. Surface it rather + # than silently submitting into a job on the wrong pool. + raise BatchJobPoolMismatchError( + f"Job {job.id} is bound to pool {bound_pool} but this task " + f"targets pool {self.pool_id}, and rebinding failed " + f"({getattr(e.error, 'code', e)}). Delete or drain the job, " + "or set a distinct *_BATCH_JOB_ID." + ) from e + def add_task( self, job_id, diff --git a/hastelib/src/hastegeo/core/utils/batch_config.py b/hastelib/src/hastegeo/core/utils/batch_config.py new file mode 100644 index 0000000..fdb792c --- /dev/null +++ b/hastelib/src/hastegeo/core/utils/batch_config.py @@ -0,0 +1,77 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +"""Fail-fast validation of the Azure Batch settings block. + +``Config.get_azure_batch_config()`` falls back to ```` strings +when an application setting is absent, so a missing setting is not detected +until Azure rejects it — surfacing as an opaque API error far from the cause +(e.g. ``InvalidPropertyValue`` on ``registryServer``). Validating before the +first Batch call lets the failure name the setting that is actually missing. +""" + +import re + +PLACEHOLDER_PATTERN = re.compile(r"<[^<>]+>") + +# Settings the runner needs for any submission, mapped to the application +# setting that supplies each one. +ALWAYS_REQUIRED = { + "account_name": "AZURE_BATCH_ACCOUNT_NAME", + "batch_url": "AZURE_BATCH_URL", + "output_container_url": "AZURE_BATCH_OUTPUT_CONTAINER_URL", +} + +# Only read when the runner creates or resizes its own pool; environments on +# pre-created (IaC/autoscale) pools never touch these. +POOL_MANAGEMENT_REQUIRED = { + "registry_server": "AZURE_BATCH_REGISTRY_SERVER", + "registry_image": "AZURE_BATCH_REGISTRY_IMAGE", + "user_assigned_identity_resource_id": ( + "AZURE_BATCH_REGISTRY_IDENTITY_RESOURCE_ID" + ), +} + + +class BatchConfigurationError(RuntimeError): + """Raised when a required Azure Batch application setting is missing.""" + + +class BatchJobPoolMismatchError(RuntimeError): + """Raised when a reused Batch job is pinned to a different pool.""" + + +def has_placeholder(value): + """True when ``value`` still contains an unresolved ``<...>`` default.""" + return isinstance(value, str) and bool(PLACEHOLDER_PATTERN.search(value)) + + +def validate_batch_config(batch_config, manage_pools=None): + """Raise ``BatchConfigurationError`` if a required setting is unresolved. + + Args: + batch_config: The dict from ``Config.get_azure_batch_config()``. + manage_pools: Whether the runner manages its own pool. Defaults to the + ``manage_pools`` entry of ``batch_config``. When false, the + pool-creation settings are not required. + """ + if manage_pools is None: + manage_pools = batch_config.get("manage_pools", True) + + required = dict(ALWAYS_REQUIRED) + if manage_pools: + required.update(POOL_MANAGEMENT_REQUIRED) + + unresolved = [] + for key, env_var in sorted(required.items(), key=lambda kv: kv[1]): + value = batch_config.get(key) + if not value or has_placeholder(value): + unresolved.append(f"{env_var} (resolved to {value!r})") + + if unresolved: + raise BatchConfigurationError( + "Azure Batch is not configured. Missing or unresolved " + "application settings: " + + "; ".join(unresolved) + + ". Set these on the Function App and restart it." + ) diff --git a/hastelib/tests/core/utils/test_batch_config.py b/hastelib/tests/core/utils/test_batch_config.py new file mode 100644 index 0000000..aca64b0 --- /dev/null +++ b/hastelib/tests/core/utils/test_batch_config.py @@ -0,0 +1,171 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. + +"""Unit tests for Azure Batch application-setting resolution + validation. + +Covers the drift between the settings the code reads and the settings the +deploy paths emit: +- the AZURE_BATCH_REGISTRY_SERVER rename (with legacy fallback + normalization), +- fail-fast validation of unresolved ```` defaults, +- rebinding a reused Batch job that is pinned to a stale pool. +""" + +from unittest.mock import MagicMock + +import pytest +from azure.batch.models import BatchErrorException, JobState +from hastegeo.core.config import REGISTRY_SERVER_PLACEHOLDER, Config +from hastegeo.core.runners.azure_batch import AzureBatchJob +from hastegeo.core.utils.batch_config import ( + BatchConfigurationError, + BatchJobPoolMismatchError, + has_placeholder, + validate_batch_config, +) + +REGISTRY_ENV = "AZURE_BATCH_REGISTRY_SERVER" +LEGACY_REGISTRY_ENV = "AZURE_BATCH_REGISTRY_SERVER_URL" + + +def _registry_server(monkeypatch, new=None, legacy=None): + for name, value in ((REGISTRY_ENV, new), (LEGACY_REGISTRY_ENV, legacy)): + if value is None: + monkeypatch.delenv(name, raising=False) + else: + monkeypatch.setenv(name, value) + return Config().get_azure_batch_config()["registry_server"] + + +def test_registry_server_uses_canonical_setting(monkeypatch): + assert _registry_server(monkeypatch, new="acr.azurecr.io") == ( + "acr.azurecr.io" + ) + + +def test_registry_server_falls_back_to_legacy_setting(monkeypatch): + # Environments provisioned before the rename only have the _URL name. + assert _registry_server(monkeypatch, legacy="https://acr.azurecr.io") == ( + "acr.azurecr.io" + ) + + +def test_registry_server_prefers_canonical_over_legacy(monkeypatch): + resolved = _registry_server( + monkeypatch, new="new.azurecr.io", legacy="https://old.azurecr.io" + ) + assert resolved == "new.azurecr.io" + + +def test_registry_server_strips_scheme_and_trailing_slash(monkeypatch): + # The Batch SDK wants a bare login server, but the app setting is often a + # URL -- normalize either shape. + assert _registry_server(monkeypatch, new="https://acr.azurecr.io/") == ( + "acr.azurecr.io" + ) + + +def test_registry_server_falls_back_to_placeholder(monkeypatch): + assert _registry_server(monkeypatch) == REGISTRY_SERVER_PLACEHOLDER + + +def test_has_placeholder_detects_unresolved_defaults(): + assert has_placeholder(".azurecr.io") + assert not has_placeholder("acr.azurecr.io") + assert not has_placeholder(None) + + +def _configured(**overrides): + config = { + "account_name": "acct", + "batch_url": "https://acct.westus2.batch.azure.com", + "output_container_url": "https://sa.blob.core.windows.net/data", + "registry_server": "acr.azurecr.io", + "registry_image": "acr.azurecr.io/hastetraining:1", + "user_assigned_identity_resource_id": "/subscriptions/x/umi", + } + config.update(overrides) + return config + + +def test_validate_passes_on_fully_configured_block(): + validate_batch_config(_configured(), manage_pools=True) + + +def test_validate_raises_and_names_the_missing_setting(): + config = _configured(registry_server=REGISTRY_SERVER_PLACEHOLDER) + with pytest.raises(BatchConfigurationError) as excinfo: + validate_batch_config(config, manage_pools=True) + assert REGISTRY_ENV in str(excinfo.value) + + +def test_validate_skips_pool_settings_when_not_managing_pools(): + # Pre-created/autoscale pools never read the registry settings, so an + # unresolved value there must not block submission. + config = _configured(registry_server=REGISTRY_SERVER_PLACEHOLDER) + validate_batch_config(config, manage_pools=False) + + +def test_validate_still_requires_core_settings_without_pool_management(): + config = _configured(account_name="") + with pytest.raises(BatchConfigurationError) as excinfo: + validate_batch_config(config, manage_pools=False) + assert "AZURE_BATCH_ACCOUNT_NAME" in str(excinfo.value) + + +def test_validate_reads_manage_pools_from_the_config_block(): + config = _configured( + registry_server=REGISTRY_SERVER_PLACEHOLDER, manage_pools=False + ) + validate_batch_config(config) + + +def _job(pool_id="selected-pool"): + job = AzureBatchJob( + account_name="acct", # pragma: allowlist secret + account_key="key", # pragma: allowlist secret + batch_url="https://acct.westus2.batch.azure.com", + pool_id=pool_id, + user_assigned_identity_resource_id="/subscriptions/x/umi", + manage_pools=False, + ) + job.batch_client = MagicMock() + return job + + +def _existing_job(bound_pool, state=JobState.active): + existing = MagicMock() + existing.id = "job-1" + existing.state = state + existing.pool_info.pool_id = bound_pool + return existing + + +def test_create_job_rebinds_a_job_pinned_to_a_stale_pool(): + job = _job(pool_id="selected-pool") + job.batch_client.job.get.return_value = _existing_job("deleted-pool") + + job.create_job("job-1") + + job.batch_client.job.patch.assert_called_once() + patched = job.batch_client.job.patch.call_args[0][1] + assert patched.pool_info.pool_id == "selected-pool" + + +def test_create_job_does_not_rebind_when_pool_already_matches(): + job = _job(pool_id="selected-pool") + job.batch_client.job.get.return_value = _existing_job("selected-pool") + + job.create_job("job-1") + + job.batch_client.job.patch.assert_not_called() + + +def test_create_job_raises_when_rebinding_is_refused(): + job = _job(pool_id="selected-pool") + job.batch_client.job.get.return_value = _existing_job("deleted-pool") + error = BatchErrorException(lambda *a, **k: None, MagicMock()) + error.error = MagicMock(code="JobStateInvalid") + job.batch_client.job.patch.side_effect = error + + with pytest.raises(BatchJobPoolMismatchError): + job.create_job("job-1") diff --git a/infra/main.json b/infra/main.json index 052568f..ba27855 100644 --- a/infra/main.json +++ b/infra/main.json @@ -5,8 +5,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "12934134840547041152" + "version": "0.45.15.27210", + "templateHash": "7848471694437751621" } }, "parameters": { @@ -280,8 +280,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "9380957498639667481" + "version": "0.45.15.27210", + "templateHash": "6467876281697702392" } }, "parameters": { @@ -363,8 +363,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "16963761547890363884" + "version": "0.45.15.27210", + "templateHash": "12082330447741191629" } }, "parameters": { @@ -457,8 +457,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "9880370881471562028" + "version": "0.45.15.27210", + "templateHash": "5477339931418214373" } }, "parameters": { @@ -672,8 +672,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "16630728933207302616" + "version": "0.45.15.27210", + "templateHash": "15711586232981596113" } }, "parameters": { @@ -923,8 +923,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "2299531501285707142" + "version": "0.45.15.27210", + "templateHash": "11423466976742084469" } }, "parameters": { @@ -1076,8 +1076,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "16571433796004974762" + "version": "0.45.15.27210", + "templateHash": "2129528397301155805" } }, "parameters": { @@ -1315,8 +1315,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "1968129085219592897" + "version": "0.45.15.27210", + "templateHash": "11668276010925679796" } }, "parameters": { @@ -1567,6 +1567,10 @@ "name": "ZIP_QUEUE_NAME", "value": "zip-queue" }, + { + "name": "EMBEDDING_QUEUE_NAME", + "value": "embedding-queue" + }, { "name": "IMAGERY_STORAGE_TYPE", "value": "blob" @@ -1644,8 +1648,8 @@ "value": "[parameters('batchPoolName')]" }, { - "name": "AZURE_BATCH_REGISTRY_SERVER_URL", - "value": "[format('https://{0}', parameters('acrLoginServer'))]" + "name": "AZURE_BATCH_REGISTRY_SERVER", + "value": "[parameters('acrLoginServer')]" }, { "name": "AZURE_BATCH_REGISTRY_IMAGE", @@ -1711,8 +1715,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "12545021642088417436" + "version": "0.45.15.27210", + "templateHash": "11828726585656867764" } }, "parameters": { @@ -2009,8 +2013,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "12545021642088417436" + "version": "0.45.15.27210", + "templateHash": "11828726585656867764" } }, "parameters": { @@ -2330,6 +2334,10 @@ "name": "ZIP_QUEUE_NAME", "value": "zip-queue" }, + { + "name": "EMBEDDING_QUEUE_NAME", + "value": "embedding-queue" + }, { "name": "IMAGERY_STORAGE_TYPE", "value": "blob" @@ -2407,8 +2415,8 @@ "value": "[parameters('batchPoolName')]" }, { - "name": "AZURE_BATCH_REGISTRY_SERVER_URL", - "value": "[format('https://{0}', parameters('acrLoginServer'))]" + "name": "AZURE_BATCH_REGISTRY_SERVER", + "value": "[parameters('acrLoginServer')]" }, { "name": "AZURE_BATCH_REGISTRY_IMAGE", @@ -2474,8 +2482,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "12545021642088417436" + "version": "0.45.15.27210", + "templateHash": "11828726585656867764" } }, "parameters": { @@ -2790,8 +2798,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "15152592237032588221" + "version": "0.45.15.27210", + "templateHash": "7962398216361329093" } }, "parameters": { @@ -2923,8 +2931,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "6825195985756767924" + "version": "0.45.15.27210", + "templateHash": "13617399659586966284" } }, "parameters": { @@ -3245,8 +3253,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "6001236453914961257" + "version": "0.45.15.27210", + "templateHash": "10647381514548241981" } }, "parameters": { @@ -3360,8 +3368,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "16832494055114601667" + "version": "0.45.15.27210", + "templateHash": "16042433164648747777" } }, "parameters": { @@ -3459,8 +3467,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "14366334891975298585" + "version": "0.45.15.27210", + "templateHash": "13664756888266805551" } }, "parameters": { @@ -3559,7 +3567,8 @@ "variables": { "registryServer": "[format('{0}.azurecr.io', parameters('acrName'))]", "scaleTargetVar": "[if(equals(parameters('nodeType'), 'LowPriority'), '$TargetLowPriorityNodes', '$TargetDedicatedNodes')]", - "autoscaleFormula": "[format('$samples = $ActiveTasks.GetSamplePercent(TimeInterval_Minute * 15);$tasks = $samples < 70 ? max(0, $ActiveTasks.GetSample(1)) : max($ActiveTasks.GetSample(1), avg($ActiveTasks.GetSample(TimeInterval_Minute * 15)));$targetVMs = $tasks > 0 ? $tasks : {0};{1} = max({2}, min($targetVMs, {3}));$NodeDeallocationOption = taskcompletion;', parameters('minNodes'), variables('scaleTargetVar'), parameters('minNodes'), parameters('maxNodes'))]", + "otherScaleTargetVar": "[if(equals(parameters('nodeType'), 'LowPriority'), '$TargetDedicatedNodes', '$TargetLowPriorityNodes')]", + "autoscaleFormula": "[format('$samples = $ActiveTasks.GetSamplePercent(TimeInterval_Minute * 15);$tasks = $samples < 70 ? max(0, $ActiveTasks.GetSample(1)) : max($ActiveTasks.GetSample(1), avg($ActiveTasks.GetSample(TimeInterval_Minute * 15)));$targetVMs = $tasks > 0 ? $tasks : {0};{1} = max({2}, min($targetVMs, {3}));{4} = 0;$NodeDeallocationOption = taskcompletion;', parameters('minNodes'), variables('scaleTargetVar'), parameters('minNodes'), parameters('maxNodes'), variables('otherScaleTargetVar'))]", "scaleSettings": "[if(equals(parameters('scaleMode'), 'Fixed'), createObject('fixedScale', createObject('targetDedicatedNodes', if(equals(parameters('nodeType'), 'Dedicated'), parameters('fixedNodeCount'), 0), 'targetLowPriorityNodes', if(equals(parameters('nodeType'), 'LowPriority'), parameters('fixedNodeCount'), 0), 'resizeTimeout', 'PT15M')), createObject('autoScale', createObject('formula', variables('autoscaleFormula'), 'evaluationInterval', 'PT5M')))]" }, "resources": [ @@ -3615,8 +3624,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "15881723511064074666" + "version": "0.45.15.27210", + "templateHash": "17391401701000584075" } }, "parameters": { @@ -3683,8 +3692,8 @@ "metadata": { "_generator": { "name": "bicep", - "version": "0.43.8.12551", - "templateHash": "18374080558664767600" + "version": "0.45.15.27210", + "templateHash": "3909520050142538177" } }, "parameters": { diff --git a/infra/modules/functions.bicep b/infra/modules/functions.bicep index 63ea211..e58914c 100644 --- a/infra/modules/functions.bicep +++ b/infra/modules/functions.bicep @@ -111,6 +111,7 @@ var appConfigSettings = [ { name: 'STATS_QUEUE_NAME', value: 'stats-queue' } { name: 'TRAIN_QUEUE_NAME', value: 'train-queue' } { name: 'ZIP_QUEUE_NAME', value: 'zip-queue' } + { name: 'EMBEDDING_QUEUE_NAME', value: 'embedding-queue' } { name: 'IMAGERY_STORAGE_TYPE', value: 'blob' } { name: 'METADATA_STORAGE_TYPE', value: 'blob' } { name: 'ARTIFACT_STORAGE_TYPE', value: 'blob' } @@ -133,7 +134,7 @@ var appConfigSettings = [ { name: 'AZURE_BATCH_OUTPUT_CONTAINER_URL', value: '${storageAccount.properties.primaryEndpoints.blob}data' } { name: 'AZURE_BATCH_TRAINING_POOL_ID', value: batchPoolName } { name: 'AZURE_BATCH_IMAGERYPREP_POOL_ID', value: batchPoolName } - { name: 'AZURE_BATCH_REGISTRY_SERVER_URL', value: 'https://${acrLoginServer}' } + { name: 'AZURE_BATCH_REGISTRY_SERVER', value: acrLoginServer } { name: 'AZURE_BATCH_REGISTRY_IMAGE', value: '${acrLoginServer}/${trainingImage}' } { name: 'AZURE_BATCH_REGISTRY_IDENTITY_RESOURCE_ID', value: umiResourceId } { name: 'STATIC_APP_SUBSCRIPTION_ID', value: subscription().subscriptionId } diff --git a/local.settings.example.jsonc b/local.settings.example.jsonc index 5527c97..ba32276 100644 --- a/local.settings.example.jsonc +++ b/local.settings.example.jsonc @@ -27,6 +27,17 @@ "AZURE_BATCH_TRAINING_POOL_ID": "", "AZURE_BATCH_IMAGERYPREP_POOL_ID": "", + // ===== OPTIONAL: shared multi-tenant pools (see docs/configuration.md) ===== + // Ordered candidate pools for capacity-aware routing. Leave unset to use + // the single *_POOL_ID above. Set AZURE_BATCH_MANAGE_POOLS to false for + // pre-created autoscale pools, and AZURE_BATCH_USE_SAS to true so blob I/O + // uses a per-job SAS instead of the pool's managed identity. + // "AZURE_BATCH_TRAINING_POOL_IDS": ",", + // "AZURE_BATCH_INFERENCE_POOL_IDS": ",", + // "AZURE_BATCH_IMAGERYPREP_POOL_IDS": ",", + // "AZURE_BATCH_USE_SAS": "false", + // "AZURE_BATCH_MANAGE_POOLS": "true", + // ===== REQUIRED: Queue Names ===== // Use unique names for local development to avoid conflicts "IMAGE_QUEUE_NAME": "my-local-image-queue", @@ -34,11 +45,12 @@ "INFERENCE_QUEUE_NAME": "my-local-inference-queue", "STATS_QUEUE_NAME": "my-local-stats-queue", "ZIP_QUEUE_NAME": "my-local-zip-queue", + "EMBEDDING_QUEUE_NAME": "my-local-embedding-queue", // ===== REQUIRED: Docker Images ===== "AZURE_BATCH_DOCKER_IMAGE": ".azurecr.io/hastetraining:", "AZURE_BATCH_IMAGERYPREP_DOCKER_IMAGE": ".azurecr.io/hasteimageryprep:", - "AZURE_BATCH_REGISTRY_SERVER_URL": "https://.azurecr.io", + "AZURE_BATCH_REGISTRY_SERVER": ".azurecr.io", // ===== REQUIRED: Local Development Paths ===== "DATA_PATH": "/absolute/path/to/haste/localtmp/data", diff --git a/spec/features/batch-config-drift/README.md b/spec/features/batch-config-drift/README.md new file mode 100644 index 0000000..f1cd7de --- /dev/null +++ b/spec/features/batch-config-drift/README.md @@ -0,0 +1,72 @@ +# Batch configuration drift + +**Status:** in-progress +**Type:** modification (fix) +**Related:** [`../batch-compute-expansion/`](../batch-compute-expansion/) — the +change whose deploy-path wiring was incomplete. + +## Problem + +Image-layer creation failed in a deployment environment before any task reached +Azure Batch: + +``` +Code: InvalidPropertyValue +PropertyName: registryServer +PropertyValue: .azurecr.io +Reason: The specified registry is an invalid docker registry server name +``` + +`.azurecr.io` is `hastegeo`'s placeholder default, so the real +setting never reached the running app. + +## Root causes + +| # | Cause | +|---|---| +| RC1 | `Config` reads `AZURE_BATCH_REGISTRY_SERVER`; `deploy_apps.sh`, `functions.bicep` and `local.settings.example.jsonc` all emitted `AZURE_BATCH_REGISTRY_SERVER_URL`, which no code read. | +| RC2 | `deploy_apps.sh` never emitted the shared-pool settings introduced with the multi-tenant pools (`AZURE_BATCH_*_POOL_IDS`, `AZURE_BATCH_USE_SAS`, `AZURE_BATCH_MANAGE_POOLS`), so environments deployed through it ran the legacy single-pool path against renamed pools. | +| RC3 | Placeholder defaults turned a missing setting into an opaque Azure error raised deep inside pool creation. | +| RC4 | Nothing compared the variables the code reads against the variables the deploy paths emit, so the drift was invisible until an environment broke. | +| RC5 | `create_job` re-enabled an existing job without re-pointing it, leaving jobs pinned to the pool that first created them — and permanently broken once that pool was deleted. | +| RC6 | `EMBEDDING_QUEUE_NAME` was emitted by neither Azure deploy path. | + +## Scope + +- `hastelib/src/hastegeo/core/config.py` — dual-read + normalize the registry + server setting. +- `hastelib/src/hastegeo/core/utils/batch_config.py` *(new)* — fail-fast + validation naming the missing application setting. +- `hastelib/src/hastegeo/core/runners/azure_batch.py` — validate before + submitting, rebind stale job/pool bindings, whitelist both container images. +- `.github/scripts/deploy_apps.sh` + `.github/workflows/deploy-apps.yml` — + emit the full setting set, with per-environment pool overrides. +- `infra/modules/functions.bicep` (+ regenerated `infra/main.json`). +- `.github/scripts/check_env_drift.py` + `.github/workflows/config-drift.yml` + *(new)* — CI guard. +- `local.settings.example.jsonc`, `docs/configuration.md`, + `docs/api/hastefuncqueues.md`, `api/hastefuncqueues/README.md`, `CHANGELOG.md`. + +## Non-goals + +- Consolidating the two divergent deploy paths (`deploy_apps.sh` vs + `infra/main.bicep`). Tracked in + [`../infra-iac-migration/`](../infra-iac-migration/). +- Migrating `azure-batch` to the 15.x track-2 SDK. + +## Agent assignment + +| Area | Implements | Validates | +|---|---|---| +| `hastelib/`, `.github/`, `infra/` | `backend-dev` | `backend-validation` | +| Docs + spec | `backend-dev` | `orchestrator` | + +## Acceptance criteria + +1. `AZURE_BATCH_REGISTRY_SERVER` resolves from the canonical name, falls back to + the legacy `_URL` name, and strips any scheme. +2. Submitting with an unresolved placeholder raises an error naming the + application setting, before any Batch API call. +3. Both deploy paths emit every required setting. +4. `check_env_drift.py` exits non-zero on the pre-fix tree and zero after. +5. A job bound to a stale pool is rebound to the selected pool. diff --git a/spec/features/batch-config-drift/design.md b/spec/features/batch-config-drift/design.md new file mode 100644 index 0000000..ba51d37 --- /dev/null +++ b/spec/features/batch-config-drift/design.md @@ -0,0 +1,96 @@ +# Design — Batch configuration drift + +## 1. Setting resolution (`config.py`) + +`registry_server` is resolved through a helper rather than a bare `os.getenv`: + +``` +AZURE_BATCH_REGISTRY_SERVER -> used as-is (scheme stripped) + else AZURE_BATCH_REGISTRY_SERVER_URL -> used + deprecation warning logged + else ".azurecr.io" -> placeholder, caught by validation +``` + +Normalization strips any `http(s)://` prefix and trailing `/`, because Azure +Batch's `ContainerRegistry.registry_server` expects a bare login server +(`myacr.azurecr.io`) while the legacy setting stored a URL. + +Dual-reading — rather than a clean break — keeps partner environments working +across the upgrade: they hold only the old setting until an operator renames it. + +## 2. Fail-fast validation (`utils/batch_config.py`) + +`validate_batch_config(batch_config, manage_pools=None)` raises +`BatchConfigurationError` listing every required setting that is empty or still +contains a ``, mapped back to the application setting name an +operator would set. + +Two tiers, because the requirement is conditional: + +| Tier | Settings | When required | +|---|---|---| +| Always | `AZURE_BATCH_ACCOUNT_NAME`, `AZURE_BATCH_URL`, `AZURE_BATCH_OUTPUT_CONTAINER_URL` | every submission | +| Pool management | `AZURE_BATCH_REGISTRY_SERVER`, `AZURE_BATCH_REGISTRY_IMAGE`, `AZURE_BATCH_REGISTRY_IDENTITY_RESOURCE_ID` | only when `AZURE_BATCH_MANAGE_POOLS` is true | + +Environments on pre-created autoscale pools never read the registry settings, so +requiring them unconditionally would break working deployments. + +**Call site.** Validation runs at the start of `AzureBatchRunner.add_task`, not +`__init__`. The runner is constructed eagerly by every `ImageryProcessor`, +including on read-only status endpoints; validating in the constructor would +fail unrelated requests. `add_task` is the actual submission boundary, and the +queue trigger's exception handler writes the message to the image layer's +`statusMessage`, so the operator sees the missing setting in the UI. + +## 3. Job/pool rebinding (`runners/azure_batch.py`) + +Batch job ids default to the configured pool id, so a job outlives the pool +whose name it borrowed. `create_job` previously did: + +``` +job exists -> enable it # keeps the ORIGINAL pool binding +job absent -> create bound to pool +``` + +That silently defeats capacity-aware routing (the job stays on whichever pool +created it, ignoring `select_pool`) and hard-fails once the original pool is +renamed or deleted — tasks queue into a job bound to a pool that no longer +exists. `_rebind_job_pool` now patches `pool_info` when the existing binding +differs from the selected pool, and raises `BatchJobPoolMismatchError` when +Batch refuses (e.g. active tasks) rather than submitting to the wrong pool. + +## 4. Deploy-path parity + +`deploy_apps.sh` gains the missing settings. Pool wiring is overridable per +environment via GitHub Environment variables, with defaults that reproduce the +previous single-pool behavior: + +| Script variable | Application setting | Default | +|---|---|---| +| `BATCH_TRAINING_POOL_ID` | `AZURE_BATCH_TRAINING_POOL_ID` | derived `-haste--pool` | +| `BATCH_IMAGERYPREP_POOL_ID` | `AZURE_BATCH_IMAGERYPREP_POOL_ID` | derived | +| `BATCH_TRAINING_POOL_IDS` | `AZURE_BATCH_TRAINING_POOL_IDS` | empty | +| `BATCH_INFERENCE_POOL_IDS` | `AZURE_BATCH_INFERENCE_POOL_IDS` | empty | +| `BATCH_IMAGERYPREP_POOL_IDS` | `AZURE_BATCH_IMAGERYPREP_POOL_IDS` | empty | +| `BATCH_USE_SAS` | `AZURE_BATCH_USE_SAS` | `false` | +| `BATCH_MANAGE_POOLS` | `AZURE_BATCH_MANAGE_POOLS` | `true` | + +The derived pool name is retained as the default only for backward +compatibility; environments on shared pools must set the overrides, because the +singular id also names the Batch job. + +## 5. CI guard (`check_env_drift.py`) + +Parses the code with `ast` (not regex) to find every `os.getenv` / +`os.environ.get` / `os.environ[...]` read, classifying a variable as **required** +when it has no default or its default still contains a ``. It then +compares that set against the settings emitted by `deploy_apps.sh` (scoped to +the `appsettings set` block, so resource tags are not misread as settings) and +`functions.bicep`, and fails when a required variable is missing from either. + +It also reports the inverse — settings emitted by a deploy path that no code +reads — which is the specific signature of a half-applied rename, and is what +made `AZURE_BATCH_REGISTRY_SERVER_URL` detectable. + +Genuinely optional variables (alternative storage backends, the local Docker +runner, container-side GDAL tuning, platform-provided values, and the +deliberately unemitted legacy fallback) are exempt via a documented `ALLOWLIST`.