Repository navigation
Conversation
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.
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 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
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 review |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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().
… 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.
|
this PR has been inactive for 2 days and has no unresolved comments. @AdarshK15, @shubpal07, please review. |
|
This is a second reminder that this PR has been inactive for 4 days and has no unresolved comments. @AdarshK15, @shubpal07, please review. |
|
/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.
There was a problem hiding this comment.
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:
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.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.
|
/gcbrun |
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.tfstatelived 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 bydeploy.shbefore 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'scloud-storage-bucketmodule is not used because it cannot carryprevent_destroyor an authoritative writer binding (seetf/state_bucket/README.md).Deterministic, always-valid name:
<deployment_name>-tfstate-<8 hex>, derived from project and deployment name. The name is lowercased, characters outsidea-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.tffiles and holds that deployment's own local state,terraform.tfvarsandadmin.auto.tfvars. Deploying another deployment name from the same checkout therefore neither collides with an earlier one nor needs files moved by hand.tf/state_bucket/are moved into their owner's directory on the next deploy. The owner is taken from thatterraform.tfvarsand must match the deployment label of the bucket recorded in the state; if it cannot be determined, or its directory already has files,deploy.shstops 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.gclouderror text.deploy.shstops 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.objectAdminbinding on this bucket only; no project-level role is added. The writer list is split by owner:deploy.shregeneratesterraform.tfvarson every deploy withdefault_writers(the<deployment>-tkfe-saprovisioning account, only if it exists, because the API rejects a binding for a missing account);state_bucket_writersin the deployment'sadmin.auto.tfvars, whichdeploy.shcreates once and never reads or rewrites.Accounts with a project-level Cloud Storage role can also reach the bucket, including the
storage.adminrolescript/service_account.shalready 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 tostate_bucket_writersand theterraform -chdir=... applycommand, 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-bucketremoves it, opt-in only:*.tfstateobject 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;-yis also given;force_destroy = falseis applied back and the bucket is kept;--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.tfvarsis read leniently (any spacing or a trailing comment); a file that is missing or names no deployment counts as possibly this one;tf/.tkfe.lock, andtf/'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.shstill keeps the bucket and now prints this command.teardown.shalso 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.shandteardown.sh -ybehave as before.Per-database namespace: every prefix is
<namespace>/<kind>s/<kind>_<id>. The namespace (YYYYMMDD-<8 hex>) is created with the TKFE database (StateNamespacemodel;python manage.py tfstate_namespaceprints 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_defaultsin the generated blueprint. Raw-template resources (networks, workbenches) get abackend.tfwhen the template is copied. Every Terraform call carries the resource's own provisioning credential, includinginit.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=falseis passed to every Terraform command that accepts it.State locks:
-lock-timeout=45m, so a second operation on the same resource waits for the first instead of failing at once.terraformprocess here is running in that directory, it is stale: OFE fails at once, giving the lock details and the exactforce-unlockcommand. Aterraformprocess 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.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:backend.tffor directories that have no backend;terraform init -migrate-state, thenterraform plan -detailed-exitcode.The original is kept as
terraform.tfstate.premigrationat the top of the resource's run-time directory (for exampleclusters/cluster_<id>/). It is kept outside any Toolkit deployment group, becausegcluster create -wmoves and later deletes a group, which would lose the rollback copy on the next reconfigure.An existing
.premigrationthat 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
gcluster createran with an environment containing only the credential (env=replaces the whole environment), so it had noHOMEand every Filestore creation failed before anything was made. It now inherits the server's environment, as the cluster path already did.gclusterrejected the image blueprint (module "network1" was not used). The Packer group nowuses the builder VPC, and the toolkit's own cross-group bridge (export-outputsfrombuilder-env, thenimport-inputsintopacker-image) supplies its inputs.creategains-w, so a retry can reuse the deployment folder. An image with no startup scripts renders an empty runner list.packer-plugin-googlecomputev1.2.7 reports the startup script's result withgcloud compute instances add-metadata ${HOSTNAME}. On the Rocky 9 Slurm imagesHOSTNAMEis 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 reportsdoneorerroritself, 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.5constraint will pick up a fixed release automatically, after which the wrapper can be removed.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.data_disk_size_gb, defaulting to 100 GB.ACTIVE, not a serial-console marker the new image never prints./home/jupyterand optionally installs the Slurm client; the managed image owns JupyterLab and its service.network_name, and no longer write the deprecatedimage_family.instance_imagevariable is removed, which also fixestflint. The deprecatedimage_familyandimage_projectvariables stay, so an old setting still fails with a clear message.Docs
docs/admin_guide.mdgains a Terraform State section:sudo) that liststerraformprocesses run by any user;configuration.yaml, and to the front end's owntf/terraform.tfvarson the client, without whichteardown.shcannot destroy the front end;backend.tfif present, otherwise the block in the generatedmain.tf, and move old.premigrationbackups aside before migrating again;teardown.sh --delete-state-bucket.The
--configdeployment 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) andteardown.sh's closing message point to the same procedures.Files
tf/state_bucket/(new root:main.tf,variables.tf,outputs.tf,provider.tf,versions.tf,README.md, and a.gitignorefor the per-deployment working directories);tf/main.tfandtf/variables.tf(terraform_state_bucketmetadata and variable);infrastructure_files/workbench_tf/google/*(Workbench Instances; README regenerated).deploy.sh:prepare_state_bucket_dir(),deploy_state_bucket()andvalidate_bucket_name(), the confirmation moved before anything is created, plusregionin 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.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.pyand the workbenchcreate_form.html/update.htmltemplates;views/clusters.py,views/credentials.py;templates/blueprint/cluster_config.yaml.j2.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 withcd community/front-end/ofe/website && python3 -m pytest ghpcfe/tests.docs/admin_guide.md,docs/AdvancedAdmin.md,docs/developer_guide.md,tf/README.md.What's been tested
Local (current head):
backend.tfcontent for networks and workbenches, and state reads;-input=false;deploy.sh, including the bucket name read from state when the output is missing;teardown.sh:force_destroywhen it refuses;main.tfunchanged;tf/terraform.tfvarsis missing; otherwise it removes only the bucket;doneanderrorreports), and the workbench form and variable changes.watch_delete_vm_optests under Python 3.14 inpytest-check, unrelated to this branch.make tests: passed.Independent deployment test, 2026-10-20 (a test project,
europe-west4-a):configuration.yaml, and a stable namespace;image_familyon no page);teardown.shoption rejected;--delete-state-bucketrefused a bucket holding a state with a resource, leaving the front end up, then removed the front end and the bucket;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.teardown.sh, which kept its bucket and printed the removal command;gcloudcall.objectAdminmember is the provisioning account;ACTIVEwith a proxy URI, a 100 GB data disk and a 150 GB boot disk;<namespace>/<kind>s/<kind>_<id>/default.tfstatewith no local state file, and each destroy leaves 0 resources.terraform applyheld a network's lock at its approval prompt. The application's apply ran with-lock-timeout=45m, loggedAcquiring state lock, and completed with no changes once the holder was interrupted.force-unlockcommand, instead of waiting 45 minutes;force-unlockcleared the lock, and the apply then succeeded.state_bucket_writersline and the apply command.teardown.sh -y --delete-state-bucketrefused, listing only that state (the bucket's old object versions, mostly released locks, were ignored), and left the front end running;main.tfwas unchanged;--delete-state-bucket <earlier deployment>declined a wrongly typed bucket name, then deleted that bucket when the name was typed correctly;tf/'s state could not be read (a realterraform showfailure), and with the name line intf/terraform.tfvarsre-spaced and commented. The server and bucket were untouched each time;force_destroywas applied back to false;develop, following the admin guide:makemigrations/migrate, restart), the migration's dry run listed both and--yesmigrated both (Migrated: 2 Failed: 0);clusters/cluster_<id>/terraform.tfstate.premigration, outside the deployment group;terraform_state_bucketin the client'stf/terraform.tfvars,teardown.shstopped with "No value for required variable" and the front end stayed up; with the line added as the guide now says, teardown completed.backend.tfwas deleted in both directories, including the cluster's deployment group;Migrated: 2).Live, earlier heads of this branch (same project): every resource type end to end.
READYwith 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 rejectedadd-metadatacall.srun hostnamereturned exit code 0.objectAdminon the state bucket alone, ran a network's full lifecycle.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-unlockby 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.