Packer: Hyper-V: setting "skip_export: true" errors unless combined with "skip_compaction: true"

[DanHam]

This has been reported by @santhoshm153 in #4131. I'm opening a new issue to track this as that issue is unrelated/closed.

The bug seems to have been introduced by #5631. I'm in the process of testing a fix that, all being well, will shortly make its way into a PR.

While I think it would be best to get a fix out for the current issue first, one point that seems somewhat confusing to me, is that the code to compact the VMs disks is embedded in the export step. In other words, the way the code is currently arranged means the Hyper-V builders only allow compacting of disks if the machine is being exported.

I wonder if this is the intention, or whether disk compaction needs to be broken out into its own step thereby allowing compaction of the VMs disks when skip_export: true is set. If anyone has any background knowledge to share on this I would be happy to hear it...

[SwampDragons]

I'd love to hear about the export step being the place we do compaction from @vijayinvites; if it doesn't need to be there, I'd be happy to break it out into a separate step.

[santhoshm153]

@DanHam , I would be happy to test the fix.

My thoughts would be to handle compaction based on skip_export status. I don't see a use case in my environment where we need to compact disks when skip_export is set to true . In other words if skip_export: true is set , then skip_compaction: true should also be set by default.

[DanHam]

I would be happy to test the fix.

@santhoshm153 Thanks! If you need a binary built with the fix please let us know what OS you are using and (hopefully!) @SwampDragons will be good enough to build one for us!

I don't see a use case in my environment where we need to compact disks when skip_export is set to true

OK. Great - so my take from that is that, bugs aside, the way the code is presently structured seems to support your assessment/needs.

[vijayinvites]

@SwampDragons , Yes its a bug. It doesn't need to be there. We can break this in to two.

[DanHam]

OK. So breaking this out does gives greater flexibility. However, it would introduce a change in behaviour for users who have been building with skip_export: true.

As we know, users setting skip_export: true will not have their disks compacted, since the code to compact the disks is in the skip export step.
If we break out the disk compaction code, the 'non-exported' disk in the output_directory/Virtual Hard Disks directory would be compacted since skip_compaction is implicitly set to false. Put another way, to maintain the current behaviour, users who have set skip_export: true would now also need to set skip_compaction: true.

@santhoshm153 @vijayinvites Can you see this change in behaviour causing an issue? In other words what would the impact of compacting that disk be? Does the disk compaction fundamentally change the disk type as for example it does with VMware e.g. thin -vs- thick provisioned disks?

If this change is problematic, one option would be to note the change in behaviour somewhere and perhaps write a fixer that would set skip_compaction: true if skip_export: true was set...

EDIT: So, from what I've read, I'm leaning towards this change in behaviour being acceptable - aside from a slightly longer build time, it shouldn't negatively impact the disk in any way. I would still like to hear from the experts on this though...

Personally, I think Packer should offer the flexibility of compacting disks whether the VM is exported or not - provided of course that this is a valid use case for some people!

[DanHam]

Following on from the discussions above, please see:

• 6393 This PR fixes this issue but does not separate out the disk compaction process from the VM export step.

  There are a number of changes that hopefully greatly improve the efficiency of the disk compaction and export processes e.g. no wasteful copying of disks and a reordering of steps so we compact before we export.
  However, this PR also introduces a slight change to the directory structure associated with the exported VM that may or may not be seen as acceptable. Please see the PR for full details.

• 6396 This PR is basically #6393, but with some additional changes that break out the disk compaction process into its own individual step.

The idea behind issuing both PR's is so that people can test and give feedback on the proposed changes. Clearly, after all the testing, discussion and possible fixes are over with, we will have a winner (or possibly two losers!!) at which point we'll either be merging or starting again!

[DanHam]

I'd be happy to break it out into a separate step.

@SwampDragons Hope you don't mind me jumping in there with #6396! However, I had everything set up for testing so it seemed a bit silly not to carry on from #6393... and of course it was a lot of fun too!

[vijayinvites]

@DanHam , Compacting the disk does not change anything except reducing the size(if it can). I don't see any issue with the behavioral change that you are going to introduce.

[DanHam]

OK. So, unless there are any other objections, I'm happy that we should move ahead with breaking out the compaction process into its own step - as per #6396.

@SwampDragons Would this be your take on this as well? If so I will close #6393.

However, this PR also introduces a slight change to the directory structure associated with the exported VM that may or may not be seen as acceptable. Please see the PR for full details.

I would appreciate getting peoples thought on this change as well, and of course feedback from actual testing with the updated code (perhaps now focusing on #6396) would be greatly appreciated.

[SwampDragons]

@DanHam I never mind when people jump in to help 😄

I'll review the code in #6396 now to let you know my thoughts on the directory structure, and I'll try to find time to test it out this week.

[DanHam]

@SwampDragons I saw the comments in #6393 (did you mean to leave them in #6396?) with regards to preserving the current directory structure for exported VM's. I agree this is probably the right course of action. However, I thought I would put it up for discussion as maintaining that directory structure would mean moving around large files.

EDIT: Actually, moving the files (as opposed to copying them) shouldn't introduce much of an overhead....

I'll try and get that sorted quickly so that you can review the whole thing in one go.

With regard to #6393 -vs- #6396; Following the response above from @vijayinvites, I'm now in favour of closing #6393 and moving forward with #6396. Are you in agreement?

[SwampDragons]

👍 yeah, if we were actually copying the files I'd say forget it, but this should be a pretty mini operation.

[DanHam]

@vijayinvites @SwampDragons @mwhooker

I'm actually wondering if I'm going a bit mad seeing this, as it must have been staring us all in the face for a while now...

In the VMCX builder template (and throughout the code) there are a bunch of references to VMXC - rather than VMCX.

For example, the 'clone_from...' option in the VMCX builder is clone_from_vmxc_path see the section in the docs HERE.

Clearly, Hyper-V VM metadata files have an extension of .vmcx - hence the name of the builder. Far as I can see, this looks as though it is a typo that has then propagated all over the place - see the output from running grep -HRni vmxc . in the root of the packer directory...

Am I right in thinking that this is a typo and that there isn't really some VMXC Hyper-V thing lurking out there somewhere?

If this is the case I will make the necessary changes and add another fixer to #6393. This seems like a sensible place to do it since there are already some changes in there requiring a fixer...

EDIT: OK. I searched the main MS Hyper-V site for VMXC - there were no results returned... Looks like I'm not going mad after all. I've fixed this in #6393

[DanHam]

Hi all. #6393 is now ready for testing so if you could provide feedback it would be much appreciated.

The original scope of the PR grew quite a bit. Going through the code I found a number of other issues that either needed tackling or were best tackled in that PR. See THIS comment in the PR for full details.

From an end users perspective, the following should be noted:

• The vhd_temp_path option has been deprecated. There is a fixer in place to automatically remove the option from any affected templates.
• As noted above, the typo in the VMCX clone_from... option has been fixed. Again there is a fixer that will automatically correct affected templates, replacing clone_from_vmxc_path with clone_from_vmcx_path
• Users can now set temp_path for VMCX builds

@SwampDragons I realise that the PR is rather large. However, it would be nice to get this in for v1.3.0 as there are obviously some changes in there requiring fixers etc. Also, there may be some increased interest in building Hyper-V VMs with Packer following the recent improvements relating to Hyper-V in Vagrant...

If you could provide a binary incorporating the changes in that PR it would be much appreciated.

[DanHam]

@vijayinvites @santhoshm153

Did either of you manage to test the fixes applied over in #6393 - packer.zip.

If not it would be great if you could give it a go and provide some feedback.

@SwampDragons

How far did you get with reviewing #6393?

[SwampDragons]

Sorry Dan -- I completely flaked on it. I've added it to the 1.3.0 milestone so I don't forget again.

[vijayinvites]

@DanHam , Hey Dan, sorry I have moved on to different projects now and I no longer use Packer. So it would be difficult to test it now.

[DanHam]

@vijayinvites Understood. No problem!

[hashibot[bot]]

I'm going to lock this issue because it has been closed for _30 days_ ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.