5 ms·

As part of the fix, they mention changing the Python etcd client code to send the request in a single packet. How would one force that behavior? And is that eve

by csears 7y ago

As part of the fix, they mention changing the Python etcd client code to send the request in a single packet. How would one force that behavior? And is that even a good idea? It seems like TCP fragmentation would be handled at a much lower level in the Python networking stack.

q3k 7y ago
No idea. This 'fix' doesn't make any sense.
- Rapzid 7y ago
  I'm skeptical and would like to see some etcd/golang github issues referenced on this.
- mst 7y ago
  It makes perfect sense as "makes it 95% less likely to hit this bug." Notice from the write up that they already also modified their code to be resilient against the bug being hit at all, so "reducing the number of times the resilience code needs to be invoked by 95%" on top of that is a net win.
- de_watcher 7y ago
  This fix is supper common. There are tonnes of software that freak out when you don't send a message in single call hopefully resulting in one packet. People don't understand that TCP alone doesn't really handle application-level messages.
MarkSweep 7y ago
I’m guessing they switched from making two system calls to send data on the socket to a single system call.
- jchw 7y ago
  This is my guess - two write calls to one write call. I think this will happen to work even in the presence of Nagle’s algorithm, because there is initially not going to be an already sent packet waiting acknowledgement, so at least the first write call will probably be unbuffered in that sense. Assuming an mtu of 1500, ~40 byte TCP header, and a little bit of HTTP/1.1 headers, that leaves probably plenty of room for the etcd PUT to not get fragmented, I think.
  - q3k 7y ago
    That's a lot of assumptions and 'probably' for what supposedly fixes a critical bug.
    - jchw 7y ago
      No disagreement here - it could certainly break in the future, and is certainly not a solution anyone should rely on. However my reading of this is that it would have worked the same way like 20 years ago, so it’s probably not imminently at risk of working differently tomorrow.
      - q3k 7y ago
        I'm not so sure. It's one `mtu 150` fatfinger away from potentially breaking again.
        jchw 7y ago
        I think mtu 150 would be noticeable quicklier than this fairly rare bug :) and probably any MTU short enough to cause a problem here probably. At least for busy servers, I’d expect to be very puzzled about a sudden dramatic uptick in network and probably CPU usage. Edit: and it would probably break DNS as well.
    - jhgg 7y ago
      There are two fixes listed. One is to improve client behavior to reduce the likelihood of an incomplete request to be handled. The other is to fix the crash bug that occurred when an incomplete request is handled. The fix would be incomplete with only the first mitigation.
- q3k 7y ago
  That doesn't guarantee the write data won't become fragmented on the wire.
hackernudes 7y ago
I don't think it was related to TCP fragmentation. It must have been a different network issue, timeout, crash, or other bug that caused the connection to close. They were relying on the server side to verify the http Content-Length header to avoid partial writes, which never happened and they consider this to be a bug in the golang http handler. Quoted from the writeup: """ We determined that the connection was reset after sending the first packet, but before the second packet could be sent. """ My bet is when they say "packet" here they really mean write/send. They describe the old version as doing one write for the http headers and one for the body. The new version makes just one write.
toast0 7y ago
There's two ways to force this: a) write the whole request in a single write call like MarkSweep said. b) use tcp socket options to change behavior of multiple writes, TCP_NOPUSH on BSD, or TCP_CORK on Linux. Option a is preferable in this case because it's a lot fewer system calls; although it's a once a minute job, so system calls on the client side don't really matter. Is it a good idea? In this case, certainly --- if the whole request is small enough to fit in the network MTU, it avoids a protocol error on the server side. In the more general case of a HTTP request where the content is known and in memory at the time of header generation, yes, I would say it's more efficient to send the content with a single context switch (buffer space permitting) and fewer network packets. With a caveat to be aware of, that if the larger network packets trigger MTU problems, the experience will be negative. If the content size is known, but the content isn't loaded into memory, such as when you're uploading a file, so fstat gave you the size, but you haven't read it yet, sending off just the headers with content-size is probably better --- if the content takes time to load, you'll get the request to the server to validate it sooner than if you waited for a full packet, and that improves processing time in case the server rejects the request, or has to do something time consuming before reading the content. If the content size is unknown, so presumably the content hasn't been chosen or generated yet, sending headers soon is usually better for a similar reason. Clearly, the etcd server should be changed to properly process HTTP requests that arrive over multiple TCP segments, but it was likely more expedient to "fix" the client as part of the rapid response. But, this client change could still be useful, as it should reduce packet processing load on the etcd server, and the various network equipment. Probably not a big impact, unless there's a ton of clients, but similar changes where the usage is higher can make a big difference.

