add support for UEFI HTTP Boot #216
Conversation
|
/ok-to-test |
Codecov Report
@@ Coverage Diff @@
## main #216 +/- ##
==========================================
- Coverage 38.90% 38.84% -0.06%
==========================================
Files 41 41
Lines 2763 2762 -1
==========================================
- Hits 1075 1073 -2
- Misses 1602 1603 +1
Partials 86 86
Continue to review full report at Codecov.
|
|
@jacobweinstock, there were changes in the main branch that now conflict with this MR. Its is safe to rebase over main? Or is there still some pending work that I should wait for? |
Hey @rgl, should be safe. I don't foresee any big changes in the very immediate future. |
rgl
force-pushed
the
rgl-http-boot
branch
4 times, most recently
from
c4b3ea0
to
cf29795
Compare
|
@jacobweinstock, this is now ready, try2 :-) |
tftp/tftp.go
Outdated
|
|
||
| type tftpTransfer struct { | ||
| log.Logger | ||
| unread []byte | ||
| start time.Time | ||
| } | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
would you be open to not using func init()? Boots' design philosophy is to avoid func init() when possible.
There was a problem hiding this comment.
It was an attempt to prevent the creation of fs.Sub(files, "ipxe") each time there is a file request.
I can re-implement GetIpxeFiles() to always call that instead. What do you think?
There was a problem hiding this comment.
This is now moot, as the DHCP server now includes the directory name in the requested file like ipxe/ipxe.pxe.
jacobweinstock
added
the
kind/feature
|
@mmlb, @jacobweinstock, I think I've addressed all comments. Can you please review this again? |
rgl
force-pushed
the
rgl-http-boot
branch
2 times, most recently
from
59fce77
to
0096aa6
Compare
|
@mmlb, I think I've addressed all the comments (except adding the |
Yep, looks good to me. I'll just want to fire it up through a sandbox run to make sure all is good ;) |
this closes tinkerbell#210 Signed-off-by: Rui Lopes <rgl@ruilopes.com>
|
@mmlb with the latest push, the test should now pass. |
This reverts commit ceb470a. Something in tinkerbell#216 broke our c2.large.arm provisions. Reverting this to unblock things that came after while I look into debugging. Some more info in [tinkerbell#225]. [tinkerbell#225]: tinkerbell#225. Signed-off-by: Manuel Mendez <mmendez@equinix.com>
This reverts commit ceb470a. Something in tinkerbell#216 broke our c2.large.arm provisions. Reverting this to unblock things that came after while I look into debugging. Some more info in [tinkerbell#225]. [tinkerbell#225]: tinkerbell#225 Signed-off-by: Manuel Mendez <mmendez@equinix.com>
## Description This reverts commit ceb470a. ## Why is this needed Something in #216 broke our c2.large.arm provisions. Reverting this to unblock things that came after while I look into debugging. Some more info in [#225]. [#225]: #225 ## How Has This Been Tested? Tested via sandbox against EM c2.large.arm
## Description This is a revert of the revert that occurred in #217 + fix that caused the original revert. There's also removal of what is effectively useless/dead code (isPXEClient/pxeClient var/handling). I also removed the leading `/` from `/nonexistent` because a `/` is already prefixed and `//nonexistent` has bothered me for years. ## Why is this needed #216 was a great contribution from @rgl, unfortunately we ran into issues on some of our Ampere eMAG systems (#225). Turns out that the eMAG boot firmware just straight up ignores filename values if the response packet is marked as `PXEClient`. #216 changed the logic subtly by setting the response packet's class as `HTTPClient` if http was supported by the booting machine, and otherwise setting `PXEClient`. Before #216 we would set `PXEClient` only for the dhcp responses that were going out to our tftp'd build of iPXE, the initial filename: response was `PXEClient`-less. Once I put back the only `PXEClient` for our iPXE everything worked out fine again (tested using sandbox), but then I realized that now pxeClient was likely useless because I suspected iPXE did not care/require it. I tested this too and found that iPXE is happy to tftp a file provided by a `PXEClient`-less DHCP response. So why keep code around that is effectively useless? Less code is better than any code so that caused me to write up d1878bd. Fixes #210 Fixes #225 This debugging also opens up the way to bring back #149 as that had to be reverted due to the same "always set `PXEClient`" issue that I was not equipped to debug easily/quickly then. ## How Has This Been Tested? Lots of testing on EM machines using github.com/tinkerbell/sandbox's terraform support (I plan to clean up and PR https://github.com/mmlb/sandbox/tree/fix-terraform-and-add-arm64-support at some point). ## How are existing users impacted? What migration steps/scripts do we need? Back to faster/better iPXE binary downloads by utilizing TCP/HTTP instead of UDP/TFTP.
Description
This adds supports for UEFI HTTP Boot.
Why is this needed
Fixes: #210
How Has This Been Tested?
Tested with:
How are existing users impacted? What migration steps/scripts do we need?
Should have no impact.
Checklist:
I have: