Skip to content

feat(ofe): store Terraform state in a versioned Cloud Storage bucket - #6539

Open
ep-nag wants to merge 5 commits into
GoogleCloudPlatform:developfrom
nagconsulting:ofe-terraform-state-in-gcs
Open

ep-nag wants to merge 5 commits into
GoogleCloudPlatform:developfrom
nagconsulting:ofe-terraform-state-in-gcs

Conversation

@ep-nag

@ep-nag ep-nag commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What this does

Moves the Terraform state of everything the Open Front End (OFE) provisions — clusters, filesystems, networks, image builders and workbenches — out of local files on the service machine and into a dedicated, versioned Cloud Storage bucket. Until now each resource's terraform.tfstate lived only on the web server's disk, with no versioning, no locking and no copy: if that machine was lost or rebuilt, Terraform could no longer destroy anything it had created, and those resources kept running and billing.

The state bucket is a separate Terraform root (tf/state_bucket/), applied by deploy.sh before the front end itself, so tearing the front end down never destroys the state of resources it created. The existing control (support) bucket is unchanged and keeps every current purpose.

The branch also fixes the failures that a full live run of every resource type exposed in the Filestore, image builder and workbench paths. Without those fixes, those resource types could not be created at all on the current images and provider releases.

State bucket

  • Bucket: versioned, force_destroy = false, prevent_destroy, uniform bucket-level access, public access prevention enforced, and a non-current-version expiry rule (90 days by default). It uses Google provider >= 6.4.0, the same floor as the workbench Terraform. It is never referenced by the front end's own Terraform root. The Toolkit's cloud-storage-bucket module is not used because it cannot carry prevent_destroy or an authoritative writer binding (see tf/state_bucket/README.md).

  • Deterministic, always-valid name: <deployment_name>-tfstate-<8 hex>, derived from project and deployment name. The name is lowercased, characters outside a-z0-9- are replaced, and it is cut to 63 characters, so long names and names containing . still give a valid bucket name. Names that were already valid (46 characters or fewer) are unchanged.

  • Confirmation first: an interactive deploy now asks "Proceed to deploy?" before anything is created, so declining no longer leaves a deletion-protected bucket behind. Config-file deploys do not prompt, as before.

  • One working directory per deployment: on the client machine, each deployment's state-bucket root runs in tf/state_bucket/deployments/<deployment_name>/, which links to the tracked .tf files and holds that deployment's own local state, terraform.tfvars and admin.auto.tfvars. Deploying another deployment name from the same checkout therefore neither collides with an earlier one nor needs files moved by hand.

    • Files that an earlier layout kept directly in tf/state_bucket/ are moved into their owner's directory on the next deploy. The owner is taken from that terraform.tfvars and must match the deployment label of the bucket recorded in the state; if it cannot be determined, or its directory already has files, deploy.sh stops before running anything and says how to move them.
  • Adoption: a deploy from a machine with no local state for this deployment finds the existing bucket and adopts it (terraform import, writers seeded from its live IAM policy) instead of creating a second, empty one.

    • Existence check: decided from a filtered bucket listing and an exact name match, not by parsing gcloud error text.
    • Clash detection: a name clash is reported only when the bucket was neither in local state nor imported in this run. Otherwise a failed apply points at Terraform's own error.
    • Local state for another bucket: deploy.sh stops with the exact commands to move that state aside. The local bucket name is read from the root's output, or from the state itself when an adoption's apply failed before writing outputs, so a rerun after such a failure proceeds instead of reporting an unknown bucket.
  • Access: an authoritative roles/storage.objectAdmin binding on this bucket only; no project-level role is added. The writer list is split by owner:

    • deploy.sh regenerates terraform.tfvars on every deploy with default_writers (the <deployment>-tkfe-sa provisioning account, only if it exists, because the API rejects a binding for a missing account);
    • administrators add other provisioning accounts, including cross-project credentials, to state_bucket_writers in the deployment's admin.auto.tfvars, which deploy.sh creates once and never reads or rewrites.

    Accounts with a project-level Cloud Storage role can also reach the bucket, including the storage.admin role script/service_account.sh already gives the provisioning account; the admin guide says so. Tightening that is outside this change.

  • Access check when a credential is registered: when an administrator registers or updates a credential in the portal, OFE asks the bucket, using that credential's own service account (testIamPermissions), whether it can read, write, list and delete state objects. It writes nothing and grants nothing. If access is missing, the portal shows a warning naming the account, with the exact line to add to state_bucket_writers and the terraform -chdir=... apply command, and says provisioning with this credential will fail until then. A missing or unreadable bucket, or an authentication failure, gives its own warning. Registration always succeeds.

  • Deleting the bucket: teardown.sh --delete-state-bucket removes it, opt-in only:

    • before destroying anything, it reads every live *.tfstate object in the bucket (all namespaces and prefixes) and refuses, listing them, if any records resources, cannot be read, or is locked; the front end is left untouched;
    • it asks for the bucket name to be typed unless -y is also given;
    • deletion protection is lifted only in a temporary copy of the root (tracked files are never edited); that change is planned first and applied only if it updates the bucket in place, so a bucket that Terraform would replace (for example after a change of region) is refused and nothing is deleted;
    • emptiness is checked again immediately before the destroy; if that check refuses, force_destroy = false is applied back and the bucket is kept;
    • the resulting state is copied back, including after an interrupt;
    • it works when the front end has already been torn down, and --delete-state-bucket <deployment_name> removes only that deployment's bucket. If that deployment's front end may still be deployed from the same checkout, the named form refuses and points to the bare flag, which tears the front end down first. tf/terraform.tfvars is read leniently (any spacing or a trailing comment); a file that is missing or names no deployment counts as possibly this one;
    • both forms fail closed: a bucket-only deletion goes ahead only when the front end is provably gone (no tf/.tkfe.lock, and tf/'s state readable and recording no front end). When the front end is absent but cannot be proven gone, the bare form refuses before any prompt or change. Both refusals say how to clear a lock file left by a failed teardown.

    Plain teardown.sh still keeps the bucket and now prints this command. teardown.sh also now rejects arguments it does not recognise with a usage message instead of ignoring them, so a mistyped option cannot silently run a plain teardown; teardown.sh and teardown.sh -y behave as before.

  • Per-database namespace: every prefix is <namespace>/<kind>s/<kind>_<id>. The namespace (YYYYMMDD-<8 hex>) is created with the TKFE database (StateNamespace model; python manage.py tfstate_namespace prints it), so a rebuilt service machine with a new database can never write over an earlier database's state.

  • Both provisioning paths: blueprint-generated resources (clusters, Filestores, image builders) get terraform_backend_defaults in the generated blueprint. Raw-template resources (networks, workbenches) get a backend.tf when the template is copied. Every Terraform call carries the resource's own provisioning credential, including init.

  • State reads use terraform state pull, never a local file. A failed state read after a successful apply marks the workbench or cluster as errored instead of destroying it. -input=false is passed to every Terraform command that accepts it.

  • State locks:

    • Lock-taking commands run with -lock-timeout=45m, so a second operation on the same resource waits for the first instead of failing at once.
    • Before running, OFE reads the resource's GCS lock. If the lock was taken on this server and no terraform process here is running in that directory, it is stale: OFE fails at once, giving the lock details and the exact force-unlock command. A terraform process whose working directory cannot be read (one run by another user) counts as running, so a lock it may hold is never reported as stale.
    • OFE never unlocks automatically. The admin guide has a section on stale locks.
  • Configuration: the bucket name reaches the server through instance metadata and configuration.yaml (server.terraform_state_bucket). A missing value is fatal rather than a silent fallback to local state.

Migration for existing deployments

script/migrate_tf_state_to_gcs.sh (--dry-run / --yes, optionally --dir <path>) moves each working directory's local state into the bucket:

  • it takes a backup before migrating, and refuses empty or invalid state;
  • it writes a backend.tf for directories that have no backend;
  • it runs terraform init -migrate-state, then terraform plan -detailed-exitcode.

The original is kept as terraform.tfstate.premigration at the top of the resource's run-time directory (for example clusters/cluster_<id>/). It is kept outside any Toolkit deployment group, because gcluster create -w moves and later deletes a group, which would lose the rollback copy on the next reconfigure.

An existing .premigration that differs from the current state is never overwritten; the script tells the operator to move it aside. A directory whose post-migration plan is not clean is marked .migration-incomplete, and a rerun reports it as failed rather than skipping it. The script uses the application's own interpreter for every call into OFE code, computes prefixes with the same function the application uses, and skips .ghpc/ and .terraform/ copies.

Fixes needed to provision every resource type

  • Filestore: gcluster create ran with an environment containing only the credential (env= replaces the whole environment), so it had no HOME and every Filestore creation failed before anything was made. It now inherits the server's environment, as the cluster path already did.
  • Image builder, blueprint: gcluster rejected the image blueprint (module "network1" was not used). The Packer group now uses the builder VPC, and the toolkit's own cross-group bridge (export-outputs from builder-env, then import-inputs into packer-image) supplies its inputs. create gains -w, so a retry can reuse the deployment folder. An image with no startup scripts renders an empty runner list.
  • Image builder, hang: packer-plugin-googlecompute v1.2.7 reports the startup script's result with gcloud compute instances add-metadata ${HOSTNAME}. On the Rocky 9 Slurm images HOSTNAME is sometimes still the FQDN when the script starts, the call is rejected, and Packer waits forever. The builder's startup script is now wrapped so that it reports done or error itself, under the instance name and zone from the metadata server. This is temporary: the upstream fix (Update instance name logic in GCP packer plugin startup script hashicorp/packer-plugin-googlecompute#355) is merged but not in any release yet. The existing ~> 1.2.5 constraint will pick up a fixed release automatically, after which the wrapper can be removed.
  • Workbenches: Google retired user-managed notebooks, so workbench creation failed outright.
    • Workbenches move to google_workbench_instance (Google provider >= 6.4.0, current network and project-factory modules), with a 150 GB boot-disk floor and a data disk.
    • The data disk is set by data_disk_size_gb, defaulting to 100 GB.
    • Readiness is detected by polling the instance for ACTIVE, not a serial-console marker the new image never prints.
    • The post-startup script only mounts attached filesystems into /home/jupyter and optionally installs the Slurm client; the managed image owns JupyterLab and its service.
    • The tfvars now include network_name, and no longer write the deprecated image_family.
  • Workbench image choice removed:
    • Why: Workbench Instances always use the default Workbench image, so any image choice was silently ignored.
    • Terraform: the unused instance_image variable is removed, which also fixes tflint. The deprecated image_family and image_project variables stay, so an old setting still fails with a clear message.
    • Portal: the image-family field is removed from the workbench create and update forms. The model field stays, to avoid a schema migration.

Docs

docs/admin_guide.md gains a Terraform State section:

  • bucket naming and access (including what project-level storage roles can reach), the per-deployment working directory on the client, and adding writers for cross-project credentials, including the warning shown at registration;
  • the namespace;
  • a recovery runbook for a lost service machine: destroy any resource from bucket state using only a backend and provider block, including leftover Slurm compute nodes;
  • what to do when a resource reports a stale state lock, with a check (run with sudo) that lists terraform processes run by any user;
  • upgrading an existing TKFE to Cloud Storage state:
    • adding the bucket name to the server's configuration.yaml, and to the front end's own tf/terraform.tfvars on the client, without which teardown.sh cannot destroy the front end;
    • refreshing the support bucket's files from the new release;
    • a note that a network or workbench defined but never created, or a filesystem defined but never started, has nothing to migrate and should be deleted and created again in the portal;
  • reverting to an older release: delete backend.tf if present, otherwise the block in the generated main.tf, and move old .premigration backups aside before migrating again;
  • removing the bucket with teardown.sh --delete-state-bucket.

The --config deployment section explains that config-file deploys create no provisioning service account, so the bucket has no writer until one is added. docs/AdvancedAdmin.md, docs/developer_guide.md (including the namespaced state prefixes and the per-deployment working directory) and teardown.sh's closing message point to the same procedures.

Files

  • Terraform:
    • tf/state_bucket/ (new root: main.tf, variables.tf, outputs.tf, provider.tf, versions.tf, README.md, and a .gitignore for the per-deployment working directories);
    • tf/main.tf and tf/variables.tf (terraform_state_bucket metadata and variable);
    • infrastructure_files/workbench_tf/google/* (Workbench Instances; README regenerated).
  • Scripts:
    • deploy.sh: prepare_state_bucket_dir(), deploy_state_bucket() and validate_bucket_name(), the confirmation moved before anything is created, plus region in config-file deploys;
    • teardown.sh: per-deployment bucket lookup and --delete-state-bucket;
    • script/migrate_tf_state_to_gcs.sh (new);
    • infrastructure_files/gcs_bucket/webserver/startup.sh (config key);
    • infrastructure_files/gcs_bucket/workbench/startup_script_template.sh;
    • infrastructure_files/workbench_tf/google/wait-for-startup/scripts/wait-for-startup-status.sh.
  • Application (website/ghpcfe/):
    • cluster_manager/utils.py: backend helpers, state pull, namespace, -input=false, lock timeout and stale-lock check;
    • cluster_manager/validate_credential.py: state bucket access check;
    • clusterinfo.py, vpc.py, workbenchinfo.py, filesystem.py, image.py;
    • image_startup.py (new);
    • models.py (StateNamespace);
    • management/commands/tfstate_namespace.py (new);
    • forms.py and the workbench create_form.html / update.html templates;
    • views/clusters.py, views/credentials.py;
    • templates/blueprint/cluster_config.yaml.j2.
  • Tests (website/ghpcfe/tests/): __init__.py, test_tf_state_backend.py, test_image_startup.py, test_migrate_tf_state_script.py, test_deploy_script.py, test_teardown_script.py, test_credential_state_bucket.py, test_workbench_changes.py (all new). They load without Django settings. Run them with cd community/front-end/ofe/website && python3 -m pytest ghpcfe/tests.
  • Docs: docs/admin_guide.md, docs/AdvancedAdmin.md, docs/developer_guide.md, tf/README.md.

What's been tested

Local (current head):

  • Committed OFE tests: 146 passed, on Python 3.14 and on Python 3.11.
  • Coverage:
    • state prefixes and namespace;
    • backend blocks in all three generated blueprints, including Filestore without Django;
    • backend.tf content for networks and workbenches, and state reads;
    • the fatal missing-bucket error, credentials on every Terraform call, -input=false;
    • lock timeout and stale-lock detection, including a process whose working directory cannot be read counting as running;
    • bucket naming and validation, existence and adoption decisions in deploy.sh, including the bucket name read from state when the output is missing;
    • per-deployment working directories: links refreshed, other deployments untouched, earlier-layout files moved to their owner, and a stop (before any Terraform call) when the owner is unknown, mismatched or already has files;
    • teardown.sh:
      • bucket lookup;
      • the emptiness check: states with resources in any namespace, unreadable states, locks and listing failures, with old object versions ignored;
      • refusal before anything is destroyed;
      • the re-check immediately before the destroy, and the restore of force_destroy when it refuses;
      • deletion from a temporary copy that leaves the tracked main.tf unchanged;
      • refusal when the protection change would be anything but an in-place update;
      • the state kept and the copy removed on an interrupt;
      • typed confirmation;
      • the named form refusing while the front end may be deployed, whether the lock file exists, the state shows it or cannot be read, the name line is re-spaced or commented, or tf/terraform.tfvars is missing; otherwise it removes only the bucket;
      • the bare form refusing before any prompt when the front end cannot be proven gone;
      • plain teardown keeping the bucket;
    • the registration access check: the registered credential is used, the exact line and command on missing permissions, missing bucket, API and auth errors reported rather than raised;
    • the migration script end to end in a synthetic tree, including backup placement and refusal to overwrite;
    • the image startup wrapper (both done and error reports), and the workbench form and variable changes.
  • pre-commit on the changed files: terraform fmt and validate, tflint, terraform-docs, shellcheck, shfmt, yamllint, codespell, PyMarkdown. The only failures are 5 pre-existing Slurm watch_delete_vm_op tests under Python 3.14 in pytest-check, unrelated to this branch.
  • make tests: passed.

Independent deployment test, 2026-10-20 (a test project, europe-west4-a):

  • deploy, with the bucket created first with the expected properties and label;
  • the per-deployment working directory, the server's metadata and configuration.yaml, and a stable namespace;
  • readiness, and a UI walk (image_family on no page);
  • an unknown teardown.sh option rejected;
  • --delete-state-bucket refused a bucket holding a state with a resource, leaving the front end up, then removed the front end and the bucket;
  • nothing was left behind.

Live, 2026-10-20 and 2026-10-21 (a test project, europe-west4-a). Every deployment was torn down, every bucket removed, and the project checked against a pre-run snapshot after each run.

  • Existing checkout and a second deployment:
    • a deployment made with the previous single-directory layout was torn down with plain teardown.sh, which kept its bucket and printed the removal command;
    • a second deployment from the same checkout then moved the first deployment's files into their own directory by itself and deployed with no manual step;
    • a copy whose files named a different owner from the bucket's label stopped before any Terraform or gcloud call.
  • Deploy:
    • the bucket was created first with versioning, uniform access, public access prevention and the 90-day rule; its only objectAdmin member is the provisioning account;
    • the server received the bucket name, and the namespace is stable.
  • Declining an interactive deploy: answering N at "Proceed to deploy?" exited with nothing created — no state bucket, no local files, the project unchanged.
  • Adoption: with the deployment's local files moved aside, the deploy imported the existing bucket and the follow-up plan was clean. Under provider 6.x the first apply after import re-records the bucket's labels in state; nothing changes in the cloud.
  • Network, subnet and workbench, created and destroyed through the application's own provisioning functions:
    • the workbench reached ACTIVE with a proxy URI, a 100 GB data disk and a 150 GB boot disk;
    • state is at <namespace>/<kind>s/<kind>_<id>/default.tfstate with no local state file, and each destroy leaves 0 resources.
  • Workbench forms: the create and update forms render (200) without an image-family field.
  • Lock wait: a real terraform apply held a network's lock at its approval prompt. The application's apply ran with -lock-timeout=45m, logged Acquiring state lock, and completed with no changes once the holder was interrupted.
  • Stale lock:
    • the holder was killed with SIGKILL, leaving the lock;
    • the application's apply failed within 6 seconds, giving the lock ID and the force-unlock command, instead of waiting 45 minutes;
    • the documented force-unlock cleared the lock, and the apply then succeeded.
  • Access check at registration (through the portal's create view): a credential for the provisioning account was saved with no warning; a credential for an account with no roles was saved with a warning naming the account, the missing permissions, the exact state_bucket_writers line and the apply command.
  • Deleting the bucket:
    • with a network still holding resources (10 in one run, 9 in another), teardown.sh -y --delete-state-bucket refused, listing only that state (the bucket's old object versions, mostly released locks, were ignored), and left the front end running;
    • once every state was empty, it tore the front end down and deleted the bucket; the tracked main.tf was unchanged;
    • --delete-state-bucket <earlier deployment> declined a wrongly typed bucket name, then deleted that bucket when the name was typed correctly;
    • the named form refused while that deployment's front end was running: with the lock file present, with the lock moved aside (the state still showing the front end), when tf/'s state could not be read (a real terraform show failure), and with the name line in tf/terraform.tfvars re-spaced and commented. The server and bucket were untouched each time;
    • with the front end's state unreadable, the bare form refused before any prompt, saying nothing had been changed and how to clear a leftover lock file;
    • after plain teardown (26 and 27 resources destroyed, bucket kept, lock removed), the named form deleted only the bucket;
    • with the region changed in the deployment's tfvars, Terraform planned a replacement and the script refused, deleting nothing; the bucket stayed in its original region;
    • with a state object written into the bucket right after the protection change, the final check refused and force_destroy was applied back to false;
    • an interrupt (SIGINT) during the plan and during the apply exited with 130, kept the local state and left no temporary copy; a rerun completed the deletion, applying the protection change as an in-place update.
  • Upgrade from the pre-feature release on develop, following the admin guide:
    • the old release created a network (10 resources) and a Slurm cluster (44 resources, state inside its deployment group) in local state;
    • after the upgrade steps (bucket created in the deployment's own directory on the client, support-bucket refresh, new code, config key, makemigrations/migrate, restart), the migration's dry run listed both and --yes migrated both (Migrated: 2 Failed: 0);
    • the cluster's backup was kept at clusters/cluster_<id>/terraform.tfstate.premigration, outside the deployment group;
    • the application's next apply on the network made no changes;
    • without terraform_state_bucket in the client's tf/terraform.tfvars, teardown.sh stopped with "No value for required variable" and the front end stayed up; with the line added as the guide now says, teardown completed.
  • Revert and re-migrate, following the admin guide:
    • backend.tf was deleted in both directories, including the cluster's deployment group;
    • state returned to local disk with clean plans;
    • a second migration refused while the old backups were in place; with them moved aside as the guide says, it moved both back (Migrated: 2).
  • Destroy from migrated state: the cluster and network were destroyed through the new code from their migrated state.

Live, earlier heads of this branch (same project): every resource type end to end.

  • Filestore: created and destroyed.
  • Custom image: built to READY with the builder torn down, then deleted. Before the image fix, the build hang reproduced three times; the serial console showed the transient FQDN hostname and the rejected add-metadata call.
  • Slurm cluster: reached ready, and a one-node srun hostname returned exit code 0.
  • Minimum IAM: a provisioning account holding only network roles, plus objectAdmin on the state bucket alone, ran a network's full lifecycle.
  • Recovery runbook: a cluster (40 resources) and a network (10) were destroyed from this machine using only bucket state.

Future work

  • Clearing a stuck lock from the portal. While Terraform works on a resource, it holds a lock in the state bucket so that two operations can't change that resource at once. If the server reboots or Terraform is killed mid-run, the lock is never released. Every later operation on that resource then fails. With this change, OFE detects that case and shows the error, but clearing the lock still needs an administrator to log in to the server and run terraform force-unlock by hand, as the admin guide describes.

    A follow-up could add an admin-only "Release lock" button. Before unlocking, it would check that the lock is still the same one and that no Terraform process on the server is using it, then ask the administrator to confirm. Removing a lock that is actually in use can corrupt the resource's state, so this is left for a separate change that can be reviewed on its own.

Terraform state for every resource the Open Front End provisions (clusters, filesystems, networks, image builders and workbenches) moves from local files on the service machine to a dedicated, versioned, deletion-protected Cloud Storage bucket with state locking, so losing the server no longer orphans billable resources. deploy.sh creates the bucket as a separate Terraform root before the front end, teardown.sh keeps it unless --delete-state-bucket is given, and a migration script moves existing local state. Also fixes Filestore, image builder and workbench provisioning failures found while testing every resource type.
@ep-nag
ep-nag requested a review from a team as a code owner October 21, 2026 13:01
@ep-nag
ep-nag requested review from AdarshK15 and shubpal07 October 21, 2026 13:01
@github-actions github-actions Bot added the external PR from external contributor label Oct 21, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, 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 significantly improves the reliability and lifecycle management of provisioned infrastructure by centralizing Terraform state in versioned Cloud Storage buckets. By decoupling state from the ephemeral service machine, it prevents resource orphaning and billing leaks. The changes include robust state bucket provisioning, a migration script for existing deployments, and necessary updates to resource provisioning paths to support the new backend and current provider versions.

Highlights

  • Terraform State Migration: Moved Terraform state from local files on the service machine to a versioned Cloud Storage bucket, ensuring state persistence across machine rebuilds.
  • State Bucket Management: Implemented a dedicated Terraform root for state buckets with deletion protection, versioning, and per-deployment working directories to prevent collisions.
  • Provisioning Fixes: Resolved critical provisioning failures for Filestore, Image Builder, and Workbench instances, including migrating Workbenches to the newer google_workbench_instance resource.
  • Security and Access: Added authoritative IAM bindings for state buckets and implemented a registration-time access check to warn administrators of missing permissions for provisioning credentials.
  • Teardown Safety: Enhanced teardown.sh with an opt-in --delete-state-bucket flag that performs safety checks to ensure the bucket is empty before deletion.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@ep-nag

ep-nag commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request transitions the Open Front End (OFE) to store Terraform state in a versioned, deletion-protected Google Cloud Storage (GCS) bucket instead of the local disk. It introduces a new Terraform root for the state bucket, updates deployment and teardown scripts to manage its lifecycle, adds a migration script for existing local states, and updates the Python backend to dynamically configure GCS backends and handle state locking. Additionally, it replaces user-managed notebooks with Vertex AI Workbench Instances and adds comprehensive unit tests. The review feedback recommends adhering to the principle of least privilege by replacing roles/storage.objectAdmin with roles/storage.objectUser for state bucket writers across the Terraform configurations, variables, deployment scripts, and documentation.

Comment thread community/front-end/ofe/deploy.sh Resolved

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces a major feature to store Terraform state for provisioned resources (clusters, filesystems, networks, images, and workbenches) in a dedicated, versioned, and deletion-protected Google Cloud Storage (GCS) bucket instead of the local disk. This prevents state loss when the service machine is rebuilt or replaced. The changes include updates to deployment and teardown scripts, a new Terraform root for provisioning the state bucket, a state migration script, and extensive updates to the Django backend to support GCS state backends, state locking, and credential validation. The review comments identify a potential AttributeError in workbenchinfo.py when accessing social account details, suggest using the more restrictive roles/storage.objectUser role instead of roles/storage.objectAdmin for state bucket writers, and recommend a performance optimization in models.py to avoid generating a new state namespace on every call to StateNamespace.current().

Comment thread community/front-end/ofe/deploy.sh Resolved
… of objectAdmin

objectUser has every permission the Terraform GCS backend and the credential access check use (objects create, delete, get, list); objectAdmin only adds object IAM and retention control. Adopting an existing bucket now seeds writers from objectUser bindings, and from objectAdmin bindings left by buckets created before this change.
…oogle account

copy_startup_script read socialaccount_set.first().uid directly, so a trusted user without a social account raised AttributeError and the workbench was never created. The OS Login name is now only logged by the startup script, so an empty name is used instead.
@github-actions

Copy link
Copy Markdown

this PR has been inactive for 2 days and has no unresolved comments. @AdarshK15, @shubpal07, please review.

@github-actions

Copy link
Copy Markdown

This is a second reminder that this PR has been inactive for 4 days and has no unresolved comments. @AdarshK15, @shubpal07, please review.

AdarshK15
AdarshK15 previously approved these changes Nov 2, 2026

@AdarshK15 AdarshK15 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@AdarshK15

Copy link
Copy Markdown
Member

/gcbrun

The developer guide still described the old /tmp/jupyterhome setup and
the rewrite of jupyter.service. Describe what the script does now on
Workbench Instances: JupyterLab and its unit are left to the image,
mounts are linked into /home/jupyter, the optional Slurm client setup,
and readiness via wait-for-startup.

@shubpal07 shubpal07 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.

Moving the state files into a separate, versioned storage bucket looks good, including the safety checks during teardown and the backup step in the migration script.

Requesting changes for two blockers where a failure right after creating a resource leaves it running in the cloud without a way to stop or delete it from the UI:

  1. clusterinfo.py: If reading back the state (or updating an existing cluster) fails after the cluster is already running in the cloud, the portal marks the cluster as "Not Managed" and hides the stop button.
  2. vpc.py: If reading back the state fails after the network is created in the cloud, the error goes unhandled and the network stays stuck in "Creating" status, hiding its action buttons in the UI.

Once terraform apply succeeds the resources exist, but a later failure
(reading the state back from the bucket, or the bookkeeping after it)
reset a cluster to "nm" or left a VPC stuck in "cm" (Creating).

Cluster: save cloud_state "m" straight after the apply, and stop the
start/reconfigure error handlers resetting it to "nm". A Retry of an
already-applied cluster no longer resets it to "nm" either.

VPC: save cloud_state "m" straight after the apply and read the state
in its own try, so a failed state pull, unreadable state or missing
output keeps the network manageable. A failed apply now sets "um"
(Unknown) instead of leaving it at Creating; this replaces a
vpc.status assignment that had no effect (VirtualNetwork has no status
field).

Add regression tests in tests/test_post_apply_state.py.

@shubpal07 shubpal07 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!

@AdarshK15

Copy link
Copy Markdown
Member

/gcbrun

@AdarshK15 AdarshK15 added the release-module-improvements Added to release notes under the "Module Improvements" heading. label Nov 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external PR from external contributor release-module-improvements Added to release notes under the "Module Improvements" heading.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants