Upgrading Tron to py3.8 + patching it with the fix #934
Conversation
We have a hunch that this is what is causing our pod event loop to have wildly delayed items
This reverts commit 483da5c.
…for handle_events
…nfrastage" This reverts commit 1f81a6d.
This reverts commit 951fcc8.
# Conflicts: # Makefile # cluster_itests/docker-compose.yml # debian/rules # requirements.txt # tox.ini # tron/core/actionrun.py
EmanElsaban
changed the title
EmanElsaban
force-pushed
the
temp-jammy-fork
branch
4 times, most recently
from
08f4f59 to
eda4bc0
Compare
EmanElsaban
requested review from
nemacysts,
Molaire,
KaspariK,
EvanKrall and
bobokun
EmanElsaban
force-pushed
the
temp-jammy-fork
branch
2 times, most recently
from
e9283b1 to
bee49c1
Compare
EmanElsaban
force-pushed
the
temp-jammy-fork
branch
2 times, most recently
from
77708fd to
028a87a
Compare
EmanElsaban
force-pushed
the
temp-jammy-fork
branch
from
028a87a to
0855f4c
Compare
| @@ -11,7 +11,7 @@ endif | |||
|
|
|||
| NOOP = true | |||
| ifeq ($(PAASTA_ENV),YELP) | |||
| export PIP_INDEX_URL ?= https://pypi.yelpcorp.com/simple | |||
| export PIP_INDEX_URL ?= http://169.254.255.254:20641/$*/simple/ | |||
There was a problem hiding this comment.
oh nice, this works! I was thinking that we'd need to do the $* bit inside the targets themselves :D
There was a problem hiding this comment.
yupp, same I thought I would need to add it at first to the targets themselves :p
tron/kubernetes.py
Outdated
| if self.deferred is None: | ||
| log.warning("Unable to get a handler for next event in queue - this should never happen!") | ||
| # TODO: figure out how to recover if we were unable to get a handler | ||
| # Not adding a callback is very bad here as this means we will never handle this event |
There was a problem hiding this comment.
super-nit if we want to be extra verbose for future-us:
| # Not adding a callback is very bad here as this means we will never handle this event | |
| # Not adding a callback is very bad here as this means we will never handle future events | |
| # This hasn't happened to date, and if it does hopefully the errback added in a previous | |
| # handle_next_event() will get us back on track for now |
tron/core/actionrun.py
Outdated
| @@ -1046,7 +1176,7 @@ def submit_command(self, attempt: ActionRunAttempt) -> Optional[KubernetesTask]: | |||
| service_account_name=attempt.command_config.service_account_name, | |||
| ports=attempt.command_config.ports, | |||
| ) | |||
| except Exception: | |||
| except InvariantException: | |||
There was a problem hiding this comment.
technically, we could have other exceptions thrown here (e.g., build_environment() or filehandler.OutputStreamSerializer() could throw - but those do seem somewhat unlikely
There was a problem hiding this comment.
hmm should we change this to except Exception to catch all other exceptions based on the hierarchy shown here: https://martinxpn.medium.com/exception-hierarchy-python-58-100-days-of-python-9d8585e6569b#:~:text=The%20Exception%20Hierarchy,class%20in%20the%20exception%20hierarchy.
There was a problem hiding this comment.
yea, unless we're extra-sure that none of the other things we do here will throw, it's probably a good idea to change this back to except Exception
There was a problem hiding this comment.
or
except (InvariantException, Exception):
There was a problem hiding this comment.
I'm honestly not sure if anything else throws or not since I didn't really touch this code, I will change it in a separate follow up PR and not here
There was a problem hiding this comment.
LOL, I'm so confused as of why it shows as added in this PR. This change was added three years ago: https://github.com/Yelp/Tron/blame/b1118433b10c06a6f15b668d2f5c184115d701e5/tron/core/actionrun.py
There was a problem hiding this comment.
according to this PR it should be Exception: https://github.com/Yelp/Tron/pull/926/files , not sure what brought it back in this PR. Will make it except Exception as this is how it is in Master branch
This PR includes python upgrades for 3.8, the patch to stop tron jobs from getting stuck, fixes to some mypy issues/ tests failing and files edited by the precommit hook + additional PR in task_processing for adding logging and handling of ADDED events (PR : Yelp/task_processing#212)