TRON-2208: Add toggle in tron config to disable retries on LOST k8s jobs #988

cuza · 2024-07-05T22:16:55Z

Summary

This PR addresses TRON-2208 by adding a toggle in the Tron configuration to disable automatic retries for Kubernetes jobs that are marked as "LOST". This is particularly useful for non-idempotent jobs where retries can be dangerous.

Changes

Configuration

tron/config/config_parse.py: Added non_retryable_exit_codes to the Kubernetes configuration schema.
tron/config/schema.py: Updated the schema to include non_retryable_exit_codes.

Core Logic

tron/core/actionrun.py:
- Modified the _exit_unsuccessful method to check for non_retryable_exit_codes and skip retries if the exit code is in this list.
- Refactored non_retryable_exit_codes to use a list instead of a tuple.
tron/kubernetes.py:
- Updated the handle_event method to set the exit code to EXIT_KUBERNETES_TASK_LOST for lost tasks.
- Added non_retryable_exit_codes to the Kubernetes configuration.
tron/utils/exitcode.py: Added a new exit code EXIT_KUBERNETES_TASK_LOST with a value of -12.

Tests

tests/config/config_parse_test.py: Updated tests to include non_retryable_exit_codes.
tests/kubernetes_test.py: Updated tests to check for the new exit code and ensure the correct behavior when a task is marked as lost.

Miscellaneous

.gitignore: Added .fleet to the ignore list.
requirements-dev-minimal.txt: Added debugpy.
requirements-dev.txt: Added debugpy.

Motivation

Given that "LOST" means Tron has lost track of a pod it already thought it had started for a job, attempting to retry/start a replacement can be dangerous for non-idempotent jobs. In the current state, these will consume retries, but with some of our EKS migration methods, LOST tasks are more likely. Therefore, we should have a way to temporarily pause retries on these.

Testing

Unit tests have been updated and added to cover the new configuration option and its effects on job retry behavior.

Related Issues

TRON-2208: Add toggle in Tron config to disable retries on LOST k8s jobs

Notes

Ensure to update your Tron configuration to include the new non_retryable_exit_codes option if you wish to use this feature.
This change is backward compatible; if the new configuration option is not set, the default behavior remains unchanged.

The _exit_unsuccessful method in the KubernetesActionRun class has been refactored to include the is_lost_task parameter. This parameter is used to determine whether auto-retries should be skipped when the disable_retries_on_lost option is enabled in the Kubernetes configuration.

tron/kubernetes.py

…ies_on_lost * origin/master: Released 2.2.6 via make release Update scribereader tests (#983) Released 2.2.5 via make release Update yelp_clog and use datetime range for S3 logs (#979) Add clarification around reverting changes that touch DynamoDB data (#982)

…ies_on_lost * origin/master: Adjust path after conf.py was moved

tron/core/actionrun.py

nemacysts · 2024-07-10T21:01:25Z

sidenote: can we add some additional context to the PR description? it can still reference the ticket, but having more context in git blame (without having to switch to a browser and go to jira) is always nice :)

cuza · 2024-07-10T21:03:25Z

sidenote: can we add some additional context to the PR description? it can still reference the ticket, but having more context in git blame (without having to switch to a browser and go to jira) is always nice :)

yeah of course, this is just a draft PR to make sure that we agreed on the implementation 😄

The Kubernetes configuration has been updated to use the `non_retryable_exit_codes` option instead of `disable_retries_on_lost`. This change allows for more flexibility in specifying the exit codes that should not trigger auto-retries for Kubernetes tasks.

…ernetesClusterRepository The `non_retryable_exit_codes` attribute in the `KubernetesCluster` and `KubernetesClusterRepository` classes has been refactored to use a tuple of integers instead of a single integer. This change allows for more flexibility in specifying multiple exit codes that should not trigger auto-retries for Kubernetes tasks.

…ies_on_lost * origin/master: Minor fix to help text for kubecontext param Support running on devbox too Support multiple clusters in sync_tron_from_k8s Released 2.3.0 via make release More type fixes for defaults Initialize KubernetesClusterRepository watcher_kubeconfig_paths the same as KubernetsCluster and correct typing Released 2.2.7 via make release Only show disable warning on tronctl disable (#990) Add validation to watcher_kubeconfig_paths too, and a minimal test Update tools/sync_tron_state_from_k8s.py Bump task_processing to get watcher_kubeconfig_paths change Support old_kubeconfig_paths Update sync_tron_from_k8s to match sanitized instance labels, cleanup comments Update sync to handle in-progress retries and don't tronctl yet; add unit tests TRON-2210: Add a tool that can sync from pod state to tron w/ tronctl # Conflicts: # tron/kubernetes.py

The `debugpy` dependency in the `requirements-dev-minimal.txt` and `requirements-dev.txt` files has been updated to version 1.8.1. This update ensures compatibility and brings in the latest features and bug fixes for debugging purposes.

The .gitignore file has been updated to include the .fleet directory. This ensures that the .fleet directory is ignored by Git and not tracked in the repository.

jfongatyelp

Looks like a nice option, would be nice to see a test where k8s_options.non_retryable_exit_codes was non-empty and checking our code does the right thing tho

jfongatyelp · 2024-08-06T19:25:39Z

tests/kubernetes_test.py

@@ -447,7 +447,7 @@ def test_handle_event_lost(mock_kubernetes_task):
        )
    )

-    assert mock_kubernetes_task.is_unknown
+    assert mock_kubernetes_task.exit_status == exitcode.EXIT_KUBERNETES_TASK_LOST


You might want to keep the old assert as well, since we do still want to verify 'lost' k8s events lead to marking the task as unknown.

sadly the way we're checking unknow is by checking exit code

def is_unknown(self): return self.exit_status is None

Ohh interesting; i didn't realize assigning the LOST's an exit code changed this behavior, thanks.

Did you happen to glance through all the other places where we use is_unknown/unknown to check this change won't change any expected behavior there?

It should not be a problem since we're only marking lost jobs with the special lost exit code

is_broken() uses the is_unknown property and it looks like there's some jobrun code that uses is_broken to gate starting new action runs - but i'm not sure if this change impacts that in a meaningful way yet

tron/core/actionrun.py

tron/kubernetes.py

Co-authored-by: Jen Patague <[email protected]>

nemacysts

i don't think there's any major blockers - it's probably fine to run this in non-prod for a bit

i'm not 100% sure about my 2 non-requirements.txt comments, but we should be able to sus any issues out by actually running this since the case we're handling here is pretty rare anyway :)

nemacysts · 2024-08-26T16:59:31Z

requirements-dev-minimal.txt

@@ -1,4 +1,5 @@
 asynctest
+debugpy


just curious: what is this for?

VSCode Python Debugger :)

https://github.com/microsoft/debugpy

tron/core/actionrun.py

nemacysts · 2024-08-26T18:54:37Z

tests/kubernetes_test.py

@@ -447,7 +447,7 @@ def test_handle_event_lost(mock_kubernetes_task):
        )
    )

-    assert mock_kubernetes_task.is_unknown
+    assert mock_kubernetes_task.exit_status == exitcode.EXIT_KUBERNETES_TASK_LOST


is_broken() uses the is_unknown property and it looks like there's some jobrun code that uses is_broken to gate starting new action runs - but i'm not sure if this change impacts that in a meaningful way yet

chore: Add disable_retries_on_lost option to Kubernetes configuration

386b902

cuza changed the title ~~chore: Add disable_retries_on_lost option to Kubernetes configuration~~ Add toggle in tron config to disable retries on LOST k8s jobs Jul 5, 2024

cuza changed the title ~~Add toggle in tron config to disable retries on LOST k8s jobs~~ TRON-2208: Add toggle in tron config to disable retries on LOST k8s jobs Jul 5, 2024

cuza commented Jul 8, 2024

View reviewed changes

tron/kubernetes.py Outdated Show resolved Hide resolved

cuza requested a review from jfongatyelp July 8, 2024 12:43

cuza added 3 commits July 8, 2024 06:10

chore: Refactor _exit_unsuccessful method in KubernetesActionRun

e3ecf1b

Merge remote-tracking branch 'origin/master' into u/cuza/disable_retr…

34b98f4

…ies_on_lost * origin/master: Adjust path after conf.py was moved

jfongatyelp reviewed Jul 10, 2024

View reviewed changes

tron/core/actionrun.py Outdated Show resolved Hide resolved

cuza added 3 commits July 31, 2024 15:12

chore: Refactor _exit_unsuccessful method in KubernetesActionRun

1ed7dbe

cuza marked this pull request as ready for review August 2, 2024 16:53

cuza added 3 commits August 2, 2024 09:56

chore: Update debugpy dependency to version 1.8.1

d58cb93

The `debugpy` dependency in the `requirements-dev-minimal.txt` and `requirements-dev.txt` files has been updated to version 1.8.1. This update ensures compatibility and brings in the latest features and bug fixes for debugging purposes.

chore: Update .gitignore to include .fleet

0cda7e5

The .gitignore file has been updated to include the .fleet directory. This ensures that the .fleet directory is ignored by Git and not tracked in the repository.

cuza requested review from nemacysts and jfongatyelp August 6, 2024 17:04

jfongatyelp reviewed Aug 6, 2024

View reviewed changes

cuza and others added 3 commits August 15, 2024 18:10

Update tron/core/actionrun.py

b8e91c3

Co-authored-by: Jen Patague <[email protected]>

chore: Refactor non_retryable_exit_codes to use list instead of tuple

fa00a34

Fixing tests

f16b828

cuza requested a review from jfongatyelp August 19, 2024 16:39

nemacysts approved these changes Aug 26, 2024

View reviewed changes

Adding self.retries_remaining = 0 when we skip a non retryable job

b9883a3

cuza requested a review from nemacysts August 27, 2024 01:02

nemacysts approved these changes Aug 28, 2024

View reviewed changes

nemacysts merged commit 2d17df3 into master Aug 28, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TRON-2208: Add toggle in tron config to disable retries on LOST k8s jobs #988

TRON-2208: Add toggle in tron config to disable retries on LOST k8s jobs #988

cuza commented Jul 5, 2024 •

edited

Loading

nemacysts commented Jul 10, 2024

cuza commented Jul 10, 2024

jfongatyelp left a comment

jfongatyelp Aug 6, 2024

cuza Aug 15, 2024

jfongatyelp Aug 15, 2024

cuza Aug 26, 2024

nemacysts Aug 26, 2024

nemacysts left a comment

nemacysts Aug 26, 2024

cuza Aug 26, 2024 •

edited

Loading

nemacysts Aug 26, 2024

TRON-2208: Add toggle in tron config to disable retries on LOST k8s jobs #988

TRON-2208: Add toggle in tron config to disable retries on LOST k8s jobs #988

Conversation

cuza commented Jul 5, 2024 • edited Loading

Summary

Changes

Configuration

Core Logic

Tests

Miscellaneous

Motivation

Testing

Related Issues

Notes

nemacysts commented Jul 10, 2024

cuza commented Jul 10, 2024

jfongatyelp left a comment

Choose a reason for hiding this comment

jfongatyelp Aug 6, 2024

Choose a reason for hiding this comment

cuza Aug 15, 2024

Choose a reason for hiding this comment

jfongatyelp Aug 15, 2024

Choose a reason for hiding this comment

cuza Aug 26, 2024

Choose a reason for hiding this comment

nemacysts Aug 26, 2024

Choose a reason for hiding this comment

nemacysts left a comment

Choose a reason for hiding this comment

nemacysts Aug 26, 2024

Choose a reason for hiding this comment

cuza Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

nemacysts Aug 26, 2024

Choose a reason for hiding this comment

cuza commented Jul 5, 2024 •

edited

Loading

cuza Aug 26, 2024 •

edited

Loading