by csears 7y ago

q3k 7y ago

No idea. This 'fix' doesn't make any sense.

Rapzid 7y ago
I'm skeptical and would like to see some etcd/golang github issues referenced on this.
mst 7y ago
It makes perfect sense as "makes it 95% less likely to hit this bug." Notice from the write up that they already also modified their code to be resilient against the bug being hit at all, so "reducing the number of times the resilience code needs to be invoked by 95%" on top of that is a net win.
de_watcher 7y ago
This fix is supper common. There are tonnes of software that freak out when you don't send a message in single call hopefully resulting in one packet. People don't understand that TCP alone doesn't really handle application-level messages.

MarkSweep 7y ago

I’m guessing they switched from making two system calls to send data on the socket to a single system call.

jchw 7y ago
This is my guess - two write calls to one write call. I think this will happen to work even in the presence of Nagle’s algorithm, because there is initially not going to be an already sent packet waiting acknowledgement, so at least the first write call will probably be unbuffered in that sense. Assuming an mtu of 1500, ~40 byte TCP header, and a little bit of HTTP/1.1 headers, that leaves probably plenty of room for the etcd PUT to not get fragmented, I think.
- q3k 7y ago
  That's a lot of assumptions and 'probably' for what supposedly fixes a critical bug.
  - jchw 7y ago
    No disagreement here - it could certainly break in the future, and is certainly not a solution anyone should rely on. However my reading of this is that it would have worked the same way like 20 years ago, so it’s probably not imminently at risk of working differently tomorrow.
    - q3k 7y ago
      I'm not so sure. It's one `mtu 150` fatfinger away from potentially breaking again.
      - jchw 7y ago
        I think mtu 150 would be noticeable quicklier than this fairly rare bug :) and probably any MTU short enough to cause a problem here probably. At least for busy servers, I’d expect to be very puzzled about a sudden dramatic uptick in network and probably CPU usage. Edit: and it would probably break DNS as well.
  - jhgg 7y ago
    There are two fixes listed. One is to improve client behavior to reduce the likelihood of an incomplete request to be handled. The other is to fix the crash bug that occurred when an incomplete request is handled. The fix would be incomplete with only the first mitigation.
q3k 7y ago
That doesn't guarantee the write data won't become fragmented on the wire.

hackernudes 7y ago

I don't think it was related to TCP fragmentation. It must have been a different network issue, timeout, crash, or other bug that caused the connection to close. They were relying on the server side to verify the http Content-Length header to avoid partial writes, which never happened and they consider this to be a bug in the golang http handler. Quoted from the writeup: """ We determined that the connection was reset after sending the first packet, but before the second packet could be sent. """ My bet is when they say "packet" here they really mean write/send. They describe the old version as doing one write for the http headers and one for the body. The new version makes just one write.

toast0 7y ago

There's two ways to force this: a) write the whole request in a single write call like MarkSweep said. b) use tcp socket options to change behavior of multiple writes, TCP_NOPUSH on BSD, or TCP_CORK on Linux. Option a is preferable in this case because it's a lot fewer system calls; although it's a once a minute job, so system calls on the client side don't really matter. Is it a good idea? In this case, certainly --- if the whole request is small enough to fit in the network MTU, it avoids a protocol error on the server side. In the more general case of a HTTP request where the content is known and in memory at the time of header generation, yes, I would say it's more efficient to send the content with a single context switch (buffer space permitting) and fewer network packets. With a caveat to be aware of, that if the larger network packets trigger MTU problems, the experience will be negative. If the content size is known, but the content isn't loaded into memory, such as when you're uploading a file, so fstat gave you the size, but you haven't read it yet, sending off just the headers with content-size is probably better --- if the content takes time to load, you'll get the request to the server to validate it sooner than if you waited for a full packet, and that improves processing time in case the server rejects the request, or has to do something time consuming before reading the content. If the content size is unknown, so presumably the content hasn't been chosen or generated yet, sending headers soon is usually better for a similar reason. Clearly, the etcd server should be changed to properly process HTTP requests that arrive over multiple TCP segments, but it was likely more expedient to "fix" the client as part of the rapid response. But, this client change could still be useful, as it should reduce packet processing load on the etcd server, and the various network equipment. Probably not a big impact, unless there's a ton of clients, but similar changes where the usage is higher can make a big difference.