wait for jobs to complete when stopping the runner #202

Merged

earl-warren merged 1 commit from earl-warren/runner:wip-signal into main 2024-06-07 10:43:02 +00:00

Conversation 4 Commits 1 Files changed 7 +373 -32

earl-warren commented 2024-06-06 13:48:13 +00:00

Contributor

• Copy link
• Copy Markdown

When receiving a signal (INT or TERM) wait for running jobs to complete instead of terminating them right away.

The new shutdown_timeout configuration setting can be used to force the termination after a grace delay. If not specified or zero it will shutdown immediately, for backward compatibility. It will be the case with existing configuration files or when a configuration file is not specified.

The config.yml created with the generate-config subcommand will however default shutdown_timeout to 3h (same as timeout) because it is likely what a new admin would expect: shutting down waits for jobs to complete and not abort them.

---

Note to reviewers:

• Add tests (100% coverage)
• See also https://gitea.com/gitea/act_runner/pulls/546 for the implementation that inspired this PR
• setup-forgejo actions/setup-forgejo#128
• end-to-end forgejo/end-to-end#197

When receiving a signal (INT or TERM) wait for running jobs to complete instead of terminating them right away. The new shutdown_timeout configuration setting can be used to force the termination after a grace delay. If not specified or zero it will shutdown immediately, for backward compatibility. It will be the case with existing configuration files or when a configuration file is not specified. The config.yml created with the generate-config subcommand will however default shutdown_timeout to 3h (same as timeout) because it is likely what a new admin would expect: shutting down waits for jobs to complete and not abort them. --- Note to reviewers: * Add tests (100% coverage) * See also https://gitea.com/gitea/act_runner/pulls/546 for the implementation that inspired this PR * setup-forgejo https://code.forgejo.org/actions/setup-forgejo/pulls/128 * end-to-end https://code.forgejo.org/forgejo/end-to-end/pulls/197

earl-warren added the

Kind/Feature

label 2024-06-06 13:48:13 +00:00

earl-warren force-pushed wip-signal from 1a8e1028ba to 44845583fa 2024-06-06 15:02:16 +00:00 Compare

earl-warren referenced this pull request 2024-06-06 16:36:12 +00:00

WIP: feat: allow graceful shutdowns (#546) #201

earl-warren force-pushed wip-signal from 5ba4238ad3 to 708ec54339 2024-06-06 23:32:17 +00:00 Compare

earl-warren force-pushed wip-signal from 708ec54339 to 2367d25adf 2024-06-06 23:48:20 +00:00 Compare

earl-warren changed title from WIP: graceful shutdown to wait for jobs to complete when stopping the runner 2024-06-06 23:49:33 +00:00

earl-warren requested review from twenty-panda 2024-06-06 23:51:26 +00:00

earl-warren force-pushed wip-signal from 2367d25adf to af763c6728 2024-06-07 00:00:03 +00:00 Compare

earl-warren commented 2024-06-07 00:03:00 +00:00

Author

Contributor

• Copy link
• Copy Markdown

The default value for the shutdown timeout is the only thing that actually make sense (same as the timeout of jobs). However it makes it a breaking change for whatever scripts expect an immediate stop. I think it is acceptable but I'm not 100% sure.

The default value for the shutdown timeout is the only thing that actually make sense (same as the timeout of jobs). However it makes it a breaking change for whatever scripts expect an immediate stop. I think it is acceptable but I'm not 100% sure.

earl-warren force-pushed wip-signal from af763c6728 to c15caa51e7 2024-06-07 00:04:40 +00:00 Compare

earl-warren force-pushed wip-signal from c15caa51e7 to 67a1d83ad7 2024-06-07 07:10:42 +00:00 Compare

earl-warren force-pushed wip-signal from 67a1d83ad7 to 7b6fe77166 2024-06-07 07:13:29 +00:00 Compare

earl-warren commented 2024-06-07 07:17:35 +00:00

Author

Contributor

• Copy link
• Copy Markdown

The implementation was changed to default to terminating immediately if shutdown_timeout is zero or unset. Defaulting to the timeout is a bad idea. Not only is it a breaking change https://code.forgejo.org/forgejo/end-to-end tests that rely on the fact that the runner can be stopped immediately (but that can easily be changed by setting the timeout to 1).

It is also problematic because there is no way to force the runner to stop immediately. Once the runner received the signal, it will stop listening to other signals and the admin will have no choice but to wait it out. Or kill -9 which would force them to manually cleanup leftovers.

The implementation was changed to default to terminating immediately if shutdown_timeout is zero or unset. Defaulting to the timeout is a bad idea. Not only is it a breaking change https://code.forgejo.org/forgejo/end-to-end tests that rely on the fact that the runner can be stopped immediately (but that can easily be changed by setting the timeout to 1). It is also problematic because there is no way to force the runner to stop immediately. Once the runner received the signal, it will stop listening to other signals and the admin will have no choice but to wait it out. Or kill -9 which would force them to manually cleanup leftovers.

cascading-pr commented 2024-06-07 08:17:02 +00:00

Contributor

• Copy link
• Copy Markdown

cascading-pr updated at actions/setup-forgejo#128

cascading-pr updated at https://code.forgejo.org/actions/setup-forgejo/pulls/128

twenty-panda approved these changes 2024-06-07 09:40:30 +00:00

earl-warren force-pushed wip-signal from 7b6fe77166 to e02e0fc5f5 2024-06-07 10:33:42 +00:00 Compare

earl-warren merged commit aa0f1facf4 into main 2024-06-07 10:43:02 +00:00

earl-warren deleted branch wip-signal 2024-06-07 10:43:03 +00:00

earl-warren referenced this pull request from a commit 2024-06-07 10:43:03 +00:00

Merge pull request 'wait for jobs to complete when stopping the runner' (#202) from earl-warren/runner:wip-signal into main

earl-warren referenced this pull request from a commit 2025-07-28 17:10:30 +00:00

chore(tests): add coverage for ./pkg/runner (#202)

earl-warren referenced this pull request 2025-09-06 16:54:17 +00:00

bug: TERM on the daemon does not always wait for the jobs to complete #944

Sign in to join this conversation.

Reviewers

No reviewers

Ghost

twenty-panda

Labels

Clear labels   

FreeBSD

  

Kind/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Chore

It is no fun but it needs to be done, this is a chore.

  

Kind/DependencyUpdate

for pull requests that are created by renovate when updating a dependency

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

  

Windows

Windows ports is an independent project, see https://github.com/Crown0815/Forgejo-runner-windows-builder

  

linux-powerpc64le

powerpc64le

  

linux-riscv64

  

linux-s390x

  

run-end-to-end-tests

Run https://code.forgejo.org/forgejo/end-to-end with a runner compiled from this pull request

  

run-forgejo-tests

Run https://codeberg.org/forgejo/forgejo tests importing the runner from this pull request

  

run-multi-platform-tests

Label a PR for automatic execution of test suite on arm64 architecture.

No labels

FreeBSD

Kind/Breaking

Kind/Bug

Kind/Chore

Kind/DependencyUpdate

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

Windows

linux-powerpc64le

linux-riscv64

linux-s390x

run-end-to-end-tests

run-forgejo-tests

run-multi-platform-tests

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

forgejo/runner!202

No description provided.