[Oglopf]

Please review our Contributing Guide before submitting a pull request.

What does this PR do?

Lets sites pin vncserver and vncpasswd to full paths in the VNC template, the same way websockify already works. A user's .bashrc (a conda env, for example) can put other binaries ahead of the site's on PATH.

• New context options vncserver_cmd and vncpasswd_cmd, defaulting to ${VNCSERVER_CMD:-vncserver} and ${VNCPASSWD_CMD:-vncpasswd}. With neither set, the script runs the same bare commands as before.
• All six bare calls in vnc.rb use them.
• The stale-session cleanup runs vncserver -kill inside awk's system(), which starts a new shell. The command is now expanded by bash and passed in with awk -v, so a VNCSERVER_CMD that's set but not exported still takes effect there.

Related issue

#208. This covers ood_core's side; see below.

Testing

• Tests included

New test/batch_connect/vnc_test.rb, the first tests for the VNC template:

• the defaults render the env-var form, and a context override puts the given path in every call site
• no bare vncserver/vncpasswd calls remain
• the cleanup snippet runs in bash against fake vncserver scripts: it uses the configured command even when VNCSERVER_CMD isn't exported, and kills only dead sessions

All five tests fail against master's vnc.rb. Reverting just the awk -v change fails the not-exported test.

Checklist

• Follows project code style and conventions
• Documentation provided (if new feature, adapter or behavior change): YARD docs for the new options. The OOD docs' batch connect VNC options would want the two new keys next to websockify_cmd; I can follow up there.
• This is a large feature and was discussed in an issue first (if applicable): N/A

Anything else?

• The failure in the original Discourse report came from openssl, which ood_core has never called. It's in bc_osc_jupyter/template/before.sh.erb, which still runs a bare openssl dgst -sha1. That needs its own fix in that repo, so it may make sense to keep Make batch connect dependencies less dependent on $PATH #208 open for it or move it there.
• vnc_container.rb has the same bare calls. I left it out because vnc_container error in clean_up function #931 changes its vncserver -kill line; I'll follow up once that merges.

[Oglopf]

Ensure this is merged before #1013 and i will rebase that PR after this is merged.

[Bubballoo3]

In trying to test this in practice, but with OSC being pretty slow at the moment I think I'm gonna have to wait for next week. I did encounter something though, and would be curious if you can replicate the issue or if I am doing something wrong

[Bubballoo3]

On an OSC compute node with a desktop running, I ran

[bsingleton@a0347 ~]$ vncserver -list

WARNING: vncserver has been replaced by a systemd unit and is now considered deprecated and removed in upstream.
Please read /usr/share/doc/tigervnc/HOWTO.md for more information.

TigerVNC server sessions:

X DISPLAY #     PROCESS ID
:11             2926002
[bsingleton@a0347 ~]$ vncserver -list | awk -v vncserver_cmd="vncserver" '/^:/{system("kill -0 "$2" 2>/dev/null || " vncserver_cmd " -kill "$1)}'
[bsingleton@a0347 ~]$ vncserver -list

WARNING: vncserver has been replaced by a systemd unit and is now considered deprecated and removed in upstream.
Please read /usr/share/doc/tigervnc/HOWTO.md for more information.

TigerVNC server sessions:

X DISPLAY #     PROCESS ID
:11             2926002

Which seems to suggest that the process was not actually killed. Are you able to replicate this or is this line of testing faulty somehow?

Note: I made this a comment because I did the same line of testing on the previous version and found the same result. So I am mostly wondering if I have encountered something legit that I should make an issue for.

[Oglopf]

Thanks for digging into this, Braeden. I think what you saw is expected rather than a bug: the cleanup line only runs vncserver -kill when kill -0 <pid> fails, i.e. when the session's PID is already dead. With a desktop running, :11 is live, so it's left alone. The old version behaved the same way, since the awk -v change only affects which binary gets called.

I checked on OSC compute nodes (tigervnc-server-1.14.1-10.el9_6, same deprecated wrapper):

• Live session: kill -0 <pid> exits 0, the cleanup line runs, and the session is still listed afterward.
• Crashed session (kill -9 on the Xvnc process): the entry drops out of vncserver -list on its own, so the cleanup line gets empty input and does nothing, with no errors.

What I haven't exercised with the real binary is the dead-PID branch actually calling vncserver -kill, since I couldn't get a stale entry to stay listed on 1.14. Only the fake-vncserver test in vnc_test.rb covers that.

If you want to reproduce this yourself, use an interactive job rather than an OOD desktop (killing Xvnc under a desktop ends the job):

srun -A <project> -N 1 -n 1 -t 00:30:00 --pty /bin/bash
vncserver :77 -geometry 800x600 -SecurityTypes None -localhost   # only if you have no ~/.vnc/passwd
vncserver -list
P=$(vncserver -list 2>/dev/null | awk -v d=":77" '$1==d{print $2}')
kill -0 "$P"; echo $?      # 0 -> live; run the cleanup one-liner, session stays listed
kill -9 "$P"; sleep 2
vncserver -list            # entry gone; cleanup one-liner is a no-op

Unrelated to this PR: vncserver on these nodes prints a deprecation warning saying it's replaced by a systemd unit, so it may be worth checking whether -kill keeps working there long-term.

[johrstrom]

Notice that that's tigervnc that's installed on the machine - you need to load a module for turbovnc.

that said, that expression doesn't work and the script just works by coincidence because after it executes that (that doesn't work) it then executes the next line that does work.

ood_core/lib/ood_core/batch_connect/templates/vnc.rb

Lines 201 to 202 in 88deb05

	#{vnc_clean}
	[[ -n ${display} ]] && vncserver -kill :${display}

Unrelated to this PR: vncserver on these nodes prints a deprecation warning saying it's replaced by a systemd unit, so it may be worth checking whether -kill keeps working there long-term.

For that reason above, I think -kill ${display} is actually the only reliable way we should be killing these servers. Not only does that awk expression not work, when it issues -list it could list out other vncservers from your other jobs on the same compute node and kill them. Display on the other hand is unique to this current job.

[Bubballoo3]

I am happy to move forward with this as I have been able to test it out in the wild and confirm both that the default state remains the same (previous apps continue to work unchanged) and that individual apps can set the commands in submit.yml.

I have not been able to find the a way to propagate environment variables to the template script (I've tried exporting from before.sh and setting job_environment in submit.yml with no success.) Throwing it out there not because I suspect anything is wrong, just not sure what the env variable case looks like in practice.