Fixing Tron logs for jobs using other services images #1001
Conversation
This commit refactors the scribereader_test.py and scribereader.py files. It introduces a new function, get_log_namespace, which retrieves the log namespace from a yelpsoa-configs configuration file. The function is used in the PaaSTALogs class in scribereader.py to set the namespace for logging. The commit also includes unit tests for the get_log_namespace function. Closes TRON-2275
cuza
changed the title
| if service: | ||
| return service, job_name, run_num, action | ||
| except FileNotFoundError: | ||
| log.warning(f"yelp-soaconfig file tron-{paasta_cluster}.{ext} not found for action_run_id {action_run_id}.") |
There was a problem hiding this comment.
hhhhmmm failing to find yelpsoa should probably be more severe than warning
There was a problem hiding this comment.
whoops - sorry, i thought i had already published this review
i do have a small worry that adding a file load + yaml parsing to the request path for the actionrun log page - but i'm probably being a little too paranoid/cautious and this should be fine :)
that said: mostly have some fix-n-ships, otherwise lgtm - i think i would have preferred threading through the service param - but that would have likely been a significantly more involved change
| @@ -81,12 +82,35 @@ def get_scribereader_host_and_port(ecosystem: str, superregion: str, region: str | |||
| return host, port | |||
|
|
|||
|
|
|||
| def decompose_action_id(action_run_id: str, paasta_cluster: str) -> Tuple[str, str, str, str]: | |||
| namespace, job_name, run_num, action = action_run_id.split(".") | |||
| for ext in ["yaml", "yml"]: | |||
There was a problem hiding this comment.
imo, we can hardcode looking at just .yaml - the only .yml file in soaconfigs is a symlink
There was a problem hiding this comment.
I wasn't sure on this one and wanted to be extra safe. I'm checking yaml before trying yml. I only added it as a fallback cuz iirc I don't think we have nothing in place preventing folks to commit a my_cluster-tron.yml file
| with open(f"/nail/etc/services/{namespace}/tron-{paasta_cluster}.{ext}") as f: | ||
| config = yaml.load(f, Loader=yaml.CSafeLoader) |
There was a problem hiding this comment.
i am a tad worried about running this every time someone wants to load logs for something, but i'm not sure if we can meaningfully cache things here since soaconfigs can change at any time
that is to say: we'll probably wanna keep an eye on things and make sure this doesn't slow things down too much (and if it does, we'll probably want to figure out how to thread the service config through tron so that we can skip this IO)
There was a problem hiding this comment.
I was very paranoid about the YAML loading as well, yaml.CSafeLoader can load the longest tron soaconfig file in about 40ms. That's probably fast enough
Co-authored-by: Luis Pérez <[email protected]>
This PR introduces a new function, get_log_namespace, which retrieves the log namespace from yelpsoa-configs. The function is used in the PaaSTALogs class in scribereader.py to set the namespace for logging.
The PR also includes unit tests for the get_log_namespace function.
Closes TRON-2275
........tested manually and seems to work fine