Expand payload to 10MB #59
Conversation
|
We may have found who is collapsing Platform memory .. |
|
I forgot to mention that this is related to partially solve the issue mentioned in: |
Is this an actual worry? If so, 2x or 3x the max payload may create additional pressure. |
Indeed, context is useful. Now this was a known limitation. It was discussed ages ago with @jordigg to implement a kind of chucking mechanism to prevent such hard limit. Worth also mentioning that @jimmypoms pointed out that Websocket should already support chunking, so it's not clear why there's this hard limit. Apart this, it can be increased to 10MB, but we should be aware that's kicking the can down the road. Also, there's a suspect that the agent is related to the abnormal usage of memory in platform. But we have no yet evidence for it. |
|
It should theoretically be possible to leverage Websocket fragmentation (I don't know how to use it from Micronaut), but it may make things more difficult because there are other messages like "command-request", "heartbeat," and "info" that we may need to "defragment" and handle on our side the JSON deserialization. So, adding a new message, "command-response-fragment", may make things easier with the current setup and more backwards compatible. On top of this, we need to stream the fragments throw Redis so we may end up fragmenting again the websocket fragments. Regarding "there's a suspect that the agent is related to the abnormal usage of memory in the platform", I suspect that the use of Overall, a more long-term solution to decouple this complexity could be to leverage Seqera Connect to handle the Agent-Platform communication channel. |
While we wait/decide on a longer-term solution, I'd move this PR forward knowing it is a stop-gap. |
|
@jordeu Any progress on this PR review? Ideally we would increase this to 15MB, but 10MB is an okay stop-gap to resolve the issue one of our key customers (Ziad at Biocommons, who is giving a talk at NF Summit BCN) is experiencing. CC: @pditommaso - Any major concerns about Platform memory, or can we safely move to 10MB? |
|
Think it's fine to move ahead, but we need to plan a more sustainable solution |
There was a problem hiding this comment.
Take into account that this 10485760 size will allow to transfer files of ~7.5MB because they are base64 encoded. We cannot increase much more without changing also the limit that we have on platform side.
Agent was never thought to transfer files, only to execute commands. In fact, we are transferring files using a command line execution of a cat. This is very inefficient and we should plan to add proper support for transferring files.
|
@jordeu Thanks for the review. The build is failing the checks. Anything we can do about that? |
|
Seems that GH Actions are broken and need an update. I can check tomorrow. |
|
@jordeu Any chance we could get this fixed on Monday so we can provide an update to the customer? Thanks! |
|
After merging #54 we'll be able to update and build this branch. |
jordeu
force-pushed
the
expand-payload
branch
from
a00f42a to
92b2932
Compare
|
Meanwhile, we wait for #54 approval, I've rebased this PR on top of that so it builds the binary and JAR distributions that can be downloaded from the artifacts section here: https://github.com/seqeralabs/tower-agent/actions/runs/10998755756 @robnewman @mbosio85 if you wish you can download and test it from there. |
jordeu
force-pushed
the
expand-payload
branch
from
92b2932 to
5ae3fb5
Compare
expands the maximum payload to 10MB