Runner detect ACPI shutdown

[DavidGOrtega]

In GCP and AWS spot instances are terminated via ACPI shutdown. This PR also fixes the resource termination stopping the runner from the cloud console

• needs a new PR that adds package acpi in TPI
• possibly closes GCP runners not unregistering when destroy is performed via terraform locally terraform-provider-iterative#178
• closes Error code 143 when the runner breaks abruptly  #1071
• closes runner restartWorkflow is not working #1072
• closes Discuss about the inclusion of SpotNotifier #663
• refactors pipelineRerun and pipelineRestart removing the later
• depends on Runner edge cases improvements #1030
• depends on logs: show terraform errors on runner failure #1039

[dacbd]

#1083

[dacbd]

Good to go to master

[0x2b3bfa0]

The systemd service wrapping cml runner suffices to send a TERM signal to the process when the instance transitions to the ACPI G2 state. I believe that this pull request solves an XY problem, unless proven otherwise.

I'm not opposed to merging this pull request if it solves a critical issue, but I would suggest, with a lower priority:

1. Reading Discuss about the inclusion of SpotNotifier #663 (comment) for more information on alternative approaches, like polling for spot instance termination signals through the internal metadata service.
2. Making sure that cml runner is able to handle gracefully a TERM signal and doesn't exit until all the cleanup routines run.

To my mind, cml runner should not include any logic to deal with termination notices other than a plain SIGTERM. All the fancy platform-specific features like shutdown and spot instance eviction detection should be part of the startup script on our Terraform provider, which should translate those signals into the aforementioned SIGTERM.

[DavidGOrtega]

The systemd service wrapping cml runner suffices to send a TERM signal to the process when the instance transitions to the ACPI G2 state. I believe that this pull request solves an XY problem, unless proven otherwise.

I'm not opposed to merging this pull request if it solves a critical issue, but I would suggest, with a lower priority:

1. Reading #663 (comment) for more information on alternative approaches, like polling for spot instance termination signals through the internal metadata service.
2. Making sure that cml runner is able to handle gracefully a TERM signal and doesn't exit until all the cleanup routines run.

This PR not only fixes the spot termination, indeed fixes the error of job restart setting the ids, and the recent bug of restarting already finished jobs
And of course you can test that this PR behaves as expected trying to stop a normal runner in AWS and GPC and see that in many cases it does not unregister while listening to the acpid always do it right

[DavidGOrtega]

should not include any logic to deal with termination notices other than a plain SIGTERM

I need to check why TPI suffices with SIGTERM. The runner with SIGTERM (behaving perfectly) is not always triggered?!
Quick test. Start a runner in GH/GCP and stop it. It wont unregister

[0x2b3bfa0]

Thanks! I'll try to reproduce. 🙏🏼

[0x2b3bfa0]

Unable to reproduce on AWS nor GCP; tried 3 batches of 30 runners on the former, and 10 runners on the latter.

[dacbd]

I can say that I was able to see the bad detection behavior once pre-pr on GCP, but I saw also the correct behavior several times, and also struggled to reproduce it. I see think that: #1016 or operating the runner process as a separate service that cml.service monitors, is a better solution.

[0x2b3bfa0]

I was able to see the bad detection behavior

Are you sure that it was caused because the cml process didn't get a SIGTERM or could it be an issue in Node.js or our code handling it?

[DavidGOrtega]

I can say that I was able to see the bad detection behavior once pre-pr on GCP, but I saw also the correct behavior several times, and also struggled to reproduce it.

This coincides with my experience and I have been always saying this... SIGTERM systemd is not consistent in my experience (and others). No idea why because it should

[dacbd]

I was able to see the bad detection behavior

Are you sure that it was caused because the cml process didn't get a SIGTERM or could it be an issue in Node.js or our code handling it?

both were from an organic spot termination and happened when I was not monitoring the instance and thus have no idea...