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.
| /* handle_compressed_payload copies the header to the front of the | ||
| * uncompressed buffer, so update the pointer here. | ||
| */ | ||
| header = (struct remote_header_v0 *) remote->buffer; |
There was a problem hiding this comment.
I've thought about this before, and I think I left it at the time in order to use localized_remote_header() as a sort of get_header() function in this case. But I'd rather not.
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.
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.
…eader. The first thing pcmk__remote_message_xml does is call localized_remote_header to check and extract the header from the incoming message. It then calls handle_compressed_payload which copies that header to the beginning of the uncompressed buffer and then appends the uncompressed payload. Calling that function again at the end of handle_compressed_payload seems to be redundant. All the byte-swapping assignments and checks have already happened on this same data. We do, however, want to update the header pointer in the caller so everything's pointing at the same data.
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.
nrwahl2
added
waiting for author
clumens
force-pushed
the
remote-decompress
branch
from
e1a54f5 to
83c7f4a
Compare
clumens
added
review: in progress
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