From e743f57d13b596bdb17f035da22949a27e1249e1 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Sat, 2 Nov 2024 11:55:22 +0100 Subject: [PATCH 01/15] Add config option for disabling reporting --- config.yaml | 4 ++++ src/grafana_agent.py | 2 ++ 2 files changed, 6 insertions(+) diff --git a/config.yaml b/config.yaml index b55ca0e8..b1676bda 100644 --- a/config.yaml +++ b/config.yaml @@ -64,3 +64,7 @@ options: to this range by Grafana Agent. type: float default: 100.0 + disable_reporting: + description: Disable reporting of enabled feature flags to Grafana. + type: boolean + default: false diff --git a/src/grafana_agent.py b/src/grafana_agent.py index 8a591c7c..a22faa6d 100644 --- a/src/grafana_agent.py +++ b/src/grafana_agent.py @@ -759,6 +759,8 @@ def _cli_args(self) -> str: if self.cert.enabled: args.append("-server.http.enable-tls") args.append("-server.grpc.enable-tls") + if self.config["disable_reporting"]: + args.append("-disable-reporting") return " ".join(args) def _generate_config(self) -> Dict[str, Any]: From 9ed33d53a594c537e24c61a73fe6637beb4e32ad Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 15:07:23 -0500 Subject: [PATCH 02/15] Rename config option --- config.yaml | 7 +++++-- src/grafana_agent.py | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/config.yaml b/config.yaml index b1676bda..f4e632d7 100644 --- a/config.yaml +++ b/config.yaml @@ -64,7 +64,10 @@ options: to this range by Grafana Agent. type: float default: 100.0 - disable_reporting: - description: Disable reporting of enabled feature flags to Grafana. + reporting_enabled: + description: | + Toggle reporting of usage info to grafana, such as enabled feature flags. + + Ref: https://grafana.com/docs/agent/latest/static/configuration/flags/#report-information-usage type: boolean default: false diff --git a/src/grafana_agent.py b/src/grafana_agent.py index a22faa6d..fdf37478 100644 --- a/src/grafana_agent.py +++ b/src/grafana_agent.py @@ -759,7 +759,7 @@ def _cli_args(self) -> str: if self.cert.enabled: args.append("-server.http.enable-tls") args.append("-server.grpc.enable-tls") - if self.config["disable_reporting"]: + if not self.config["reporting_enabled"]: args.append("-disable-reporting") return " ".join(args) From f5184c46fc2744d300301b4fbf3d230344426df6 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 15:11:33 -0500 Subject: [PATCH 03/15] Fix default --- config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.yaml b/config.yaml index f4e632d7..dca8e79f 100644 --- a/config.yaml +++ b/config.yaml @@ -70,4 +70,4 @@ options: Ref: https://grafana.com/docs/agent/latest/static/configuration/flags/#report-information-usage type: boolean - default: false + default: true From 8fbcbdfe01d8ff94f5492f0d5144c6b8967ccbf0 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 15:38:36 -0500 Subject: [PATCH 04/15] Scenario test draft --- tests/scenario/conftest.py | 8 ++++ .../scenario/test_config_reporting_enabled.py | 47 +++++++++++++++++++ tox.ini | 2 +- 3 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 tests/scenario/test_config_reporting_enabled.py diff --git a/tests/scenario/conftest.py b/tests/scenario/conftest.py index e658e08c..48266939 100644 --- a/tests/scenario/conftest.py +++ b/tests/scenario/conftest.py @@ -1,5 +1,8 @@ import shutil from pathlib import Path +from ops.testing import Context + +from charm import GrafanaAgentK8sCharm import pytest @@ -12,3 +15,8 @@ def vroot(tmp_path) -> Path: shutil.rmtree(root) shutil.copytree(CHARM_ROOT / "src", root / "src") return root + + +@pytest.fixture +def ctx(): + yield Context(GrafanaAgentK8sCharm) diff --git a/tests/scenario/test_config_reporting_enabled.py b/tests/scenario/test_config_reporting_enabled.py new file mode 100644 index 00000000..52b5d718 --- /dev/null +++ b/tests/scenario/test_config_reporting_enabled.py @@ -0,0 +1,47 @@ +from ops.testing import State, Container +from configparser import ConfigParser + + +containers = [Container(name="agent", can_connect=True)] + + +def test_reporting_enabled(ctx): + # GIVEN the "reporting_enabled" config option is set to True + state = State( + leader=True, config={"reporting_enabled": True}, containers=containers + ) + + # WHEN config-changed fires + out = ctx.run(ctx.on.config_changed(), state) + + # THEN the config file is written WITHOUT the [analytics] section being rendered + simulated_pebble_filesystem = out.get_container("grafana").get_filesystem(ctx) + grafana_config_path = simulated_pebble_filesystem / "etc/grafana/grafana-config.ini" + + config = ConfigParser() + config.read(grafana_config_path) + assert "analytics" not in config + + +def test_reporting_disabled(ctx): + # GIVEN the "reporting_enabled" config option is set to False + state = State(leader=True, config={"reporting_enabled": False}, containers=containers) + # WHEN config-changed fires + out = ctx.run(ctx.on.config_changed(), state) + + # THEN the config file is written WITH the [analytics] section being rendered + simulated_pebble_filesystem = out.get_container("grafana").get_filesystem(ctx) + grafana_config_path = simulated_pebble_filesystem / "etc/grafana/grafana-config.ini" + + config = ConfigParser() + config.read(grafana_config_path) + assert "analytics" in config + assert dict(config["analytics"]) == { + "reporting_enabled": "false", + "check_for_updates": "false", + "check_for_plugin_updates": "false", + } + + # AND the "grafana" service is restarted + # TODO Does it make sense to check this if the charm under test's lifetime is only for the config-changed? + # TODO How to assert this? diff --git a/tox.ini b/tox.ini index abb622ae..654cbbaa 100644 --- a/tox.ini +++ b/tox.ini @@ -85,7 +85,7 @@ deps = -r{toxinidir}/requirements.txt pytest cosl - ops-scenario >= 6.1.5,<7.0.0 + ops[testing] commands = pytest -vv --tb native --log-cli-level=INFO -s {posargs} {[vars]tst_path}/scenario --ignore {[vars]tst_path}/scenario/test_k8s From 59a770667aea0e6f95dcdb03a2a0070a169f96e2 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 15:45:35 -0500 Subject: [PATCH 05/15] Fix scenario test --- .../scenario/test_config_reporting_enabled.py | 27 +++---------------- 1 file changed, 4 insertions(+), 23 deletions(-) diff --git a/tests/scenario/test_config_reporting_enabled.py b/tests/scenario/test_config_reporting_enabled.py index 52b5d718..34fad357 100644 --- a/tests/scenario/test_config_reporting_enabled.py +++ b/tests/scenario/test_config_reporting_enabled.py @@ -14,13 +14,8 @@ def test_reporting_enabled(ctx): # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) - # THEN the config file is written WITHOUT the [analytics] section being rendered - simulated_pebble_filesystem = out.get_container("grafana").get_filesystem(ctx) - grafana_config_path = simulated_pebble_filesystem / "etc/grafana/grafana-config.ini" - - config = ConfigParser() - config.read(grafana_config_path) - assert "analytics" not in config + # THEN the service layer does NOT include the "-disable-reporting" arg + assert "-disable-reporting" not in out.get_container("agent").layers.services["agent"].to_dict().command def test_reporting_disabled(ctx): @@ -29,19 +24,5 @@ def test_reporting_disabled(ctx): # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) - # THEN the config file is written WITH the [analytics] section being rendered - simulated_pebble_filesystem = out.get_container("grafana").get_filesystem(ctx) - grafana_config_path = simulated_pebble_filesystem / "etc/grafana/grafana-config.ini" - - config = ConfigParser() - config.read(grafana_config_path) - assert "analytics" in config - assert dict(config["analytics"]) == { - "reporting_enabled": "false", - "check_for_updates": "false", - "check_for_plugin_updates": "false", - } - - # AND the "grafana" service is restarted - # TODO Does it make sense to check this if the charm under test's lifetime is only for the config-changed? - # TODO How to assert this? + # THEN the service layer INCLUDES the "-disable-reporting" arg + assert "-disable-reporting" in out.get_container("agent").layers.services["agent"].to_dict().command From 3daa286200fd9cefeb18625198ae5b00a11925f7 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 15:57:04 -0500 Subject: [PATCH 06/15] Fix scenario test --- src/grafana_agent.py | 1 + tests/scenario/test_config_reporting_enabled.py | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/grafana_agent.py b/src/grafana_agent.py index fdf37478..7a21891a 100644 --- a/src/grafana_agent.py +++ b/src/grafana_agent.py @@ -148,6 +148,7 @@ def __init__(self, *args): for rules in [self.loki_rules_paths, self.dashboard_paths]: if not os.path.isdir(rules.dest): + rules.src.mkdir(parents=True, exist_ok=True) shutil.copytree(rules.src, rules.dest, dirs_exist_ok=True) self._remote_write = PrometheusRemoteWriteConsumer( diff --git a/tests/scenario/test_config_reporting_enabled.py b/tests/scenario/test_config_reporting_enabled.py index 34fad357..745bdac4 100644 --- a/tests/scenario/test_config_reporting_enabled.py +++ b/tests/scenario/test_config_reporting_enabled.py @@ -15,7 +15,7 @@ def test_reporting_enabled(ctx): out = ctx.run(ctx.on.config_changed(), state) # THEN the service layer does NOT include the "-disable-reporting" arg - assert "-disable-reporting" not in out.get_container("agent").layers.services["agent"].to_dict().command + assert "-disable-reporting" not in out.get_container("agent").layers["agent"].services["agent"].command def test_reporting_disabled(ctx): @@ -25,4 +25,4 @@ def test_reporting_disabled(ctx): out = ctx.run(ctx.on.config_changed(), state) # THEN the service layer INCLUDES the "-disable-reporting" arg - assert "-disable-reporting" in out.get_container("agent").layers.services["agent"].to_dict().command + assert "-disable-reporting" in out.get_container("agent").layers["agent"].services["agent"].command From 207bb92a7f470304d33c8f0a809101e8988d5afc Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:33:45 -0500 Subject: [PATCH 07/15] Fix scenario tests --- tests/scenario/conftest.py | 12 ------- tests/scenario/test_dashboard_transfer.py | 13 +++----- tests/scenario/test_setup_statuses.py | 28 +++++----------- tests/scenario/test_start_statuses.py | 32 +++++------------- tests/scenario/test_tracing_integration.py | 38 +++++++++++----------- 5 files changed, 40 insertions(+), 83 deletions(-) diff --git a/tests/scenario/conftest.py b/tests/scenario/conftest.py index 48266939..62a7bcb5 100644 --- a/tests/scenario/conftest.py +++ b/tests/scenario/conftest.py @@ -1,21 +1,9 @@ -import shutil -from pathlib import Path from ops.testing import Context from charm import GrafanaAgentK8sCharm import pytest -CHARM_ROOT = Path(__file__).parent.parent.parent - - -@pytest.fixture -def vroot(tmp_path) -> Path: - root = Path(str(tmp_path.absolute())) - shutil.rmtree(root) - shutil.copytree(CHARM_ROOT / "src", root / "src") - return root - @pytest.fixture def ctx(): diff --git a/tests/scenario/test_dashboard_transfer.py b/tests/scenario/test_dashboard_transfer.py index 0babdd77..46d9da62 100644 --- a/tests/scenario/test_dashboard_transfer.py +++ b/tests/scenario/test_dashboard_transfer.py @@ -3,7 +3,7 @@ import json from cosl import GrafanaDashboard -from scenario import Container, Context, Relation, State +from ops.testing import Container, Context, Relation, State from charm import GrafanaAgentK8sCharm @@ -12,7 +12,7 @@ def encode_as_dashboard(dct: dict): return GrafanaDashboard._serialize(json.dumps(dct).encode("utf-8")) -def test_dashboard_propagation(vroot): +def test_dashboard_propagation(ctx): # This test verifies that if the charm receives a dashboard via the requirer databag, # it is correctly transferred to the provider databag. @@ -29,23 +29,18 @@ def test_dashboard_propagation(vroot): } consumer = Relation( "grafana-dashboards-consumer", - relation_id=1, remote_app_data={"dashboards": json.dumps(data)}, ) - provider = Relation("grafana-dashboards-provider", relation_id=2) + provider = Relation("grafana-dashboards-provider") - ctx = Context(charm_type=GrafanaAgentK8sCharm, charm_root=vroot) state = State( relations=[consumer, provider], leader=True, containers=[Container("agent", can_connect=True)], ) - with ctx.manager( - state=state, - event=consumer.changed_event, - ) as mgr: + with ctx(ctx.on.relation_changed(consumer), state=state) as mgr: dash = mgr.charm.dashboards[0] assert dash["charm"] == expected["charm"] assert dash["title"] == expected["title"] diff --git a/tests/scenario/test_setup_statuses.py b/tests/scenario/test_setup_statuses.py index 9de833f3..4bc0c94a 100644 --- a/tests/scenario/test_setup_statuses.py +++ b/tests/scenario/test_setup_statuses.py @@ -2,42 +2,30 @@ # See LICENSE file for licensing details. from ops import BlockedStatus, UnknownStatus, pebble -from scenario import Container, Context, ExecOutput, State +from ops.testing import Container, Context, State, Exec import charm -def test_install(vroot): - context = Context( - charm.GrafanaAgentK8sCharm, - charm_root=vroot, - ) - out = context.run("install", State()) +def test_install(ctx): + out = ctx.run(ctx.on.install(), State()) assert out.unit_status == UnknownStatus() -def test_start(vroot): - context = Context( - charm.GrafanaAgentK8sCharm, - charm_root=vroot, - ) - out = context.run("start", State()) +def test_start(ctx): + out = ctx.run(ctx.on.start(), State()) assert out.unit_status == UnknownStatus() -def test_charm_start_with_container(vroot): +def test_charm_start_with_container(ctx): agent = Container( name="agent", can_connect=True, - exec_mock={("/bin/agent", "-version"): ExecOutput(stdout="42.42")}, + execs={Exec(["/bin/agent", "-version"], return_code=0, stdout="42.42")}, ) - context = Context( - charm.GrafanaAgentK8sCharm, - charm_root=vroot, - ) state = State(containers=[agent]) - out = context.run(agent.pebble_ready_event, state) + out = ctx.run(ctx.on.pebble_ready(agent), state) assert out.unit_status == BlockedStatus( "Missing incoming ('requires') relation: metrics-endpoint|logging-provider|tracing-provider|grafana-dashboards-consumer" diff --git a/tests/scenario/test_start_statuses.py b/tests/scenario/test_start_statuses.py index bb4bfc5a..903ca160 100644 --- a/tests/scenario/test_start_statuses.py +++ b/tests/scenario/test_start_statuses.py @@ -4,12 +4,10 @@ from pathlib import Path from ops import pebble -from scenario import Container, Context, ExecOutput, State +from ops.testing import Container, Context, Exec, State, UnknownStatus from charm import GrafanaAgentK8sCharm -CHARM_ROOT = Path(__file__).parent.parent.parent - @dataclasses.dataclass class _MockProc: @@ -21,36 +19,24 @@ def _subp_run_mock(*a, **kw): return _MockProc(0) -def test_install(vroot): - ctx = Context( - charm_type=GrafanaAgentK8sCharm, - charm_root=vroot, - ) - out = ctx.run(state=State(), event="install") - assert out.unit_status == ("unknown", "") +def test_install(ctx): + out = ctx.run(ctx.on.install(), state=State()) + assert out.unit_status == UnknownStatus() -def test_start(vroot): - ctx = Context( - charm_type=GrafanaAgentK8sCharm, - charm_root=vroot, - ) - out = ctx.run(state=State(), event="start") +def test_start(ctx): + out = ctx.run(ctx.on.start(), state=State()) assert out.unit_status.name == "unknown" -def test_charm_start_with_container(vroot): +def test_charm_start_with_container(ctx): agent = Container( name="agent", can_connect=True, - exec_mock={("/bin/agent", "-version"): ExecOutput(stdout="42.42")}, + execs={Exec(["/bin/agent", "-version"], return_code=0, stdout="42.42")}, ) - ctx = Context( - charm_type=GrafanaAgentK8sCharm, - charm_root=vroot, - ) - out = ctx.run(state=State(containers=[agent]), event=agent.pebble_ready_event) + out = ctx.run(ctx.on.pebble_ready(agent), state=State(containers=[agent])) assert out.unit_status.name == "blocked" agent_out = out.get_container("agent") diff --git a/tests/scenario/test_tracing_integration.py b/tests/scenario/test_tracing_integration.py index 9a0b5be5..19c87931 100644 --- a/tests/scenario/test_tracing_integration.py +++ b/tests/scenario/test_tracing_integration.py @@ -1,29 +1,29 @@ from unittest.mock import patch import pytest -import scenario import yaml from charms.tempo_k8s.v1.charm_tracing import charm_tracing_disabled from charms.tempo_k8s.v2.tracing import Receiver, TracingProviderAppData, TracingRequirerAppData from ops import pebble +from ops.testing import Context, State, Container, Relation from charm import GrafanaAgentK8sCharm from grafana_agent import CONFIG_PATH @pytest.fixture -def ctx(vroot): +def ctx(): with charm_tracing_disabled(): with patch("socket.getfqdn", new=lambda: "localhost"): - yield scenario.Context(GrafanaAgentK8sCharm, charm_root=vroot) + yield Context(GrafanaAgentK8sCharm) @pytest.fixture def base_state(): - yield scenario.State( + yield State( leader=True, containers=[ - scenario.Container( + Container( "agent", can_connect=True, # set it to inactive so we can detect when an event has caused it to start @@ -35,14 +35,14 @@ def base_state(): def test_tracing_relation(ctx, base_state): # GIVEN a tracing relation over the tracing-provider endpoint - tracing = scenario.Relation( + tracing = Relation( "tracing-provider", remote_app_data=TracingRequirerAppData(receivers=["otlp_http", "otlp_grpc"]).dump(), ) state = base_state.replace(relations=[tracing]) # WHEN we process any setup event for the relation - state_out = ctx.run(tracing.changed_event, state) + state_out = ctx.run(ctx.on.relation_changed(tracing), state) agent = state_out.get_container("agent") @@ -58,11 +58,11 @@ def test_tracing_relation(ctx, base_state): def test_tracing_relations_in_and_out(ctx, base_state): # GIVEN a tracing relation over the tracing-provider endpoint and one over tracing - tracing_provider = scenario.Relation( + tracing_provider = Relation( "tracing-provider", remote_app_data=TracingRequirerAppData(receivers=["otlp_http", "otlp_grpc"]).dump(), ) - tracing = scenario.Relation( + tracing = Relation( "tracing", remote_app_data=TracingProviderAppData( receivers=[ @@ -73,7 +73,7 @@ def test_tracing_relations_in_and_out(ctx, base_state): state = base_state.replace(relations=[tracing, tracing_provider]) # WHEN we process any setup event for the relation - state_out = ctx.run(tracing.changed_event, state) + state_out = ctx.run(ctx.on.relation_changed(tracing), state) agent = state_out.get_container("agent") @@ -89,11 +89,11 @@ def test_tracing_relations_in_and_out(ctx, base_state): def test_tracing_relation_passthrough(ctx, base_state): # GIVEN a tracing relation over the tracing-provider endpoint and one over tracing - tracing_provider = scenario.Relation( + tracing_provider = Relation( "tracing-provider", remote_app_data=TracingRequirerAppData(receivers=["otlp_http", "otlp_grpc"]).dump(), ) - tracing = scenario.Relation( + tracing = Relation( "tracing", remote_app_data=TracingProviderAppData( receivers=[ @@ -104,7 +104,7 @@ def test_tracing_relation_passthrough(ctx, base_state): state = base_state.replace(relations=[tracing, tracing_provider]) # WHEN we process any setup event for the relation - state_out = ctx.run(tracing.changed_event, state) + state_out = ctx.run(ctx.on.relation_changed(tracing), state) # THEN we act as a tracing provider for 'tracing-provider', and as requirer for 'tracing' tracing_out = TracingRequirerAppData.load(state_out.get_relations("tracing")[0].local_app_data) @@ -132,11 +132,11 @@ def test_tracing_relation_passthrough(ctx, base_state): ) def test_tracing_relation_passthrough_with_force_enable(ctx, base_state, force_enable): # GIVEN a tracing relation over the tracing-provider endpoint and one over tracing - tracing_provider = scenario.Relation( + tracing_provider = Relation( "tracing-provider", remote_app_data=TracingRequirerAppData(receivers=["otlp_http", "otlp_grpc"]).dump(), ) - tracing = scenario.Relation( + tracing = Relation( "tracing", remote_app_data=TracingProviderAppData( receivers=[ @@ -151,7 +151,7 @@ def test_tracing_relation_passthrough_with_force_enable(ctx, base_state, force_e relations=[tracing, tracing_provider], ) # WHEN we process any setup event for the relation - state_out = ctx.run(tracing.changed_event, state) + state_out = ctx.run(ctx.on.relation_changed(tracing), state) # THEN we act as a tracing provider for 'tracing-provider', and as requirer for 'tracing' tracing_out = TracingRequirerAppData.load(state_out.get_relations("tracing")[0].local_app_data) @@ -179,11 +179,11 @@ def test_tracing_relation_passthrough_with_force_enable(ctx, base_state, force_e ) def test_tracing_sampling_config_is_present(ctx, base_state, sampling_config): # GIVEN a tracing relation over the tracing-provider endpoint and one over tracing - tracing_provider = scenario.Relation( + tracing_provider = Relation( "tracing-provider", remote_app_data=TracingRequirerAppData(receivers=["otlp_http", "otlp_grpc"]).dump(), ) - tracing = scenario.Relation( + tracing = Relation( "tracing", remote_app_data=TracingProviderAppData( receivers=[ @@ -194,7 +194,7 @@ def test_tracing_sampling_config_is_present(ctx, base_state, sampling_config): state = base_state.replace(relations=[tracing, tracing_provider], config=sampling_config) # WHEN we process any setup event for the relation - state_out = ctx.run(tracing.changed_event, state) + state_out = ctx.run(ctx.ok.relation_changed(tracing), state) agent = state_out.get_container("agent") From b4268354312e56fe495fe7ccd0517b4ec34b7d6d Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:34:04 -0500 Subject: [PATCH 08/15] fmt --- tests/scenario/conftest.py | 3 +-- .../scenario/test_config_reporting_enabled.py | 18 ++++++++++-------- tests/scenario/test_dashboard_transfer.py | 4 +--- tests/scenario/test_setup_statuses.py | 4 +--- tests/scenario/test_start_statuses.py | 5 +---- tests/scenario/test_tracing_integration.py | 2 +- 6 files changed, 15 insertions(+), 21 deletions(-) diff --git a/tests/scenario/conftest.py b/tests/scenario/conftest.py index 62a7bcb5..c6e928ab 100644 --- a/tests/scenario/conftest.py +++ b/tests/scenario/conftest.py @@ -1,9 +1,8 @@ +import pytest from ops.testing import Context from charm import GrafanaAgentK8sCharm -import pytest - @pytest.fixture def ctx(): diff --git a/tests/scenario/test_config_reporting_enabled.py b/tests/scenario/test_config_reporting_enabled.py index 745bdac4..d519e8b4 100644 --- a/tests/scenario/test_config_reporting_enabled.py +++ b/tests/scenario/test_config_reporting_enabled.py @@ -1,21 +1,20 @@ -from ops.testing import State, Container -from configparser import ConfigParser - +from ops.testing import Container, State containers = [Container(name="agent", can_connect=True)] def test_reporting_enabled(ctx): # GIVEN the "reporting_enabled" config option is set to True - state = State( - leader=True, config={"reporting_enabled": True}, containers=containers - ) + state = State(leader=True, config={"reporting_enabled": True}, containers=containers) # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) # THEN the service layer does NOT include the "-disable-reporting" arg - assert "-disable-reporting" not in out.get_container("agent").layers["agent"].services["agent"].command + assert ( + "-disable-reporting" + not in out.get_container("agent").layers["agent"].services["agent"].command + ) def test_reporting_disabled(ctx): @@ -25,4 +24,7 @@ def test_reporting_disabled(ctx): out = ctx.run(ctx.on.config_changed(), state) # THEN the service layer INCLUDES the "-disable-reporting" arg - assert "-disable-reporting" in out.get_container("agent").layers["agent"].services["agent"].command + assert ( + "-disable-reporting" + in out.get_container("agent").layers["agent"].services["agent"].command + ) diff --git a/tests/scenario/test_dashboard_transfer.py b/tests/scenario/test_dashboard_transfer.py index 46d9da62..c3307324 100644 --- a/tests/scenario/test_dashboard_transfer.py +++ b/tests/scenario/test_dashboard_transfer.py @@ -3,9 +3,7 @@ import json from cosl import GrafanaDashboard -from ops.testing import Container, Context, Relation, State - -from charm import GrafanaAgentK8sCharm +from ops.testing import Container, Relation, State def encode_as_dashboard(dct: dict): diff --git a/tests/scenario/test_setup_statuses.py b/tests/scenario/test_setup_statuses.py index 4bc0c94a..257c3d58 100644 --- a/tests/scenario/test_setup_statuses.py +++ b/tests/scenario/test_setup_statuses.py @@ -2,9 +2,7 @@ # See LICENSE file for licensing details. from ops import BlockedStatus, UnknownStatus, pebble -from ops.testing import Container, Context, State, Exec - -import charm +from ops.testing import Container, Exec, State def test_install(ctx): diff --git a/tests/scenario/test_start_statuses.py b/tests/scenario/test_start_statuses.py index 903ca160..9f129b9f 100644 --- a/tests/scenario/test_start_statuses.py +++ b/tests/scenario/test_start_statuses.py @@ -1,12 +1,9 @@ # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. import dataclasses -from pathlib import Path from ops import pebble -from ops.testing import Container, Context, Exec, State, UnknownStatus - -from charm import GrafanaAgentK8sCharm +from ops.testing import Container, Exec, State, UnknownStatus @dataclasses.dataclass diff --git a/tests/scenario/test_tracing_integration.py b/tests/scenario/test_tracing_integration.py index 19c87931..d854c7a1 100644 --- a/tests/scenario/test_tracing_integration.py +++ b/tests/scenario/test_tracing_integration.py @@ -5,7 +5,7 @@ from charms.tempo_k8s.v1.charm_tracing import charm_tracing_disabled from charms.tempo_k8s.v2.tracing import Receiver, TracingProviderAppData, TracingRequirerAppData from ops import pebble -from ops.testing import Context, State, Container, Relation +from ops.testing import Container, Context, Relation, State from charm import GrafanaAgentK8sCharm from grafana_agent import CONFIG_PATH From 42fb794251099e139ed99eedcea594c3896ac804 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 17:18:58 -0500 Subject: [PATCH 09/15] Restart if command differs --- src/charm.py | 10 ++++++++-- src/grafana_agent.py | 6 +++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/charm.py b/src/charm.py index f90a5134..19377c99 100755 --- a/src/charm.py +++ b/src/charm.py @@ -98,16 +98,22 @@ def _layer(self) -> Layer: "summary": "agent layer", "description": "pebble config layer for Grafana Agent", "services": { - "agent": { + self._name: { "override": "replace", "summary": "agent", - "command": f"/bin/agent {self._cli_args()}", + "command": self._command(), "startup": "enabled", }, }, }, ) + def _command(self) -> str: + return f"/bin/agent {self._cli_args()}" + + def is_command_changed(self) -> bool: + return self._container.get_plan().services[self._name].command == self._command() + def _on_dashboards_changed(self, _event) -> None: logger.info("updating dashboards") diff --git a/src/grafana_agent.py b/src/grafana_agent.py index 7a21891a..3e228594 100644 --- a/src/grafana_agent.py +++ b/src/grafana_agent.py @@ -446,6 +446,10 @@ def positions_dir(self) -> str: """Return the positions directory.""" raise NotImplementedError("Please override the positions_dir method") + def is_command_changed(self) -> bool: + """Return True if the command used to start the agent is different from what it would be now.""" + raise NotImplementedError("Please override the command method") + def run(self, cmd: List[str]): """Run cmd on the workload. @@ -635,7 +639,7 @@ def _update_config(self) -> None: # File does not yet exist? Processing a deferred event? old_config = None - if config == old_config: + if config == old_config and not self.is_command_changed(): # Nothing changed, possibly new installation. Move on. self.status.update_config = None return From f2bd7805618030e120aeef4f940642ef1454d83f Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 17:20:47 -0500 Subject: [PATCH 10/15] Doc --- src/charm.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/charm.py b/src/charm.py index 19377c99..27a786ed 100755 --- a/src/charm.py +++ b/src/charm.py @@ -112,6 +112,7 @@ def _command(self) -> str: return f"/bin/agent {self._cli_args()}" def is_command_changed(self) -> bool: + """Compare the current command we'd issue with the one in the pebble layer.""" return self._container.get_plan().services[self._name].command == self._command() def _on_dashboards_changed(self, _event) -> None: From 712abd7e3b6d2d76c7bbe6172513e8b3147d83f9 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 17:35:06 -0500 Subject: [PATCH 11/15] Fix --- src/charm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/charm.py b/src/charm.py index 27a786ed..9a813f80 100755 --- a/src/charm.py +++ b/src/charm.py @@ -113,7 +113,7 @@ def _command(self) -> str: def is_command_changed(self) -> bool: """Compare the current command we'd issue with the one in the pebble layer.""" - return self._container.get_plan().services[self._name].command == self._command() + return self._container.get_plan().services[self._name].command != self._command() def _on_dashboards_changed(self, _event) -> None: logger.info("updating dashboards") From f5eeec2b04204663e61aaf5de5facf6a15900f7a Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 17:46:39 -0500 Subject: [PATCH 12/15] More robustness --- src/charm.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/charm.py b/src/charm.py index 9a813f80..f052d10c 100755 --- a/src/charm.py +++ b/src/charm.py @@ -113,7 +113,9 @@ def _command(self) -> str: def is_command_changed(self) -> bool: """Compare the current command we'd issue with the one in the pebble layer.""" - return self._container.get_plan().services[self._name].command != self._command() + if svc := self._container.get_plan().services.get(self._name): + return svc.command != self._command() + return True def _on_dashboards_changed(self, _event) -> None: logger.info("updating dashboards") From fa0fcd1823e0825c773c406bd412c095f5dbd48b Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Thu, 14 Nov 2024 09:46:00 +0100 Subject: [PATCH 13/15] adjusted scenario tests to work with ops[testing] --- tests/scenario/test_tracing_integration.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/scenario/test_tracing_integration.py b/tests/scenario/test_tracing_integration.py index d854c7a1..ff0804da 100644 --- a/tests/scenario/test_tracing_integration.py +++ b/tests/scenario/test_tracing_integration.py @@ -1,3 +1,4 @@ +import dataclasses from unittest.mock import patch import pytest @@ -27,7 +28,7 @@ def base_state(): "agent", can_connect=True, # set it to inactive so we can detect when an event has caused it to start - service_status={"agent": pebble.ServiceStatus.INACTIVE}, + service_statuses={"agent": pebble.ServiceStatus.INACTIVE}, ) ], ) @@ -40,7 +41,7 @@ def test_tracing_relation(ctx, base_state): remote_app_data=TracingRequirerAppData(receivers=["otlp_http", "otlp_grpc"]).dump(), ) - state = base_state.replace(relations=[tracing]) + state = dataclasses.replace(base_state, relations=[tracing]) # WHEN we process any setup event for the relation state_out = ctx.run(ctx.on.relation_changed(tracing), state) @@ -71,7 +72,7 @@ def test_tracing_relations_in_and_out(ctx, base_state): ).dump(), ) - state = base_state.replace(relations=[tracing, tracing_provider]) + state = dataclasses.replace(base_state, relations=[tracing, tracing_provider]) # WHEN we process any setup event for the relation state_out = ctx.run(ctx.on.relation_changed(tracing), state) @@ -102,7 +103,7 @@ def test_tracing_relation_passthrough(ctx, base_state): ).dump(), ) - state = base_state.replace(relations=[tracing, tracing_provider]) + state = dataclasses.replace(base_state, relations=[tracing, tracing_provider]) # WHEN we process any setup event for the relation state_out = ctx.run(ctx.on.relation_changed(tracing), state) @@ -146,7 +147,7 @@ def test_tracing_relation_passthrough_with_force_enable(ctx, base_state, force_e ) # AND given we're configured to always enable some protocols - state = base_state.replace( + state = dataclasses.replace(base_state, config={f"always_enable_{proto}": True for proto in force_enable}, relations=[tracing, tracing_provider], ) @@ -192,9 +193,9 @@ def test_tracing_sampling_config_is_present(ctx, base_state, sampling_config): ).dump(), ) - state = base_state.replace(relations=[tracing, tracing_provider], config=sampling_config) + state = dataclasses.replace(base_state, relations=[tracing, tracing_provider], config=sampling_config) # WHEN we process any setup event for the relation - state_out = ctx.run(ctx.ok.relation_changed(tracing), state) + state_out = ctx.run(ctx.on.relation_changed(tracing), state) agent = state_out.get_container("agent") From 8d78633be284db0b7ad0950fdc9669f558e57e9a Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Thu, 14 Nov 2024 09:46:52 +0100 Subject: [PATCH 14/15] gitignored egg-infos --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index b8ae2d8b..46f229aa 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,4 @@ __pycache__/ tests/integration/*-tester/lib/ .env cos-tool* +*.egg-info From 3423903b60753145c75649ec1ce454c91b732fb7 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Thu, 14 Nov 2024 09:55:31 +0100 Subject: [PATCH 15/15] lint --- tests/scenario/test_tracing_integration.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/scenario/test_tracing_integration.py b/tests/scenario/test_tracing_integration.py index ff0804da..8d8dd47f 100644 --- a/tests/scenario/test_tracing_integration.py +++ b/tests/scenario/test_tracing_integration.py @@ -147,7 +147,8 @@ def test_tracing_relation_passthrough_with_force_enable(ctx, base_state, force_e ) # AND given we're configured to always enable some protocols - state = dataclasses.replace(base_state, + state = dataclasses.replace( + base_state, config={f"always_enable_{proto}": True for proto in force_enable}, relations=[tracing, tracing_provider], ) @@ -193,7 +194,9 @@ def test_tracing_sampling_config_is_present(ctx, base_state, sampling_config): ).dump(), ) - state = dataclasses.replace(base_state, relations=[tracing, tracing_provider], config=sampling_config) + state = dataclasses.replace( + base_state, relations=[tracing, tracing_provider], config=sampling_config + ) # WHEN we process any setup event for the relation state_out = ctx.run(ctx.on.relation_changed(tracing), state)