From 52f84b3e82a491597ac42c027a68b967a89e6e16 Mon Sep 17 00:00:00 2001 From: Dave Cuza Date: Thu, 17 Oct 2024 15:24:20 -0700 Subject: [PATCH 1/3] Refactor scribereader_test.py and scribereader.py 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 --- tests/utils/scribereader_test.py | 62 ++++++++++++++++++++++++++++++++ tron/utils/scribereader.py | 25 +++++++++++-- 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/tests/utils/scribereader_test.py b/tests/utils/scribereader_test.py index d2a01eb29..e4bf77bcc 100644 --- a/tests/utils/scribereader_test.py +++ b/tests/utils/scribereader_test.py @@ -2,8 +2,10 @@ from unittest import mock import pytest +import yaml import tron.utils.scribereader +from tron.utils.scribereader import get_log_namespace from tron.utils.scribereader import read_log_stream_for_action_run try: @@ -420,3 +422,63 @@ def test_read_log_stream_for_action_run_min_date_and_max_date_for_long_output(): # The expected output should be max_lines plus the # extra line for 'This output is truncated.' message assert len(output) == max_lines + 1 + + +def test_get_log_namespace_yml_file_found(): + action_run_id = "namespace.job.1234.action" + paasta_cluster = "fake_cluster" + config_content = """ + job: + actions: + action: + service: test_service + """ + with mock.patch("builtins.open", mock.mock_open(read_data=config_content)), mock.patch( + "yaml.safe_load", return_value=yaml.safe_load(config_content) + ): + result = get_log_namespace(action_run_id, paasta_cluster) + assert result == "test_service" + + +def test_get_log_namespace_file_not_found(): + action_run_id = "namespace.job.1234.action" + paasta_cluster = "fake_cluster" + with mock.patch("builtins.open", side_effect=FileNotFoundError): + result = get_log_namespace(action_run_id, paasta_cluster) + assert result == "namespace" + + +def test_get_log_namespace_yaml_error(): + action_run_id = "namespace.job.1234.action" + paasta_cluster = "fake_cluster" + with mock.patch("builtins.open", mock.mock_open(read_data="invalid_yaml")), mock.patch( + "yaml.safe_load", side_effect=yaml.YAMLError + ): + result = get_log_namespace(action_run_id, paasta_cluster) + assert result == "namespace" + + +def test_get_log_namespace_generic_error(): + action_run_id = "namespace.job.1234.action" + paasta_cluster = "fake_cluster" + with mock.patch("builtins.open", mock.mock_open(read_data="some_data")), mock.patch( + "yaml.safe_load", side_effect=Exception + ): + result = get_log_namespace(action_run_id, paasta_cluster) + assert result == "namespace" + + +def test_get_log_namespace_service_not_found(): + action_run_id = "namespace.job.1234.action" + paasta_cluster = "fake_cluster" + config_content = """ + job: + actions: + action: + command: "sleep 10" + """ + with mock.patch("builtins.open", mock.mock_open(read_data=config_content)), mock.patch( + "yaml.safe_load", return_value=yaml.safe_load(config_content) + ): + result = get_log_namespace(action_run_id, paasta_cluster) + assert result == "namespace" diff --git a/tron/utils/scribereader.py b/tron/utils/scribereader.py index c736ac3b8..d0d3ed5fa 100644 --- a/tron/utils/scribereader.py +++ b/tron/utils/scribereader.py @@ -9,7 +9,8 @@ from typing import Optional from typing import Tuple -import staticconf # type: ignore +import staticconf +import yaml # type: ignore from tron.config.static_config import get_config_watcher from tron.config.static_config import NAMESPACE @@ -81,12 +82,32 @@ def get_scribereader_host_and_port(ecosystem: str, superregion: str, region: str return host, port +def get_log_namespace(action_run_id: str, paasta_cluster: str) -> str: + namespace, job_name, _, action = action_run_id.split(".") + for ext in ["yaml", "yml"]: + try: + with open(f"/nail/etc/services/{namespace}/tron-{paasta_cluster}.{ext}") as f: + config = yaml.safe_load(f) + service = config.get(job_name, {}).get("actions", {}).get(action, {}).get("service", None) + if service: + return service + except FileNotFoundError: + log.warning(f"yelp-soaconfig file tron-{paasta_cluster}.{ext} not found.") + except yaml.YAMLError as e: + log.error(f"Error parsing YAML file tron-{paasta_cluster}.{ext}: {e}") + except Exception as e: + log.error(f"Error reading service for {action} from file tron-{paasta_cluster}.{ext}: {e}") + + return namespace + + class PaaSTALogs: def __init__(self, component: str, paasta_cluster: str, action_run_id: str) -> None: self.component = component self.paasta_cluster = paasta_cluster self.action_run_id = action_run_id - namespace, job_name, run_num, action = action_run_id.split(".") + _, job_name, run_num, action = action_run_id.split(".") + namespace = get_log_namespace(action_run_id, paasta_cluster) # in our logging infra, things are logged to per-instance streams - but # since Tron PaaSTA instances are of the form `job_name.action`, we need # to escape the period since some parts of our infra will reject streams From 0f015b91dd4d89ba00d6038ff5bd1e2cef914339 Mon Sep 17 00:00:00 2001 From: Dave Cuza Date: Thu, 17 Oct 2024 15:41:44 -0700 Subject: [PATCH 2/3] update type annotations in scribereader.py --- tron/utils/scribereader.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tron/utils/scribereader.py b/tron/utils/scribereader.py index d0d3ed5fa..1036f6714 100644 --- a/tron/utils/scribereader.py +++ b/tron/utils/scribereader.py @@ -10,7 +10,7 @@ from typing import Tuple import staticconf -import yaml # type: ignore +import yaml from tron.config.static_config import get_config_watcher from tron.config.static_config import NAMESPACE @@ -88,7 +88,9 @@ def get_log_namespace(action_run_id: str, paasta_cluster: str) -> str: try: with open(f"/nail/etc/services/{namespace}/tron-{paasta_cluster}.{ext}") as f: config = yaml.safe_load(f) - service = config.get(job_name, {}).get("actions", {}).get(action, {}).get("service", None) + service: Optional[str] = ( + config.get(job_name, {}).get("actions", {}).get(action, {}).get("service", None) + ) if service: return service except FileNotFoundError: From e8e3c4d7284e1677e9815a5907b2fabfbb95a37c Mon Sep 17 00:00:00 2001 From: Dave Cuza Date: Thu, 17 Oct 2024 15:44:38 -0700 Subject: [PATCH 3/3] Fix type annotation error in scribereader.py --- tron/utils/scribereader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tron/utils/scribereader.py b/tron/utils/scribereader.py index 1036f6714..c1596a605 100644 --- a/tron/utils/scribereader.py +++ b/tron/utils/scribereader.py @@ -9,7 +9,7 @@ from typing import Optional from typing import Tuple -import staticconf +import staticconf # type: ignore import yaml from tron.config.static_config import get_config_watcher