OSC / ood_core

Open OnDemand core library
https://osc.github.io/ood_core/
MIT License
10 stars 30 forks source link

vnc: run websockify as background process #813

Closed utkarshayachit closed 1 year ago

utkarshayachit commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

utkarshayachit commented 1 year ago

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

utkarshayachit commented 1 year ago

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