From 685d78c854bed5ca13d466599b3cf47dd2d940f8 Mon Sep 17 00:00:00 2001 From: Marcelo Henrique Neppel Date: Sat, 19 Oct 2024 09:36:47 -0300 Subject: [PATCH] [DPE-5601] Add pgBackRest logrotate configuration (#645) * Add pgBackRest logrotate configuration Signed-off-by: Marcelo Henrique Neppel * Update how logrotate starts Signed-off-by: Marcelo Henrique Neppel * Refactor method name and add unit tests Signed-off-by: Marcelo Henrique Neppel * Merge linting changes --------- Signed-off-by: Marcelo Henrique Neppel Co-authored-by: Dragomir Penev --- src/backups.py | 8 +++ src/charm.py | 3 + src/cluster.py | 7 ++- src/constants.py | 2 + src/rotate_logs.py | 72 +++++++++++++++++++++++ templates/pgbackrest.logrotate.j2 | 10 ++++ tests/unit/test_backups.py | 13 +++- tests/unit/test_cluster.py | 11 ++++ tests/unit/test_rotate_logs.py | 98 +++++++++++++++++++++++++++++++ 9 files changed, 221 insertions(+), 3 deletions(-) create mode 100644 src/rotate_logs.py create mode 100644 templates/pgbackrest.logrotate.j2 create mode 100644 tests/unit/test_rotate_logs.py diff --git a/src/backups.py b/src/backups.py index 7068dc7342..95e4be6ac6 100644 --- a/src/backups.py +++ b/src/backups.py @@ -35,6 +35,7 @@ PGBACKREST_CONF_PATH, PGBACKREST_CONFIGURATION_FILE, PGBACKREST_EXECUTABLE, + PGBACKREST_LOGROTATE_FILE, PGBACKREST_LOGS_PATH, POSTGRESQL_DATA_PATH, ) @@ -1153,6 +1154,13 @@ def _render_pgbackrest_conf_file(self) -> bool: # Render pgBackRest config file. self.charm._patroni.render_file(f"{PGBACKREST_CONF_PATH}/pgbackrest.conf", rendered, 0o644) + # Render the logrotate configuration file. + with open("templates/pgbackrest.logrotate.j2") as file: + template = Template(file.read()) + self.charm._patroni.render_file( + PGBACKREST_LOGROTATE_FILE, template.render(), 0o644, change_owner=False + ) + return True def _restart_database(self) -> None: diff --git a/src/charm.py b/src/charm.py index c83d786631..5a82e993c9 100755 --- a/src/charm.py +++ b/src/charm.py @@ -102,6 +102,7 @@ ) from relations.db import EXTENSIONS_BLOCKING_MESSAGE, DbProvides from relations.postgresql_provider import PostgreSQLProvider +from rotate_logs import RotateLogs from upgrade import PostgreSQLUpgrade, get_postgresql_dependencies_model from utils import new_password @@ -170,6 +171,7 @@ def __init__(self, *args): juju_version = JujuVersion.from_environ() run_cmd = "/usr/bin/juju-exec" if juju_version.major > 2 else "/usr/bin/juju-run" self._observer = ClusterTopologyObserver(self, run_cmd) + self._rotate_logs = RotateLogs(self) self.framework.observe(self.on.cluster_topology_change, self._on_cluster_topology_change) self.framework.observe(self.on.install, self._on_install) self.framework.observe(self.on.leader_elected, self._on_leader_elected) @@ -203,6 +205,7 @@ def __init__(self, *args): charm=self, relation="restart", callback=self._restart ) self._observer.start_observer() + self._rotate_logs.start_log_rotation() self._grafana_agent = COSAgentProvider( self, metrics_endpoints=[{"path": "/metrics", "port": METRICS_PORT}], diff --git a/src/cluster.py b/src/cluster.py index 80fee8b746..0e0e1f5641 100644 --- a/src/cluster.py +++ b/src/cluster.py @@ -546,7 +546,7 @@ def promote_standby_cluster(self) -> None: if self.get_primary() is None: raise ClusterNotPromotedError("cluster not promoted") - def render_file(self, path: str, content: str, mode: int) -> None: + def render_file(self, path: str, content: str, mode: int, change_owner: bool = True) -> None: """Write a content rendered from a template to a file. Args: @@ -554,6 +554,8 @@ def render_file(self, path: str, content: str, mode: int) -> None: content: the data to be written to the file. mode: access permission mask applied to the file using chmod (e.g. 0o640). + change_owner: whether to change the file owner + to the snap_daemon user. """ # TODO: keep this method to use it also for generating replication configuration files and # move it to an utils / helpers file. @@ -562,7 +564,8 @@ def render_file(self, path: str, content: str, mode: int) -> None: file.write(content) # Ensure correct permissions are set on the file. os.chmod(path, mode) - self._change_owner(path) + if change_owner: + self._change_owner(path) def render_patroni_yml_file( self, diff --git a/src/constants.py b/src/constants.py index 943400ea57..f55769ec0b 100644 --- a/src/constants.py +++ b/src/constants.py @@ -84,3 +84,5 @@ PLUGIN_OVERRIDES = {"audit": "pgaudit", "uuid_ossp": '"uuid-ossp"'} SPI_MODULE = ["refint", "autoinc", "insert_username", "moddatetime"] + +PGBACKREST_LOGROTATE_FILE = "/etc/logrotate.d/pgbackrest.logrotate" diff --git a/src/rotate_logs.py b/src/rotate_logs.py new file mode 100644 index 0000000000..48b2c30fa2 --- /dev/null +++ b/src/rotate_logs.py @@ -0,0 +1,72 @@ +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Background process for rotating logs.""" + +import logging +import os +import subprocess +from time import sleep + +from ops.charm import CharmBase +from ops.framework import Object +from ops.model import ActiveStatus + +from constants import PGBACKREST_LOGROTATE_FILE + +logger = logging.getLogger(__name__) + +# File path for the spawned rotate logs process to write logs. +LOG_FILE_PATH = "/var/log/rotate_logs.log" + + +class RotateLogs(Object): + """Rotate logs every minute.""" + + def __init__(self, charm: CharmBase): + super().__init__(charm, "rotate-logs") + self._charm = charm + + def start_log_rotation(self): + """Start the rotate logs running in a new process.""" + if ( + not isinstance(self._charm.unit.status, ActiveStatus) + or self._charm._peers is None + or not os.path.exists(PGBACKREST_LOGROTATE_FILE) + ): + return + if "rotate-logs-pid" in self._charm.unit_peer_data: + # Double check that the PID exists. + pid = int(self._charm.unit_peer_data["rotate-logs-pid"]) + try: + os.kill(pid, 0) + return + except OSError: + pass + + logging.info("Starting rotate logs process") + + # Input is generated by the charm + pid = subprocess.Popen( # noqa: S603 + ["/usr/bin/python3", "src/rotate_logs.py"], + # File should not close + stdout=open(LOG_FILE_PATH, "a"), # noqa: SIM115 + stderr=subprocess.STDOUT, + ).pid + + self._charm.unit_peer_data.update({"rotate-logs-pid": f"{pid}"}) + logging.info(f"Started rotate logs process with PID {pid}") + + +def main(): + """Main loop that calls logrotate.""" + while True: + # Input is constant + subprocess.run(["/usr/sbin/logrotate", "-f", PGBACKREST_LOGROTATE_FILE]) # noqa: S603 + + # Wait 60 seconds before executing logrotate again. + sleep(60) + + +if __name__ == "__main__": + main() diff --git a/templates/pgbackrest.logrotate.j2 b/templates/pgbackrest.logrotate.j2 new file mode 100644 index 0000000000..f65878648b --- /dev/null +++ b/templates/pgbackrest.logrotate.j2 @@ -0,0 +1,10 @@ +/var/snap/charmed-postgresql/common/var/log/pgbackrest/*.log { + rotate 10 + missingok + notifempty + nocompress + daily + create 0600 snap_daemon snap_daemon + dateext + dateformat -%Y%m%d_%H:%M.log +} diff --git a/tests/unit/test_backups.py b/tests/unit/test_backups.py index c99e68bd15..6dc2702248 100644 --- a/tests/unit/test_backups.py +++ b/tests/unit/test_backups.py @@ -1691,13 +1691,24 @@ def test_render_pgbackrest_conf_file(harness, tls_ca_chain_filename): # Check the template is opened read-only in the call to open. assert mock.call_args_list[0][0] == ("templates/pgbackrest.conf.j2",) + # Get the expected content from a file. + with open("templates/pgbackrest.conf.j2") as file: + template = Template(file.read()) + log_rotation_expected_content = template.render() + # Ensure the correct rendered template is sent to _render_file method. calls = [ call( "/var/snap/charmed-postgresql/current/etc/pgbackrest/pgbackrest.conf", expected_content, 0o644, - ) + ), + call( + "/etc/logrotate.d/pgbackrest.logrotate", + log_rotation_expected_content, + 0o644, + change_owner=False, + ), ] if tls_ca_chain_filename != "": calls.insert(0, call(tls_ca_chain_filename, "fake-tls-ca-chain", 0o644)) diff --git a/tests/unit/test_cluster.py b/tests/unit/test_cluster.py index f0a1e04012..bd4a630434 100644 --- a/tests/unit/test_cluster.py +++ b/tests/unit/test_cluster.py @@ -285,6 +285,17 @@ def test_render_file(peers_ips, patroni): # Ensure the file is chown'd correctly. _chown.assert_called_with(filename, uid=35, gid=35) + # Test when it's requested to not change the file owner. + mock.reset_mock() + _pwnam.reset_mock() + _chmod.reset_mock() + _chown.reset_mock() + with patch("builtins.open", mock, create=True): + patroni.render_file(filename, "rendered-content", 0o640, change_owner=False) + _pwnam.assert_not_called() + _chmod.assert_called_once_with(filename, 0o640) + _chown.assert_not_called() + def test_render_patroni_yml_file(peers_ips, patroni): with ( diff --git a/tests/unit/test_rotate_logs.py b/tests/unit/test_rotate_logs.py new file mode 100644 index 0000000000..70b870f021 --- /dev/null +++ b/tests/unit/test_rotate_logs.py @@ -0,0 +1,98 @@ +# Copyright 2024 Canonical Ltd. +# See LICENSE file for licensing details. +from unittest.mock import Mock, PropertyMock, patch + +import pytest +from ops.charm import CharmBase +from ops.model import ActiveStatus, Relation, WaitingStatus +from ops.testing import Harness + +from rotate_logs import RotateLogs + + +class MockCharm(CharmBase): + def __init__(self, *args): + super().__init__(*args) + + self.rotate_logs = RotateLogs(self) + + @property + def _peers(self) -> Relation | None: + return None + + @property + def unit_peer_data(self) -> dict: + """Unit peer relation data object.""" + if self._peers is None: + return {} + + return self._peers.data[self.unit] + + +@pytest.fixture(autouse=True) +def harness(): + harness = Harness(MockCharm, meta="name: test-charm") + harness.begin() + yield harness + harness.cleanup() + + +def test_start_log_rotation(harness): + with ( + patch("builtins.open") as _open, + patch("subprocess.Popen") as _popen, + patch("os.path.exists") as _exists, + patch.object(MockCharm, "_peers", new_callable=PropertyMock) as _peers, + ): + # Test that nothing is done if there is already a running process. + _peers.return_value = Mock(data={harness.charm.unit: {"rotate-logs-pid": "1"}}) + _exists.return_value = True + harness.charm.rotate_logs.start_log_rotation() + _popen.assert_not_called() + + # Test that nothing is done if the charm is not in an active status. + harness.charm.unit.status = WaitingStatus() + _peers.return_value = Mock(data={harness.charm.unit: {}}) + harness.charm.rotate_logs.start_log_rotation() + _popen.assert_not_called() + + # Test that nothing is done if peer relation is not available yet. + harness.charm.unit.status = ActiveStatus() + _peers.return_value = None + harness.charm.rotate_logs.start_log_rotation() + _popen.assert_not_called() + + # Test that nothing is done if the logrotate file does not exist. + _peers.return_value = Mock(data={harness.charm.unit: {}}) + _exists.return_value = False + harness.charm.rotate_logs.start_log_rotation() + _popen.assert_not_called() + + # Test that nothing is done if there is already a running process. + _popen.return_value = Mock(pid=1) + _exists.return_value = True + harness.charm.rotate_logs.start_log_rotation() + _popen.assert_called_once() + + +def test_start_log_rotation_already_running(harness): + with ( + patch("builtins.open") as _open, + patch("subprocess.Popen") as _popen, + patch("os.kill") as _kill, + patch("os.path.exists") as _exists, + patch.object(MockCharm, "_peers", new_callable=PropertyMock) as _peers, + ): + harness.charm.unit.status = ActiveStatus() + _peers.return_value = Mock(data={harness.charm.unit: {"rotate-logs-pid": "1234"}}) + _exists.return_value = True + harness.charm.rotate_logs.start_log_rotation() + _kill.assert_called_once_with(1234, 0) + assert not _popen.called + _kill.reset_mock() + + # If process is already dead, it should restart. + _kill.side_effect = OSError + harness.charm.rotate_logs.start_log_rotation() + _kill.assert_called_once_with(1234, 0) + _popen.assert_called_once()