Repository navigation
Clean up backup E2E workspaces after failed provisioning - #5171
Closed
Jay W (JayDoubleu) wants to merge 1 commit into
Closed
Jay W (JayDoubleu) wants to merge 1 commit into
Jay W (JayDoubleu) wants to merge 1 commit into
Conversation
This was referenced Oct 9, 2026
Unit Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 2b48a91. |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused lifecycle change satisfies the stated acceptance criteria and is covered by passing regression tests.
0 open findings
What changed in this PR
Updates backup E2E tests to reliably clean up workspaces after provisioning or assertion failures while preserving the original error.
Changes:
- Uses
temporary_resource()for workspace lifecycle management. - Adds six cleanup regression tests.
- Records the fix in the Unreleased changelog.
| File | Description |
|---|---|
CHANGELOG.md |
Records the cleanup fix. |
e2e_tests/test_backups.py |
Adopts failure-safe temporary resource handling. |
e2e_tests/unit_tests/test_backup_workspace_cleanup.py |
Tests success, failure, cleanup, and rejected-creation paths. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
This was referenced Oct 10, 2026
Collaborator
Author
Collaborator
Author
Jay W (JayDoubleu)
added
the
v0.30.0-prep-v1
Jay W (JayDoubleu)
added a commit
that referenced
this pull request
Oct 29, 2026
) * Clean up backup test workspaces after failure (#5135) Source PRs: #5171 * Make one Azure Backup registration attempt before Terraform registers (#5135) Every backup-enabled workspace failed to deploy. Azure Backup returned CloudInternalError when Terraform registered the workspace storage account with the new vault. A scratch reproduction in switzerlandnorth showed that the first registration in a new vault usually fails, whatever the storage settings: 4 of 5 new vaults failed it, as did 3 of 3 TRE deployments. Every later registration in the same vault succeeded (9 of 9). A failed attempt leaves no container behind. A new step runs before the Terraform registration: - If the storage account is already registered, it does nothing. - Otherwise it makes one registration attempt and waits for the result. - If that attempt succeeds, it removes the registration again, so that Terraform creates and owns it. A scratch Terraform apply covered a new vault and a re-run on a registered account. Unit tests run the step against a fake Azure CLI. * Try the backup registration warm-up up to five times (#5135) In two live runs, the warm-up attempt failed with CloudInternalError, and the Terraform registration right after it failed with the same error. In two earlier runs, the Terraform registration passed after a failed warm-up attempt. The warm-up now tries up to five times, with 60 seconds between the attempts. It removes the first registration that succeeds, so that Terraform creates and owns the registration. After five failed attempts, Terraform makes its own attempt, as before. * Wrap a long CHANGELOG entry (#5135) The Markdown lint rejects a line with more than 400 characters. The entry now continues on lines that start with two spaces, as an earlier entry in the file does. The rendered text does not change. * Validate OpenAI model prerequisites and private lifecycle (#5150) * Create the OpenAI private endpoint after the model deployment (#5249) The private endpoint and the model deployment depended only on the account, so Terraform created them at the same time. Azure rejected the endpoint with AccountProvisioningStateInvalid while the deployment updated the account. Make the endpoint depend on the deployment, and check this order in the bundle contract test. * Disable public network access on the MySQL server (#5148) The MySQL flexible server had public network access enabled. It had no firewall rules, so no public client could connect, but the server kept a public endpoint. A live MySQL run in Switzerland North showed publicNetworkAccess Enabled. The Azure SQL bundle disables public access, and the SQL test checks it. The server now sets public_network_access to Disabled, so that clients can connect only through the private endpoint. azurerm 3.117.0 cannot set this value, so the bundle moves to azurerm 4.27.0, as the Gitea and OHDSI bundles use. In 4.27.0 a change of this value is an in-place update, so an upgrade keeps the server and its data. * Add a private MySQL lifecycle check (#5148) The MySQL bundle selection ran only the generic install test. It did not show that a workspace client can use the database. The new case creates a workspace, a Guacamole service, a Linux VM and the MySQL service. The Linux VM installs the MySQL client through Nexus, reads the administrator password from the workspace Key Vault through a temporary role assignment on that secret, and writes a row through private DNS with TLS. The case then changes the SKU from Burstable to General Purpose and reads the row again. The SQL lifecycle helper now accepts its deadline variable, enters Nexus as a prerequisite that leaves after the VM, and can get a new token for clean-up, because this case can outlive the create token. * Give the MySQL probe script a shebang (#5148) The Lint job of the MySQL pull request failed on e2e_tests/resources/mysql_probe.sh. Shellcheck reported SC2148, because the file had no shebang, and bash-exec reported that the file was not executable. The probe code added the shebang only to the generated script. The file now starts with the shebang and is executable. The probe code puts the validated values after the first line, so the generated script does not change. * Run the Linux VM actions in the Guacamole Linux VM check (#5145) The Linux VM check proved the install, the Nexus-backed bootstrap and the delete. It did not run the stop, start and reset_password actions. After the bootstrap, the check now runs stop and start and reads the VM power state from Azure after each action. It then runs reset_password and makes sure that the Key Vault secret with the VM credentials has a new version. The check reads only the secret metadata through Azure Resource Manager, not the secret value. The check now requests the workspace owner token after Nexus, because the VM delete uses it and Nexus can take a long time. * Retry the backup registration reads and the unregister of the warm-up (#5135) The warm-up step read any error of a registration read as "not registered". On an upgrade, one throttled read could start a PUT and a DELETE on the registration that Terraform owns. The step now treats only a not-found answer as "not registered". It reads two more times after another error, and then it fails. The step sent the unregister one time. Azure can answer it with a short CloudInternalError, and the step then failed and left a registration that Terraform cannot create. The step now sends the unregister again each minute while the registration exists. The registration URL has no host, so az rest uses the Resource Manager endpoint of the current cloud. * Use one workspace owner token for the MySQL and Linux VM cleanup (#5148) The MySQL test asked for a new owner token before each step and gave the cleanup a function that asked for another one. The stated reason was that a long case can outlive the token. But the owner token is a TokenProvider, and get_auth_header gets a current token from it for each request. So one token stays valid for the whole case and its cleanup. The cleanup token parameter, its unit test and the comment in the Linux VM test are removed. * Stop pinning the OpenAI bundle version in the unit tests (#5150) Two unit tests compared the OpenAI bundle version with "1.1.0". The E2E helper tests run on each change to a porter.yaml file, so the next version bump of the bundle would fail them. The schema test now accepts 1.1.0 or later, because 1.1.0 is the first version with the new default model. The selection test compares the report with the version in porter.yaml. The coverage text says "1.1.0 or later". * Use the bundle cloud and login mode in the backup warm-up and wait for a final status (#5135) The warm-up logged in with the managed identity of the default cloud. It now selects the cloud from AZURE_ENVIRONMENT and logs in with the service principal when ARM_USE_MSI is false, as the resource processor does. The defaults keep the managed identity in AzureCloud. A failed read of the registration status ended the wait, and a status that stayed InProgress let the next attempt start. Another PUT could then overlap a running registration. The step now reads the status again after a failed read, and it fails if no final status comes within 10 minutes. * Check the MySQL settings after the SKU change and keep the VM actions outside AzureCloud (#5148, #5145) The MySQL case checked public network access and secure transport only before the SKU change. It now checks both again after the change, so the case fails if the update turns either setting back. The Linux VM checks read the power state and the secret version through a Resource Manager client that supports AzureCloud only. In other clouds, the case now runs the three actions without these reads. * Stop the backup warm-up when an accepted registration cannot be followed (#5135) If Azure accepted the registration PUT but the step found no operation header, the step waited 120 seconds and went on. A missing registration does not prove that the operation ended, so a later PUT or the Terraform registration could overlap it. The step now keeps the result of the PUT and reads the operation header in any case. After a rejected PUT, it waits and tries again, as before. After an accepted PUT without an operation, it goes on only if the registration exists, and it fails otherwise. * Wait for the backup registration status when no operation is returned (#5135) Azure can accept the registration PUT without an Azure-AsyncOperation header. Then the warm-up had no operation to follow, and it treated an existing container as a completed registration. The container can exist before the registration ends. So the warm-up now waits until the container shows the status Registered, for 10 minutes at most. If the status does not come, the step fails and does not send the unregister. If the unregister still fails after 10 minutes, the error now tells the operator to unregister the storage account in the vault and to update the workspace again. A comment at the early return explains why the step does not remove a registration that exists. * Unregister a warm-up registration only after it ended (#5135) An error from the registration PUT does not prove that Azure rejected it. For example, Azure can accept the PUT and the response can get lost. Then the warm-up treated the attempt as rejected, and after 120 seconds it sent the unregister if the container existed, also when the registration still ran. Now one check runs before each unregister, for all attempt results: the container must show the status Registered within 10 minutes. If it does not, the step fails and does not send the unregister. The attempt without an operation to follow uses the same check. * Stop the backup warm-up when a PUT gets no answer from Azure (#5135) The warm-up treated each failed registration PUT as a rejection. But Azure can accept a PUT whose response gets lost, and the container of that registration can show some minutes later. Then the warm-up sent the next PUT while the first registration still ran. After five such attempts, it ended with success, and Terraform started its own registration over the running one. With --debug, az rest prints the response status when Azure answers. Now the warm-up treats a failed PUT as a rejection only when Azure answered with a 4xx or 5xx status. Without an answer, the result is unknown, so the step stops, and no other registration overlaps it.
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.
When a backup workspace fails provisioning,
post_resource()raises before the test receives its path. The existingfinallyblock then skips cleanup and leaves the workspace behind. Use the existingtemporary_resource()helper to capture ownership before polling and attempt teardown after deployment or assertion failures. Preserve the original failure if teardown also fails.Related to #5170. Refs #5135. Draft follow-up outside v0.30.0; non-blocking for that release. This fixes the test cleanup defect, not the unresolved Azure Backup
CloudInternalErrorduring storage-account registration.Backup-enabled/disabled assertions and
delete_backups_on_uninstall=truefor the disposable backup-enabled workspace remain unchanged. No Terraform, network, identity, provider, dependency or workflow definitions change. E2E tests have no independently versioned component; the change is recorded in Unreleased.Validation:
git diff --checkpassed.2b48a91f3. GitHub CI results are tracked on this draft.Keep #5135 open for the Azure registration investigation and later backup validation. The current v0.30.0 candidate is unchanged.