Enable compression on remote messages - #4137
Conversation
nrwahl2
left a comment
nrwahl2
left a comment
There was a problem hiding this comment.
Reviewed everything EXCEPT the last commit.
|
|
||
| free(remote->buffer); | ||
| remote->buffer = uncompressed; | ||
| header = localized_remote_header(remote); |
There was a problem hiding this comment.
As written, assigning this to header is pointless. header goes out of scope one line later when we return from this function.
We need this function to take a pointer-to-pointer argument, in order for this value to reach the caller. Then use *header in all the places where we use header currently.
I suspect this would break if the endianness differed between the systems.
Currently the header argument could be const. However, it will need to remain non-const when we change it to a pointer-to-pointer.
Edit to add: It works because a later commit moves this assignment back to the caller. At that point, this function can take a const argument and doesn't need a pointer-to-pointer.
We could add a note that this function invalidates the header pointer in the caller. header actually always points to remote->buffer; it exists as a variable for type casting purposes. This function frees and reassigns remote->buffer on success.
Another idea that sounds a bit appealing to me right now (very late at night lol)... if the following is valid syntax, we could have a separate header struct object, instead of pointing at remote->buffer. Like:
struct remote_header_v0 header = { 0, };
...
header = *((struct remote_header_v0 *) remote->buffer);
Just to have a little less coupling after the assignment. I'm not passionate about this though.
There was a problem hiding this comment.
I've updated this area as follows:
- Remove the unnecessary
header = localized_remote_header(remote);from the patch that introduceshandle_compressed_payload - Do the assignment after the one caller.
- Add doxygen on
handle_compressed_payloadto explain that it invalidates the header parameter. - Make the header parameter const.
Doing so allowed me the "Remove a redundant call..." patch, and then since I changed the comments, I also dropped the "Minor reformatting..." patch.
I can make further changes in this area if necessary.
nrwahl2
left a comment
nrwahl2
left a comment
There was a problem hiding this comment.
Looks good. Minor, annoying comments, most of which don't strictly have to be acted upon.
nrwahl2
added
waiting for author
e1a54f5 to
83c7f4a
Compare
clumens
added
review: in progress
83c7f4a to
d63bedf
Compare
|
@nrwahl2 Do you have any additional comments on this PR? If not, I'm gonna merge it. |
nrwahl2
left a comment
nrwahl2
left a comment
There was a problem hiding this comment.
I wrote most of these comments a month ago, but I never finished and submitted the review. Finished now.
| &new_size) == pcmk_rc_ok) { | ||
|
|
||
| /* The above call might have given us a new_size value that's | ||
| * different from the original payload_len, so we need to repeat the |
There was a problem hiding this comment.
No action needed:
You'd expect the new_size would be less than or equal to payload_len since we're compressing it. But the libbz2 doc cryptically says "To guarantee that the compressed data will fit in its buffer, allocate an output buffer of size 1% larger than the uncompressed data, plus six hundred extra bytes." Which I guess is somehow a detail of the compression algorithm.
There was a problem hiding this comment.
Yeah, that's a pretty odd comment. I wish there were more explanation. I wonder if it needs some temporary working space to shuffle things around, or if there's some pathological case where compressed data can end up larger than uncompressed data and that's the maximum size it could ever be.
There was a problem hiding this comment.
For posterity:
The "1%" and "six hundred" go back to the beginning of the manual, with no explanation that I can find in the commit messages or anything.
https://sourceware.org/git/?p=bzip2.git;a=commitdiff;h=977101ad5f833f5c0a574bfeea408e5301a6b052
The most recent version of the manual (which wasn't appearing for me in web searches) at least says the following:
Compression is always performed, even if the compressed file is slightly larger than the original. Files of less than about one hundred bytes tend to get larger, since the compression mechanism has a constant overhead in the region of 50 bytes. Random data (including the output of most file compressors) is coded at about 8.05 bits per byte, giving an expansion of around 0.5%.
There was a problem hiding this comment.
If we touch this code again, it wouldn't hurt to a comment that links to or quotes from the manual, so that we know where the recommendation came from at least.
| dest_len = max; | ||
| rc = BZ2_bzBuffToBuffCompress(compressed, &dest_len, uncompressed, length, | ||
| PCMK__BZ2_BLOCKS, 0, PCMK__BZ2_WORK); | ||
| *result_len = dest_len; |
There was a problem hiding this comment.
...We really should never run into the bizarre scenario where UINT_MAX > SIZE_MAX, dest_len > length (i.e., the compressed size is greater than the uncompressed size, which is apparently possible based on the recommended value for max), and this assignment overflows. I'm fine with not worrying about that.
This just splits the code that handles the possibility of a compressed payload out of pcmk__remote_message_xml and into its own function. Also return a standard pacemaker return code.
* Perform this check before even looking at the payload. * The error message doesn't need to refer to being able to decompress the payload. It doesn't have anything to do with that. * Remove a redundant check that happens later on during payload XML parsing.
Even though it's sent plain text, it might still be compressed and therefore printing it out will just look like garbage.
Outside of testing, it's always 0.
The idea here is that we can consolidate some of the integer overflow checking into this function, making its callers less complicated. Note that send_cpg_text needs a lot of the same checking that's been added to pcmk__remote_send_xml recently, though we don't need to be quite as defensive in that function. We don't allow just any client to connect to the cluster layer, so the potential for bad actors should be considerably lower. For the moment I'm just making sure the function continues to compile. Also note that send_cpg_text never returns false indicating that sending failed. There's probably a lot of error handling that could be done.
This has been enabled on the receive side of remote connections since 2013 just in case we ever want to use it. However, because we've never sent compressed messages, nothing ever used this code and so it's developed various bugs. I think it's better if we just enable this code instead of carrying around unused and buggy code. There's not really much to do in pcmk__remote_send_xml. We just need to make sure that the message is big enough to make compression worth it, do the compression, and make sure all the sizes are correct in the message header. We also need to be careful to free things at the right time and with the right function given that pcmk__compress allocates memory for the compressed version. Two additional notes: * I've chosen PCMK__BZ2_THRESHOLD as the definition of "big enough" because we are using this elsewhere for the same purpose, but we could experiment with different values here. I do not want to make this a user-exposed tuneable. * You may remember that I previously removed compression support for IPC messages in favor of multi-part messages. Adding compression support here might seem a little inconsistent, but I'm unsure that we can actually do that and still retain backwards compatibility. At the least, we would have to bump the remote message protocol and probably change the header format which would make it difficult to support older remote systems. For now at least, using the compression code we essentially already supported is easiest.
I don't find this information useful, though it could be handy to see that compression happened. Also, use %zu as the format specifier for a size_t in the error message.
d63bedf to
cad5f33
Compare
2 participants
Decompression on the receive side has been supported forever in a "just in case we ever want this..." basis. However, the fact that it was unused meant there were bugs in it, for instance the recent integer overflow stuff. So, I'm enabling it here both to ensure it gets used and therefore is less buggy and to continue improving our scalability.
I've tested the following combinations to try to make sure there's no backwards compatibility problems here:
✓ old cluster, new remote CIB, no compression
✓ old cluster, new remote CIB, force compression
✓ old cluster, new remote node, no compression
✓ old cluster, new remote node, force compression
✓ new cluster, old remote CIB, no compression
✓ new cluster, old remote CIB, force compression
✓ new cluster, new remote node, no compression
✓ new cluster, new remote node, force compression
✓ new cluster, old remote node, no compression
✓ new cluster, old remote node, force compression