Just to clarify, you would like to see: v0.9.10 pre-patch client talk to v0.9.10 patch server v0.9.10 patch client talk to v0.9.10 pre-patch server

Would the test code I used in my cover letter be sufficient? If so, I could probably test this fairly easily and quickly today.

On 03/15/2012 05:51 AM, Daniel P. Berrange wrote: > On Thu, Mar 15, 2012 at 06:22:38AM -0500, Jesse Cook wrote: >> Just to clarify, you would like to see: I have not actually tried this, but based on my understanding of the RPC famework, I'd expect: >> >> v0.9.10 pre-patch client talk to v0.9.10 patch server If there are 256 or fewer names, this works as it always did. If there are more than 256 names, then the server tries to send back more array elements than the client is prepared to receive - the client will treat the response as an invalid RPC, and my recollection is that under these circumstances, the client then forcefully disconnects the session under the assumption that the server is broken. To avoid breaking such clients, you'd probably need some way for clients to warn the server if they are prepared to receive more than the old max (similar to how we had to add VIR_TYPED_PARAM_STRING_OKAY as a way for clients to tell the server that they were prepared to handle typed strings being returned).  Basically, any time you change the RPC to allow more data to be returned than what was previously possible, the server should not return that additional data unless the client has negotiated that it is aware of the additional data. >> v0.9.10 patch client talk to v0.9.10 pre-patch server No problem for the client.  The server will never successfully return more than 256 names.  However, if the client requests more than 256 names, I'm not sure whether the server will silently truncate to 256 or whether it will return an RPC error because it was unable to provide the reply. > > And for comparison > >   v0.9.10 pre-patch client talk to v0.9.10 pre-patch server Should be the same behavior as a v0.9.10-patch client talking to a pre-patch server - you can't successfully send more than 256 names, but I'm not sure how the server will react if the client requests too many names.  Actually, it might be slightly different - the client might reject the request up front without even contacting the server. > > Obviously for > >   v0.9.10 patch client talk to v0.9.10 patch server > > I'd expect "just works" :-) Of course, if both sides are prepared to handle the new RPC protocol, it should just work. -- Eric Blake   eblake(a)redhat.com    +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 03/15/2012 08:26 PM, Jesse Cook wrote: > > v0.9.10 client did not want to play nicely with the v0.9.10 server via > qemu+ssh. I got frustrated and just tried running the test from a > client running an older version of libvirt. When connecting to > v0.9.10, it behaved the same way the pre-patched did in my cover > letter. I don't have full test results because of the communication > errors I was getting. I'll try to either figure it out tomorrow or > just use the older version as the client (pre-patch and patch). Since qemu already uses rpc mechanism, I've found it easy to mix-and-match by doing this in two terminals both logged in as root, and both located in a libvirt.git checkout directory (if you don't mind building as root; if you do mind building as root, then have three, where the third is a regular user running as non-root to do the builds): setup: 1# git clone libvirt ... 1# cd libvirt 1# ./autobuild.sh --system 1# apply patches I'm interested in testing 1# make 2# cd libvirt old libvirtd server, old virsh client: 1# service libvirtd start 2# virsh ... 1# service libvirtd stop old libvirtd, new virsh 1# service libvirtd start 2# tools/virsh ... 1# service libvirtd stop new libvirtd, old virsh 1# daemon/libvirtd 2# virsh ... 1# Ctrl-C new libvirtd, new virsh 1# daemon/libvirtd 2# tools/virsh ... 1# Ctrl-C That gives me testing of all four protocol combinations, assuming that the installed version is a current release without my patch, and my just-built in-tree version has my patch. No need to complicate protocol testing with multi-host qemu+ssh:// stuff. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Thu, Mar 15, 2012 at 08:31:32AM -0600, Eric Blake wrote: > On 03/14/2012 07:42 PM, Jesse J. Cook wrote: >> 256 (8 bits) is insufficient for large scale deployments. 65536 (16 bits) is a >> more appropriate limit and should be sufficient. You are more likely to run >> into other system limitations first, such as the 31998 inode link limit on >> ext3. > correctly. See this comment: > > /* Maximum length of a memory peek buffer message. > * Note applications need to be aware of this limit and issue multiple > * requests for large amounts of data. > */ > const REMOTE_DOMAIN_MEMORY_PEEK_BUFFER_MAX = 65536; > > That is, you cannot send any RPC with more than 64k data, because . > With a cap of 256 pool names, that means each pool name can be (on > average) about 256 bytes before you hit the RPC cap. Not quite right, you meant to look at virnetrpcprotocol.x: /* Size of message payload */ const VIR_NET_MESSAGE_PAYLOAD_MAX = 262120;

