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

Add auto-delete boot disk as an option on vm-instance #548

Conversation

nick-stroud
Copy link
Collaborator

If even only for testing, I often want to turn auto-delete off. This exposes the option without changing the default behavior.

Submission Checklist

  • Have you installed and run this change against pre-commit? (pre-commit install)
  • Are all tests passing? (make tests)
  • Have you written unit tests to cover this change?
  • Is unit test coverage still above 80%?
  • Have you updated all applicable documentation?
  • Have you followed the guidelines in our Contributing document?

Copy link
Member

@tpdownes tpdownes left a comment

Choose a reason for hiding this comment

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

Although I understand the value in broad strokes, it's not clear to me what workflow you are trying to enable. In particular, the way I read the code, updating var.startup_script should not cause the VM to be deleted/re-created.

modules/compute/vm-instance/variables.tf Outdated Show resolved Hide resolved
@tpdownes tpdownes assigned nick-stroud and unassigned tpdownes Sep 14, 2022
@nick-stroud nick-stroud force-pushed the vm-instance_auto-delete-boot-disk branch from 93a703a to cc76315 Compare September 14, 2022 15:46
@nick-stroud
Copy link
Collaborator Author

it's not clear to me what workflow you are trying to enable.

In the current state, If I have waited a long time for startup scripts to install something and then I want to tweak a machine parameter that forces replacement, I will lose the disk and have to wait for startup to run again.

@nick-stroud nick-stroud assigned tpdownes and unassigned nick-stroud Sep 14, 2022
@tpdownes
Copy link
Member

I have some concerns about effectively managing the destruction of the disk twice (explicitly and implicitly) that this change does not address. I also suspect you could address some of your concerns by shutting the machine down, running Terraform, and then turning it back on.

That said, it's a knob that arguably merits existence.

In medium term I think we should probably manage the creation and destruction of the boot disk entirely implicitly (with auto deletion either on or off) and allow the user to supply externally-created secondary disks.

@tpdownes tpdownes assigned nick-stroud and unassigned tpdownes Sep 14, 2022
@nick-stroud nick-stroud merged commit ea68256 into GoogleCloudPlatform:develop Sep 14, 2022
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.

2 participants