Merge lp:~gabriel-samfira/python-tx-tftp/python-tx-tftp into lp:python-tx-tftp

Proposed by Gabriel Samfira
Status: Needs review
Proposed branch: lp:~gabriel-samfira/python-tx-tftp/python-tx-tftp
Merge into: lp:python-tx-tftp
Diff against target: 12 lines (+1/-1)
1 file modified
tftp/session.py (+1/-1)
To merge this branch: bzr merge lp:~gabriel-samfira/python-tx-tftp/python-tx-tftp
Reviewer Review Type Date Requested Status
Gavin Panella Needs Information
Andres Rodriguez (community) Approve
Review via email: mp+208862@code.launchpad.net

Commit message

Increase MAX_BLOCK_SIZE to 8192

Description of the change

The current MAX_BLOCK_SIZE only allows files of up to ~87 MB in size.
Considering that a windows winpe ramdisk can grow to about ~270 MB,
it would just fail after block nr 65535. Increasing this constant
will allow larger files to be transmited.

It is not mandatory for the client to ask for a block size as large as this. It
is just a higher cap, up to which a client may negotiate.

Preferably, this would be configurable per backend, and not hardcoded
in session.py.

To post a comment you must log in.
Revision history for this message
Andres Rodriguez (andreserl) wrote :

lgtm!

review: Approve
Revision history for this message
Gavin Panella (allenap) wrote :

I wonder why it was set to 1400. It's close to the typical Ethernet LAN MTU of 1500, so I wonder if there's a reason for this setting, but this might well be a coincidence too. The upstream maintainer and original author is https://github.com/shylent, and the upstream branch is https://github.com/shylent/python-tx-tftp. It's worth making a pull request out of this change and see what he/she thinks. Ultimately we'd want to get this change upstream anyway.

Another question: is TFTP really a good choice for moving files of ~270MB across a network, or are you forced to do this? I suspect a TCP-based mechanism, HTTP perhaps, would be quicker and possibly more reliable.

review: Needs Information
Revision history for this message
Alessandro Pilotti (alexpilotti) wrote :

This is the way in which Windows handles the initial WinPE image deployment. SMB file transfer is used for the remaining steps.

As a comparison, the Microsoft TFTP service has a default block size of 16384.

A higher value then 1400 (in the UPD packet size limits) speeds up the deployment in scenarios where fragmentation is avoided (e.g. LAN) as the TFTP protocol expects an ACK for every UDP packet sent.

I agree that it looks like they preferred to stay in the default MTU safety margins.

We sent a pull request upstream as well: https://github.com/shylent/python-tx-tftp/pull/17

Thanks!

Revision history for this message
Gabriel Samfira (gabriel-samfira) wrote :

Hello Gavin,

Unfortunately there is no other way to transfer the ramdisk using officially supported methods. In the case of a Windows PXE boot, pxeboot.0 gets loaded first which pulls bootmgr.exe. This binary is responsible for pulling in the winpe ramdisk via TFTP, which is...huge. With that said, transferring the ramdisk via TFTP with a block size of 8192 should not be a problem. The maximum size for a UDP message is 65507 bytes (http://en.wikipedia.org/wiki/User_Datagram_Protocol).

Any frame larger then 1500 will be fragmented by the TCP/IP stack. I suspect that this operation is faster then sending out individual smaller frames, but I will have to test this. If need be, I can have a stab at TFTP roll-over, and reset the blocknum back to 0 once it reaches 65535. This will allow, in theory, an unlimited filesize, but to be honest, I do not believe this to be necessary.

I would have loved to transfer this behemoth via HTTP.

Revision history for this message
Gabriel Samfira (gabriel-samfira) wrote :

So, I implemented rollover. It works as well, but compared to just making MAX_BLOCK_SIZE bigger, its painfully slow. We can have both, to be honest...

I updated the pull request here:

https://github.com/shylent/python-tx-tftp/pull/17

Feel free to test the changes. They work well in my setup, but having someone else have a stab at it can not hurt.

Revision history for this message
Gavin Panella (allenap) wrote :

The larger block size works fine for me on my home network, using tftp-hpa as the client. I'll try to give it a go next week on another network.

The change to rollover probably deserves a test - the maintainer will probably ask for one - so it might be worth splitting the changes up again.

Revision history for this message
Gabriel Samfira (gabriel-samfira) wrote :

I split up the changes into 2 pull requests. Also added tests for the rollover bit. You can find them here:

https://github.com/shylent/python-tx-tftp/pull/17
https://github.com/shylent/python-tx-tftp/pull/18

Revision history for this message
Andres Rodriguez (andreserl) wrote :

@Gavin,

CAn you please land this so I can go ahead and upload to Ubuntu? Thanks

Revision history for this message
Gavin Panella (allenap) wrote :

The target branch is an import from GitHub. Gabriel has submitted the change upstream, and the maintainer seems happy with it, but has not yet merged it.

Revision history for this message
Gabriel Samfira (gabriel-samfira) wrote :

Changes have been merged.

Revision history for this message
Gabriel Samfira (gabriel-samfira) wrote :

Hello there,

The changes I requested upstream have been merged:

https://github.com/shylent/python-tx-tftp/pull/17

Would it be possible to sync the above revision? This merge implements rollover tftp, which allows transfering files of any size, without changing the MAX_BLOCK_SIZE.

Revision history for this message
Andres Rodriguez (andreserl) wrote :

IIRC,

This broke other environments. Let's test various other environments before
we pull it in
Thanks
On Oct 20, 2014 11:15 AM, "Gabriel Samfira" <email address hidden>
wrote:

> Hello there,
>
> The changes I requested upstream have been merged:
>
> https://github.com/shylent/python-tx-tftp/pull/17
>
> Would it be possible to sync the above revision? This merge implements
> rollover tftp, which allows transfering files of any size, without changing
> the MAX_BLOCK_SIZE.
> --
>
> https://code.launchpad.net/~gabriel-samfira/python-tx-tftp/python-tx-tftp/+merge/208862
> You are reviewing the proposed merge of
> lp:~gabriel-samfira/python-tx-tftp/python-tx-tftp into lp:python-tx-tftp.
>

Revision history for this message
Gabriel Samfira (gabriel-samfira) wrote :

The BLOCK_MAX_SIZE (PR #18) broke some environs, the rollover thing (PR #17) should not break anything :-). Feel free to test on any setup available to you. On esxi, kvm and our NUC setup, I had no issues.

Unmerged revisions

39. By Gabriel Samfira on 2014-02-28

Increase MAX_BLOCK_SIZE to 8192

The current MAX_BLOCK_SIZE only allows files of up to ~87 MB in size.
Considering that a windows winpe ramdisk can grow to about ~270 MB,
it would just fail after block nr 65535. Increasing this constant
will allow larger files to be transmited.

It is not mandatory for the client to ask for a block size as large as this. It
is just a higher cap, up to which a client may negotiate.

Preferably, this would be configurable per backend, and not hardcoded
in session.py.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'tftp/session.py'
--- tftp/session.py 2012-09-25 05:29:40 +0000
+++ tftp/session.py 2014-02-28 18:06:19 +0000
@@ -9,7 +9,7 @@
9from twisted.internet.protocol import DatagramProtocol9from twisted.internet.protocol import DatagramProtocol
10from twisted.python import log10from twisted.python import log
1111
12MAX_BLOCK_SIZE = 140012MAX_BLOCK_SIZE = 8192
1313
1414
15class WriteSession(DatagramProtocol):15class WriteSession(DatagramProtocol):

Subscribers

People subscribed via source and target branches

to all changes: