After the patch, the output will be:

@nywilken Can you help to have a review?

@lbajolet-hashicorp As you said, we can keep the error volume for troubleshooting, I have removed the lines for bad volume cleanup, can you review it again?

@lbajolet-hashicorp any change to review again?

Hi @zhenggu,

Apologies it took me a bit to revisit this PR again.

I'm not sure I said something to the effect of "keeping the error volume for troubleshooting", but this would be valid, however I'm not certain this is a good approach broadly speaking.

The GetVolumeStatus call succeeds and returns a volume state of "error", we don't know necessarily why at this point, but all we know is that we won't be able to build, do we expect to be able to get insights on why the volume errorred after the VM gets torn down?
It feels like cleaning-up should happen to me, and if you're troubleshooting a build, this is a job for the --on-error flag so Packer doesn't automatically cleanup everything before you tell it to, this way you can inspect your VM/volumes.

I'm worried that systematically considering volume provisioning errors as an OK scenario and not cleaning-up after us will mean that there'll be a bunch of volumes to cleanup manually after the fact, so I'm cautious on this change.

One thing that I probably wasn't clear with in the first place (sorry about that), was regarding the cleanup strategy, I see we have a doCleanup flag which kinda feels like a duplicate for volumeID since both are set at the same time. Unless there's an option to not cleanup the volume after the build maybe?

I would suggest maybe revisiting the logic in the Run for the step:

1. Can we remove doCleanup? If it has an external use (linked to a configuration flag for example) we can't, but otherwise at first glance it looks like duplicated logic to me.
2. Regarding volumeID, we should have it created once the volume exists, it should be removed after the build ends, whether we successfully created the volume, or if there was an error during provisioning.
3. At Cleanup, the logic can be simplified to return immediately if volumeID is empty instead of looking at doCleanup
4. I'm not sure we need to call GetVolumeStatus at cleanup, do we? If the volumeID is not empty, this means a volume needs to be cleaned-up, if it failed to be created in the first place, the check above will take care not to execute anything here.
5. In the end we can just call Delete on the volume, not caring about its state before that, unless I'm missing something in which case please feel free to clarify this.

Let me know if there's something else you need from me, and if you need help with the code feel free to ping, not sure when I'll be able to, but I can take a look at it as well.

Thanks!

@lbajolet-hashicorp I am totally agree with you, will submit a patch soon.

@lbajolet-hashicorp I have updated the patch to restruct volume clean up logic, and no issue on testing, please have a review again, thanks!

@lbajolet-hashicorp have updated the code, please have a review again.