Fixes for OFE + Apptainer modules - #6047
Conversation
…line-fixes Fix OFE baseline dependencies and job accounting
…m/develop Sync official develop into NAG develop
…m/develop Sync official develop into NAG develop
…ugh example Adds community/modules/container/apptainer-runtime and apptainer-app for staging and running Apptainer/Singularity SIF images from Google Artifact Registry on Cluster Toolkit-deployed VMs, plus an example blueprint (apptainer-artifact-registry-openfoam.yaml) demonstrating them together with an Artifact Registry REMOTE_REPOSITORY pull-through mirror of Docker Hub. Also fixes community/modules/container/artifact-registry's registry_url output, which referenced deployment_name instead of the actual generated repository_id and so pointed at a repository that doesn't exist.
Local tfenv version-pin file, not a convention this repo's tooling reads (the Makefile checks the terraform version directly) or has ever tracked elsewhere.
This module has no dependency on Open OnDemand and isn't scoped to a particular frontend, so drop language that implied otherwise.
…m/develop Sync official develop into NAG develop
…ptainer-module Apptainer Module Addition
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Open Front End (OFE) infrastructure and introduces new container management capabilities via Apptainer. It improves job state tracking, updates dependencies, refines template syntax, and adds robust modules for staging and running containerized applications from Google Artifact Registry. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new Apptainer modules (apptainer-app and apptainer-runtime) and an example blueprint for staging OpenFOAM via a Google Artifact Registry pull-through cache. It also updates the front-end daemon to query Slurm job states using sacct when they are no longer in the active queue. The review feedback highlights critical improvements to prevent plan-time crashes in the new Terraform modules when network_storage is empty, a correction to template escaping for shell PID variables ($$$$), and a reminder to register the new example blueprint in examples/README.md per the repository style guide.
| install_root_resolved = var.install_root != null ? var.install_root : var.network_storage[var.network_storage_index].local_mount | ||
| install_root_clean = local.install_root_resolved == "/" ? "" : trimsuffix(local.install_root_resolved, "/") |
There was a problem hiding this comment.
To prevent plan-time crashes when var.network_storage is empty and var.install_root is null, use a try() block to safely access the local_mount attribute. This allows the variables to be safely evaluated during plan time without throwing index out of bounds errors.
install_root_resolved = var.install_root != null ? var.install_root : try(var.network_storage[var.network_storage_index].local_mount, null)
install_root_clean = local.install_root_resolved == null ? "" : (local.install_root_resolved == "/" ? "" : trimsuffix(local.install_root_resolved, "/"))
References
- To allow variables to be explicitly set to null without automatically overriding them to default configurations, keep the variable nullable and use try() blocks in the main configuration to safely access its attributes and prevent plan-time crashes.
| install_root_resolved = var.install_root != null ? var.install_root : var.network_storage[var.network_storage_index].local_mount | ||
| install_root_clean = local.install_root_resolved == "/" ? "" : trimsuffix(local.install_root_resolved, "/") |
There was a problem hiding this comment.
To prevent plan-time crashes when var.network_storage is empty and var.install_root is null, use a try() block to safely access the local_mount attribute. This allows the variables to be safely evaluated during plan time without throwing index out of bounds errors.
install_root_resolved = var.install_root != null ? var.install_root : try(var.network_storage[var.network_storage_index].local_mount, null)
install_root_clean = local.install_root_resolved == null ? "" : (local.install_root_resolved == "/" ? "" : trimsuffix(local.install_root_resolved, "/"))
References
- To allow variables to be explicitly set to null without automatically overriding them to default configurations, keep the variable nullable and use try() blocks in the main configuration to safely access its attributes and prevent plan-time crashes.
| exit 0 | ||
| fi | ||
|
|
||
| tmp_path="$${output_path}.tmp.$$" |
There was a problem hiding this comment.
In Terraform templates, $$ is the escape sequence for a single $. Because $$ at the end of the line is not followed by {, it will still be unescaped to a single $, resulting in tmp_path="${output_path}.tmp.$" in the generated shell script. To output a literal $$ (the shell PID variable) in the generated script, you must use $$$$.
tmp_path="$${output_path}.tmp.$$$$"
| # https://hub.docker.com/r/opencfd/openfoam-default/tags before deploying, | ||
| # and update openfoam_image_tag below if it has moved on. | ||
|
|
||
| blueprint_name: apptainer-artifact-registry-openfoam |
There was a problem hiding this comment.
Since a new example blueprint has been added, please ensure it is registered in the main index in examples/README.md to maintain discoverability and adhere to the repository style guide.
References
- If new examples (core or community) are added, ensure they are added to the index in
examples/README.md. (link)
- Wrap network_storage local_mount access in try() in both apptainer-app and apptainer-runtime so plan-time evaluation of the install_root fallback branch doesn't throw index-out-of-bounds when network_storage is empty. - Escape the shell tmp-file suffix as $$$$ so the generated staging script gets a literal $$ (PID) instead of Terraform collapsing it to a single $. - Register apptainer-artifact-registry-openfoam.yaml in examples/README.md.
…ptainer-module fix: address apptainer module review feedback
Two pre-commit hooks fail on the apptainer modules in CI: - terraform_tflint: apptainer-runtime declared project_id, deployment_name and region but never referenced them, tripping terraform_unused_declarations. Unlike apptainer-app, which uses all three in its generated manifest, the runtime module only emits layout and MODULEPATH runners and has no use for them. Remove them; the blueprint never set them explicitly, they were only auto-injected globals. - terraform-readme: both READMEs were generated by terraform-docs v0.20.0, which emits compact table separators. CI installs terraform-docs@latest (v0.24.0), which emits padded ones, matching every other README in the repo. Regenerate via tools/autodoc/terraform_docs.sh.
…er-fixes fix: satisfy pre-commit for apptainer modules
|
Fixed precommit stuff now |
| settings: | ||
| name_prefix: $(vars.deployment_name)-vm | ||
| machine_type: $(vars.vm_machine_type) | ||
| service_account_email: $(hpc_service_account.service_account_email) |
There was a problem hiding this comment.
instead of referencing the module setting, can we add it to the use clause?
There was a problem hiding this comment.
Absolutely. I've applied this fix now in 8d6445a
| * [hpc-slurm6-tpu.yaml](#hpc-slurm6-tpuyaml--) ![community-badge] ![experimental-badge] | ||
| * [hpc-slurm6-tpu-maxtext.yaml](#hpc-slurm6-tpu-maxtextyaml--) ![community-badge] ![experimental-badge] | ||
| * [hpc-slurm6-apptainer.yaml](#hpc-slurm6-apptaineryaml--) ![community-badge] ![experimental-badge] | ||
| * [apptainer-artifact-registry-openfoam.yaml](#apptainer-artifact-registry-openfoamyaml--) ![community-badge] ![experimental-badge] |
There was a problem hiding this comment.
Could you add entries for apptainer-app and apptainer-runtime in modules/README.md file as well?
There was a problem hiding this comment.
I have done so now. I've also included the artifact-registry section as I noticed this was missing too!
| fi | ||
|
|
||
| install -d -m 0700 "$${HOME:-/root}/.apptainer" | ||
| apptainer registry login --username=oauth2accesstoken --password="$access_token" "oras://$registry_host" |
There was a problem hiding this comment.
auth_runner logs in using oras://, but the stage_runner pulls using docker://. Since Apptainer isolates credentials by protocol, this mismatch will likely cause 401 Unauthorized errors on private images. Should we update the login to use docker://$registry_host so the credentials match the pull?
There was a problem hiding this comment.
Updated it to use docker:// now
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new community modules (apptainer-app and apptainer-runtime) and an example blueprint (apptainer-artifact-registry-openfoam.yaml) to support staging and running Apptainer SIF images using Artifact Registry as a pull-through cache. It also updates the frontend daemon (ghpcfe_c2daemon.py) to query sacct for final job state information when a job leaves the queue, and refactors several Jinja2 templates. Feedback on the daemon changes suggests handling multiple sacct entries for requeued jobs by returning the latest run's state, and ignoring the UNLIMITED placeholder in Slurm time parsing to prevent spurious warning logs.
| for line in proc.stdout.splitlines(): | ||
| if not line.strip(): | ||
| continue | ||
| fields = line.split("|") | ||
| if len(fields) < 4: | ||
| continue | ||
| sacct_jobid, state, start_time, end_time = fields[:4] | ||
| if sacct_jobid != str(jobid): | ||
| continue | ||
|
|
||
| result = {} | ||
| normalized_state = _normalize_slurm_state(state) | ||
| if normalized_state: | ||
| result["job_state"] = [normalized_state] | ||
|
|
||
| start_epoch = _parse_slurm_time(start_time) | ||
| if start_epoch is not None: | ||
| result["start_time"] = {"number": start_epoch} | ||
|
|
||
| end_epoch = _parse_slurm_time(end_time) | ||
| if end_epoch is not None: | ||
| result["end_time"] = {"number": end_epoch} | ||
|
|
||
| if normalized_state: | ||
| logger.info( | ||
| "sacct returned job %s with state %s", jobid, normalized_state | ||
| ) | ||
| return result or None |
There was a problem hiding this comment.
When a Slurm job is requeued or resubmitted, sacct can return multiple entries for the same JobID. Returning on the first match will return the older run's state and times instead of the final completed run. Iterating through all lines and keeping the latest matching one ensures the most recent run's state and times are returned.
latest_result = None
for line in proc.stdout.splitlines():
if not line.strip():
continue
fields = line.split("|")
if len(fields) < 4:
continue
sacct_jobid, state, start_time, end_time = fields[:4]
if sacct_jobid != str(jobid):
continue
result = {}
normalized_state = _normalize_slurm_state(state)
if normalized_state:
result["job_state"] = [normalized_state]
start_epoch = _parse_slurm_time(start_time)
if start_epoch is not None:
result["start_time"] = {"number": start_epoch}
end_epoch = _parse_slurm_time(end_time)
if end_epoch is not None:
result["end_time"] = {"number": end_epoch}
if normalized_state:
latest_result = result
if latest_result:
logger.info(
"sacct returned job %s with state %s", jobid, latest_result["job_state"][0]
)
return latest_resultThere was a problem hiding this comment.
This has been addressed in 8d6445a
Note: I used greatest-Start rather than Gemini's last-row suggestion (although this is used as a fallback).
| def _parse_slurm_time(value): | ||
| if not value or value in ["Unknown", "None", "N/A"]: | ||
| return None |
There was a problem hiding this comment.
Slurm can return UNLIMITED as a placeholder for end times of running or non-expiring jobs. Adding UNLIMITED to the ignored values list prevents unnecessary warning logs.
| def _parse_slurm_time(value): | |
| if not value or value in ["Unknown", "None", "N/A"]: | |
| return None | |
| def _parse_slurm_time(value): | |
| if not value or value in ["Unknown", "None", "N/A", "UNLIMITED"]: | |
| return None |
- log in to Artifact Registry with docker://, matching the pull scheme - wire the service account through the vm use clause - list the container modules in modules/README.md - take the latest sacct row for requeued jobs - treat Slurm's UNLIMITED as an absent time
|
All suggestions added / addressed now. |
AdarshK15
left a comment
AdarshK15
left a comment
There was a problem hiding this comment.
LGTM, @rahimkhan19 please take a look.
|
|
||
| locals { | ||
| install_root_resolved = var.install_root != null ? var.install_root : try(var.network_storage[var.network_storage_index].local_mount, "") | ||
| install_root_clean = local.install_root_resolved == "/" ? "" : trimsuffix(local.install_root_resolved, "/") |
There was a problem hiding this comment.
Nit: is ternary condition required here? trimsuffix("/", "/") will anyway evaluate to "".
AdarshK15
added
the
release-new-modules
3 participants
1. OFE baseline fixes
Job accounting:
ghpcfe_c2daemon.pynow falls back tosacctwhen a Slurm job drops out ofsqueue, so completed/failed jobs still get final state, start/end times recorded instead of going untracked.Dependency fix: adds missing
PyYAMLto the c2_daemon's installed Python packages.Template fixes: converts blueprint Jinja templates (
artifact_registry_config,cloudsql_config,cluster_config,filesystem_config,partition_config) from{# #}comments to{% comment %}blocks, and adds a missingnetwork_storageoutput wiring infilesystem_config.yaml.j2.2. Apptainer container modules
New modules:
community/modules/container/apptainer-runtimeandapptainer-appfor staging/running Apptainer/Singularity SIF images pulled from Google Artifact Registry on Cluster Toolkit VMs.New example blueprint:
apptainer-artifact-registry-openfoam.yamldemonstrating both modules with an Artifact RegistryREMOTE_REPOSITORYpull-through mirror of Docker Hub.Bug fix bundled in:
artifact-registry'sregistry_urloutput was pointing atdeployment_nameinstead of the actual generatedrepository_idreferencing a repository that doesn't exist. Fixed to usegoogle_artifact_registry_repository.artifact_registry.repository_idinstead.