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

vnc: run websockify as background process #813

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

utkarshayachit
Copy link
Contributor

Instead of running as a daemon, making websockify run as a background process. This ensures that the process gets cleaned up property if and when job is terminated. With certain job schedulers/environments (e.g. OpenPBS+AzHOP), this avoids dangling processes lingering post job termination.

@utkarshayachit
Copy link
Contributor Author

not doing the same of vnc_container since there the issue of potential dangling processes won't exist since when the container is terminated, so will the websockify process.

@johrstrom
Copy link
Contributor

OK - I'll have to pull this down and take a closer look at it. BTW - it looks like it conflicts with the other PR I just merged.

@utkarshayachit
Copy link
Contributor Author

sure thing...I've pushed an updated to resolve the conflict as well. BTW, in my experiments I was added a cleanup code in cleanup_sscript to find and the started websockify process as well. While not strictly necessary, perhaps makes sense to add that to cleanup everything gracefully, what do you think.

here's the snipped. If you think it is reasonable I can update the MR to add a cleaned up version of the same:

find_websockify_pid() {
  local ws_port=$1
  # this matches command line used in start_websockify().
  local command_line="#{websockify_cmd} $ws_port"
  local pid=$(ps -eo pid,cmd | grep "$command_line" | grep -v grep | awk '{print \$1}')
  echo "$pid"
}
# cleanup websockify, if started
if [ -n "$websocket" ]; then
  ws_pid=$(find_websockify_pid ${websocket})
  if [ -z "$ws_pid" ]; then
    echo "[warning] websockify process not found!" >&2
  else
    echo "[info] killing websockify [pid=$ws_pid]" >&2
    kill $ws_pid
  fi
fi

@johrstrom
Copy link
Contributor

While not strictly necessary, perhaps makes sense to add that to cleanup everything gracefully, what do you think.

Errmm... I'm not sure. Or at least I don't like grepping through ps output to find it. If we do, or need to, maybe we should save the PID to a $PWD/.websockify_pid or similar (quickly tried to look through /proc/self, but couldn't find anything).

@utkarshayachit
Copy link
Contributor Author

Errmm... I'm not sure. Or at least I don't like grepping through ps output to find it.

Roger that. We can table that for now. I'll try to investigate in due time to see if there are better ways of getting the websockify PID.

Instead of running as a daemon, making websockify run as a background
process. This ensures that the process gets cleaned up property if and
when job is terminated. With certain job schedulers/environments
(e.g. OpenPBS+AzHOP), this avoids dangling processes lingering post job
termination.
@utkarshayachit
Copy link
Contributor Author

the latest revision looks at websockify output to ensure the process is started successfully as well.

Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, sorry for the delay. With the new semester starting, I had a lot of OSC work to complete.

In any case, I've looked this over now and it appears to work well. Thanks for the addition!

@johrstrom johrstrom merged commit 9cb9dd3 into OSC:master Aug 28, 2023
3 checks passed
@utkarshayachit
Copy link
Contributor Author

no problem at all! Thanks for the review/merge.

@utkarshayachit utkarshayachit deleted the background_websockify branch August 28, 2023 21:28
@johrstrom johrstrom mentioned this pull request Nov 28, 2023
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