Fix/azure ci - #280
Closed
l3abak wants to merge 23 commits into
Closed
Conversation
auto-merge was automatically disabled
August 17, 2026 11:25
Rebase failed
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends job runner configuration capabilities across multiple job handlers by introducing optional overrides for Radix image tags and configurable compute resources for Azure Container Instances, while also aligning local-container environment variable injection with the Azure handler’s behavior.
Changes:
- Add optional
imageTagNameoverride support for the Radix job handler and its blueprint. - Add optional
computeResource(CPU/memory) to the Container blueprint and implement it in the Azure Container Instances handler. - Update the local container handler’s
environmentVariableshandling to support name-based lookup (in addition to legacyNAME=valueentries).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/job_handler_plugins/radix/init.py | Sends an optional imageTagName alongside the Radix job payload. |
| src/job_handler_plugins/local_container/init.py | Changes env-var injection to support env var names (and legacy NAME=value strings). |
| src/job_handler_plugins/azure_container_instances/init.py | Adds support for computeResource CPU/memory overrides and updates defaults. |
| app/data/WorkflowDS/Blueprints/Radix.json | Adds optional imageTagName attribute to the Radix runner blueprint. |
| app/data/WorkflowDS/Blueprints/Container.json | Adds optional computeResource attribute to the Container blueprint. |
| app/data/WorkflowDS/Blueprints/ComputeResource.json | Introduces a new blueprint for CPU/memory resource requests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+103
to
+108
| if "computeResource" in runner_entity: | ||
| compute_resource = runner_entity["computeResource"] | ||
| if "memory" in compute_resource: | ||
| memory_in_gb = compute_resource["memory"] | ||
| if "cpu" in compute_resource: | ||
| cpu = compute_resource["cpu"] |
Comment on lines
116
to
120
| if memory_in_gb > 16.0 or cpu > 4.0: | ||
| logger.warning( | ||
| f"Specified compute resources for job '{self.job.job_uid}' are above the maximum of 16 CPU and 4 GB memory. " | ||
| + f"Using default values of 2 CPU and 2 GB memory." | ||
| + "Using default values of 2 CPU and 2 GB memory." | ||
| ) |
Comment on lines
101
to
102
| memory_in_gb = 2.0 | ||
| cpu = 2.0 |
Move Azure SDK construction out of JobHandler.__init__ into a lazy aci_client @Property. Deployments that don't use the AzureContainer backend no longer need AZURE_JOB_SP_* / IMAGE_REGISTRY_* secrets. - Add _REQUIRED_CONFIG list and _check_required_config() helper - Add AzureHandlerConfigError (missing/invalid settings) and AzureHandlerAuthError (AAD rejected credentials) exception types - Wrap begin_create_or_update().result() in start() so an expired service-principal secret surfaces as AzureHandlerAuthError instead of a bare 500 - Improve the fragmentary 'Container image in job runner' ValueError
Linux reports SIGKILL/OOM/SIGSEGV as negative exit codes. The previous guard 'exit_code >= 1' left those runs stuck at the caller's prior status. Match on 'exit_code != 0' (with a None guard) instead.
The previous match only handled Running/Terminated/Waiting, so Pending, Succeeded, Failed and Canceled fell through to 'self.job.status', leaving e.g. a container that died during image pull stuck at STARTING forever. Adds explicit cases for Succeeded/Pending (-> STARTING), Failed/Canceled (-> FAILED), and a default arm that logs the unmapped state and returns UNKNOWN.
remove() and progress() were mutating the global module logger's level around ARM calls and restoring it afterwards. In an async FastAPI process this leaks: a concurrent request logs at the wrong level for the duration of the toggle. The 'azure.*' loggers are already pinned to WARNING at import time (line 27), so the toggles were redundant on top of being unsafe.
Every important line was doing both logger.info() and print(). print bypasses log config, level filters, and structured formatting, so the same message ended up in the container log twice with different formatting. Keep the logger call; drop the print.
Previously any out-of-range cpu or memory value silently reset both to (2, 2) - so a job asking for 8 CPU would get 2 with only a warning, and any over-limit request lost the user's memory choice too. Clamp each dimension independently against the documented ACI Norway East limits (0.5-4 CPU, 0.5-16 GB), and only warn when the value was actually adjusted.
ClientAuthenticationError is now handled by AzureHandlerAuthError, but every other ARM failure from begin_create_or_update (quota, invalid image, region capacity, container-group name collision, ...) still surfaced as a bare 500. Introduce AzureHandlerProvisionError carrying the ARM status_code and error_code, and wrap the LROPoller call to raise it. The FastAPI boundary can now translate to a meaningful upstream error code.
The 'wait for container to reach Running/Terminated' loop was silently falling through when the container never got there, and then logging *** started successfully ***. Callers marked the job as running and it would stay stuck in that state forever. Use for/else on the while loop to raise TimeoutError with the last observed container_state instead. The container group is still created, so the caller (or a subsequent remove()) can clean it up.
- Catch ResourceNotFoundError -> return (COMPLETED, 'already removed') so retries and duplicate delete calls become idempotent instead of hitting a bare 500. - Wrap ARM errors: ClientAuthenticationError -> AzureHandlerAuthError, other HttpResponseError -> AzureHandlerProvisionError with status/error codes, matching start(). - Distinguish 'Failed'/'Canceled' terminal statuses (-> FAILED) from 'InProgress' after the polling budget (-> UNKNOWN); previously everything non-'Succeeded' collapsed to UNKNOWN.
progress() was doing list_logs + container_groups.get every poll - two ARM round-trips per job per tick. On top of that, list_logs on a container that hasn't started yet is guaranteed to return nothing. - Fetch container_groups.get first (single round-trip). - Only call list_logs when the container is Running or Terminated. - Also catch ClientAuthenticationError -> AzureHandlerAuthError, matching start()/remove(). - Guard the events[-1] fallback against AttributeError and IndexError (previous code only caught TypeError, and the trailing bare 'pass' was dead code).
Adds a uuid.UUID(...) sanity check on AZURE_JOB_SP_CLIENT_ID, AZURE_JOB_SP_TENANT_ID, and AZURE_JOB_SUBSCRIPTION so a typo or a 'changeme' placeholder is rejected with a clear message before the first AAD round-trip.
Adds a small _JobLoggerAdapter that prepends '[job_uid=<uid>]' to every message and stashes it on the handler as self._log. Handler log calls no longer have to interpolate self.job.job_uid manually, and log aggregators can group on the consistent prefix.
Register exception handlers so the typed AzureHandler* exceptions
introduced in the previous commits translate to sensible HTTP
responses instead of bare 500s:
- AzureHandlerConfigError -> 503 + Retry-After: 300
('backend not configured on this deployment')
- AzureHandlerAuthError -> 502
('AAD rejected the SP credentials')
- AzureHandlerProvisionError -> 502
('ARM rejected the container-group op'), with arm_status_code
and arm_error_code in ErrorResponse.extra.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this pull request change?
Why is this pull request needed?
Issues related to this change