Skip to content
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

Fixed OOD shell timeout issue by injecting heartbeat function #14

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wtripp180901
Copy link
Contributor

@wtripp180901 wtripp180901 commented Jul 19, 2023

For some reason the OOD shell doesn't seem to be affected by either ClientAliveInterval in the sshd config or ServerAliveInterval in the ssh config. This only happens via the OOD shell, I tried ssh-ing from the login pod to itself as OOD would and the issue doesn't occur. This could potentially be a bug
https://discourse.openondemand.org/t/ssh-keep-alive-shell-access-on-ondemand/384/14
OSC/ondemand#2269
https://discourse.openondemand.org/t/novnc-and-shell-session-timeout-after-1-minute/1622

I found a horrible fix here:
https://discourse.openondemand.org/t/novnc-and-shell-session-timeout-after-1-minute/1622/6

I'm mounting the file from the OOD source with those changes in before startup

diff of modified app.js file with the original:

101,102c101,102
<   let match = url.match(host_path_rx),
<   hostname = null,
---
>   let match = url.match(host_path_rx),
>   hostname = null,
112,117d111
< function noop() {}
<
< function heartbeat() {
<   this.isAlive = true;
< }
<
160,162d153
<   ws.isAlive = true;
<   ws.on('pong', heartbeat);
<
168,175d158
<
< const interval = setInterval(function ping() {
<   wss.clients.forEach(function each(ws) {
<     if (ws.isAlive === false) return ws.terminate();
<     ws.isAlive = false;
<     ws.ping(noop);
<   });
< }, 30000);

@wtripp180901 wtripp180901 requested a review from sjpb July 19, 2023 14:17
Base automatically changed from ood to main August 15, 2023 10:06
@sjpb
Copy link
Collaborator

sjpb commented Aug 18, 2023

@wtripp180901 could you find a way of providing a diff between the upstream file and the one this PR injects please? It can just go into the first comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants