Skip to content

fix(batch): sync Batch application settings across code and deploy paths - #98

Merged
jQuinRivero merged 1 commit into
mainfrom
v-joaquinri/fix-batch-config-drift
Jul 27, 2026
Merged

fix(batch): sync Batch application settings across code and deploy paths#98
jQuinRivero merged 1 commit into
mainfrom
v-joaquinri/fix-batch-config-drift

Conversation

@jQuinRivero

@jQuinRivero jQuinRivero commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Problem

Image-layer creation failed in a deployment environment before any task reached Azure Batch:

Code: InvalidPropertyValue
PropertyName:  registryServer
PropertyValue: <registry-name>.azurecr.io
Reason: The specified registry is an invalid docker registry server name

<registry-name>.azurecr.io is hastegeo's placeholder default — proof the real setting never reached the running app.

Root causes

1. AZURE_BATCH_REGISTRY_SERVER was read by code but emitted by nothing.

Surface Variable
hastelib/src/hastegeo/core/config.py reads AZURE_BATCH_REGISTRY_SERVER
.github/scripts/deploy_apps.sh emitted AZURE_BATCH_REGISTRY_SERVER_URL
infra/modules/functions.bicep emitted AZURE_BATCH_REGISTRY_SERVER_URL
local.settings.example.jsonc emitted AZURE_BATCH_REGISTRY_SERVER_URL

AZURE_BATCH_REGISTRY_SERVER_URL was read by zero lines of Python. Because deploy_apps.sh re-sets the whole app-settings block on every run, a manual portal fix is clobbered by the next deploy.

2. deploy_apps.sh never emitted the shared-pool settings. The multi-tenant Batch pool work added five settings that functions.bicep sets but deploy_apps.sh — the script behind deploy-apps.yml — did not: AZURE_BATCH_{TRAINING,INFERENCE,IMAGERYPREP}_POOL_IDS, AZURE_BATCH_USE_SAS, AZURE_BATCH_MANAGE_POOLS. Environments deployed through it silently ran the legacy single-pool path against renamed pools. EMBEDDING_QUEUE_NAME was missing from both paths.

3. 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 — it never re-pointed it. So capacity-aware spillover was silently ineffective, and once a pool was renamed or deleted, every task queued into a job bound to a pool that no longer existed.

Changes

Code

  • config.py — resolve AZURE_BATCH_REGISTRY_SERVER, falling back to the deprecated _URL name and stripping any scheme (the Batch SDK wants a bare login server; the legacy setting stored a URL). Dual-read rather than a clean break, so existing deployments keep working until an operator renames the setting.
  • new utils/batch_config.py — fail fast naming the unset application setting instead of surfacing an opaque Azure error from inside pool creation. Two tiers: registry settings are only required when AZURE_BATCH_MANAGE_POOLS is true, so environments on pre-created autoscale pools aren't broken.
  • runners/azure_batch.py — validate at the submission boundary (add_task, not __init__ — the runner is constructed eagerly on read-only status paths); rebind a stale job/pool binding; whitelist both training and imageryprep images on created pools, matching batchPool.bicep.

Deploy paths

  • deploy_apps.sh + deploy-apps.yml — emit the full setting set, with per-environment pool overrides that default to the previous behavior.
  • functions.bicep (+ regenerated main.json), local.settings.example.jsonc.

Guard

  • new check_env_drift.py + config-drift.yml — parses the code with ast to find every env read, classifies a variable as required when it has no default or its default is still a <placeholder>, and fails when a deploy path is missing it. Also reports the inverse (settings emitted but read by nothing) — the exact signature of a half-applied rename.

Verification

  • The guard catches both root causes on pre-fix main (validated in a clean worktree): flags AZURE_BATCH_REGISTRY_SERVER and all three *_POOL_IDS, exit 1. Passes on this branch, exit 0.
  • 13 new unit tests covering setting precedence, scheme normalization, placeholder validation (both tiers), and job rebinding. 101 passed, 7 skipped locally; the only non-passing modules need dev deps absent from my local env (pytest-mock, geopandas, shapely).
  • pre-commit run clean on all changed files (black, isort, flake8, detect-secrets).
  • az bicep build regenerates main.json; diff is the two intended settings plus generator-version churn.

