-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: adds WebSocket ping to interactive terminal (#14191) #14192
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #14192 +/- ##
==========================================
+ Coverage 49.61% 49.62% +0.01%
==========================================
Files 256 257 +1
Lines 43829 43973 +144
==========================================
+ Hits 21744 21820 +76
- Misses 19948 20006 +58
- Partials 2137 2147 +10
☔ View full report in Codecov by Sentry. |
This adds a WebSocket ping message on a 5-second interval, sent from the server to the client. This ensures that the interactive terminal will remain open and won't be closed by load balancers that are reaping idle connections. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erhudy let's get this into 2.8 at least. If it doesn't cause any problems, we can cherry-pick back to previous releases.
/cherry-pick release-2.8 |
This adds a WebSocket ping message on a 5-second interval, sent from the server to the client. This ensures that the interactive terminal will remain open and won't be closed by load balancers that are reaping idle connections. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
This adds a WebSocket ping message on a 5-second interval, sent from the server to the client. This ensures that the interactive terminal will remain open and won't be closed by load balancers that are reaping idle connections. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
…#14192) (#14399) * fix: adds WebSocket ping to interactive terminal (#14191) (#14192) This adds a WebSocket ping message on a 5-second interval, sent from the server to the client. This ensures that the interactive terminal will remain open and won't be closed by load balancers that are reaping idle connections. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> * fix: adds WebSocket ping to interactive terminal (#14191) (#14192) This adds a WebSocket ping message on a 5-second interval, sent from the server to the client. This ensures that the interactive terminal will remain open and won't be closed by load balancers that are reaping idle connections. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> --------- Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> Co-authored-by: Edmund Rhudy <erhudy@users.noreply.github.com>
…goproj#14192) This adds a WebSocket ping message on a 5-second interval, sent from the server to the client. This ensures that the interactive terminal will remain open and won't be closed by load balancers that are reaping idle connections. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
What version was this added to, I' on |
Fwiw, I tried to upgrade to v2.8.3+77556d9 but it still seems to hang, any idea of what version got fixed or if there is a further problem. |
I am running 2.8.2 and the fix is certainly present there, which I just verified by leaving a terminal open for more than 60 seconds. |
@erhudy do you have a way for me to confirm that the fix is trying to keep the terminal open or not. So I can determine if this is a new bug. Or if some reason the fix isn't working. B/c it seems like maybe I have an edge case that the fix isn't considering. I think the issue maybe that the Terminal is locking before if hits 1 min, like for example I tried to click off and come back after 45 secs and the terminal had locked. I can check timeouts or anything else people can think of that may effect this with a Load Balancer in GCP b/c it works fine with port-forrwarding to the ArgoCD Server. Seems like the timeout for the LB is set to 30 secs so let me try to increase that and test again. |
Alright so in GCP the default timeout for a backend service is 30 secs, with the default settings the terminal was hanging. After increasing that timeout > 60 secs (currently set for 3600 secs ie 1 hr) and I've been able to wait 10+ mins and return to a working terminal. Everything is working now as expected. I would recoomend that the docs https://argo-cd.readthedocs.io/en/stable/operator-manual/web_based_terminal/ be updated to call out the 60 sec check that now exists and that for LB's the timeout needs to be > 60 secs |
That seems unusual. I don't have a Google Cloud environment to test in - I developed this fix against an AWS ALB where the timeout is 60 seconds, and my terminals definitely live longer than that with no activity now. It shouldn't be necessary to set a 1h timeout on your LB. I did check DevTools in Edge/Firefox/Safari to see if they recorded ping/pong activity for WebSocket but did not see anything on any browser I checked. |
#14271 |
…goproj#14192) This adds a WebSocket ping message on a 5-second interval, sent from the server to the client. This ensures that the interactive terminal will remain open and won't be closed by load balancers that are reaping idle connections. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
This adds a WebSocket ping message on a 5-second interval, sent from the server to the client. This ensures that the interactive terminal will remain open and won't be closed by load balancers that are reaping idle connections.
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist:
Please see Contribution FAQs if you have questions about your pull-request.