infra: propagate custom timeouts to Cloud Workflows preprocessing and postprocessing steps#172
infra: propagate custom timeouts to Cloud Workflows preprocessing and postprocessing steps#172dwnoble wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces configurable timeouts for preprocessing and postprocessing Cloud Run jobs within the ingestion workflow. It adds variables for these timeouts, integrates them into the workflow definition, and parses them in the stack module. The review feedback suggests wrapping the timeout parsing logic in try() blocks to prevent Terraform evaluation errors if the timeout values are null or empty.
| preprocessing_job_timeout = tonumber(replace(var.ingestion_config.preprocessing_job_timeout, "s", "")) | ||
| postprocessing_job_timeout = tonumber(replace(var.ingestion_config.postprocessing_job_timeout, "s", "")) |
There was a problem hiding this comment.
If preprocessing_job_timeout or postprocessing_job_timeout is null or empty (e.g., when ingestion is disabled or timeouts are omitted), replace() or tonumber() will throw a Terraform evaluation error.
Using try() provides a safe fallback to null, allowing the downstream workflow module to use its own default values.
preprocessing_job_timeout = try(tonumber(replace(var.ingestion_config.preprocessing_job_timeout, "s", "")), null)
postprocessing_job_timeout = try(tonumber(replace(var.ingestion_config.postprocessing_job_timeout, "s", "")), null)
References
- When designing nested Terraform modules, set the top-level variable default to null and declare the attribute as optional in intermediate module configuration objects. This ensures that the default value defined in the innermost module is respected and not overridden by parent defaults.
| - "--import_list" | ||
| - '$${json.encode_to_string(import_list)}' | ||
| - "--no-dry_run" | ||
| connector_params: |
There was a problem hiding this comment.
A small nit, I dont think you're propagating these to the actual pre/post processing jobs as the timeout, it's just the polling connection.
You also can pass in these timeouts in the cloud run job containers running these.
Maybe passing it in both the cloud run job + the polling connector is the right approach? Can you update the worfklow.yaml connector params on the preprocessing polling too? I hardcoded that value (3993c71)
| timeout: ${preprocessing_job_timeout} | ||
| result: run_res | ||
|
|
||
|
|
This change exposes and propagates user-defined timeouts (
ingestion_preprocessing_job_timeoutandingestion_postprocessing_job_timeout) from the stack infrastructure variables down to the Google Cloud Workflows orchestrator steps.Problem
Large dataset ingestions can exceed the default 30-minute (1800s) Google Cloud Workflows connector polling limit for long-running operations (LROs). Although custom timeouts are defined in the stack configuration, they were not being forwarded to the workflow definition. Consequently, the workflow orchestrator prematurely timed out with a
TimeoutErrorduring operation status polling, even when the underlying Cloud Run jobs were running correctly.Solution
module.stacktomodule.ingestion_workflow.templatefileinsidemodules/ingestion/workflow/main.tf.timeoutparameter inside theconnector_paramsblock for bothrun_cloud_run_job(preprocessing) andrun_cloud_run_agg_job(postprocessing) steps inworkflow.yaml.