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

--on-error=ask: wait for the user input #226

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

edigaryev
Copy link
Contributor

@edigaryev edigaryev commented Mar 14, 2024

Problem: currently, when using --on-error=ask it picks a default askCleanup value if the user hasn't provided an input after 100 milliseconds, which seems to be almost impossible to do as a human.

Solution: simplify the logic and fully wait for the user's input when --on-error=ask is specified.

@nywilken
Copy link
Contributor

Hi @edigaryev thanks for opening up this change request. It is unlikely that we will accept this PR that removes the cancellation altogether because we don't want Packer to sit and wait around indefinitely. When running with --on-error=ask we expect that the user is running Packer interactively in order to respond to the prompt.

I can see your rationale about 100 milliseconds not being enough time so I think we could consider bumping the wait time a little but not remove it.

That said, looking at the linked Tart PR. If a user is cancelling the Packer run with CTRL-C I would expect the behavior being reported, Packer deletes anything it created. If the users wishes to debug a created VM they can use the --debug flag to step through each step or add a debug provisioner.

If you believe bumping the wait time is not enough please open an issue with your use case so that we can better understand what you are trying to solve by running a Packer build with on-error=ask.

@torarnv
Copy link

torarnv commented Mar 14, 2024

That said, looking at the linked Tart PR. If a user is cancelling the Packer run with CTRL-C I would expect the behavior being reported, Packer deletes anything it created.

So pressing Ctrl+C does not constitute an error, and as a result -on-error= should not be consulted?

That's unfortunate, as a user pressing Cltr+C may in many cases be because there was (semantically) an error during the provisioning e.g., that was not reported back as a failure.

As the default option for -on-error is cleanup, I don't see the harm in respecting -on-error in this case (Ctrl+C) as well? Without the option, Ctrl+C will clean up, as you say. But if the user wishes to do something else, we respect that?

If that's not an option, then Packer should at least skip the whole on-error logic. Right now, it prints the query, which would indicate it is respected:

==> macos.tart-cli.install-os: Creating virtual machine...
    macos.tart-cli.install-os: Installing OS...
    macos.tart-cli.install-os: 0%
Cancelling build after receiving interrupt
==> macos.tart-cli.install-os: Failed to create a VM: Bad exit status: -1
==> macos.tart-cli.install-os: Step "stepCreateVM" failed
==> macos.tart-cli.install-os: [c] Clean up and exit, [a] abort without cleanup, or [r] retry step (build may fail even if retry succeeds)? a
==> macos.tart-cli.install-os: Cleaning up virtual machine...
    macos.tart-cli.install-os: failed to open lock file file:///Users/torarne/.tart/vms/macos-preinstall:13.6.0+22G120/config.json: No such file or directory
Build 'macos.tart-cli.install-os' errored after 1 second 874 milliseconds: Failed to create a VM: Bad exit status: -1

@edigaryev
Copy link
Contributor Author

It is unlikely that we will accept this PR that removes the cancellation altogether because we don't want Packer to sit and wait around indefinitely.

The Packer already sits and waits around indefinitely with the current logic in place, so no change here:

packer

If you believe bumping the wait time is not enough please open an issue with your use case so that we can better understand what you are trying to solve by running a Packer build with on-error=ask.

I'll pass this to @sparshev here, as he was the one who was requesting this Ctrl+C behavior in the first place: cirruslabs/packer-plugin-tart#115.

But overall I think this is a bad UX and needs to be fixed either way.

@sparshev
Copy link

Sorry @edigaryev , but it looks like miscommunication, my original request was about the Tart VM stops immediately in case I press Ctrl-C even with --on-error=ask. With VMX packer plugin I did not saw this happening, so I suspect Int signal is somehow passed to the Tart VM process through the packer and plugin or the process is killed due to out of scope. Quite sure it's not the issue with the packer or sdk.

So I will test tart packer plugin again in the near future and come back with an ticket or will not in case it's working as expected - maybe the latest changes for cleanup are working and preventing Int signal to be passed to Tart VM, so still will need to test the latest plugin.

@torarnv
Copy link

torarnv commented Mar 27, 2024

@nywilken Please reconsider this PR. This is the current behavior of Packer when pressing Ctrl+C:

With explicit -on-error=cleanup, cleans up as expected:

❯ packer build -on-error=cleanup -parallel-builds=1 -only='*provision' templates/macos.pkr.hcl
macos.tart-cli.provision: output will be in this color.

