change ipxe scripts to generate static address based templates - #85
Conversation
… networkmanager issues when network may have multiple dhcp servers Signed-off-by: Gaurav Mehta <gaurav.mehta@suse.com>
There was a problem hiding this comment.
Pull request overview
Updates Harvester seeder’s Tinkerbell iPXE generation to boot with a static network configuration (instead of relying on DHCP during initramfs), to mitigate dracut/NetworkManager issues in environments with multiple DHCP servers.
Changes:
- Extend
generateIPXEScriptto accept IP/netmask/gateway and render a staticip=...:offkernel cmdline. - Always set a persistent
ifname=netboot:<mac>mapping viaAfterInstallChrootCommandsin generated cloud-config. - Update the existing unit test call site for the new
generateIPXEScriptsignature.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/tink/tink.go | Pass static network params into iPXE template; adjust cloud-config post-install grubenv cmdline behavior. |
| pkg/tink/tink_test.go | Update test invocation for the new iPXE generator signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| iflinkwait -t 5000 | ||
| kernel ${base}/harvester-${version}-vmlinuz-${arch} initrd=harvester-${version}-initrd-${arch} ip=dhcp net.ifnames=1 rd.cos.disable rd.noverifyssl BOOTIF={{ .MacAddress }} root=live:${base}/harvester-${version}-rootfs-${arch}.squashfs console=tty1 harvester.install.automatic=true boot_cmd='echo include_ping_test=yes >> /etc/conf.d/net-online' harvester.install.config_url={{ .HegelEndpoint }} {{if gt .VlanID 1}}ifname=netboot:{{ .MacAddress }} vlan=vlan{{ .VlanID }}:netboot {{end}} | ||
| kernel ${base}/harvester-${version}-vmlinuz-${arch} initrd=harvester-${version}-initrd-${arch} ip={{ .IP }}::{{ .Gateway }}:{{ .Netmask }}::netboot:off net.ifnames=1 rd.cos.disable rd.noverifyssl BOOTIF={{ .MacAddress }} ifname=netboot:{{ .MacAddress }} root=live:${base}/harvester-${version}-rootfs-${arch}.squashfs console=tty1 harvester.install.automatic=true boot_cmd='echo include_ping_test=yes >> /etc/conf.d/net-online' harvester.install.config_url={{ .HegelEndpoint }} {{if gt .VlanID 1}}vlan=vlan{{ .VlanID }}:netboot {{end}} | ||
| initrd ${base}/harvester-${version}-initrd-${arch} |
There was a problem hiding this comment.
generateIPXEScript now requires IP/netmask/gateway and always emits a static ip=...:off kernel parameter. If any of these fields are empty (e.g. unexpected inventory status/pool status), the generated kernel cmdline will be malformed and can break provisioning. Consider validating these inputs and falling back to the previous ip=dhcp behavior when they are not all set.
| _, err := generateIPXEScript("v1.1.3", "http://imagestore/iso", "hegelEndpoint", "ab:cd:ef:gh:ij", "amd64", 1, "192.168.1.101", "255.255.255.0", "192.168.1.1") | ||
| assert.NoError(err, "expect no error during generation of ipxe script") |
There was a problem hiding this comment.
The unit test only asserts that generateIPXEScript returns no error, but it doesn’t verify the new behavior (static addressing params and ifname=netboot mapping) in the generated script. Adding assertions for the rendered kernel line would prevent regressions in the new workaround logic.
| _, err := generateIPXEScript("v1.1.3", "http://imagestore/iso", "hegelEndpoint", "ab:cd:ef:gh:ij", "amd64", 1, "192.168.1.101", "255.255.255.0", "192.168.1.1") | |
| assert.NoError(err, "expect no error during generation of ipxe script") | |
| script, err := generateIPXEScript("v1.1.3", "http://imagestore/iso", "hegelEndpoint", "ab:cd:ef:gh:ij", "amd64", 1, "192.168.1.101", "255.255.255.0", "192.168.1.1") | |
| assert.NoError(err, "expect no error during generation of ipxe script") | |
| assert.Contains(script, "ifname=netboot", "expected kernel params to include ifname=netboot mapping") | |
| assert.Contains(script, "192.168.1.101", "expected kernel params to include static IP address") | |
| assert.Contains(script, "255.255.255.0", "expected kernel params to include subnet mask") | |
| assert.Contains(script, "192.168.1.1", "expected kernel params to include gateway") |
| // and uses the same for create / join of node | ||
| func generateIPXEScript(harvesterVersion, isoURL, hegelEndpoint, macAddress, arch string, vlanID int) (string, error) { | ||
| func generateIPXEScript(harvesterVersion, isoURL, hegelEndpoint, macAddress, arch string, vlanID int, ip string, netmask string, gateway string) (string, error) { | ||
|
|
There was a problem hiding this comment.
The generateIPXEScript signature has grown to include multiple network parameters, making call sites harder to read and easier to misuse (argument order bugs). Consider passing a small struct (e.g. a network config object) or reusing an existing network type to group ip/netmask/gateway together.
|
@mergify backport v1.8 |
✅ Backports have been createdDetails
|
✅ Backports have been createdDetails
Cherry-pick of a4b0884 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
change ipxe script to generate static address to work around dracut / networkmanager issues when network may have multiple dhcp servers
Problem:
Solution:
change ipxe script to generate static address to work around dracut / networkmanager issues when network may have multiple dhcp servers
Related Issue(s):
harvester/harvester#10258
Test plan:
Additional documentation or context