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 deletion of Guacamole VM in stopped state #4245

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

marrobi
Copy link
Member

@marrobi marrobi commented Jan 2, 2025

Draft - what think about this approach with a script to ensure child resources are deleted? Need to do other VMs.

Add a configuration option to Guacamole VM templates to skip shutdown and force deletion, resolving the issue where VMs in a stopped state cannot be deleted. Update relevant Terraform and Porter files to implement this change and increment version numbers accordingly.

Fixes #4135

Fixes microsoft#4135

Add `skip_shutdown_and_force_delete` configuration to Guacamole VM templates to fix deletion issue.

* Modify `main.tf` files in `templates/workspace_services/guacamole/user_resources/guacamole-azure-export-reviewvm/terraform`, `guacamole-azure-import-reviewvm/terraform`, `guacamole-azure-linuxvm/terraform`, and `guacamole-azure-windowsvm/terraform` to include `skip_shutdown_and_force_delete = true` under the `virtual_machine` block in the `features` section.
* Update `porter.yaml` files in `templates/workspace_services/guacamole/user_resources/guacamole-azure-export-reviewvm`, `guacamole-azure-import-reviewvm`, `guacamole-azure-windowsvm`, and `guacamole-azure-linuxvm` to increment the version numbers.
* Update `CHANGELOG.md` to include the fix for the deletion issue.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/microsoft/AzureTRE/issues/4135?shareId=XXXX-XXXX-XXXX-XXXX).
@marrobi
Copy link
Member Author

marrobi commented Jan 2, 2025

@tamirkamara @jonnyry attempt number 2, thoughts on this approach?

@marrobi
Copy link
Member Author

marrobi commented Jan 3, 2025

@dram1964 fancy testing this one?

@dram1964
Copy link

dram1964 commented Jan 3, 2025

Hi @marrobi, I did test the original fix, but didn't realise that this left the os_disk deployed. Happy to test out the new code, but I was wondering whether this is the best approach.
Given that the virtual_machine {skip_shutdown_and_force_delete = true } feature is designed to preserve sub-resources of the VM, the additional script is a 'fix for the fix'.
Would it be better to add an az vm start operation to the 'delete' step, since the 'delete' operation appears to expect the vm to be started?

@marrobi
Copy link
Member Author

marrobi commented Jan 3, 2025

Hi @marrobi, I did test the original fix, but didn't realise that this left the os_disk deployed. Happy to test out the new code, but I was wondering whether this is the best approach. Given that the virtual_machine {skip_shutdown_and_force_delete = true } feature is designed to preserve sub-resources of the VM, the additional script is a 'fix for the fix'. Would it be better to add an az vm start operation to the 'delete' step, since the 'delete' operation appears to expect the vm to be started?

Agree, that's another option, just worry about turning a VM on that has been intentionally stopped. Imagine it has a start-up script that does stuff, or you turn it off as it has some malware. I'd prefer to just get rid of it.

@tamirkamara
Copy link
Collaborator

tamirkamara commented Jan 5, 2025

Wouldn't it be easier to use azure cli to delete the VM if it's not on and then let TF do its thing without any other modification?
We already have the VM resource ID available so just need to query if it exists in the right state and delete.
p.s. there seem to be a PR on AzureRM for this. So probably should add a comment to any fix here that can be removed when will move to version 4.14 (maybe?)

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.

Unable to delete a Guacamole User Resource if it is in the 'stopped' state
3 participants