From 7ed870948374b83416c013fc2513e3dbbcdaf851 Mon Sep 17 00:00:00 2001 From: Roman Tomjak <6570684+romantomjak@users.noreply.github.com> Date: Wed, 15 Jul 2020 23:07:02 +0100 Subject: [PATCH 1/6] add option to configure network adapter multiqueue support --- builder/proxmox/config.go | 14 +++++--- builder/proxmox/config.hcl2spec.go | 22 ++++++------ builder/proxmox/config_test.go | 56 ++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 15 deletions(-) diff --git a/builder/proxmox/config.go b/builder/proxmox/config.go index 22cb6786d..f97ec090a 100644 --- a/builder/proxmox/config.go +++ b/builder/proxmox/config.go @@ -67,11 +67,12 @@ type Config struct { } type nicConfig struct { - Model string `mapstructure:"model"` - MACAddress string `mapstructure:"mac_address"` - Bridge string `mapstructure:"bridge"` - VLANTag string `mapstructure:"vlan_tag"` - Firewall bool `mapstructure:"firewall"` + Model string `mapstructure:"model"` + PacketQueues int `mapstructure:"packet_queues"` + MACAddress string `mapstructure:"mac_address"` + Bridge string `mapstructure:"bridge"` + VLANTag string `mapstructure:"vlan_tag"` + Firewall bool `mapstructure:"firewall"` } type diskConfig struct { Type string `mapstructure:"type"` @@ -233,6 +234,9 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) { if c.NICs[idx].Bridge == "" { errs = packer.MultiErrorAppend(errs, errors.New(fmt.Sprintf("network_adapters[%d].bridge must be specified", idx))) } + if c.NICs[idx].Model != "virtio" && c.NICs[idx].PacketQueues > 0 { + errs = packer.MultiErrorAppend(errs, errors.New(fmt.Sprintf("network_adapters[%d].packet_queues can only be set for 'virtio' driver", idx))) + } } for idx := range c.Disks { if c.Disks[idx].StoragePool == "" { diff --git a/builder/proxmox/config.hcl2spec.go b/builder/proxmox/config.hcl2spec.go index 73a70b6a1..fe0e09c34 100644 --- a/builder/proxmox/config.hcl2spec.go +++ b/builder/proxmox/config.hcl2spec.go @@ -247,11 +247,12 @@ func (*FlatdiskConfig) HCL2Spec() map[string]hcldec.Spec { // FlatnicConfig is an auto-generated flat version of nicConfig. // Where the contents of a field with a `mapstructure:,squash` tag are bubbled up. type FlatnicConfig struct { - Model *string `mapstructure:"model" cty:"model" hcl:"model"` - MACAddress *string `mapstructure:"mac_address" cty:"mac_address" hcl:"mac_address"` - Bridge *string `mapstructure:"bridge" cty:"bridge" hcl:"bridge"` - VLANTag *string `mapstructure:"vlan_tag" cty:"vlan_tag" hcl:"vlan_tag"` - Firewall *bool `mapstructure:"firewall" cty:"firewall" hcl:"firewall"` + Model *string `mapstructure:"model" cty:"model" hcl:"model"` + PacketQueues *int `mapstructure:"packet_queues" cty:"packet_queues" hcl:"packet_queues"` + MACAddress *string `mapstructure:"mac_address" cty:"mac_address" hcl:"mac_address"` + Bridge *string `mapstructure:"bridge" cty:"bridge" hcl:"bridge"` + VLANTag *string `mapstructure:"vlan_tag" cty:"vlan_tag" hcl:"vlan_tag"` + Firewall *bool `mapstructure:"firewall" cty:"firewall" hcl:"firewall"` } // FlatMapstructure returns a new FlatnicConfig. @@ -266,11 +267,12 @@ func (*nicConfig) FlatMapstructure() interface{ HCL2Spec() map[string]hcldec.Spe // The decoded values from this spec will then be applied to a FlatnicConfig. func (*FlatnicConfig) HCL2Spec() map[string]hcldec.Spec { s := map[string]hcldec.Spec{ - "model": &hcldec.AttrSpec{Name: "model", Type: cty.String, Required: false}, - "mac_address": &hcldec.AttrSpec{Name: "mac_address", Type: cty.String, Required: false}, - "bridge": &hcldec.AttrSpec{Name: "bridge", Type: cty.String, Required: false}, - "vlan_tag": &hcldec.AttrSpec{Name: "vlan_tag", Type: cty.String, Required: false}, - "firewall": &hcldec.AttrSpec{Name: "firewall", Type: cty.Bool, Required: false}, + "model": &hcldec.AttrSpec{Name: "model", Type: cty.String, Required: false}, + "packet_queues": &hcldec.AttrSpec{Name: "packet_queues", Type: cty.Number, Required: false}, + "mac_address": &hcldec.AttrSpec{Name: "mac_address", Type: cty.String, Required: false}, + "bridge": &hcldec.AttrSpec{Name: "bridge", Type: cty.String, Required: false}, + "vlan_tag": &hcldec.AttrSpec{Name: "vlan_tag", Type: cty.String, Required: false}, + "firewall": &hcldec.AttrSpec{Name: "firewall", Type: cty.Bool, Required: false}, } return s } diff --git a/builder/proxmox/config_test.go b/builder/proxmox/config_test.go index e9889ba38..c5667c4ff 100644 --- a/builder/proxmox/config_test.go +++ b/builder/proxmox/config_test.go @@ -8,6 +8,17 @@ import ( "github.com/hashicorp/packer/template" ) +func mandatoryConfig(t *testing.T) map[string]interface{} { + return map[string]interface{}{ + "proxmox_url": "https://my-proxmox.my-domain:8006/api2/json", + "username": "apiuser@pve", + "password": "supersecret", + "iso_file": "local:iso/Fedora-Server-dvd-x86_64-29-1.2.iso", + "node": "my-proxmox", + "ssh_username": "root", + } +} + func TestRequiredParameters(t *testing.T) { var c Config _, err := c.Prepare(make(map[string]interface{})) @@ -166,3 +177,48 @@ func TestAgentSetToFalse(t *testing.T) { t.Errorf("Expected Agent to be false, got %t", b.config.Agent) } } + +func TestNetworkAdapterPacketQueueSupport(t *testing.T) { + drivertests := []struct { + expectedToFail bool + model string + }{ + {expectedToFail: false, model: "virtio"}, + {expectedToFail: true, model: "e1000"}, + {expectedToFail: true, model: "e1000-82540em"}, + {expectedToFail: true, model: "e1000-82544gc"}, + {expectedToFail: true, model: "e1000-82545em"}, + {expectedToFail: true, model: "i82551"}, + {expectedToFail: true, model: "i82557b"}, + {expectedToFail: true, model: "i82559er"}, + {expectedToFail: true, model: "ne2k_isa"}, + {expectedToFail: true, model: "ne2k_pci"}, + {expectedToFail: true, model: "pcnet"}, + {expectedToFail: true, model: "rtl8139"}, + {expectedToFail: true, model: "vmxnet3"}, + } + + for _, tt := range drivertests { + device := make(map[string]interface{}) + device["bridge"] = "vmbr0" + device["model"] = tt.model + device["packet_queues"] = 2 + + devices := make([]map[string]interface{}, 0) + devices = append(devices, device) + + cfg := mandatoryConfig(t) + cfg["network_adapters"] = devices + + var c Config + _, err := c.Prepare(cfg) + + if tt.expectedToFail == true && err == nil { + t.Error("expected config preparation to fail, but no error occured") + } + + if tt.expectedToFail == false && err != nil { + t.Errorf("expected config preparation to succeed, but %s", err.Error()) + } + } +} From adf1e294599a24d8f6b6c3b167db93ad6fd4396f Mon Sep 17 00:00:00 2001 From: Roman Tomjak <6570684+romantomjak@users.noreply.github.com> Date: Wed, 15 Jul 2020 23:41:25 +0100 Subject: [PATCH 2/6] update docs --- website/pages/docs/builders/proxmox.mdx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/website/pages/docs/builders/proxmox.mdx b/website/pages/docs/builders/proxmox.mdx index 1dd34d189..e0a5319a3 100644 --- a/website/pages/docs/builders/proxmox.mdx +++ b/website/pages/docs/builders/proxmox.mdx @@ -141,6 +141,15 @@ builder. - `firewall` (bool) - If the interface should be protected by the firewall. Defaults to `false`. + + - `packet_queues` (int) - Number of packet queues to be used on the device. + Values greater than 1 indicate that the multiqueue feature is activated. + For best performance, set this to the number of cores available to the + virtual machine. CPU load on the host and guest systems will increase as + the traffic increases, so set this option only when the VM has to handle + a great number of incoming connections, such as when the VM is running as + a router, reverse proxy or a busy HTTP server. Requires `virtio` network + adapter. Defaults to `0`. - `disks` (array of objects) - Disks attached to the virtual machine. Example: From a65157a91b674b531c637797a8c10588be4b5f05 Mon Sep 17 00:00:00 2001 From: Roman Tomjak <6570684+romantomjak@users.noreply.github.com> Date: Thu, 16 Jul 2020 17:41:56 +0100 Subject: [PATCH 3/6] use helper method for mandatory config --- builder/proxmox/config_test.go | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/builder/proxmox/config_test.go b/builder/proxmox/config_test.go index c5667c4ff..5726b3cb3 100644 --- a/builder/proxmox/config_test.go +++ b/builder/proxmox/config_test.go @@ -146,39 +146,21 @@ func TestBasicExampleFromDocsIsValid(t *testing.T) { } func TestAgentSetToFalse(t *testing.T) { - // only the mandatory attributes are specified - const config = `{ - "builders": [ - { - "type": "proxmox", - "proxmox_url": "https://my-proxmox.my-domain:8006/api2/json", - "username": "apiuser@pve", - "password": "supersecret", - "iso_file": "local:iso/Fedora-Server-dvd-x86_64-29-1.2.iso", - "ssh_username": "root", - "node": "my-proxmox", - "qemu_agent": false - } - ] - }` + cfg := mandatoryConfig(t) + cfg["qemu_agent"] = false - tpl, err := template.Parse(strings.NewReader(config)) - if err != nil { - t.Fatal(err) - } - - b := &Builder{} - _, warn, err := b.Prepare(tpl.Builders["proxmox"].Config) + var c Config + warn, err := c.Prepare(cfg) if err != nil { t.Fatal(err, warn) } - if b.config.Agent != false { - t.Errorf("Expected Agent to be false, got %t", b.config.Agent) + if c.Agent != false { + t.Errorf("Expected Agent to be false, got %t", c.Agent) } } -func TestNetworkAdapterPacketQueueSupport(t *testing.T) { +func TestPacketQueueSupportForNetworkAdapters(t *testing.T) { drivertests := []struct { expectedToFail bool model string From d3d7cc3e59bea677addb2ced5267de382ba30844 Mon Sep 17 00:00:00 2001 From: Roman Tomjak <6570684+romantomjak@users.noreply.github.com> Date: Thu, 16 Jul 2020 17:56:58 +0100 Subject: [PATCH 4/6] configure packet queues on nics --- builder/proxmox/step_start_vm.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/builder/proxmox/step_start_vm.go b/builder/proxmox/step_start_vm.go index 63d961125..11b9c2760 100644 --- a/builder/proxmox/step_start_vm.go +++ b/builder/proxmox/step_start_vm.go @@ -106,6 +106,10 @@ func generateProxmoxNetworkAdapters(nics []nicConfig) proxmox.QemuDevices { setDeviceParamIfDefined(devs[idx], "bridge", nics[idx].Bridge) setDeviceParamIfDefined(devs[idx], "tag", nics[idx].VLANTag) setDeviceParamIfDefined(devs[idx], "firewall", strconv.FormatBool(nics[idx].Firewall)) + + if nics[idx].PacketQueues > 0 { + devs[idx]["queues"] = nics[idx].PacketQueues + } } return devs } From b0d8f69bdadeccf399c4864c1f3dcb262f78d53e Mon Sep 17 00:00:00 2001 From: Roman Tomjak <6570684+romantomjak@users.noreply.github.com> Date: Thu, 16 Jul 2020 18:03:46 +0100 Subject: [PATCH 5/6] tweak docs --- website/pages/docs/builders/proxmox.mdx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/website/pages/docs/builders/proxmox.mdx b/website/pages/docs/builders/proxmox.mdx index e0a5319a3..730c8de8a 100644 --- a/website/pages/docs/builders/proxmox.mdx +++ b/website/pages/docs/builders/proxmox.mdx @@ -146,10 +146,10 @@ builder. Values greater than 1 indicate that the multiqueue feature is activated. For best performance, set this to the number of cores available to the virtual machine. CPU load on the host and guest systems will increase as - the traffic increases, so set this option only when the VM has to handle - a great number of incoming connections, such as when the VM is running as - a router, reverse proxy or a busy HTTP server. Requires `virtio` network - adapter. Defaults to `0`. + the traffic increases, so activate this option only when the VM has to + handle a great number of incoming connections, such as when the VM is + operating as a router, reverse proxy or a busy HTTP server. Requires + `virtio` network adapter. Defaults to `0`. - `disks` (array of objects) - Disks attached to the virtual machine. Example: From a2220e5f08c51aeaa3666e0158c98a99f587af12 Mon Sep 17 00:00:00 2001 From: Roman Tomjak <6570684+romantomjak@users.noreply.github.com> Date: Thu, 16 Jul 2020 18:19:22 +0100 Subject: [PATCH 6/6] make linter happy --- builder/proxmox/config.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/builder/proxmox/config.go b/builder/proxmox/config.go index f97ec090a..3c2c4237f 100644 --- a/builder/proxmox/config.go +++ b/builder/proxmox/config.go @@ -179,7 +179,7 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) { // (currently zfspool|lvm|rbd|cephfs), the format parameter is mandatory. Make sure this is still up to date // when updating the vendored code! if !contains([]string{"zfspool", "lvm", "rbd", "cephfs"}, c.Disks[idx].StoragePoolType) && c.Disks[idx].DiskFormat == "" { - errs = packer.MultiErrorAppend(errs, errors.New(fmt.Sprintf("disk format must be specified for pool type %q", c.Disks[idx].StoragePoolType))) + errs = packer.MultiErrorAppend(errs, fmt.Errorf("disk format must be specified for pool type %q", c.Disks[idx].StoragePoolType)) } } if c.SCSIController == "" { @@ -222,7 +222,7 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) { errs = packer.MultiErrorAppend(errs, errors.New("proxmox_url must be specified")) } if c.proxmoxURL, err = url.Parse(c.ProxmoxURLRaw); err != nil { - errs = packer.MultiErrorAppend(errs, errors.New(fmt.Sprintf("Could not parse proxmox_url: %s", err))) + errs = packer.MultiErrorAppend(errs, fmt.Errorf("Could not parse proxmox_url: %s", err)) } if c.Node == "" { errs = packer.MultiErrorAppend(errs, errors.New("node must be specified")) @@ -232,18 +232,18 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) { } for idx := range c.NICs { if c.NICs[idx].Bridge == "" { - errs = packer.MultiErrorAppend(errs, errors.New(fmt.Sprintf("network_adapters[%d].bridge must be specified", idx))) + errs = packer.MultiErrorAppend(errs, fmt.Errorf("network_adapters[%d].bridge must be specified", idx)) } if c.NICs[idx].Model != "virtio" && c.NICs[idx].PacketQueues > 0 { - errs = packer.MultiErrorAppend(errs, errors.New(fmt.Sprintf("network_adapters[%d].packet_queues can only be set for 'virtio' driver", idx))) + errs = packer.MultiErrorAppend(errs, fmt.Errorf("network_adapters[%d].packet_queues can only be set for 'virtio' driver", idx)) } } for idx := range c.Disks { if c.Disks[idx].StoragePool == "" { - errs = packer.MultiErrorAppend(errs, errors.New(fmt.Sprintf("disks[%d].storage_pool must be specified", idx))) + errs = packer.MultiErrorAppend(errs, fmt.Errorf("disks[%d].storage_pool must be specified", idx)) } if c.Disks[idx].StoragePoolType == "" { - errs = packer.MultiErrorAppend(errs, errors.New(fmt.Sprintf("disks[%d].storage_pool_type must be specified", idx))) + errs = packer.MultiErrorAppend(errs, fmt.Errorf("disks[%d].storage_pool_type must be specified", idx)) } }