Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to set a higher maximum block size for TFTP #93

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

nshalman
Copy link
Member

@nshalman nshalman commented Oct 4, 2023

Description

This PR adds the ability to set a higher maximum block size for TFTP transfers.

Why is this needed

We are troubleshooting some misbehaving NICs that might benefit from support for larger block sizes. They appear to request 1468 which makes sense on a link with MTU of 1500.

How Has This Been Tested?

Tested with atftp

How are existing users impacted? What migration steps/scripts do we need?

The default of 512 is preserved and made explicit in this codebase.

@nshalman
Copy link
Member Author

nshalman commented Oct 4, 2023

If this proves useful it would probably benefit from some unit testing exercising the code paths which I have not done yet.

@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #93 (80c98a1) into main (c92a076) will increase coverage by 97.28%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##           main      #93       +/-   ##
=========================================
+ Coverage      0   97.28%   +97.28%     
=========================================
  Files         0        5        +5     
  Lines         0      442      +442     
=========================================
+ Hits          0      430      +430     
- Misses        0        8        +8     
- Partials      0        4        +4     
Files Coverage Δ
cmd.go 96.25% <100.00%> (ø)
ipxedust.go 96.02% <100.00%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nshalman nshalman marked this pull request as ready for review October 4, 2023 18:12
@nshalman
Copy link
Member Author

nshalman commented Oct 5, 2023

Explicitly tested and confirmed working:

[nix-shell:~/ipxedust]$ jobs
[1]+  Running                 ./bin/ipxe-linux -tftp-addr 127.0.0.1:2069 -log-level debug -tftp-blocksize 4096 &

[nix-shell:~/ipxedust]$ rm undionly.kpxe ; echo get undionly.kpxe | atftp 127.0.0.1 2069 --option "blksize 1486" --trace 2>&1 | tail
{"level":"info","event":"get","filename":"undionly.kpxe","uri":"undionly.kpxe","client":{"IP":"127.0.0.1","Port":37290,"Zone":""},"macFromURI":"","v":0,"logger":"ipxe","bytesSent":89859,"contentSize":89859,"caller":"github.com/tinkerbell/ipxedust/itftp/itftp.go:111","time":1696524693030,"message":"file served"}
sent ACK <block: 57>
received 1. DATA <block: 58, size 1486>, update last received block: 57 → 58
sent ACK <block: 58>
received 1. DATA <block: 59, size 1486>, update last received block: 58 → 59
sent ACK <block: 59>
received 1. DATA <block: 60, size 1486>, update last received block: 59 → 60
sent ACK <block: 60>
received 1. DATA <block: 61, size 699>, update last received block: 60 → 61
sent ACK <block: 61>
tftp>

@jacobweinstock jacobweinstock added the ready-to-merge Mergify: Ready for Merging label Oct 5, 2023
@mergify mergify bot merged commit 152536c into tinkerbell:main Oct 5, 2023
7 checks passed
@nshalman nshalman deleted the set-blocksize branch October 5, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Mergify: Ready for Merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants