Skip to content

Adding idempotent label to Tron pods - TRON-2154 #4076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

EmanElsaban
Copy link
Contributor

@EmanElsaban EmanElsaban commented May 30, 2025

This PR adds a new field in soaconfigs for "idempotent", which will help us identify which Tron actions are idempotent vs which ones are not. It also creates a pod label with its value

Testing

Label set to true for test_load_foo1 job that has it set in soaconfigs

 (py38-linux) emanelsabban@dev208-uswest1adevc:~/pg/paasta$ kubectl-eks-infrastage -n tron describe pod/compute-infra-test-service.test--load--foo1.4837.foo.p42zob
INFO: You are using an Okta authenticated kubectl wrapper
Name:             compute-infra-test-service.test--load--foo1.4837.foo.p42zob
Namespace:        tron
Priority:         0
Service Account:  default
Node:             ip-10-40-16-118.us-west-1.compute.internal/10.40.16.118
Start Time:       Fri, 30 May 2025 14:09:17 -0700
Labels:           app.kubernetes.io/managed-by=tron
                  paasta.yelp.com/cluster=infrastage
                  paasta.yelp.com/instance=test_load_foo1.foo
                  paasta.yelp.com/pool=default
                  paasta.yelp.com/service=compute-infra-test-service
                  tron.yelp.com/idempotent-action=true
                  ...

Label set to false for other jobs that dont add idempotent key in soaconfigs

(py38-linux) emanelsabban@dev208-uswest1adevc:~/pg/paasta$ kubectl-eks-infrastage -n tron describe pod/paasta-contract-monitor.k8s.2468.sleep.2cujrz
INFO: You are using an Okta authenticated kubectl wrapper
Name:             paasta-contract-monitor.k8s.2468.sleep.2cujrz
Namespace:        tron
Priority:         0
Service Account:  paasta--arn-aws-iam-528741615426-role-paasta-contract-monitor-eks
Node:             ip-10-40-11-250.us-west-1.compute.internal/10.40.11.250
Start Time:       Fri, 30 May 2025 14:10:00 -0700
Labels:           app.kubernetes.io/managed-by=tron
                  paasta.yelp.com/cluster=infrastage
                  paasta.yelp.com/instance=k8s.sleep
                  paasta.yelp.com/pool=default
                  paasta.yelp.com/service=paasta-contract-monitor
                  tron.yelp.com/idempotent-action=false
                  ...

@EmanElsaban EmanElsaban requested a review from a team as a code owner May 30, 2025 19:44
Copy link
Member

@nemacysts nemacysts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, i'm not sure if we should document this flag in the paasta RTD or the tron one - https://paasta.readthedocs.io/en/latest/yelpsoa_configs.html#tron-clustername-yaml mentions that we only document paasta-specific stuff...but then we document some tron-only stuff anyway :p

that said: this generally looks good to me - although i'd prefer if more folks from the team would chime in on your proposal doc to make sure we're not just unilaterally picking what we want to do :p

"idempotent": {
"type": "boolean",
"default": false,
"$comment": "This will be used to determine whether the action can be retried without side effects."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to drop:

Suggested change
"$comment": "This will be used to determine whether the action can be retried without side effects."
"$comment": If true, this action can be safely retried multiple times without unwanted side-effects."

@@ -1046,6 +1051,9 @@ def format_tron_action_dict(action_config: TronActionConfig):
limit=63,
suffix=4,
),
"tron.yelp.com/idempotent-action": str(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd personally prefer

Suggested change
"tron.yelp.com/idempotent-action": str(
"tron.yelp.com/is-idempontent": str(

...but i also can't really justify why, so up to you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants