[Oglopf]

What does this PR do?

vnc_clean walked vncserver -list and killed sessions whose PID failed kill -0, and clean_script then ran a second kill by display.

• In vnc_container, the PIDs from -list are from the instance's PID namespace, so the host-side kill -0 can't tell live from dead. vnc_clean killed our session and the second kill then errored. The second kill also ran on the host rather than in the container (vnc_container error in clean_up function #931).
• In vnc, the check passed for our live session, so the second line was the one doing the work.
• -list can also include the user's other sessions on the node.

Now both templates' vnc_clean kill only this job's display (inside the instance for vnc_container), and the second line is gone. The [[ -n ${display} ]] guard stays inside vnc_clean, because it also runs in the startup retry loop and after a failed start, before display is set. vncserver -kill : errors on TurboVNC.

Behavior changes:

• Old crashed sessions are no longer swept from ~/.vnc.
• A site that overrides :vnc_clean now owns the whole cleanup.

Related issue

Closes #931

Testing

• Tests included

• test/batch_connect/vnc_test.rb and test/batch_connect/vnc_container_test.rb: vnc_clean kills only this job's display, does nothing before the server starts, and clean_script kills once.

• test/batch_connect/vnc_cleanup_test.rb (new): runs each whole rendered job script against fake vncserver, websockify and container commands. Checks what gets killed on a normal exit, a script failure, a websockify failure, and a server that never starts. Also checks that another job's session is left alone, and that vnc_container kills inside the instance before instance stop. Fails on master for vnc_container error in clean_up function #931.

bundle exec rake test and bundle exec rake spec are green.

Checklist

• Follows project code style and conventions
• Documentation provided (if new feature, adapter or behavior change) — Yes, though we say this is the case in our docs already, not it is just truly the case.
• This is a large feature and was discussed in an issue first (if applicable) — N/A

Anything else?

Not yet run against a real container with this branch. The end-to-end test uses fake vncserver and container commands, and doesn't reproduce the instance's PID namespace. Killing by display doesn't depend on PIDs, though, so the namespace shouldn't matter here.

The container clean_script now renders as:

[[ -e "clean.sh" ]] && source "clean.sh"
module load singularity

[[ -n ${display} ]] && singularity exec instance://<id> vncserver -kill :${display}
singularity instance stop <id>

@johrstrom, if the apptainer setup from your review is still around, a run against this branch would confirm it.

[johrstrom]

I'm finding that the default vnc_clean (issued above this) already does this correctly.

ood_core/lib/ood_core/batch_connect/templates/vnc_container.rb

Lines 251 to 255 in 88deb05

	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)}')
	end.to_s
	end

I've got this output here where the default vnc_clean already stops so when this is issued, it errors.

+ awk '/^:/{system("kill -0 "$2" 2>/dev/null || apptainer exec instance://990eb0d3-fac6-40af-b0e7-31aefcbb7bfb vncserver -kill "$1)}'
+ apptainer exec instance://990eb0d3-fac6-40af-b0e7-31aefcbb7bfb vncserver -list
Killing Xvnc process ID 54
+ [[ -n 1 ]]
+ apptainer exec instance://990eb0d3-fac6-40af-b0e7-31aefcbb7bfb vncserver -kill :1

Can't find file /root/.vnc/p0220.ten.osc.edu:1.pid
You'll have to kill the Xvnc process manually

Seems strange to even have this 2nd fallback that just checks for the display environment variable and not an actual vnc server because the environment variable doesn't clear when the vncserver stops. I.e., [[ -n $display ]] will always be true.

I'm also finding that the original clean this is based off of has the opposite behavior - the default vnc_clean fails and always comes to this fallback which then works...

Still looking into why one would work and the other wouldn't but it seems to me we don't need this fallback and what's in vnc_clean should work.

[johrstrom]

I am now realizing vnc_clean works by coincidence in this file - kill -0 <containerPID> fails because the container is PID namespaced and containerPID is not the host PID. So the || expression then moves on to correctly kill the process.

Not sure why they used an || expression in the vnc template or even bothered to grab it out of vncserver -list.

In any case, it seems to me that vnc_clean should be updated in both files and this 2nd fallback removed. I think vncserver -kill :${display} is the way to go without extracting from vncserver -list as there may be multiple servers running anyhow.

[Oglopf]

Really sorry at the delay, but this got a bit more involved in the tests than i was expecting.

Agreed, and done in both templates. vnc_clean now kills by display (-kill :${display}, run inside the instance for vnc_container), and the second kill line in clean_script is gone.

I kept [[ -n ${display} ]], moved inside vnc_clean. You're right that it's always true once the server has started, so it doesn't tell us whether the server is alive. That's not its job. vnc_clean also runs before each attempt in the startup retry loop, and in clean_up 1 when the server never starts, and display isn't set yet in either place. Without the guard those would run vncserver -kill : with an empty display, which TurboVNC rejects with an error, so a failed start would fill the log with those errors on top of the real one.

Tests:

• vnc_test.rb and vnc_container_test.rb: vnc_clean kills only this job's display, does nothing before the server starts, and clean_script kills exactly once.
• New vnc_cleanup_test.rb: runs the whole rendered job script against fake vncserver, websockify and container commands, and checks what gets killed when the script exits, fails, websockify fails, or the server never starts. It also checks that another job's session on the node is left alone, and that the container template kills inside the instance before instance stop. It fails on master for the original vnc_container error in clean_up function #931 bug, and runs in about 2.5s.

Two side effects:

• The stale-session sweep is gone, so old crashed sessions aren't cleaned up from ~/.vnc anymore. Given what you found about -list across jobs and PID namespaces, that seemed like the right trade.
• A site that overrides :vnc_clean now owns the whole cleanup, since the hard-coded kill line is gone. I can add a CHANGELOG note if you'd like.

Also rebased onto master and moved the container test to test/batch_connect/.

[johrstrom]

A site that overrides :vnc_clean now owns the whole cleanup, since the hard-coded kill line is gone. I can add a CHANGELOG note if you'd like.

CHANGELOG does not appear to be up to date, but yea I think that's probably a good idea to update it. Doesn't look like the official docs say what vnc_clean does so that'll need an update too.

[Oglopf]

Added a CHANGELOG entry under Unreleased (plus the missing one for #1006 since why not). For the docs, I opened OSC/ood-documentation#1438, which documents vnc_clean along with the vncserver_cmd/vncpasswd_cmd options from #1006. It should merge after this one, since it describes the new vnc_clean behavior.

[johrstrom]

Both vnc and vnc_container templates work for me.