Have BKILL signal SIGKILL after SIGTERM #7482

jonathan-eq · 2024-03-20T07:48:21Z

Issue
Resolves #7405

Approach
This PR makes the LSF drivers spawn a new process to run bkill -s SIGKILL in a fire-and-forget manner.

(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

jonathan-eq · 2024-03-20T07:51:25Z

I wanted to use preexec_fn=os.setpgrp when spawning the process; to make sure it is not stopped if the main process quits. This was stopped by ruff, but maybe it is not needed.

src/ert/scheduler/lsf_driver.py:205:13: PLW1509 `preexec_fn` argument is unsafe when using threads

codecov-commenter · 2024-03-20T08:07:56Z

Codecov Report

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

Project coverage is 84.45%. Comparing base (a71a6df) to head (5b063dd).
Report is 12 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7482      +/-   ##
==========================================
- Coverage   84.51%   84.45%   -0.07%     
==========================================
  Files         387      387              
  Lines       23081    23087       +6     
  Branches      877      886       +9     
==========================================
- Hits        19507    19498       -9     
- Misses       3463     3482      +19     
+ Partials      111      107       -4

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

jonathan-eq · 2024-03-20T11:00:13Z

~~BLOCKED BY #7433~~

xjules · 2024-03-22T07:41:05Z

Have you actually tested that it works?

berland · 2024-03-22T08:45:55Z

Use ruff rule PLW1509 on the command line to get a further explanation.

jonathan-eq · 2024-03-22T11:49:42Z

I am not sure if the C part works. It seems to be logging duplicate. I wonder what happens when I pass NULL to the stdout-arg/stderr-arg instead of nullptr or a file. Is it then redirected to stdout? (Notice how it in the middle stops for a bit, then starts printing the same JobId again. Is this from the separate process?)

^C
Killing simulations...
Job <850975>: Job has already finished
Job <850971>: Job has already finished
Job <850973>: Job has already finished
Job <850977> is being terminated
Job <850979>: Job has already finished
Job <850981>: Job has already finished
Job <850983>: Job has already finished
Job <850986> is being terminated
Job <850988>: Job has already finished
Job <850990> is being terminated
Job <850992>: Job has already finished
Job <850994>: Job has already finished
Job <850996> is being terminated
Job <850998>: Job has already finished
Job <851000> is being terminated
Job <851002>: Job has already finished
Job <851004> is being terminated
Job <851006>: Job has already finished
Job <851008>: Job has already finished
Job <851010>: Job has already finished
Job <851012>: Job has already finished
Job <851016> is being terminated
Job <851014> is being terminated
Job <851018> is being terminated
Job <851020> is being terminated
Job <851022> is being terminated
Job <851024> is being terminated
Job <851026> is being terminated
Job <851028> is being terminated
Job <851030> is being terminated
Job <851032> is being terminated
Job <851034> is being terminated
Job <851036> is being terminated
Job <851038> is being terminated
Job <851040> is being terminated
Job <851044> is being terminated
Job <851042> is being terminated
Job <851046> is being terminated
Job <851048> is being terminated
Job <851050> is being terminated
Job <851052> is being terminated
Job <851054> is being terminated
Job <851056> is being terminated
Job <851058> is being terminated
Job <851060> is being terminated
Job <851062> is being terminated
Job <851064> is being terminated
Job <851066> is being terminated
Job <851068> is being terminated
Job <851070> is being terminated
Job <851074> is being terminated
Job <851072> is being terminated
Job <851076> is being terminated
Job <851078> is being terminated
Job <851080> is being terminated
Job <851082> is being terminated
Job <851084> is being terminated
Job <851086> is being terminated
Job <851089> is being terminated
Job <851090> is being terminated
Job <850975>: Job has already finished
Job <850971>: Job has already finished
Job <851092> is being terminated
Job <850977>: Job has already finished
Job <850973>: Job has already finished
Job <851094> is being terminated
Job <850979>: Job has already finished
Job <851095> is being terminated
Job <850981>: Job has already finished
Job <850983>: Job has already finished
Job <850986>: Job has already finished
Job <850988>: Job has already finished
Job <850990>: Job has already finished
Job <850992>: Job has already finished
Job <850994>: Job has already finished
(py38-venv) [jonak@st-linrgs342 ert]$ Job <850996>: Job has already finished
Job <850998>: Job has already finished
Job <851000>: Job has already finished
Job <851002>: Job has already finished
Job <851004>: Job has already finished
Job <851006>: Job has already finished
Job <851008>: Job has already finished
Job <851010>: Job has already finished
Job <851012>: Job has already finished
Job <851014>: Job has already finished
Job <851016>: Job has already finished
Job <851018>: Job has already finished
Job <851020>: Job has already finished
Job <851022>: Job has already finished
Job <851024>: Job has already finished
Job <851026>: Job has already finished
Job <851028>: Job has already finished
Job <851030>: Job has already finished
Job <851032>: Job has already finished
Job <851034>: Job has already finished
Job <851036>: Job has already finished
Job <851038>: Job has already finished
Job <851040>: Job has already finished
Job <851042>: Job has already finished
Job <851044>: Job has already finished
Job <851046>: Job has already finished
Job <851048>: Job has already finished
Job <851050>: Job has already finished
Job <851052>: Job has already finished
Job <851054>: Job has already finished
Job <851056>: Job has already finished
Job <851058>: Job has already finished
Job <851060>: Job has already finished
Job <851062>: Job has already finished
Job <851064>: Job has already finished
Job <851066>: Job has already finished
Job <851068>: Job has already finished
Job <851070>: Job has already finished
Job <851072>: Job has already finished
Job <851074>: Job has already finished
Job <851076>: Job has already finished
Job <851078>: Job has already finished
Job <851080>: Job has already finished
Job <851082>: Job has already finished
Job <851084>: Job has already finished
Job <851086>: Job has already finished
Job <851089>: Job has already finished
Job <851090>: Job has already finished
Job <851092>: Job has already finished
Job <851094>: Job has already finished
Job <851095>: Job has already finished

EDIT:
*** FOUND IT! ***

/* The _addopen() call will first close the fd and then reopen it;
     if no file is specified for stdout/stderr redirect the child will
     send stdout & stderr to whereever the parent process was already
     sending it.
    */

jonathan-eq · 2024-03-22T12:03:54Z

ruff rule PLW1509

They actually had a really good description!

berland · 2024-03-25T16:14:24Z

src/clib/lib/job_queue/lsf_driver.cpp

                           job->lsf_jobnr_char);
-
-        spawn_blocking(driver->rsh_cmd, 2, (const char **)argv, NULL, NULL);
-


I don't get why this spawn_blocking is removed, what use is there of argv when you just free it?

You are right, this was removed by accident

src/clib/lib/job_queue/lsf_driver.cpp

berland · 2024-03-25T16:17:23Z

src/clib/lib/job_queue/lsf_driver.cpp

        free(argv[1]);
        free(argv);
+
+        char **argv2 = (char **)calloc(2, sizeof *argv2);
+        argv2[0] = driver->remote_lsf_server;


I have not checked what CHECK_ALLOC is doing, but should it be included here as well?

CHECK_ALLOC was something that was introduced because that's what util_malloc did. There is no reason for it to exist.

tests/unit_tests/scheduler/test_lsf_driver.py

This commits makes the LSF driver spawn a non-blocking separate process running `bkill -s SIGKILL` to make sure the job is actually torn down.

xjules

Looks good now! 🚀

jonathan-eq added the release-notes:misc Automatically categorise as miscellaneous change in release notes label Mar 20, 2024

jonathan-eq self-assigned this Mar 20, 2024

jonathan-eq force-pushed the further-improve-bkill branch 2 times, most recently from 182a004 to fae11b0 Compare March 20, 2024 09:38

jonathan-eq added the blocked label Mar 20, 2024

jonathan-eq force-pushed the further-improve-bkill branch from fae11b0 to 1ce8dda Compare March 21, 2024 08:24

jonathan-eq removed the blocked label Mar 21, 2024

jonathan-eq force-pushed the further-improve-bkill branch 16 times, most recently from 28b7b3b to d6140b4 Compare March 22, 2024 11:40

jonathan-eq force-pushed the further-improve-bkill branch from d6140b4 to a0ba7c8 Compare March 22, 2024 12:03

jonathan-eq force-pushed the further-improve-bkill branch 5 times, most recently from 77f6356 to 8e3f6b9 Compare March 25, 2024 09:01

berland reviewed Mar 25, 2024

View reviewed changes

src/clib/lib/job_queue/lsf_driver.cpp Show resolved Hide resolved

berland reviewed Mar 25, 2024

View reviewed changes

jonathan-eq force-pushed the further-improve-bkill branch from 8e3f6b9 to 5b063dd Compare April 2, 2024 07:01

xjules reviewed Apr 2, 2024

View reviewed changes

tests/unit_tests/scheduler/test_lsf_driver.py Outdated Show resolved Hide resolved

Have lsf driver bkill with SIGKILL after SIGTERM

7220a76

This commits makes the LSF driver spawn a non-blocking separate process running `bkill -s SIGKILL` to make sure the job is actually torn down.

jonathan-eq force-pushed the further-improve-bkill branch from 5b063dd to 7220a76 Compare April 2, 2024 11:37

xjules approved these changes Apr 2, 2024

View reviewed changes

jonathan-eq merged commit 3bc1f30 into equinor:main Apr 2, 2024
37 checks passed

eivindjahren mentioned this pull request Apr 25, 2024

Create release notes for version 10.0.0 #7770

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have BKILL signal SIGKILL after SIGTERM #7482

Have BKILL signal SIGKILL after SIGTERM #7482

jonathan-eq commented Mar 20, 2024 •

edited

Loading

jonathan-eq commented Mar 20, 2024

codecov-commenter commented Mar 20, 2024 •

edited

Loading

jonathan-eq commented Mar 20, 2024 •

edited

Loading

xjules commented Mar 22, 2024

berland commented Mar 22, 2024

jonathan-eq commented Mar 22, 2024 •

edited

Loading

jonathan-eq commented Mar 22, 2024

berland Mar 25, 2024

jonathan-eq Apr 2, 2024

berland Mar 25, 2024

pinkwah Mar 26, 2024

xjules left a comment

		job->lsf_jobnr_char);

		spawn_blocking(driver->rsh_cmd, 2, (const char **)argv, NULL, NULL);

Have BKILL signal SIGKILL after SIGTERM #7482

Have BKILL signal SIGKILL after SIGTERM #7482

Conversation

jonathan-eq commented Mar 20, 2024 • edited Loading

When applicable

jonathan-eq commented Mar 20, 2024

codecov-commenter commented Mar 20, 2024 • edited Loading

Codecov Report

jonathan-eq commented Mar 20, 2024 • edited Loading

xjules commented Mar 22, 2024

berland commented Mar 22, 2024

jonathan-eq commented Mar 22, 2024 • edited Loading

jonathan-eq commented Mar 22, 2024

berland Mar 25, 2024

Choose a reason for hiding this comment

jonathan-eq Apr 2, 2024

Choose a reason for hiding this comment

berland Mar 25, 2024

Choose a reason for hiding this comment

pinkwah Mar 26, 2024

Choose a reason for hiding this comment

xjules left a comment

Choose a reason for hiding this comment

jonathan-eq commented Mar 20, 2024 •

edited

Loading

codecov-commenter commented Mar 20, 2024 •

edited

Loading

jonathan-eq commented Mar 20, 2024 •

edited

Loading

jonathan-eq commented Mar 22, 2024 •

edited

Loading