Merge lp:~silverdrake11/landscape-charm/manual_db_config_LNDENG-308 into lp:~mitchburton/landscape-charm/op-framework

[Kevin Nasto]

Added postgres manual configs to charm

[Mitch Burton (mitchburton)]

I'm getting a few test regressions when I run ./run_tests (after fixing the newline issue mentioned inline).

If these preexist this change, then I apologize. They still should be fixed though.

Here's tests I did and the results:
1. deploy a juju model with landscape-server, haproxy, and rabbitmq-server
2. install postgresql and other deps (python3-apt postgresql-contrib postgresql-.*-debversion postgresql-plpython3-*) in a non-juju managed LXC
3. edit pg_hba.conf and postgresql.conf to allow the landscape unit to manage postgres
4. juju config with all four db_* config options

Result:

Next test
Same as above except skip step 3

Result: "failed to update database schema"

then I performed step 3, ran juju config --reset db_host,db_password,db_port,db_user, then perofrmed step 4

Result: "failed to update database schema", even though databases and tables were created.

Looks like once that BlockedStatus (or WaitingStatus) is tripped, it's difficult to recover from it short of replacing the landscape-server unit entirely.

I think you need to update the _stored["db"] in _on_config_changed as you removed that update from _migrate_schema_bootstrap.

review: Needs Fixing

[lp:~silverdrake11/landscape-charm/manual_db_config_LNDENG-308]

434. By Kevin Nasto on 2023-02-06

Added waiting status if is successful

[Mitch Burton (mitchburton)]

LGTM after changes

review: Approve

=== modified file 'config.yaml'
--- config.yaml	2022-10-18 18:27:37 +0000
+++ config.yaml	2023-02-06 16:13:35 +0000
@@ -115,3 +115,23 @@
     description: |
       Comma-separated list of nagios servicegroups. If empty, the
       nagios-context will be used as the servicegroup.
+  db_host:
+    type: string
+    default:
+    description: |
+      Optionally specify the host instead of getting it from the postgres charm
+  db_password:
+    type: string
+    default:
+    description: Optionally specify db password
+  db_port:
+    type: string
+    default:
+    description: |
+      Optionally specify the db port instead of getting it from the postgres
+      charm. Falls back to the default postgres port
+  db_user:
+    type: string
+    default:
+    description: |
+      Optionally specify the postgres user instead of getting it from the charm

=== modified file 'src/charm.py'
--- src/charm.py	2023-01-25 19:57:43 +0000
+++ src/charm.py	2023-02-06 16:13:35 +0000
@@ -37,7 +37,7 @@
 
 from settings_files import (
     prepend_default_settings, update_default_settings, update_service_conf,
-    write_license_file, write_ssl_cert)
+    write_license_file, write_ssl_cert, update_db_conf, DEFAULT_POSTGRES_PORT)
 
 logger = logging.getLogger(__name__)
 
@@ -185,6 +185,27 @@
                 "package-upload": {"root-url": root_url},
             })
 
+        config_host = self.model.config.get("db_host")
+        config_password = self.model.config.get("db_password")
+        config_port = self.model.config.get("db_port")
+        config_user = self.model.config.get("db_user")
+        db_kargs = {}
+        if config_host:
+            db_kargs["host"] = config_host
+        if config_password:
+            db_kargs["password"] = config_password
+        if config_port:
+            db_kargs["port"] = config_port
+        if config_user:
+            db_kargs["user"] = config_user
+        if db_kargs:
+            update_db_conf(**db_kargs)
+            if self._migrate_schema_bootstrap():
+                self.unit.status = WaitingStatus("Waiting on relations")
+                self._stored.ready["db"] = True
+            else:
+                return
+
         if isinstance(prev_status, BlockedStatus):
             self.unit.status = prev_status
 
@@ -297,10 +318,15 @@
         unit_data = event.relation.data[event.unit]
 
         required_relation_data = ["master", "allowed-units", "port", "user"]
