Have lsf_driver specify SIGKILL when bkilling

[jonathan-eq]

This commit makes the lsf_driver use bkill with the SIGKILL signal instead of the default SIGINT.

Issue
Resolves #7443

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

• PR title captures the intent of the changes, and is fitting for release notes.
• Added appropriate release note label
• Commit history is consistent and clean, in line with the contribution guidelines.
• Make sure tests pass locally (after every commit!)

When applicable

• When there are user facing changes: Updated documentation
• New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
• Large PR: Prepare changes in small commits for more convenient review
• Bug fix: Add regression test for the bug
• Bug fix: Create Backport PR to latest release

[berland]

If the mother ERT process is terminated (say an eager user hits ctrl-c), this signal will be propagated and kill this subprocess, we might not want that, we want this script with bkills live happily independent of mother Ert.

I think you want to do as here:

ert/src/ert/scheduler/local_driver.py

Line 70 in 2b180e9

preexec_fn=os.setpgrp,

in order to avoid this.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 84.52%. Comparing base (e4d25a1) to head (9d23320).
Report is 2 commits behind head on main.

Files	Patch %	Lines
src/clib/lib/job_queue/lsf_driver.cpp	0.00%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7433      +/-   ##
==========================================
- Coverage   85.25%   84.52%   -0.74%     
==========================================
  Files         387      387              
  Lines       23055    23069      +14     
  Branches      881      887       +6     
==========================================
- Hits        19655    19498     -157     
- Misses       3290     3464     +174     
+ Partials      110      107       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jonathan-eq]

Should I change this in the job queue driver too?

[berland]

Should I change this in the job queue driver too?

Yes I think that makes sense.

[jonathan-eq]

I marked it as a bug fix because bkill uses different signals than qdel, and the users got confused because it showed as KeyboardInterrupt in the logs.

[pinkwah]

This is not the correct behaviour. There needs to be a SIGKILL after a while, otherwise a hanging job will hang forever taking up resources.

[berland]

This is not the correct behaviour. There needs to be a SIGKILL after a while, otherwise a hanging job will hang forever taking up resources.

Do we expect the job to react differently to SIGTERM than SIGINT? The job can choose to ignore both. I am thinking that changing to SIGTERM solves the confusion for the users, and that sending an additional SIGKILL later is a new feature.

[jonathan-eq]

I updated this to do SIGKILL instead of SIGTERM.

[xjules]

I think it looks good now! Good job @jonathan-eq ! 🚀

[berland]

Have you performed a manual test of the "kill all realizations" button in the GUI with this change? (or some other real-world test of the changed C-code?)

[jonathan-eq]

Have you performed a manual test of the "kill all realizations" button in the GUI with this change? (or some other real-world test of the changed C-code?)

It does indeed NOT work.

 SIGKILL 785865: Illegal signal value
 SIGKILL 785866: Illegal signal value
 SIGKILL 785867: Illegal signal value
 SIGKILL 785868: Illegal signal value
 SIGKILL 785869: Illegal signal value
 SIGKILL 785870: Illegal signal value
 SIGKILL 785871: Illegal signal value
 SIGKILL 785872: Illegal signal value
 SIGKILL 785873: Illegal signal value

[jonathan-eq]

Have you performed a manual test of the "kill all realizations" button in the GUI with this change? (or some other real-world test of the changed C-code?)

It does indeed NOT work.

 SIGKILL 785865: Illegal signal value
 SIGKILL 785866: Illegal signal value
 SIGKILL 785867: Illegal signal value
 SIGKILL 785868: Illegal signal value
 SIGKILL 785869: Illegal signal value
 SIGKILL 785870: Illegal signal value
 SIGKILL 785871: Illegal signal value
 SIGKILL 785872: Illegal signal value
 SIGKILL 785873: Illegal signal value

Fixed it!!

[xjules]

Very good! 🚀