Fix #121

[remi-espie]

After working on #115 (which fixed #7), a bug was discovered when using 2 mutually exclusives parameters: skip_export and keep_registered. This PR fixes this by ensuring both parameters are not set together and explaining it in the documentation.

This PR closes #121.

[remi-espie]

I wonder though, where does the incompatibility between the two options stem from? Logically I'd think both can be used together, and while warning that they can't is a solution here, I wonder if we can make it work for this case as at first glance it seems they shouldn't be mutually exclusive.

Hello again,

As of right now, both options are incompatible because skip_export would try to move the VM files to the output_directory (default or set by the user, as said in doc). However, if you move the VM files, then you can't access it from the temp_path. And Hyper-V, which has kept the VM registered in the temp_path (thanks to keep_registered) is not notified that the VM files have changed location, making it not working (error: files not found). Plus, it would be kind of illogical to have both parameter set and working at the same time. With skip_export, the files are moved to another directory. With keep_registered, I believe that most user would not want the VM files to be moved.

We may want to change the VM file location to the output_directory in Hyper-V if both parameter were to be set at the same time, but I don't know how to do that while keeping the VM registered in Hyper-V.

What do you think ?

[lbajolet-hashicorp]

Hi @remi-espie,

I believe there's a documentation issue maybe here, my understanding was that skip_export would do nothing when it came to building the output artifact, but looking at the code, the option moves the disk to the output directory, leaving the rest of the files in tmp_path I presume? I understand why the reported error happens then, the code you submitted makes sense, however I would maybe suggest changing the configuration structures to make the behaviour of the option clearer.

Other plugins have a skip option typically (for example the Amazon plugin has skip_create_ami, which doesn't create AMI/Snapshots from the instance), this one behaves quite differently, which threw me off a bit. I would suggest eventually renaming the option to keep_only_disk (or something in the same vein) to make it clearer that there is some data exported by the plugin, just not everything that makes up a VM.

Then I would think we can introduce an option that completely skips exporting anything from the build directory.

Alternatively, I would think we can change the PS code for the MoveCreatedVHDsToOutputDir function also, so that it Copy-Item instead of Move-Item for the disks, preserving the complete structure of the VM in case keep_registered is set, and essentially changing nothing otherwise.

What do you think? Would the alternative make sense? The implementation may be quite simple if my understanding is correct, but please feel free to correct me if I'm mistaken.

Thanks!

[remi-espie]

Hi again,

the [skip_export] option moves the disk to the output directory, leaving the rest of the files in tmp_path I presume?

After trying, yes indeed. It leaves a few Hyper-V configuration files.

I would suggest eventually renaming the option to keep_only_disk

Sure, it would make more sense indeed. I would have renamed it export_disk_only, I think it emphasize better that the disk will be moved to the output_directory. However, should we break compatibility or ensure that skip_export is still a valid parameter (with a warning saying that the parameter name has changed) ? What would you advise ?

Then I would think we can introduce an option that completely skips exporting anything from the build directory.

I believe that is exactly why I started using keep_registered. In top of keeping the VM registered in Hyper-V, everything stays in the build directory. In addition, the temp_path is actually the build directory, so changing this variable allows to have the VM build directory wherever you want.

We could also rename this parameter, in which case, same question as before: should I add a warning ?

Alternatively, I would think we can change the PS code for the MoveCreatedVHDsToOutputDir function also, so that it Copy-Item instead of Move-Item for the disks, preserving the complete structure of the VM in case keep_registered is set, and essentially changing nothing otherwise.

I understand where you'r going with that, but I have to say I'm not a fan of having the VM file (which can be quite large, particularly for Windows VM) copied twice on the machine. Plus, Windows never cleans its %TEMP% directory, so it would result in "bloated" machines and user not understanding why their disk is full... So to be clear, I would not do that myself.

As always, thanks for the review and comments ^^

[lbajolet-hashicorp]

Hi @remi-espie,

Sure, it would make more sense indeed. I would have renamed it export_disk_only, I think it emphasize better that the disk will be moved to the output_directory. However, should we break compatibility or ensure that skip_export is still a valid parameter (with a warning saying that the parameter name has changed) ? What would you advise ?

In general I advise not breaking without warning :) For the next version of the plugin, we have to preserve compatibility, and start warning about the deprecated option. Since skip_export is ambiguous, I suggest deprecating it by showing a warning if it is defined (during Prepare is probably the best time to do so), hinting at its replacement once we decided how to name it. export_disk_only sounds good to me!

Note: this can be done in a separate PR to be clear, we don't need to cram every improvement in there :)

I believe that is exactly why I started using keep_registered. In top of keeping the VM registered in Hyper-V, everything stays in the build directory. In addition, the temp_path is actually the build directory, so changing this variable allows to have the VM build directory wherever you want.

And I think it makes sense, the more I think about it, the more I'm thinking maybe keep_registered should change the export to not do anything at all instead of copying/compressing the data from the temp_dir into output_directory.

We could also rename this parameter, in which case, same question as before: should I add a warning ?

I would advise against that, keep_registered is well-named imho, we probably don't need to do anything with it.

I understand where you'r going with that, but I have to say I'm not a fan of having the VM file (which can be quite large, particularly for Windows VM) copied twice on the machine. Plus, Windows never cleans its %TEMP% directory, so it would result in "bloated" machines and user not understanding why their disk is full... So to be clear, I would not do that myself.

I tend to agree with you in principle, that being said, the fact that keeps_registered works in tandem with skip_export = false (default) means that we already have duplicated data on disk, since the data for the complete VM (disks included) are exported as the build artifact in output_directory, in addition to not cleaning up temp_directory. In this sense, I would think that copying the disk instead of moving it is likely an intended consequence of both options being picked. The one downside of my proposal to copy the disk instead of moving it is that potentially the operation may be long as we need to proper copy the data instead of the entry in the filesystem (assuming both temp_directory and output_directory are on the same physical disk, otherwise both operations are equally costly).

Note: Windows may not clear %TEMP%, but we do remove the contents of the build directory assuming you don't have keep_registered = true during the cleanup phase. It can still linger if the plugin hard crashes before it can cleanup its data, but this is the case regardless if we copy or move data around.

I wanted to suggest not exporting anything by default if keep_registered is defined, but this may be problematic as the option's been in the wild for a while now, so I would be wary of changing its behaviour, as users may be negatively surprised by this change. I can see scenarios where one creates a VM, keeps the existing one around for playing with, while still wanting to export it for later use, so maybe mutual exclusivity between the two is out of the question for now.

Despite the risk of data duplication, I would still argue in favour of copying the disk instead of moving it around. This likely means an increase in build time for those that use this option with both temp/output directories in the same volume, but it keeps usage consistent throughout the builder. If build times are a concern, I would think we can forward the keep_registered option to the driver, and use the appropriate file copy/move if it is needed. I think having keep_registered and skip_export (as badly named as it is) mutually exclusive seems weird if we still accept exporting the full VM while keeping the existing one alive, so for the sake of consistency, I think we should fix the problematic interaction between both options instead of prohibiting it completely.

Maybe as a follow-up PR we can deprecate/rename skip_export, and add a proper skip that does not export anything if a user chooses so.

What do you think? Does this approach look sensible to you?