Operator action required

Existing deployments should rename the AZURE_BATCH_REGISTRY_SERVER_URL application setting to AZURE_BATCH_REGISTRY_SERVER (value: bare login server, e.g. myacr.azurecr.io) on the api and queues Function Apps. The legacy name still works until then.

Also 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, so a mismatch means the api app reads status from a job the queues app never wrote to.

Notes

  • spec/features/batch-config-drift/ documents the root causes and design; cross-references batch-compute-expansion, whose deploy wiring was incomplete.
  • Consolidating the two divergent deploy paths (deploy_apps.sh vs infra/main.bicep) is out of scope — tracked in spec/features/infra-iac-migration/.

@jQuinRivero
jQuinRivero requested a review from a team as a code owner July 27, 2026 13:40
`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 `<registry-name>.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
@jQuinRivero
jQuinRivero force-pushed the v-joaquinri/fix-batch-config-drift branch from 8637f9d to 1cc4ee3 Compare July 27, 2026 13:48

@prbatero prbatero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jQuinRivero
jQuinRivero merged commit 2e9e938 into main Jul 27, 2026
13 checks passed
@jQuinRivero
jQuinRivero deleted the v-joaquinri/fix-batch-config-drift branch July 27, 2026 14:02
@github-actions

Copy link
Copy Markdown

RC artifacts ready

All branch deployment references use the same RC tag:

  • hastegeo_version: 1.0.27rc5
  • training_image_tag: 1.0.27rc5
  • imageprep_image_tag: 1.0.27rc5
  • wheel: hastegeo-1.0.27rc5-py3-none-any.whl

jQuinRivero added a commit that referenced this pull request Jul 28, 2026
* fix(batch): scope Batch job ids to the selected pool

Capacity-aware spillover failed whenever two tasks were in flight:

    Job <h100-pool> is bound to pool <h100-pool> but this task targets pool
    <t4-pool>, and rebinding failed (OperationInvalidForCurrentState).

Three properties collide. The Batch job id is static (it defaults to the
singular pool id), the pool is chosen per task by select_pool, and a Batch job
is permanently bound to the pool it was created against -- re-pointable only
while it has no active tasks. So the first task created a job on the preferred
pool, and a second task that spilled over to another pool collided with it.
Spillover broke in exactly the concurrent case it exists to serve.

Rebinding, added in #98, was the wrong remedy: one static job id cannot span
multiple pools, and once concurrency exists it cannot be rebound at all.

- new resolve_job_id(): derive the job id from the routed pool, so each pool
  gets its own job. Environments with a single candidate pool keep their
  existing job id, so no jobs are renamed there. Truncation to the 64-char
  Batch limit removes characters from the base, never the pool, so two pools
  can never collapse onto the same id.
- create_job() is now graceful and returns the job id it actually used: reuse
  when the binding matches, rebind when the job is idle, otherwise fall back to
  a pool-scoped job instead of failing the submission.
- processors capture the returned job id. All five persist jobId into metadata,
  which is later used for status polling and log retrieval, so the id the
  runner actually used has to be the one recorded.
- drop BatchJobPoolMismatchError, now unreachable.

* fix(deploy): store Batch pool config as secrets and suppress settings dump

Pool ids were wired through GitHub Environment *variables*, but this repo is
public and Actions logs are public with it. GitHub masks secret values in logs
and does not mask variables, and `az functionapp config appsettings set` was
printing the resulting settings JSON to stdout, so the values would have landed
in a public log.

Store them as environment secrets instead, matching how RESOURCE_PREFIX,
LOCATION, RESOURCE_SUFFIX and STATIC_APP_DOMAIN are already handled, and add
`--output none` so the app-settings dump is not emitted at all.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd98b384-7e26-47a9-8c03-6f9594eb611c

---------

Co-authored-by: Joaquin Rivero <v-joaquinri@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd98b384-7e26-47a9-8c03-6f9594eb611c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants