Add test for comp_req__launch_man__fast_shutdown_support - #573
Add test for comp_req__launch_man__fast_shutdown_support#573TimoSteuerwaldETAS wants to merge 5 commits into
Conversation
GitHub Actions
Waiting
GitHub Actions
Waiting
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
be3609c to
8342435
Compare
GitHub Actions
Waiting
GitHub Actions
Waiting
| assert proc.is_running(), "Launch manager exited before it could be killed" | ||
| code, out = target.execute(f"kill -9 {proc.pid()}") | ||
| assert code == 0, f"Failed to SIGKILL launch manager (pid {proc.pid()}): {out!r}" | ||
| proc.wait(timeout_s=3.0) |
There was a problem hiding this comment.
Is this really the intended implementation of "fast shutdown" by killing the launch manager?
There was a problem hiding this comment.
At least this is how I have understood it. This is what I have noted:
Some kind of brute force shutdown. (May be) related to qnx feature which is watching the running processes, if launch manager is gone, system gets restarted anyways. This is most probably faster then stop triggering the watchdog and waiting for the timeout which initiates the system restart.
GitHub Actions
Waiting
GitHub Actions
Waiting
There was a problem hiding this comment.
In #564 we removed this actually.
But I also think if you're going to add this to the new method, I think instead we should have a parameter called expected_return_code: int = 0 instead so that it is more generic.
|
|
||
| # The child processes must survive the death of their parent. | ||
| assert _pid_alive(target, daemon_pid), ( | ||
| f"Control daemon (pid {daemon_pid}) died with the launch manager" |
There was a problem hiding this comment.
Should we test that they survive? Not sure it's relevant for the requirement.
| """ | ||
| Objective: Verifies that processes started by the Launch Manager keep running | ||
| when the Launch Manager itself is killed with SIGKILL, i.e. without any chance | ||
| to tear its children down. |
There was a problem hiding this comment.
I'm not sure if the fast shutdown should be a sigkill, in that case I'm not sure if there is anything to test really as the LCM doesn't really have anything to say about getting killed.
In that case I also don't know if we should even have a requirement for this, as there is no code we write that changes the behavior.
But if this improves the component req coverage then I guess we might as well test it?
I think we need to clarify why the req is required.
There was a problem hiding this comment.
I see your point, we can do so again today.
3 participants
This test verifies the following requirement:
Part of #317 .