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

Allow to recreate VMs if in DONE state #563

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

Thiryn
Copy link
Contributor

@Thiryn Thiryn commented Jul 22, 2024

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for PR followers and do not help prioritize the request

Description

Detect done state for VMs and reconcile them via recreation.
Also renames vm variable into vmInfo to prevent collision with the github.com/OpenNebula/one/src/oca/go/src/goca/schemas/vm package import.

References

Fixes #562

New or Affected Resource(s)

  • opennebula_virtual_router_instance
  • opennebula_virtual_machine

Checklist

  • I have created an issue and I have mentioned it in References
  • My code follows the style guidelines of this project (use go fmt)
  • My changes generate no new warnings or errors
  • I have updated the unit tests and they pass succesfuly
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation (if needed)
  • I have updated the changelog file

@Thiryn
Copy link
Contributor Author

Thiryn commented Jul 22, 2024

Will update changelog and docs if this gets approved

Copy link
Collaborator

@treywelsh treywelsh left a comment

Choose a reason for hiding this comment

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

It's not a final review but a bunch of considerations to open a discussion and dig a bit more


// waitForVMState waits until the VM with vmId has the desiredState.
// returns an error if timeout is reached.
func waitForVMState(vmId int, desiredState vm.State, timeout time.Duration) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

VM already has some VM state checking methods, should we share/reuse some code or should we consider that it's ok to have separated test code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I probably missed that, if there is something to be reuse, to wait for the state then we definitely should use it, can you point me towards it?

Copy link
Collaborator

@treywelsh treywelsh Aug 29, 2024

Choose a reason for hiding this comment

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

d.Set("gname", vmInfo.GName)
d.Set("state", vmInfo.StateRaw)
d.Set("lcmstate", vmInfo.LCMStateRaw)
if vm.State(vmInfo.StateRaw) == vm.Done {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it the proper way to do ?
There's a -replace option to terraform
And if we consider it's ok, do we want this behavior everytime, for everyone or should it be conditioned by an other attribute with a name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, unsetting the "ID" parameter is the way to tell terraform that the previously existing resource no longer exists (see an example on AWS's provider here.

A VM in done state can be considered as non-existing anymore. As it is expected that terraform recreates the resource when they do not exist anymore, this behaviour should be default.

An example for that expectation is drift checks, if you create a VM, someone deletes it from the UI, the expectation is that terraform will tell you there is a drift.

The thing to consider is the non-backward compatible behaviour this brings to the vm resource, this might be worth of a major version release.

Maybe this could be behind a recreate_on_delete boolean on the resource on an initial minor update, with a deprecation notice for the next major update.
This also affects the vrouter resource.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@frousselet what do you think on this ?
Should we consider this behavior to be protected by a recreate_on_delete attribute ?

Copy link

This pull request is stale because it has been open for 30 days with no activity and it is not in a milestone. Remove 'status: stale' label or comment, or this will be closed in 5 days.

@Thiryn
Copy link
Contributor Author

Thiryn commented Sep 29, 2024

nope, not stale...

@dgarcia18 dgarcia18 self-assigned this Oct 17, 2024
@dgarcia18
Copy link
Contributor

@Thiryn your work on this is much appreciated. Since it's a bug fix, we don't anticipate it breaking backwards compatibility. We'll go ahead and proceed with the merge

@dgarcia18 dgarcia18 merged commit 8236db8 into OpenNebula:master Oct 17, 2024
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.

Manually Deleted VMs are not recreated
3 participants