fix(vm): better handle of ctrl+c when qemu is not responding

[TheNotary]

Contributor's Note

• I have added / updated documentation in /docs for any user-facing features or additions.
• I have added / updated templates in /example for any new or updated resources / data sources.
• I have ran make example to verify that the change works as expected.

I'm still getting up to speed with GoLang, and I'm finding it very tricky to know whether my code is readable or not. Usually this type of refactor shortens the code up, but in this case it got longer so I'm reluctant raising this PR. (Was the deep nesting intentional to shorten the code?)

I had a hunch that using a golang tick pattern here might help, but I thought I'd just check in and see what others thought before racking my brain further. If someone wants to pick up this PR and finish the refactor, that would be great, it's been really slow going for me and I might learn faster just seeing the end form.

Proof of Work

To reproduce the results, in terraform, configure a VM to agent.enabled = true but then in the VM you're testing against, just delete the qemu-guest-agent and reboot. Thereafter, a terraform plan operation will likely hang, and ctrl + C will be unable to exit from the provider's operation because of the structure of the loop and the use of continue statements.

Below is a screenshot of this patch allowing ctrl + c to be captured.

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment 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 pull request followers and do not help prioritize the request

[otopetrik]

Thanks, it makes the code look more readable.

Improving the behavior on Ctrl-C with Proxmox-enabled yet VM-missing qemu-guest-agent would be nice.

Current behavior:

• terraform apply, creating VM and waiting for it. Ctrl-C handled quickly
• terraform plan, refreshing state, Ctrl-C handled quickly
• terraform apply, refreshing state, Ctrl-C requires second press

Tried the PR, the behavior seems the same (tested on Terraform v1.5.3, newer versions may differ...)

Tried modifying it to checking ctx.Deadline and ctx.Done, but with no luck.
It looks like the ctx has no idea, that it should stop when Ctrl-C is pressed in "refreshing state" as part of terraform apply.
Which is weird, because terraform plan and terraform apply should probably do the same thing, at least until the they display the plan.

TF_LOG=trace terraform apply shows after Ctrl-C:

[TRACE] provider.terraform-provider-proxmox_v0.35.0: plugin received interrupt signal, ignoring: count=1 timestamp=2023-10-21T...
it seems to be from here https://github.com/hashicorp/go-plugin/blob/main/server.go#L448

That gave me an idea for the following modification:

	ch := make(chan os.Signal, 1)
	signal.Notify(ch, os.Interrupt)
	for timeElapsed.Seconds() < timeMaxSeconds {
		timeElapsed = time.Since(timeStart)

		// check if terraform wants to shut us down (we try to poll the ctx every 200ms)
		if ctx.Err() != nil {
			return nil, fmt.Errorf("error waiting for VM network interfaces: %w", ctx.Err())
		}

		select {
		case <-ch:
			{
				return nil, fmt.Errorf("error by signal")
			}
		default:
		}
                ....

terraform apply can then be terminated in planning stages using single Ctrl-C.

Getting signal directly instead of using contex seems like an ugly hack, but I no idea if there even is a non-hacky solution.

[TheNotary]

Didn't notice this until just now. First thanks for the code quality feedback! About the difference in behavior between apply vs plan, that's interesting and unfortunate to learn, I hadn't been monitoring apply in my investigation. I quick google found a post from timblacktu announcing his annoyance with TF tending to hang, and him ctrl + cing (twice I assume) and then restoring his state file.

If your sigint check does the trick, I say go for it. I'm getting the impression, Hashicorp didn't spend a lot of time what to do when a user ctrl + cs an apply operation. I don't remember how I adopted my TF workflow, but I catch myself always performing "plan" followed by "apply". I wonder if I picked up on that because plan is easier to bail from on most TF plugins??? Since working with this plugin provider, I've started to jump right to apply because refreshing the state takes a while (maybe it can be speed up, or maybe my infrastructure is too dated to be doing anything in 2023).

I accidentally clicked the update branch but I pushed it back to how it was where I left off. It looks like you're using a case/switch that looks more elegant. If you'd like to own this fix from here, I'd appreciate it. I'm away from my infrastructure at the moment so it would be difficult for me to do testing until I'm done traveling.

[bpg]

It looks like the new linter started complaining about some older code not related to this PR. #645 should fix it, so please merge / rebase on main

[bpg]

Thanks for the PR, @TheNotary!

I feel like most of those "sleep & loop" waiting patterns can be updated to something more elegant, there are bunch of libraries around that can handle waiting, with different type of strategies, better error handling, etc, etc., but even this improvement is great!

I've incorporated @otopetrik 's suggestion and tested in my env, everything seems to be working as expected, so

LGTM! 🚀