-        missing_relation_data = [i for i in required_relation_data if i not in unit_data]
+        missing_relation_data = [
+            i for i in required_relation_data if i not in unit_data
+        ]
         if missing_relation_data:
-            logger.info("db relation not yet ready. Missing keys: {}".format(
-                missing_relation_data))
+            logger.info(
+                "db relation not yet ready. Missing keys: {}".format(
+                    missing_relation_data
+                )
+            )
             self.unit.status = ActiveStatus("Unit is ready")
             self._update_ready_status()
             return
@@ -317,36 +343,57 @@
 
         # We can't use unit_data["host"] because it can return the IP of the secondary
         master = dict(s.split("=", 1) for s in unit_data["master"].split(" "))
-        host = master["host"]
-        password = master["password"]
-        port = unit_data["port"]
-        user = unit_data["user"]
-
-        update_service_conf({
-            "stores": {
-                "host": "{}:{}".format(host, port),
-                "password": password,
-            },
-            "schema": {
-                "store_user": user,
-                "store_password": password,
-            },
-        })
-
-        # Ensure the database users and schemas are set up.
+
+        # Override db config if manually set in juju
+        config_host = self.model.config.get("db_host")
+        if config_host:
+            host = config_host
+        else:
+            host = master["host"]
+
+        config_password = self.model.config.get("db_password")
+        if config_password:
+            password = config_password
+        else:
+            password = master["password"]
+
+        config_port = self.model.config.get("db_port")
+        if config_port:
+            port = config_port
+        else:
+            port = unit_data["port"]
+        if not port:
+            port = DEFAULT_POSTGRES_PORT  # Fall back to postgres default port if still not set
+
+        config_user = self.model.config.get("db_user")
+        if config_user:
+            user = config_user
+        else:
+            user = unit_data["user"]
+
+        update_db_conf(host=host, port=port, user=user, password=password)
+
+        if not self._migrate_schema_bootstrap():
+            return
+
+        self._stored.ready["db"] = True
+        self.unit.status = ActiveStatus("Unit is ready")
+        self._update_ready_status()
+
+    def _migrate_schema_bootstrap(self):
+        """
+        Migrates schema along with the bootstrap command which ensures that the
+        databases along with the landscape user exists. Returns True on success
+        """
         try:
-            check_call(["/usr/bin/landscape-schema", "--bootstrap"])
+            check_call([SCHEMA_SCRIPT, "--bootstrap"])
+            return True
         except CalledProcessError as e:
             logger.error(
                 "Landscape Server schema update failed with return code %d",
-                e.returncode)
-            self.unit.status = BlockedStatus(
-                "Failed to update database schema")
-            return
-
-        self._stored.ready["db"] = True
-        self.unit.status = ActiveStatus("Unit is ready")
-        self._update_ready_status()
+                e.returncode,
+            )
+            self.unit.status = BlockedStatus("Failed to update database schema")
 
     def _amqp_relation_joined(self, event: RelationJoinedEvent) -> None:
         self._stored.ready["amqp"] = False

=== modified file 'src/settings_files.py'
--- src/settings_files.py	2023-01-03 22:10:55 +0000
+++ src/settings_files.py	2023-02-06 16:13:35 +0000
@@ -7,6 +7,7 @@
 
 import os
 from base64 import b64decode, binascii
+from collections import defaultdict
 from configparser import ConfigParser
 from urllib.request import urlopen
 from urllib.error import URLError
@@ -23,6 +24,8 @@
 SERVICE_CONF = "/etc/landscape/service.conf"
 SSL_CERT_PATH = "/etc/ssl/certs/landscape_server_ca.crt"
 
+DEFAULT_POSTGRES_PORT = "5432"
+
 
 class LicenseFileReadException(Exception):
     pass
@@ -130,3 +133,17 @@
     except binascii.Error:
         raise SSLCertReadException(
             "Unable to decode b64-encoded SSL certificate")
+
+
+def update_db_conf(host=None, password=None, port=DEFAULT_POSTGRES_PORT, user=None):
+    """Postgres specific settings override"""
+    to_update = defaultdict(dict)
+    if host:  # Note that host is required if port is changed
+        to_update["stores"]["host"] = "{}:{}".format(host, port)
+    if password:
+        to_update["stores"]["password"] = password
+        to_update["schema"]["store_password"] = password
+    if user:
+        to_update["schema"]["store_user"] = user
+    if to_update:
+        update_service_conf(to_update)
\ No newline at end of file

=== modified file 'tests/test_charm.py'
--- tests/test_charm.py	2023-01-25 19:57:43 +0000
+++ tests/test_charm.py	2023-02-06 16:13:35 +0000
@@ -10,7 +10,7 @@
 from pwd import struct_passwd
 from subprocess import CalledProcessError
 from tempfile import TemporaryDirectory
-from unittest.mock import DEFAULT, Mock, patch
+from unittest.mock import DEFAULT, Mock, patch, call
 
 import yaml
 
@@ -302,29 +302,108 @@
                 "password": "testpass",
             },
         }
-        patches = patch.multiple(
-            "charm",
-            check_call=DEFAULT,
-            update_service_conf=DEFAULT,
-        )
 
-        with patches as mocks:
-            self.harness.charm._db_relation_changed(mock_event)
+        with patch("charm.check_call") as check_call_mock:
+            with patch(
+                "settings_files.update_service_conf"
+            ) as update_service_conf_mock:
+                self.harness.charm._db_relation_changed(mock_event)
 
         status = self.harness.charm.unit.status
         self.assertIsInstance(status, WaitingStatus)
         self.assertTrue(self.harness.charm._stored.ready["db"])
 
-        mocks["update_service_conf"].assert_called_once_with({
-            "stores": {
-                "host": "1.2.3.4:5678",
-                "password": "testpass",
-            },
-            "schema": {
-                "store_user": "testuser",
-                "store_password": "testpass",
-            },
-        })
+        update_service_conf_mock.assert_called_once_with(
+            {
+                "stores": {
+                    "host": "1.2.3.4:5678",
+                    "password": "testpass",
+                },
+                "schema": {
+                    "store_user": "testuser",
+                    "store_password": "testpass",
+                },
+            }
+        )
+
+    def test_db_manual_configs_used(self):
+        self.harness.disable_hooks()
+        self.harness.update_config(
+            {
+                "db_host": "hello",
+                "db_port": "world",
+                "db_user": "test",
+                "db_password": "test_pass",
+            }
+        )
+        mock_event = Mock()
+        mock_event.relation.data = {
+            mock_event.unit: {
+                "allowed-units": self.harness.charm.unit.name,
+                "master": "host=1.2.3.4 password=testpass",
+                "host": "1.2.3.4",
+                "port": "5678",
+                "user": "testuser",
+                "password": "testpass",
+            },
+        }
+
+        with patch("charm.check_call") as check_call_mock:
+            with patch(
+                "settings_files.update_service_conf"
+            ) as update_service_conf_mock:
+                self.harness.charm._db_relation_changed(mock_event)
+
+        update_service_conf_mock.assert_called_once_with(
+            {
+                "stores": {
+                    "host": "hello:world",
+                    "password": "test_pass",
+                },
+                "schema": {
+                    "store_user": "test",
+                    "store_password": "test_pass",
+                },
+            }
+        )
+
+    def test_db_manual_configs_used_partial(self):
+        """
+        Test that if some of the manual configs are provided, the rest are
+        gotten from the postgres unit
+        """
+        self.harness.disable_hooks()
+        self.harness.update_config({"db_host": "hello", "db_port": "world"})
+        mock_event = Mock()
+        mock_event.relation.data = {
+            mock_event.unit: {
+                "allowed-units": self.harness.charm.unit.name,
+                "master": "host=1.2.3.4 password=testpass",
+                "host": "1.2.3.4",
+                "port": "5678",
+                "user": "testuser",
+                "password": "testpass",
+            },
+        }
+
+        with patch("charm.check_call") as check_call_mock:
+            with patch(
+                "settings_files.update_service_conf"
+            ) as update_service_conf_mock:
+                self.harness.charm._db_relation_changed(mock_event)
+
+        update_service_conf_mock.assert_called_once_with(
+            {
+                "stores": {
+                    "host": "hello:world",
+                    "password": "testpass",
+                },
+                "schema": {
+                    "store_user": "testuser",
+                    "store_password": "testpass",
+                },
+            }
+        )
 
     def test_db_relation_changed_called_process_error(self):
         mock_event = Mock()