On 03/15/2012 08:31 AM, Eric Blake wrote: > It may also be worth considering the addition of _just_ a new ranged RPC > call, but no new public API, by making the public API smart enough to > automatically call the ranged RPC multiple times to build up a single > return to the user.  After all, if we exposed the ranged call to the > user, then each client will have to add the logic to make the multiple > calls; whereas our current limitation is _only_ in the RPC aspect (once > the data is across the RPC and into the target process' memory space, > there's no longer a 64k cap to worry about). Thinking more about this idea: Suppose we have the following in remote_protocol.x: Existing: struct remote_storage_pool_list_volumes_args {    remote_nonnull_storage_pool pool;    int maxnames; }; struct remote_storage_pool_list_volumes_ret {    remote_nonnull_string names<REMOTE_STORAGE_VOL_NAME_LIST_MAX>; /* insert@1 */ }; ...    REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES = 92, /* autogen skipgen priority:high */ (note the change from autogen to skipgen for src/remote) New: struct remote_storage_pool_list_volumes_get_transaction_args {    remote_nonnull_storage_pool pool;    int maxnames; }; struct remote_storage_pool_list_volumes_get_transaction_ret {    int transaction; }; struct remote_storage_pool_list_volumes_transaction_args {    remote_nonnull_storage_pool pool;    int transaction;    int start; }; struct remote_storage_pool_list_volumes_ret {    remote_nonnull_string names<REMOTE_STORAGE_VOL_NAME_LIST_MAX>; /* insert@1 */ }; ...    REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_GET_TRANSACTION = ... /* skipgen skipgen priority:high */    REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_TRANSACTION = ... /* skipgen skipgen priority:high */ Then, in pseudocode, we change the currently-generated src/remote handler for storagePoolListVolumes to do: if maxnames <= REMOTE_STORAGE_VOL_NAME_LIST_MAX    use REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES /* existing code */ else {    int transaction, i = 0;    pass pool and maxnames through REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_GET_TRANSACTION to set transaction    while (i < maxnames) {        pass pool and transaction through REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_TRANSACTION to get 128 more names        i += REMOTE_STORAGE_VOL_NAME_LIST_MAX;    } } Then, in daemon/remote.c, we leave the existing handler (which calls back into virStoragePoolListVolumes with 256 or less entries) untouched, and add two new hand-written RPC handlers: the handler for REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_GET_TRANSACTION:    register that a sequenced transaction has been started    allocate an array of maxnames and position and call virStoragePoolListVolumes, and tie that result to the sequence    return the sequence transaction number the handler for REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_TRANSACTION:    validates that the sequence number is valid, and that the start value matches the recorded position    prepares the return RPC with the next 128 names from the array tied to the sequence, and updates the position    if the position reached the end, free the array and end the sequenced transaction we also update virnetserver.c to support sequenced commands - the idea here is that any time a transaction will need more than one RPC message between client and server, then a transaction number can be generated which reserves memory tied to the transaction; as long as that transaction number exists, then further commands sent with reference to that transaction can reuse that memory; the transaction is then closed if the server and client complete the transaction, but also closed (and frees the reserved memory) if the connection disappears.  To avoid a misbehaving client from DoS'ing a server, we could also require that while a transaction is open, the next RPC call from the client must reference that transaction, and must come within a reasonable time limit.  To some extent, we already have a 'continue' status for a message; right now, a 'call' can only be replied by 'ok' or 'error', but this would be a case where 'call' is now replied by 'continue' to state that a transaction is in effect, and where we could reuse the 'serial' for as long as the transaction is in effect. At any rate, I don't have code to back this up, but it's certainly food for thought on whether it makes sense to try and enhance our use of RPC to support transactions without also having to add new public API. -- Eric Blake   eblake(a)redhat.com    +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 03/15/2012 08:31 AM, Eric Blake wrote: > It may also be worth considering the addition of _just_ a new ranged RPC > call, but no new public API, by making the public API smart enough to > automatically call the ranged RPC multiple times to build up a single > return to the user. After all, if we exposed the ranged call to the > user, then each client will have to add the logic to make the multiple > calls; whereas our current limitation is _only_ in the RPC aspect (once > the data is across the RPC and into the target process' memory space, > there's no longer a 64k cap to worry about). Thinking more about this idea: Suppose we have the following in remote_protocol.x: Existing: struct remote_storage_pool_list_volumes_args { remote_nonnull_storage_pool pool; int maxnames; }; struct remote_storage_pool_list_volumes_ret { remote_nonnull_string names<REMOTE_STORAGE_VOL_NAME_LIST_MAX>; /* insert@1 */ }; ... REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES = 92, /* autogen skipgen priority:high */ (note the change from autogen to skipgen for src/remote) New: struct remote_storage_pool_list_volumes_get_transaction_args { remote_nonnull_storage_pool pool; int maxnames; }; struct remote_storage_pool_list_volumes_get_transaction_ret { int transaction; }; struct remote_storage_pool_list_volumes_transaction_args { remote_nonnull_storage_pool pool; int transaction; int start; }; struct remote_storage_pool_list_volumes_ret { remote_nonnull_string names<REMOTE_STORAGE_VOL_NAME_LIST_MAX>; /* insert@1 */ }; ... REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_GET_TRANSACTION = ... /* skipgen skipgen priority:high */ REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_TRANSACTION = ... /* skipgen skipgen priority:high */ Then, in pseudocode, we change the currently-generated src/remote handler for storagePoolListVolumes to do: if maxnames <= REMOTE_STORAGE_VOL_NAME_LIST_MAX use REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES /* existing code */ else { int transaction, i = 0; pass pool and maxnames through REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_GET_TRANSACTION to set transaction while (i < maxnames) { pass pool and transaction through REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_TRANSACTION to get 128 more names i += REMOTE_STORAGE_VOL_NAME_LIST_MAX; } } Then, in daemon/remote.c, we leave the existing handler (which calls back into virStoragePoolListVolumes with 256 or less entries) untouched, and add two new hand-written RPC handlers: the handler for REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_GET_TRANSACTION: register that a sequenced transaction has been started allocate an array of maxnames and position and call virStoragePoolListVolumes, and tie that result to the sequence return the sequence transaction number the handler for REMOTE_PROC_STORAGE_POOL_LIST_VOLUMES_TRANSACTION: validates that the sequence number is valid, and that the start value matches the recorded position prepares the return RPC with the next 128 names from the array tied to the sequence, and updates the position if the position reached the end, free the array and end the sequenced transaction we also update virnetserver.c to support sequenced commands - the idea here is that any time a transaction will need more than one RPC message between client and server, then a transaction number can be generated which reserves memory tied to the transaction; as long as that transaction number exists, then further commands sent with reference to that transaction can reuse that memory; the transaction is then closed if the server and client complete the transaction, but also closed (and frees the reserved memory) if the connection disappears. To avoid a misbehaving client from DoS'ing a server, we could also require that while a transaction is open, the next RPC call from the client must reference that transaction, and must come within a reasonable time limit. To some extent, we already have a 'continue' status for a message; right now, a 'call' can only be replied by 'ok' or 'error', but this would be a case where 'call' is now replied by 'continue' to state that a transaction is in effect, and where we could reuse the 'serial' for as long as the transaction is in effect.

On 03/15/2012 11:48 AM, Daniel P. Berrange wrote: > Using the sequence of RPC calls to iterate over this is only > addressing that second part of memory usage. So I'm not really > feeling that's a huge win, given the complexity it introduces. > > I'm inclined to say, that if people are creating setups with > 1000's of volume & guests, then they can probably spare the > extra memory for us to increase the main RPC message payload > limit somewhat. Our own docs/internals/rpc.html.in states: >       Although the protocol itself defines many arbitrary sized data values in the >       payloads, to avoid denial of service attack there are a number of size limit >       checks prior to encoding or decoding data. There is a limit on the maximum >       size of a single RPC message, limit on the maximum string length, and limits >       on any other parameter which uses a variable length array. These limits can >       be raised, subject to agreement between client/server, without otherwise >       breaking compatibility of the RPC data on the wire. Increasing limits makes sense, as long as we have a sane way to do it while ensuring that on version mismatch, a large packet from the sender doesn't crash an older receiver expecting the smaller limit. In our XDR, should we be converting some of our 'type name<LIST_MAX>' over to 'type name<>' notations, to state that they are effectively unlimited within the larger bounds of the overall packet size?  Is 256k for overall packet size still okay for the problem at hand? -- Eric Blake   eblake(a)redhat.com    +1-919-301-3266 Libvirt virtualization library http://libvirt.org