==> macos.tart-cli.provision: Cloning virtual machine...
==> macos.tart-cli.provision: Updating virtual machine resources...
==> macos.tart-cli.provision: Inspecting machine disk image...
==> macos.tart-cli.provision: Getting partition table...
==> macos.tart-cli.provision: Starting the virtual machine...
==> macos.tart-cli.provision: Successfully started the virtual machine...
Cancelling build after receiving interrupt
==> macos.tart-cli.provision: Waiting for the tart process to exit...
==> macos.tart-cli.provision: Waiting for SSH to become available...
==> macos.tart-cli.provision: Cleaning up virtual machine...
Build 'macos.tart-cli.provision' errored after 2 seconds 16 milliseconds: Build was cancelled.

==> Wait completed after 2 seconds 16 milliseconds
Cleanly cancelled builds after being interrupted.

With explicit -on-error=abort, aborts as expected, skipping stepCleanVM:

❯ packer build -on-error=abort -parallel-builds=1 -only='*provision' templates/macos.pkr.hcl
macos.tart-cli.provision: output will be in this color.

==> macos.tart-cli.provision: Cloning virtual machine...
==> macos.tart-cli.provision: Updating virtual machine resources...
==> macos.tart-cli.provision: Inspecting machine disk image...
==> macos.tart-cli.provision: Getting partition table...
==> macos.tart-cli.provision: Starting the virtual machine...
==> macos.tart-cli.provision: Successfully started the virtual machine...
Cancelling build after receiving interrupt
==> macos.tart-cli.provision: Interrupted, aborting...
==> macos.tart-cli.provision: Waiting for SSH to become available...
==> macos.tart-cli.provision: aborted: skipping cleanup of step "stepRun"
==> macos.tart-cli.provision: aborted: skipping cleanup of step "stepDiskFilePrepare"
==> macos.tart-cli.provision: aborted: skipping cleanup of step "stepSetVM"
==> macos.tart-cli.provision: aborted: skipping cleanup of step "stepCloneVM"
==> macos.tart-cli.provision: aborted: skipping cleanup of step "stepCleanVM"
Build 'macos.tart-cli.provision' errored after 736 milliseconds 341 microseconds: Build was cancelled.

==> Wait completed after 736 milliseconds 373 microseconds
Cleanly cancelled builds after being interrupted.

With explicit -on-error=ask, halts and asks user, pausing forever:

❯ packer build -on-error=ask -parallel-builds=1 -only='*provision' templates/macos.pkr.hcl
macos.tart-cli.provision: output will be in this color.

==> macos.tart-cli.provision: Cloning virtual machine...
==> macos.tart-cli.provision: Updating virtual machine resources...
==> macos.tart-cli.provision: Inspecting machine disk image...
==> macos.tart-cli.provision: Getting partition table...
==> macos.tart-cli.provision: Starting the virtual machine...
==> macos.tart-cli.provision: Successfully started the virtual machine...
Cancelling build after receiving interrupt
==> macos.tart-cli.provision: Waiting for SSH to become available...
==> macos.tart-cli.provision: Step "StepConnect" failed
==> macos.tart-cli.provision: [c] Clean up and exit, [a] abort without cleanup, or [r] retry step (build may fail even if retry succeeds)?

But, regardless of what the answer is:

==> macos.tart-cli.provision: [c] Clean up and exit, [a] abort without cleanup, or [r] retry step (build may fail even if retry succeeds)? 
c
==> macos.tart-cli.provision: Waiting for the tart process to exit...
==> macos.tart-cli.provision: Cleaning up virtual machine...
Build 'macos.tart-cli.provision' errored after 59 seconds 419 milliseconds: Build was cancelled.

==> Wait completed after 59 seconds 419 milliseconds
Cleanly cancelled builds after being interrupted.
==> macos.tart-cli.provision: [c] Clean up and exit, [a] abort without cleanup, or [r] retry step (build may fail even if retry succeeds)? 
a
==> macos.tart-cli.provision: Waiting for the tart process to exit...
==> macos.tart-cli.provision: Cleaning up virtual machine...
Build 'macos.tart-cli.provision' errored after 2 seconds 482 milliseconds: Build was cancelled.

==> Wait completed after 2 seconds 482 milliseconds
Cleanly cancelled builds after being interrupted.
==> macos.tart-cli.provision: [c] Clean up and exit, [a] abort without cleanup, or [r] retry step (build may fail even if retry succeeds)? 
r
==> macos.tart-cli.provision: Waiting for the tart process to exit...
==> macos.tart-cli.provision: Cleaning up virtual machine...
Build 'macos.tart-cli.provision' errored after 2 seconds 224 milliseconds: Build was cancelled.

==> Wait completed after 2 seconds 224 milliseconds
Cleanly cancelled builds after being interrupted.

My expectation is that the behavior of ask matches the explicit clean and abort behaviors when I make the final choice.

It there something missing on the Tart Packer plugin to support this? Or is it a bug in Packer that the command-line vs ask behaviors don't match?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants