Numerous improvements - #22
Open
seanmil wants to merge 14 commits into
Open
Conversation
|
|
Author
|
Notes:
|
|
Nice to see the packer-plugin-kubevirt evolving! |
Author
I took a quick look at the existing examples and, yes, I believe it would be worth adjusting them. I'll work on that and push an update. Thanks! |
Contributor
|
/cc |
codingben
suggested changes
Oct 5, 2025
codingben
left a comment
codingben
left a comment
Contributor
There was a problem hiding this comment.
Looks good, thanks for taking care of this. I think examples should be updated as well and I want to make sure that the changes being tested against a real k8s cluster.
Refactor communicator-related options to leverage and align with the provided functionality in the packer-plugin-sdk. This change preserves backward compatibility with options having different names but marks them as deprecated in favor of the packer-plugin-sdk provided ones.
Refactor the communicator setup to remove the custom setup code and instead use the packer-plugin-sdk provided setup.
Author
|
I just pushed a big update versus my first go at it. In order to properly build the whole set of systems I needed in my environment I needed to add more configuration options. I did include updated and expanded examples (adding SLES and Ubuntu), as well as significantly expanded unit test coverage vs. the last push. |
Modeled after the VMware plugin's IP wait, this adds a wait on the availability of the VM's IP and then provides the IP for use in other steps. If no forwarding is in use and no IP/host was manually specified, the detected IP will be used as the default.
With the addition of ip_wait_timeout the installation_wait_timeout option should no longer be marked as as required option.
Enable port forwarding to, by default, select and using a local ephemeral port as the forwarding port. Also, adjust port forwarding routines to permit connection failures gracefully allowing for retries to support the communicator wait for SSH/WinRM workflows.
If enabled, the builder will wait for the Guest Agent to be reported as online prior to moving to the wait_for_ip step.
Allow the temporary VM name to be specified separately from the desired resulting DataSource name. If not specified, VMName will default to the final DataSource name.
Add new configuration options media_content, media_label, and keep_media to allow better flexibility and configurability of the built ConfigMap-based media specification.
Enable users to specify their own container pull path for the VirtIO Container image with the required Windows drivers. Also, update the default container URL to the latest stable version available.
Expose the ability to set the accessMode and volumeMode used when creating the PVC holding the temporary VM's rootdisk and the PVC backing the resulting DataVolume built image.
Refactor the create_bootablevolume and create_virtualmachine tests to better support easy overrides of configuration values to test different scenarios, and using this new refactor, expand the testing coverage.
When specifying the "name" parameter in a simple HCL configuration, using a static block definition, it gets correctly defined. However, when using dynamic blocks the value set for "name" does not get propagated into the builder configuration structure. Exposing template_name as a separate parameter, and defaulting it to "name" if defined, preserves backwards compatibility but also allows the resulting template volume name to be properly set when leveraging dynamic blocks in the configuration.
|
I just went through this PR as I'm currently working on s390 kubevirt images and was glad of how many cool things are there 👍 |
|
@codingben is there anything we can do to help make progress with the packer-plugin-kubevirt? E.g. split this PR into smaller pieces or anything else that makes it easier to review and make momentum? |
Contributor
|
@cskarby I'm no longer active here as I'm contributing to KubeVirt projects. @tanmay-hc @anurag5sh Do you plan to review this PR? |
|
Would be really nice to see these changes merged. Especially the ssh communicator ones, since the current kubevirt source, requires the ssh host to be set, This Makes the plugin virtually impossible to use in environments where the ip of the built VM is set dynamically. Kind of sad that it takes so long to review and make it happen |
peschmae
pushed a commit
to peschmae/packer-plugin-kubevirt
that referenced
this pull request
Mar 24, 2026
…, wait steps, media config, access/volume modes
There was a problem hiding this comment.
Pull request overview
This PR modernizes the KubeVirt ISO builder by aligning communicator configuration with the packer-plugin-sdk, reducing custom communicator code, and adding new optional wait steps (guest IP + guest agent) along with improved media handling and documentation updates.
Changes:
- Adopt packer-plugin-sdk communicator configuration and connection steps (SSH/WinRM) and update docs/examples accordingly.
- Add
wait_for_ipandwait_for_agentsteps (plus config/docs) to support richer build-time readiness detection. - Improve media handling via
media_content/media_files, add PVC access/volume mode options, and enhance port forwarding behavior/documentation.
Reviewed changes
Copilot reviewed 39 out of 44 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/builder/kubevirt-iso/windows/windows.pkr.hcl | Updates Windows example to template autounattend content and align WinRM config with SDK conventions. |
| examples/builder/kubevirt-iso/windows/autounattend.xml.tpl | Makes AcceptEula templated via templatefile input. |
| examples/builder/kubevirt-iso/ubuntu/ubuntu.pkr.hcl | Adds a new Ubuntu ISO example showcasing cloud-init media + guest agent waiting and SSH communicator settings. |
| examples/builder/kubevirt-iso/ubuntu/ubuntu-iso.yaml | Adds a DataVolume manifest for fetching the Ubuntu installer ISO. |
| examples/builder/kubevirt-iso/ubuntu/autoinstall.yml | Adds Ubuntu autoinstall configuration (cloud-init) for unattended install. |
| examples/builder/kubevirt-iso/sles/sles.pkr.hcl | Adds a new SLES ISO example with SSH communicator and guest agent waiting. |
| examples/builder/kubevirt-iso/sles/sles-iso.yaml | Adds a DataVolume manifest template for a SLES ISO. |
| examples/builder/kubevirt-iso/sles/autoinst.xml | Adds SLES AutoYaST profile used by the SLES example. |
| examples/builder/kubevirt-iso/rhel/rhel.pkr.hcl | Updates RHEL example to show media_content usage and new wait behavior. |
| examples/builder/kubevirt-iso/README.md | Links new Ubuntu and SLES examples. |
| examples/builder/kubevirt-iso/fedora/fedora.pkr.hcl | Updates Fedora example to reflect new wait/communicator conventions. |
| docs/builders/iso.mdx | Restructures builder docs into clearer sections and includes new partials (media/wait/communicator). |
| docs-partials/builder/kubevirt/iso/WaitIpConfig-not-required.mdx | Adds generated docs for wait_for_ip configuration. |
| docs-partials/builder/kubevirt/iso/WaitForAgentConfig-not-required.mdx | Adds generated docs for wait_for_agent configuration. |
| docs-partials/builder/kubevirt/iso/PortForwardConfig-not-required.mdx | Adds generated docs for new port-forwarding controls. |
| docs-partials/builder/kubevirt/iso/MediaConfig-not-required.mdx | Adds generated docs for media_files/media_content support. |
| docs-partials/builder/kubevirt/iso/Config-required.mdx | Updates required config docs to use template_name terminology. |
| docs-partials/builder/kubevirt/iso/Config-not-required.mdx | Documents new optional fields (vm_name, access_mode, volume_mode, virtio_container, etc.). |
| builder/kubevirt/iso/step_wait_for_ip.hcl2spec.go | Adds HCL2 spec generation output for WaitIpConfig. |
| builder/kubevirt/iso/step_wait_for_ip.go | Implements wait_for_ip step and IP settling logic. |
| builder/kubevirt/iso/step_wait_for_ip_test.go | Adds tests for wait_for_ip behavior. |
| builder/kubevirt/iso/step_wait_for_agent.hcl2spec.go | Adds (currently incomplete) HCL2 spec generation output for wait-for-agent. |
| builder/kubevirt/iso/step_wait_for_agent.go | Implements wait_for_agent step (Guest Agent availability). |
| builder/kubevirt/iso/step_wait_for_agent_test.go | Adds tests for wait_for_agent behavior. |
| builder/kubevirt/iso/step_stop_virtualmachine.go | Switches stop logic to use VMName (temporary VM) rather than image/template name. |
| builder/kubevirt/iso/step_stop_virtualmachine_test.go | Updates tests for VMName usage. |
| builder/kubevirt/iso/step_start_portforward.hcl2spec.go | Adds HCL2 spec generation output for PortForwardConfig. |
| builder/kubevirt/iso/step_start_portforward.go | Refactors port-forwarding step to support ephemeral ports and forwarding disablement. |
| builder/kubevirt/iso/step_start_portforward_test.go | Updates port-forward tests for new factory contract and communicator config struct. |
| builder/kubevirt/iso/step_create_virtualmachine.go | Extends VM creation to include media label, virtio container, and PVC access/volume mode. |
| builder/kubevirt/iso/step_create_virtualmachine_test.go | Expands VM creation tests for Linux/Windows disk/volume layout and PVC modes. |
| builder/kubevirt/iso/step_create_bootablevolume.go | Updates bootable volume creation to use template/vm naming split and PVC modes. |
| builder/kubevirt/iso/step_create_bootablevolume_test.go | Updates/expands tests for clone DV and DataSource labeling and PVC modes. |
| builder/kubevirt/iso/step_copy_media_files.hcl2spec.go | Adds HCL2 spec generation output for MediaConfig. |
| builder/kubevirt/iso/step_copy_media_files.go | Refactors media handling to support both file-copy and inline content and adds keep_media. |
| builder/kubevirt/iso/step_copy_media_files_test.go | Updates tests for media file reading and inline media_content. |
| builder/kubevirt/iso/step_boot_command.go | Switches VNC boot automation to use VMName. |
| builder/kubevirt/iso/resources.go | Refactors resource constructors (VM disks/volumes, configmap) and adds access/volume mode conversion helpers. |
| builder/kubevirt/iso/config.hcl2spec.go | Updates HCL2 config schema to include new communicator/media/wait/port-forward fields. |
| builder/kubevirt/iso/config.go | Refactors config to embed SDK communicator config + new sub-configs and adds backwards-compat remapping. |
| builder/kubevirt/iso/builder.go | Reworks build pipeline to use SDK communicator steps and adds optional wait steps + port-forward control. |
| builder/kubevirt/common/portforwarder.go | Modifies port-forwarder to support ephemeral port allocation and more tolerant connection handling. |
| builder/kubevirt/common/comm_config.go | Adds helper functions to select communicator host/port from state (forwarded host/port, guest IP, explicit host). |
| .web-docs/components/builder/iso/README.md | Updates generated web docs to include new config sections and SDK communicator docs. |
Files not reviewed (5)
- builder/kubevirt/iso/config.hcl2spec.go: Generated file
- builder/kubevirt/iso/step_copy_media_files.hcl2spec.go: Generated file
- builder/kubevirt/iso/step_start_portforward.hcl2spec.go: Generated file
- builder/kubevirt/iso/step_wait_for_agent.hcl2spec.go: Generated file
- builder/kubevirt/iso/step_wait_for_ip.hcl2spec.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+63
to
+67
| localAddress = "localhost" | ||
| remotePort = s.Config.Comm.Port() | ||
|
|
||
| ui.Sayf("Preparing port forwarding from %s:%d to VM on port %d", localAddress, localPort, remotePort) | ||
|
|
Comment on lines
+66
to
70
| ui.Sayf("Preparing port forwarding from %s:%d to VM on port %d", localAddress, localPort, remotePort) | ||
|
|
||
| address, _ := net.ResolveIPAddr("ip", localAddress) | ||
| vmi := s.Client.VirtualMachineInstance(namespace) | ||
|
|
Comment on lines
84
to
88
| conn, err := listener.Accept() | ||
| if err != nil { | ||
| log.Log.Errorf("error accepting connection: %v", err) | ||
| return | ||
| fmt.Printf("port forwarder: error accepting connection: %v", err) | ||
| continue | ||
| } |
Comment on lines
+51
to
+58
| if c.SettleTimeout == 0 { | ||
| c.SettleTimeout = 5 * time.Second | ||
| } | ||
| if c.WaitTimeout == 0 { | ||
| c.WaitTimeout = 30 * time.Minute | ||
| } | ||
|
|
||
| return errs |
Comment on lines
+26
to
+29
| // Amount of time to wait for VM's IP, similar to 'ssh_timeout'. | ||
| // Defaults to `30m` (30 minutes). Refer to the Golang | ||
| // [ParseDuration](https://golang.org/pkg/time/#ParseDuration) | ||
| // documentation for full details. |
Comment on lines
+87
to
+89
| build { | ||
| sources = ["source.kubevirt-iso.fedora"] | ||
|
|
Comment on lines
+18
to
+22
| source "kubevirt-iso" "fedora" { | ||
| # Kubernetes configuration | ||
| kube_config = var.kube_config | ||
| name = "sles-15-sp7-rand-85" | ||
| namespace = "images" |
Comment on lines
+67
to
+69
| build { | ||
| sources = ["source.kubevirt-iso.fedora"] | ||
|
|
| # ISO configuration | ||
| iso_volume_name = "rhel-10-x86-64-iso" | ||
|
|
||
| # PVC configuratoin |
Comment on lines
+30
to
+37
| // Content is a map of content to include in the generated media volume. | ||
| // The map keys are the filenames and the map values are the file | ||
| // contents. This permits the use of HCL functions such as `file` and | ||
| // `templatefile` to populate the ConfigMap. | ||
| // If a filename matches a file included in `media_files` then the | ||
| // contents specified here takes precedence. | ||
| // Defaults to an empty list. | ||
| Content map[string]string `mapstructure:"media_content" required:"false"` |
Collaborator
|
@seanmil thanks for raising this PR. I will review this but do note that I don't have any prior experience working with kubevirt. Could you address the copilot comments (if at all they are valid). Once the conflicts are resolved we can get this PR move forward. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR contains a series of somewhat related improvements including: