[GitHub] [zookeeper] ztzg opened a new pull request #1716: ZOOKEEPER-4306: Avoid `CloseSessionTxn`s larger than `jute.maxbuffer`

[GitBox <gi...@apache.org>]

ztzg opened a new pull request #1716:
URL: https://github.com/apache/zookeeper/pull/1716


   Allowing a serialized `CloseSessionTxn` to grow larger than `jute.maxbuffer` is a really bad idea: not only does it quickly makes the target ensemble unavailable, it even prevents it from restarting.
   
   Such availability issues have been reported or fixed in the past, notably in [ZOOKEEPER-2101](https://issues.apache.org/jira/browse/ZOOKEEPER-2101) and [ZOOKEEPER-3496](https://issues.apache.org/jira/browse/ZOOKEEPER-3496).  This one is a bit different because of the "distance" between the cause and its effect.
   
   The size of a `CloseSessionTxn` transaction is directly related to the total length of the paths of the ephemeral nodes belonging to the session to be closed.  That "mass" of data can be built incrementally, across many requests; it is not that difficult for a small "leak" to push the transaction size over `jute.maxbuffer` if a session lasts long enough.
   
   This series adds serialization size bookkeeping to the per-session ephemeral node management, and fails ephemeral node creation requests which would potentially result in an "overflowing" `CloseSessionTxn`.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] ztzg commented on pull request #1716: ZOOKEEPER-4306: Avoid `CloseSessionTxn`s larger than `jute.maxbuffer`

[GitBox <gi...@apache.org>]

ztzg commented on pull request #1716:
URL: https://github.com/apache/zookeeper/pull/1716#issuecomment-863486548


   Cc: @hanm, @symat, @anmolnar.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] ztzg commented on pull request #1716: ZOOKEEPER-4306: Avoid `CloseSessionTxn`s larger than `jute.maxbuffer`

[GitBox <gi...@apache.org>]

ztzg commented on pull request #1716:
URL: https://github.com/apache/zookeeper/pull/1716#issuecomment-866691807


   Hi @eolivelli,
   
   Thank you for the initial review!
   
   Perhaps I should create a parallel thread on the `dev` mailing list, so that we can gather a maximum of ideas; what do you think? In the meantime, here are a few precisions and questions:
   
   > I am not sure this is the right way to address the problem.
   
   Right. Me neither, but I thought I'd start the discussion with a concrete proposal, so here we are :)
   
   > The problem is in close session, here you are adding an hard (unpredictable and u configurable) bound on the number of ephemeral nodes.
   
   Well, yes and no.
   
   The "unpreditable" bound exists—it's just that right now it does not cause failures at creation time, but rather a **hard** crash of the whole ensemble at session close. Hitting the new error is bad news, but not worse news than the status quo :)
   
   It is possible to "manually" delete the nodes, of course, in which case there is no issue at session close—but having that many ephemerals in a session probably means that the application has gone berserk.
   
   > If the problem is only about close session we could work only on that case.
   
   Yes, but. As far as I can tell, other options include:
   
   1.  Splitting `closeSession` into multiple transactions (or records) when necessary. Would this seem like a promising approach to you? I don't think we have a precedent in ZooKeeper, do we? This is something I considered, but I thought I'd start with this patch as this alternative might open a can of worms;
   
   2.  Ignoring `jute.maxbuffer` when reading (certain kinds of) records. One could argue that a record should be "good enough to read" if a peer (as opposed to a client) decided it was "good enough to write."
   
   One problem with these options, though, is that the amount of data still is under client control, and mostly not covered by the quota mechanism—they are, in a way, a loophole allowing clients to work around the (admittedly quite limited) DoS protection afforded by Jute.
   
   In any case: would you have additional suggestions to append to that list?
   
   > If you want to go down this way of limiting the number of ephemeral we must make it configurable somehow.
   
   First, we could of course make the "feature" optional—but again: the ensemble is in great danger if the limit is reached, which is why I did not.
   
   Note that the limit **is** configurable, though, even if not independently—it simply tracks `jute.maxbuffer`. Also:
   
   1.  A configurable limit only makes sense if set lower than `jute.maxbuffer`, as exceeding the latter would still expose the ensemble;
   
   2.  Allowing the admin to specify lower values would in effect be some kind of global "session size quota" setting. Which might or might not be useful.
   
   3.  I totally agree that specifying the limit as a **size** (as opposed to a count) is quite unintuitive—but this is the resource which needs protection.
   
   > You are also introducing a new error code, how do old clients will handle that error code?
   
   Badly, particularly when it comes to synchronous operations. (This is a common problem with the current ZooKeeper protocol; most clients are not very good at dealing with unexpected codes.) I actually have some patches about that which I should polish and submit here.
   
   But hey, at least they won't crash the ensemble :)
   
   Cheers, -D
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org