@@ -338,30 +417,97 @@
                 "password": "testpass",
             },
         }
-        patches = patch.multiple(
-            "charm",
-            check_call=DEFAULT,
-            update_service_conf=DEFAULT,
-        )
 
-        with patches as mocks:
-            mocks["check_call"].side_effect = CalledProcessError(127, "ouch")
-            self.harness.charm._db_relation_changed(mock_event)
+        with patch("charm.check_call") as check_call_mock:
+            with patch(
+                "settings_files.update_service_conf"
+            ) as update_service_conf_mock:
+                check_call_mock.side_effect = CalledProcessError(127, "ouch")
+                self.harness.charm._db_relation_changed(mock_event)
 
         status = self.harness.charm.unit.status
         self.assertIsInstance(status, BlockedStatus)
         self.assertFalse(self.harness.charm._stored.ready["db"])
 
-        mocks["update_service_conf"].assert_called_once_with({
-            "stores": {
-                "host": "1.2.3.4:5678",
-                "password": "testpass",
-            },
-            "schema": {
-                "store_user": "testuser",
-                "store_password": "testpass",
+        update_service_conf_mock.assert_called_once_with(
+            {
+                "stores": {
+                    "host": "1.2.3.4:5678",
+                    "password": "testpass",
+                },
+                "schema": {
+                    "store_user": "testuser",
+                    "store_password": "testpass",
+                },
             }
-        })
+        )
+
+    def test_on_manual_db_config_change(self):
+        """
+        Test that the manual db settings are reflected if a config change happens later
+        """
+
+        mock_event = Mock()
+        mock_event.relation.data = {
+            mock_event.unit: {
+                "allowed-units": self.harness.charm.unit.name,
+                "master": "host=1.2.3.4 password=testpass",
+                "host": "1.2.3.4",
+                "port": "5678",
+                "user": "testuser",
+                "password": "testpass",
+            },
+        }
+
+        with patch("charm.check_call") as check_call_mock:
+            with patch(
+                "settings_files.update_service_conf"
+            ) as update_service_conf_mock:
+                self.harness.charm._db_relation_changed(mock_event)
+                self.harness.update_config({"db_host": "hello", "db_port": "world"})
+
+        self.assertEqual(update_service_conf_mock.call_count, 2)
+        self.assertEqual(
+            update_service_conf_mock.call_args_list[1],
+            call(
+                {
+                    "stores": {
+                        "host": "hello:world",
+                    },
+                }
+            ),
+        )
+
+    def test_on_manual_db_config_change_block_if_error(self):
+        """
+        If the schema migration doesn't go through on a manual config change,
+        then block unit status
+        """
+        mock_event = Mock()
+        mock_event.relation.data = {
+            mock_event.unit: {
+                "allowed-units": self.harness.charm.unit.name,
+                "master": "host=1.2.3.4 password=testpass",
+                "host": "1.2.3.4",
+                "port": "5678",
+                "user": "testuser",
+                "password": "testpass",
+            },
+        }
+
+        with patch("charm.check_call") as check_call_mock:
+            with patch(
+                "settings_files.update_service_conf"
+            ) as update_service_conf_mock:
+                self.harness.charm._db_relation_changed(mock_event)
+
+        with patch("charm.check_call") as check_call_mock:
+            check_call_mock.side_effect = CalledProcessError(127, "ouch")
+            self.harness.update_config({"db_host": "hello", "db_port": "world"})
+
+        status = self.harness.charm.unit.status
+        self.assertIsInstance(status, BlockedStatus)
+
 
     def test_amqp_relation_joined(self):
         unit = self.harness.charm.unit