From 852f51b2d437b11770d1a8813bc3da04609c79c3 Mon Sep 17 00:00:00 2001 From: Travis Ravert Date: Fri, 25 Sep 2026 18:51:32 -0400 Subject: [PATCH 1/7] wrap clean_script as vnc_clean already does --- .../batch_connect/templates/vnc_container.rb | 2 +- test/job/adapters/vnc_container_test.rb | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 test/job/adapters/vnc_container_test.rb diff --git a/lib/ood_core/batch_connect/templates/vnc_container.rb b/lib/ood_core/batch_connect/templates/vnc_container.rb index bbda18c41..ca0a3625e 100644 --- a/lib/ood_core/batch_connect/templates/vnc_container.rb +++ b/lib/ood_core/batch_connect/templates/vnc_container.rb @@ -198,7 +198,7 @@ def clean_script module load #{container_module} #{vnc_clean} - [[ -n ${display} ]] && vncserver -kill :${display} + [[ -n ${display} ]] && #{container_command} exec instance://#{@instance_name} vncserver -kill :${display} #{container_command} instance stop #{@instance_name} EOT end diff --git a/test/job/adapters/vnc_container_test.rb b/test/job/adapters/vnc_container_test.rb new file mode 100644 index 000000000..5ac7a8a06 --- /dev/null +++ b/test/job/adapters/vnc_container_test.rb @@ -0,0 +1,29 @@ +# test/batch_connect/templates/vnc_container_test.rb +require 'test_helper' +require 'ood_core/batch_connect/templates/vnc_container' + +class VNCContainerTest < Minitest::Test + def template(context = {}) + OodCore::BatchConnect::Templates::VNC_Container.new( { work_dir: '/tmp'}.merge(context) ) + end + + def test_clean_script_kills_vnc_inside_the_container + script = template.send(:clean_script) + + assert_includes(script, 'singularity exec instance://') + refute_match(/&&\s+vncserver -kill/, script) + end + + def test_clean_script_uses_configured_container_command + script = template(container_command: 'apptainer').send(:clean_script) + + assert_includes(script, 'apptainer exec instance://') + refute_includes(script, 'singularity exec instance://') + end + + def test_clean_script_stops_the_instance_after_killing_vnc + script = template.send(:clean_script) + + assert_operator(script.index('vncserver -kill'), :<, script.index('instance stop')) + end +end \ No newline at end of file From 44156b3982ad83bfd857790d6a3dbcef9a4c1c49 Mon Sep 17 00:00:00 2001 From: Travis Ravert Date: Tue, 13 Oct 2026 15:25:27 -0700 Subject: [PATCH 2/7] stop the vnc session by display in vnc_clean vnc_clean walked vncserver -list and killed sessions whose PID failed kill -0. In vnc_container those PIDs are from the instance's PID namespace, so the host-side check can't tell live from dead. It failed for our live session, killed it, and the second kill in clean_script then errored on the missing pid file. In vnc the check passed for our live session, so the second line was the one doing the work. Kill by display in vnc_clean for both templates and drop the second line. display is unset until the server starts, so the call in the startup loop and after a failed start does nothing. --- lib/ood_core/batch_connect/templates/vnc.rb | 17 ++-- .../batch_connect/templates/vnc_container.rb | 16 ++-- test/batch_connect/vnc_container_test.rb | 84 +++++++++++++++++++ test/batch_connect/vnc_test.rb | 42 +++++++--- test/job/adapters/vnc_container_test.rb | 29 ------- 5 files changed, 133 insertions(+), 55 deletions(-) create mode 100644 test/batch_connect/vnc_container_test.rb delete mode 100644 test/job/adapters/vnc_container_test.rb diff --git a/lib/ood_core/batch_connect/templates/vnc.rb b/lib/ood_core/batch_connect/templates/vnc.rb index a5cbac861..005a22733 100644 --- a/lib/ood_core/batch_connect/templates/vnc.rb +++ b/lib/ood_core/batch_connect/templates/vnc.rb @@ -60,8 +60,8 @@ class VNC < Template # @option context [#to_s] :extra_args ("") any extra arguments used # when initializing the vnc server process (not set if blank or # `:vnc_args` is set) (assumes you don't modify `:before_script`) - # @option context [#to_s] :vnc_clean ("...") script used to clean up - # any active vnc sessions (assumes you don't modify `:before_script` + # @option context [#to_s] :vnc_clean ("...") script used to stop + # this job's vnc session (assumes you don't modify `:before_script` # or `:clean_script`) # @see Template def initialize(context = {}) @@ -200,13 +200,12 @@ def after_script EOT end - # Clean up the running VNC server and any other stale VNC servers + # Stop the VNC server this job started def clean_script <<-EOT.gsub(/^ {14}/, "") #{super} #{vnc_clean} - [[ -n ${display} ]] && #{vncserver_cmd} -kill :${display} EOT end @@ -252,13 +251,13 @@ def vnc_args end.to_s end - # Clean up any stale VNC sessions + # Stop this job's VNC server. Kill by display rather than walking + # `vncserver -list`, which also lists the user's other sessions on + # this node. `display` is unset until the server has started, so + # this does nothing in the startup loop or after a failed start. def vnc_clean context.fetch(:vnc_clean) do - # awk's system() runs a new shell, so expand the command here - # and hand it to awk; an unexported $VNCSERVER_CMD would - # otherwise be lost in that shell. - %(#{vncserver_cmd} -list | awk -v vncserver_cmd="#{vncserver_cmd}" '/^:/{system("kill -0 "$2" 2>/dev/null || " vncserver_cmd " -kill "$1)}') + %([[ -n ${display} ]] && #{vncserver_cmd} -kill :${display}) end.to_s end end diff --git a/lib/ood_core/batch_connect/templates/vnc_container.rb b/lib/ood_core/batch_connect/templates/vnc_container.rb index ca0a3625e..e56de6d44 100644 --- a/lib/ood_core/batch_connect/templates/vnc_container.rb +++ b/lib/ood_core/batch_connect/templates/vnc_container.rb @@ -56,8 +56,8 @@ class VNC_Container < Template # @option context [#to_s] :extra_args ("") any extra arguments used # when initializing the vnc server process (not set if blank or # `:vnc_args` is set) (assumes you don't modify `:before_script`) - # @option context [#to_s] :vnc_clean ("...") script used to clean up - # any active vnc sessions (assumes you don't modify `:before_script` + # @option context [#to_s] :vnc_clean ("...") script used to stop + # this job's vnc session (assumes you don't modify `:before_script` # or `:clean_script`) # @option context [#to_s] :container_path ("vnc_container.sif") the path # to the container with VNC @@ -191,14 +191,13 @@ module load #{container_module} EOT end - # Clean up the running VNC server and any other stale VNC servers + # Stop the VNC server this job started, then its container def clean_script <<-EOT.gsub(/^ {14}/, "") #{super} module load #{container_module} #{vnc_clean} - [[ -n ${display} ]] && #{container_command} exec instance://#{@instance_name} vncserver -kill :${display} #{container_command} instance stop #{@instance_name} EOT end @@ -247,10 +246,15 @@ def vnc_args end.to_s end - # Clean up any stale VNC sessions + # Stop this job's VNC server. Kill by display rather than walking + # `vncserver -list`: the PIDs it reports are from inside the + # instance's PID namespace, so `kill -0` against them on the host + # can't tell live sessions from dead ones. `display` is unset until + # the server has started, so this does nothing in the startup loop + # or after a failed start. def vnc_clean context.fetch(:vnc_clean) do - %(#{container_command} exec instance://#{@instance_name} vncserver -list | awk '/^:/{system("kill -0 "$2" 2>/dev/null || #{container_command} exec instance://#{@instance_name} vncserver -kill "$1)}') + %([[ -n ${display} ]] && #{container_command} exec instance://#{@instance_name} vncserver -kill :${display}) end.to_s end end diff --git a/test/batch_connect/vnc_container_test.rb b/test/batch_connect/vnc_container_test.rb new file mode 100644 index 000000000..d0d737f45 --- /dev/null +++ b/test/batch_connect/vnc_container_test.rb @@ -0,0 +1,84 @@ +require 'test_helper' +require 'ood_core/batch_connect/templates/vnc_container' +require 'open3' +require 'tmpdir' + +# The VNC server runs inside an apptainer/singularity instance, so stopping +# it has to happen inside the instance too (ood_core#931). +class BatchConnectVNCContainerTest < Minitest::Test + def template(context = {}) + OodCore::BatchConnect::Templates::VNC_Container.new({ work_dir: '/tmp' }.merge(context)) + end + + def test_clean_script_kills_vnc_inside_the_container + script = template.send(:clean_script) + + assert_includes(script, 'singularity exec instance://') + refute_match(/&&\s+vncserver -kill/, script) + end + + def test_clean_script_uses_configured_container_command + script = template(container_command: 'apptainer').send(:clean_script) + + assert_includes(script, 'apptainer exec instance://') + refute_includes(script, 'singularity exec instance://') + end + + def test_clean_script_stops_the_instance_after_killing_vnc + script = template.send(:clean_script) + + assert_operator(script.index('vncserver -kill'), :<, script.index('instance stop')) + end + + # The kill used to happen twice: once from vnc_clean and again from a + # second line, which then errored because the pid file was already gone. + def test_clean_script_kills_once + script = template.send(:clean_script) + + assert_equal(1, script.scan('vncserver -kill').length) + end + + # vncserver -list inside the instance reports PIDs from the instance's PID + # namespace, which mean nothing to kill -0 on the host. Kill by display. + def test_cleanup_kills_only_this_jobs_display + Dir.mktmpdir do |dir| + container = fake_container(dir) + tpl = template(container_command: container) + cleanup = tpl.send(:vnc_clean) + + _out, status = Open3.capture2('bash', '-c', "display=2\n#{cleanup}") + + assert(status.success?) + assert_equal( + "exec instance://#{tpl.instance_variable_get(:@instance_name)} vncserver -kill :2\n", + File.read("#{container}.calls") + ) + end + end + + # display is only set once the server has started, so cleanup in the + # startup loop or after a failed start must not touch the container. + def test_cleanup_does_nothing_before_the_server_starts + Dir.mktmpdir do |dir| + container = fake_container(dir) + cleanup = template(container_command: container).send(:vnc_clean) + + Open3.capture2('bash', '-c', cleanup) + + refute(File.exist?("#{container}.calls")) + end + end + + private + + # A stand-in container command that records how it was called. + def fake_container(dir) + path = File.join(dir, 'container') + File.write(path, <<~BASH) + #!/bin/bash + echo "$*" >> "#{path}.calls" + BASH + File.chmod(0o755, path) + path + end +end diff --git a/test/batch_connect/vnc_test.rb b/test/batch_connect/vnc_test.rb index 48204c3b8..5429fe0a8 100644 --- a/test/batch_connect/vnc_test.rb +++ b/test/batch_connect/vnc_test.rb @@ -27,9 +27,9 @@ def test_context_sets_full_paths_everywhere vncpasswd_cmd: '/opt/TurboVNC/bin/vncpasswd' ).to_s - # The stale-session cleanup (list + kill) runs in two places, the startup - # loop and clean_up, so: 2 x 2, plus --help, start and the final kill. - assert_equal(7, script.scan('/opt/TurboVNC/bin/vncserver').length) + # vnc_clean runs in the startup loop and in clean_up, plus --help and + # the start itself. + assert_equal(4, script.scan('/opt/TurboVNC/bin/vncserver').length) assert_equal(1, script.scan('/opt/TurboVNC/bin/vncpasswd').length) end @@ -41,10 +41,8 @@ def test_no_bare_vnc_commands_are_left refute_match(/\bvncpasswd\b/, code) end - # The stale-session cleanup kills sessions from inside awk's system(), - # which starts a new shell. A $VNCSERVER_CMD that was set but not exported - # must still be the command that runs there, not whatever `vncserver` is - # first on the PATH. + # $VNCSERVER_CMD set but not exported must still be the command that + # runs, not whatever `vncserver` is first on the PATH. def test_cleanup_uses_the_configured_command_even_when_not_exported Dir.mktmpdir do |dir| configured = fake_vncserver(dir, 'configured') @@ -54,28 +52,50 @@ def test_cleanup_uses_the_configured_command_even_when_not_exported script = <<~BASH PATH="#{File.dirname(on_path)}:$PATH" VNCSERVER_CMD="#{configured}" + display=2 #{cleanup} BASH _out, status = Open3.capture2('bash', '-c', script) assert(status.success?) - assert_equal(":1\n", read_kills(configured)) + assert_equal(":2\n", read_kills(configured)) assert_equal('', read_kills(on_path)) end end - def test_cleanup_only_kills_dead_sessions + # The user can have other sessions on the same node (other jobs, or ones + # vncserver -list still reports). Only this job's display gets killed. + def test_cleanup_kills_only_this_jobs_display Dir.mktmpdir do |dir| vncserver = fake_vncserver(dir, 'plain') cleanup = build_template(vncserver_cmd: vncserver).send(:vnc_clean) - _out, status = Open3.capture2('bash', '-c', cleanup) + _out, status = Open3.capture2('bash', '-c', "display=2\n#{cleanup}") assert(status.success?) - assert_equal(":1\n", read_kills(vncserver)) + assert_equal(":2\n", read_kills(vncserver)) end end + # display is only set once the server has started, so cleanup in the + # startup loop or after a failed start must not kill anything. + def test_cleanup_does_nothing_before_the_server_starts + Dir.mktmpdir do |dir| + vncserver = fake_vncserver(dir, 'plain') + cleanup = build_template(vncserver_cmd: vncserver).send(:vnc_clean) + + Open3.capture2('bash', '-c', cleanup) + + assert_equal('', read_kills(vncserver)) + end + end + + def test_clean_script_kills_once + script = build_template.send(:clean_script) + + assert_equal(1, script.scan('vncserver} -kill').length) + end + private # A stand-in vncserver in its own directory. `-list` reports display :1 diff --git a/test/job/adapters/vnc_container_test.rb b/test/job/adapters/vnc_container_test.rb deleted file mode 100644 index 5ac7a8a06..000000000 --- a/test/job/adapters/vnc_container_test.rb +++ /dev/null @@ -1,29 +0,0 @@ -# test/batch_connect/templates/vnc_container_test.rb -require 'test_helper' -require 'ood_core/batch_connect/templates/vnc_container' - -class VNCContainerTest < Minitest::Test - def template(context = {}) - OodCore::BatchConnect::Templates::VNC_Container.new( { work_dir: '/tmp'}.merge(context) ) - end - - def test_clean_script_kills_vnc_inside_the_container - script = template.send(:clean_script) - - assert_includes(script, 'singularity exec instance://') - refute_match(/&&\s+vncserver -kill/, script) - end - - def test_clean_script_uses_configured_container_command - script = template(container_command: 'apptainer').send(:clean_script) - - assert_includes(script, 'apptainer exec instance://') - refute_includes(script, 'singularity exec instance://') - end - - def test_clean_script_stops_the_instance_after_killing_vnc - script = template.send(:clean_script) - - assert_operator(script.index('vncserver -kill'), :<, script.index('instance stop')) - end -end \ No newline at end of file From 14d229021aa1fb90ae975c4822a2c3d940469d18 Mon Sep 17 00:00:00 2001 From: Travis Ravert Date: Wed, 14 Oct 2026 14:34:38 -0700 Subject: [PATCH 3/7] run the vnc templates end to end to check what clean_up kills The unit tests check vnc_clean on its own. These render each template, run the whole job script against fake vncserver, websockify and container commands, and check which displays get killed when the script exits, when it fails, when websockify fails, and when the server never starts. Also that another job's session is left alone, and that the container template kills inside the instance before stopping it. --- test/batch_connect/vnc_cleanup_test.rb | 238 +++++++++++++++++++++++++ 1 file changed, 238 insertions(+) create mode 100644 test/batch_connect/vnc_cleanup_test.rb diff --git a/test/batch_connect/vnc_cleanup_test.rb b/test/batch_connect/vnc_cleanup_test.rb new file mode 100644 index 000000000..713c57c38 --- /dev/null +++ b/test/batch_connect/vnc_cleanup_test.rb @@ -0,0 +1,238 @@ +require 'test_helper' +require 'ood_core/batch_connect/templates/vnc' +require 'ood_core/batch_connect/templates/vnc_container' +require 'open3' +require 'tmpdir' +require 'fileutils' + +# Runs the whole rendered job script, start to clean_up, against fake +# vncserver/websockify/container commands, and checks which displays get +# killed on each exit path. The unit tests check vnc_clean on its own; these +# check that clean_up actually reaches it with `display` set, and only then. +class BatchConnectVNCCleanupTest < Minitest::Test + # Real TurboVNC's startup line, which the template parses for the display. + DESKTOP_LINE = "Desktop 'TurboVNC: fakehost:%d (user)' started on display fakehost:%d" + + def setup + # setsid and timeout are util-linux/coreutils; not on macOS by default + %w[setsid timeout pgrep].each do |cmd| + skip("needs #{cmd}") unless ENV['PATH'].split(':').any? { |d| File.executable?(File.join(d, cmd)) } + end + + @dir = Dir.mktmpdir + @bin = File.join(@dir, 'bin') + @state = File.join(@dir, 'state') + @work = File.join(@dir, 'work') + FileUtils.mkdir_p([@bin, @state, @work]) + write_fakes + end + + def teardown + # Stop any fake Xvnc a failing test left behind + Dir[File.join(@state, '*.pid')].each do |f| + Process.kill('KILL', File.read(f).to_i) rescue nil + end + FileUtils.rm_rf(@dir) + end + + def test_vnc_kills_its_display_once_when_the_script_exits + status = run_job(:vnc, script_exit: 0) + + assert_equal(0, status.exitstatus, output) + assert_equal([':1'], kills) + refute(xvnc_alive?(1), 'Xvnc for :1 should be stopped') + end + + def test_vnc_kills_its_display_when_the_script_fails + status = run_job(:vnc, script_exit: 3) + + assert_equal(1, status.exitstatus, output) + assert_equal([':1'], kills) + end + + # clean_up 1 after the server started, from the websockify step + def test_vnc_kills_its_display_when_websockify_fails + status = run_job(:vnc, script_exit: 0, websockify: '/bin/false') + + assert_equal(1, status.exitstatus, output) + assert_equal([':1'], kills) + end + + # Real vncserver -kill with an empty display errors out ("Can't tell if : + # is on "), so with no server started nothing should be killed. + def test_vnc_kills_nothing_when_the_server_never_starts + status = run_job(:vnc, script_exit: 0, start_fails: true) + + assert_equal(1, status.exitstatus, output) + assert_equal([], kills) + end + + # Another of the user's jobs on the same node has its own live session, + # and an old crashed one is still listed. Neither is this job's to clean + # up. (The old -list sweep killed the stale one; that was dropped on + # purpose, since in a container it couldn't tell live from dead.) + def test_vnc_leaves_other_sessions_alone + other = start_other_session(5) + stale_session(6) + + run_job(:vnc, script_exit: 0) + + assert_equal([':1'], kills) + assert(alive?(other), "the other job's session on :5 was killed") + ensure + Process.kill('KILL', other) rescue nil + end + + def test_container_kills_its_display_inside_the_instance_then_stops_it + status = run_job(:vnc_container, script_exit: 0) + + assert_equal(0, status.exitstatus, output) + assert_equal([':1'], kills) + calls = container_calls + kill = calls.index { |c| c.include?('vncserver -kill') } + stop = calls.index { |c| c.start_with?('instance stop') } + refute_nil(kill, calls.join("\n")) + assert_operator(kill, :<, stop) + assert_match(%r{\Aexec instance://\S+ vncserver -kill :1\z}, calls[kill]) + end + + def test_container_kills_nothing_but_still_stops_the_instance_when_the_server_never_starts + status = run_job(:vnc_container, script_exit: 0, start_fails: true) + + assert_equal(1, status.exitstatus, output) + assert_equal([], kills) + assert(container_calls.any? { |c| c.start_with?('instance stop') }, container_calls.join("\n")) + end + + private + + def run_job(kind, script_exit:, websockify: nil, start_fails: false) + File.write(File.join(@work, 'script.sh'), "#!/bin/bash\nexit #{script_exit}\n") + File.chmod(0o755, File.join(@work, 'script.sh')) + + vncserver = File.join(@bin, start_fails ? 'vncserver-fails' : 'vncserver') + script = + if kind == :vnc + OodCore::BatchConnect::Templates::VNC.new( + work_dir: @work, + vncserver_cmd: vncserver, + vncpasswd_cmd: File.join(@bin, 'vncpasswd'), + websockify_cmd: websockify || File.join(@bin, 'websockify') + ).to_s + else + # Inside the fake instance, bare `vncserver` resolves via PATH + FileUtils.ln_sf(vncserver, File.join(@bin, 'container-path', 'vncserver')) + OodCore::BatchConnect::Templates::VNC_Container.new( + work_dir: @work, + container_command: 'fakecontainer', + websockify_cmd: 'websockify' + ).to_s + end + + # The container template calls `module load`; sites provide it via Lmod + script = "module () { :; }\n#{script}" + env = { + 'PATH' => "#{@bin}/container-path:#{@bin}:#{ENV['PATH']}", + 'FAKE_STATE' => @state + } + # Each batch job runs in its own session, and the template relies on that + # (`pgrep -s 0 Xvnc`), so give the job one here too. + @output, status = Open3.capture2e(env, 'timeout', '60', 'setsid', '-w', 'bash', '-c', script, chdir: @work) + status + end + + attr_reader :output + + def kills + file = File.join(@state, 'kills') + File.exist?(file) ? File.read(file).split : [] + end + + def container_calls + file = File.join(@state, 'container') + File.exist?(file) ? File.read(file).lines.map(&:chomp) : [] + end + + def xvnc_alive?(display) + file = File.join(@state, "#{display}.pid") + File.exist?(file) && alive?(File.read(file).to_i) + end + + def alive?(pid) + Process.kill(0, pid) + true + rescue Errno::ESRCH + false + end + + # A live session from another job. It isn't in the job's session, just as + # another job's processes wouldn't be. + def start_other_session(display) + pid = Process.spawn(File.join(@bin, 'Xvnc'), '300', %i[out err] => File::NULL) + Process.detach(pid) + File.write(File.join(@state, "#{display}.pid"), pid.to_s) + pid + end + + def stale_session(display) + File.write(File.join(@state, "#{display}.pid"), '999999') + end + + def write_fakes + FileUtils.mkdir_p(File.join(@bin, 'container-path')) + + # The template finds the server with `pgrep -s 0 Xvnc`, which matches the + # process name, so the fake server is `sleep` under the name Xvnc. + sleep = ENV['PATH'].split(':').map { |d| File.join(d, 'sleep') }.find { |f| File.executable?(f) } + FileUtils.ln_s(sleep, File.join(@bin, 'Xvnc')) + + fake(:vncserver, <<~BASH) + state="$FAKE_STATE" + case "$1" in + --help|-help) echo "usage: vncserver ... -nohttpd"; exit 0 ;; + -list) + echo "TurboVNC sessions:"; echo; printf 'X DISPLAY #\\tPROCESS ID\\n' + for f in "$state"/*.pid; do [[ -e $f ]] && printf ':%s\\t\\t%s\\n' "$(basename "$f" .pid)" "$(cat "$f")"; done + exit 0 ;; + -kill) + d="${2#:}" + if [[ -z $d ]]; then echo "Can't tell if $2 is on fakehost"; exit 2; fi + echo "$2" >> "$state/kills" + if [[ ! -e "$state/$d.pid" ]]; then echo "Can't find file $state/$d.pid"; exit 1; fi + kill "$(cat "$state/$d.pid")" 2>/dev/null; rm -f "$state/$d.pid"; exit 0 ;; + esac + d=1; while [[ -e "$state/$d.pid" ]]; do d=$((d+1)); done + nohup "$(dirname "$(readlink -f "$0")")/Xvnc" 300 >/dev/null 2>&1 & + echo $! > "$state/$d.pid" + echo; printf "#{DESKTOP_LINE}\\n" $d $d; echo + BASH + + fake(:'vncserver-fails', <<~BASH) + [[ "$1" == --help || "$1" == -help ]] && exit 0 + echo "Fatal server error: fake failure"; exit 1 + BASH + + fake(:vncpasswd, 'cat') + + # -D (container template) daemonizes; otherwise log the line the vnc + # template waits for and stay up until clean_up kills it. + fake(:websockify, <<~BASH) + [[ "$1" == -D ]] && exit 0 + echo "WebSocket server settings: proxying from :$1 to $3" + exec sleep 300 + BASH + + # `exec instance://NAME cmd...` runs cmd; instance start/stop just log. + fake(:fakecontainer, <<~BASH) + echo "$*" >> "$FAKE_STATE/container" + [[ "$1" == exec ]] && { shift 2; exec "$@"; } + exit 0 + BASH + end + + def fake(name, body) + path = File.join(@bin, name.to_s) + File.write(path, "#!/bin/bash\n#{body}") + File.chmod(0o755, path) + end +end From 2522e00aaab7d9a4f83db939d042b2e1abbc4e11 Mon Sep 17 00:00:00 2001 From: Travis Ravert Date: Wed, 14 Oct 2026 15:54:03 -0700 Subject: [PATCH 4/7] shorten the vnc template's waits in the end to end test Most of the test's run time was the template's own sleeps: up to 0.9s between each of 10 start retries, and 1s while polling for websockify. A sleep on the job's PATH shortens just those and runs any other sleep as normal. 15s to about 2.5s. --- test/batch_connect/vnc_cleanup_test.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/batch_connect/vnc_cleanup_test.rb b/test/batch_connect/vnc_cleanup_test.rb index 713c57c38..4c8dea473 100644 --- a/test/batch_connect/vnc_cleanup_test.rb +++ b/test/batch_connect/vnc_cleanup_test.rb @@ -186,6 +186,14 @@ def write_fakes sleep = ENV['PATH'].split(':').map { |d| File.join(d, 'sleep') }.find { |f| File.executable?(f) } FileUtils.ln_s(sleep, File.join(@bin, 'Xvnc')) + # The template's own waits (up to 0.9s between start retries, 1s while + # polling for websockify) are most of this file's run time. Shorten + # just those; any other sleep runs as normal. + fake(:sleep, <<~BASH) + [[ $# -eq 1 && ( $1 == 1 || $1 =~ ^0\.[0-9]+s?$ ) ]] && exec #{sleep} 0.01 + exec #{sleep} "$@" + BASH + fake(:vncserver, <<~BASH) state="$FAKE_STATE" case "$1" in From 9768ac72edcb0538c061fa080505630bf0f62fff Mon Sep 17 00:00:00 2001 From: Travis Ravert Date: Wed, 14 Oct 2026 16:16:33 -0700 Subject: [PATCH 5/7] keep the end to end vnc test fast and tidy on Lmod systems Lmod sets BASH_ENV, so every bash the test starts sourced its init first. With a 0.1s init that took the file from 2.5s to 41s. module is stubbed in the test, so unset BASH_ENV for the job. The fake websockify also outlived each test: the template starts it inside $(...), so clean_up's pkill -P $$ can't reach it. Teardown now kills it. --- test/batch_connect/vnc_cleanup_test.rb | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/test/batch_connect/vnc_cleanup_test.rb b/test/batch_connect/vnc_cleanup_test.rb index 4c8dea473..7107b83dc 100644 --- a/test/batch_connect/vnc_cleanup_test.rb +++ b/test/batch_connect/vnc_cleanup_test.rb @@ -28,10 +28,11 @@ def setup end def teardown - # Stop any fake Xvnc a failing test left behind - Dir[File.join(@state, '*.pid')].each do |f| - Process.kill('KILL', File.read(f).to_i) rescue nil - end + # Stop any fake Xvnc a failing test left behind, and fake websockify + pids = Dir[File.join(@state, '*.pid')].map { |f| File.read(f).to_i } + procs = File.join(@state, 'procs') + pids += File.read(procs).split.map(&:to_i) if File.exist?(procs) + pids.each { |pid| Process.kill('KILL', pid) rescue nil } FileUtils.rm_rf(@dir) end @@ -133,7 +134,13 @@ def run_job(kind, script_exit:, websockify: nil, start_fails: false) script = "module () { :; }\n#{script}" env = { 'PATH' => "#{@bin}/container-path:#{@bin}:#{ENV['PATH']}", - 'FAKE_STATE' => @state + 'FAKE_STATE' => @state, + # Lmod sets BASH_ENV so `module` works in scripts, which makes every + # bash here (the job and each fake) source its init first. That's + # most of the run time on a cluster login node, and none of it is + # needed: `module` is stubbed above. + 'BASH_ENV' => nil, + 'ENV' => nil } # Each batch job runs in its own session, and the template relies on that # (`pgrep -s 0 Xvnc`), so give the job one here too. @@ -227,6 +234,9 @@ def write_fakes fake(:websockify, <<~BASH) [[ "$1" == -D ]] && exit 0 echo "WebSocket server settings: proxying from :$1 to $3" + # The template starts this inside $(...), so clean_up's pkill -P $$ + # can't reach it; teardown kills it instead. + echo $$ >> "$FAKE_STATE/procs" exec sleep 300 BASH From baf884b5d24432313f4f91aba1df702ffe0335fc Mon Sep 17 00:00:00 2001 From: Travis Ravert Date: Wed, 14 Oct 2026 16:35:01 -0700 Subject: [PATCH 6/7] wait for the fake Xvnc before the fake vncserver returns Real vncserver doesn't return until Xvnc is up. The fake did, so on a busy CI runner the template's pgrep -s 0 Xvnc could run before the background process became Xvnc, see no server, and retry on the next display. Clean-up then killed :2 and the tests expecting :1 failed. --- test/batch_connect/vnc_cleanup_test.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/batch_connect/vnc_cleanup_test.rb b/test/batch_connect/vnc_cleanup_test.rb index 7107b83dc..9037a3e31 100644 --- a/test/batch_connect/vnc_cleanup_test.rb +++ b/test/batch_connect/vnc_cleanup_test.rb @@ -218,7 +218,16 @@ def write_fakes esac d=1; while [[ -e "$state/$d.pid" ]]; do d=$((d+1)); done nohup "$(dirname "$(readlink -f "$0")")/Xvnc" 300 >/dev/null 2>&1 & - echo $! > "$state/$d.pid" + xvnc=$! + echo $xvnc > "$state/$d.pid" + # Real vncserver doesn't return until Xvnc is up. Without this wait + # the template's `pgrep -s 0 Xvnc` can run first on a busy machine, + # see no server, and retry on the next display. + for i in $(seq 500); do + read -r comm < /proc/$xvnc/comm 2>/dev/null + [[ $comm == Xvnc ]] && break + #{sleep} 0.01 + done echo; printf "#{DESKTOP_LINE}\\n" $d $d; echo BASH From 3e410ce744bd268c0bc138e5b7927ebda0846a3c Mon Sep 17 00:00:00 2001 From: Travis Ravert Date: Tue, 20 Oct 2026 09:25:35 -0700 Subject: [PATCH 7/7] add CHANGELOG enry for the vnc_clean change --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd2ac11cc..3da266caa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] +- VNC templates now stop only the job's own VNC server, by display, in [1000](https://github.com/OSC/ood_core/pull/1000). + This fixes the `vnc_container` clean up error in [931](https://github.com/OSC/ood_core/issues/931). + The default `vnc_clean` no longer clears other stale sessions found by `vncserver -list`, and sites + that override `vnc_clean` are now responsible for stopping the VNC server. + +- `vncserver` and `vncpasswd` can be pinned to full paths with `vncserver_cmd`/`vncpasswd_cmd` or `$VNCSERVER_CMD`/`$VNCPASSWD_CMD` in [1006](https://github.com/OSC/ood_core/pull/1006). + - Add `cluster_info` support to the PBS Pro adapter, reporting active/total processors, gpus, and nodes. # [0.26.1] - 07-31-2024