Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix VM actions when workspace storage doesn't allow shared key access #4222

Merged
merged 2 commits into from
Dec 26, 2024

Conversation

tamirkamara
Copy link
Collaborator

Resolves #4221

What is being addressed

Workspace storage that doesn't allow shared key access blocks any VM activity post initial deployment.

How is this addressed

  • The TF data object can't be used as that uses shared key to access Azure Files. Since the only usage of the object is to get the share name which we anyway receive as a variable (and pass in the data object), we can just remove it and use the variable directly. This will remove the need for TF to reference the Azure Files object and avoid this problem.

Copy link

github-actions bot commented Dec 25, 2024

Unit Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 2299dd1.

♻️ This comment has been updated with latest results.

@tamirkamara
Copy link
Collaborator Author

/test-extended

Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/12493778693 (with refid fc97343a)

(in response to this comment from @tamirkamara)

@jonnyry
Copy link
Collaborator

jonnyry commented Dec 25, 2024

Hi @tamirkamara have you seen this error when running the extended e2e before? I received the same error running error on my azurerm version upgrade PR as well…

https://github.com/microsoft/AzureTRE/actions/runs/12483029966

Trying to figure out what’s causing the osdisk resource to be left over, but not had much luck so far.

@tamirkamara
Copy link
Collaborator Author

@jonnyry I don't know how but the behavior of the os disk auto delete has changed and removing the VM doesn't auto delete its disks. I checked and the relevant TF setting has existed in AzureRM 3+ so this isn't it probably... At any case, I will update this PR retest and see if it works.

@tamirkamara
Copy link
Collaborator Author

Actually, this is where it comes from: https://registry.terraform.io/providers/hashicorp/Azurerm/latest/docs/guides/features-block#skip_shutdown_and_force_delete-1
@marrobi was too fast on the trigger here (and me as well to approve :-)) I will revert the prior change.

@tamirkamara
Copy link
Collaborator Author

/test-extended

Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/12495737490 (with refid fc97343a)

(in response to this comment from @tamirkamara)

@jonnyry
Copy link
Collaborator

jonnyry commented Dec 25, 2024

Actually, this is where it comes from: https://registry.terraform.io/providers/hashicorp/Azurerm/latest/docs/guides/features-block#skip_shutdown_and_force_delete-1 @marrobi was too fast on the trigger here (and me as well to approve :-)) I will revert the prior change.

Ah great spot thanks 🙏

@marrobi
Copy link
Member

marrobi commented Dec 26, 2024

@tamirkamara
Copy link
Collaborator Author

tamirkamara commented Dec 26, 2024

Sorry, my fault. Does it need this?

https://registry.terraform.io/providers/a2m1/azurerm/latest/docs/guides/features-block#delete_os_disk_on_deletion-1

@marrobi No. First, that last setting is anyway set to true. Secondly, the setting you used is meant for immediate delete while keeping the resources behind to make them available for attachment to another VM.

Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/12495737490 (with refid fc97343a)

(in response to this comment from @tamirkamara)

Copy link
Collaborator

@LizaShak LizaShak left a comment

Choose a reason for hiding this comment

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

LGTM. Left only one tiny comment

@tamirkamara
Copy link
Collaborator Author

/test-force-approve
The pipeline failed in an unrelated place.

Copy link

🤖 pr-bot 🤖

✅ Marking tests as complete (for commit 2299dd1)

(in response to this comment from @tamirkamara)

@tamirkamara tamirkamara merged commit fd4debf into main Dec 26, 2024
12 checks passed
@tamirkamara tamirkamara deleted the tamirkamara/4221-fix-shared-storage branch December 26, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shared storage can block VM activities
5 participants