Merge ~silverdrake11/landscape-charm:install-and-bootstrap-bug_lndeng-466 into landscape-charm:main

[Kevin Nasto]

Fixed install/retry/config bug. Fixed bootstrap account on start. Fixed broken tests

Manual testing instructions:

Install race:

Use the following code to trigger a failure in the install and make sure it first errors then resolves itself

import random
num = random.randint(1,3)
if num == 3:
    logger.error("SUCCESS!!!!!")
    check_call(["add-apt-repository", "-y", landscape_ppa])
else:
    logger.error("SOMETHING WENT WRONG {}".format(num))

Bootstrap bug:
Make sure that filling out these fields before deploying in the bundle, creates the admin

admin_email:
admin_name:
admin_password:

[Kevin Nasto (silverdrake11)]

I learned that Juju's exponential retry behavior can be triggered by raising an exception. Some things are useful for this such as this install bug/ race condition. The install initially fails but will resolve itself when it's ready. Other things are not useful for this, like erroring out on a mistake in the config (retrying doesn't change the config value). So because of this, raising an exception can be useful for other charm functions in the future or when needed.

[Kevin Nasto (silverdrake11)]

See diff comment

[Mitch Burton (mitchburton)]

+1 LGTM. Test based on RNG is not the world's best testing experience but it only took me three tries.

review: Approve

[Kevin Nasto (silverdrake11)]

> +1 LGTM. Test based on RNG is not the world's best testing experience but it
> only took me three tries.

Third time's the charm

diff --git a/src/charm.py b/src/charm.py
index d883217..313208b 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -204,8 +204,6 @@ class LandscapeServerCharm(CharmBase):
                 "package-upload": {"root-url": root_url},
             })
 
-        self._bootstrap_account()
-
         config_host = self.model.config.get("db_host")
         schema_password = self.model.config.get("db_schema_password")
         landscape_password = self.model.config.get("db_landscape_password")
@@ -230,6 +228,8 @@ class LandscapeServerCharm(CharmBase):
             else:
                 return
 
+        self._bootstrap_account()
+
         if isinstance(prev_status, BlockedStatus):
             self.unit.status = prev_status
 
@@ -245,22 +245,9 @@ class LandscapeServerCharm(CharmBase):
             # Add the Landscape Server PPA and install via apt.
             check_call(["add-apt-repository", "-y", landscape_ppa])
             apt.add_package(["landscape-server", "landscape-hashids"])
-        except PackageNotFoundError:
-            logger.error("Landscape package not found in package cache "
-                         "or on system")
-            self.unit.status = BlockedStatus("Failed to install packages")
-            return
-        except PackageError as e:
-            logger.error(
-                "Could not install landscape-server package. Reason: %s",
-                e.message)
-            self.unit.status = BlockedStatus("Failed to install packages")
-            return
-        except CalledProcessError as e:
-            logger.error("Package install failed with return code %d",
-                         e.returncode)
-            self.unit.status = BlockedStatus("Failed to install packages")
-            return
+        except (PackageNotFoundError, PackageError, CalledProcessError) as exc:
+            logger.error("Failed to install packages")
+            raise exc  # This will trigger juju's exponential retry
 
         # Write the config-provided SSL certificate, if it exists.
         config_ssl_cert = self.model.config["ssl_cert"]
@@ -417,10 +404,12 @@ class LandscapeServerCharm(CharmBase):
     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
+        databases along with the landscape user exists. In addition creates
+        admin if configured. Returns True on success
         """
         try:
             check_call([SCHEMA_SCRIPT, "--bootstrap"])
+            self._bootstrap_account()
             return True
         except CalledProcessError as e:
             logger.error(
diff --git a/src/settings_files.py b/src/settings_files.py
index 6eb7231..ba8211d 100644
--- a/src/settings_files.py
+++ b/src/settings_files.py
@@ -37,6 +37,10 @@ class SSLCertReadException(Exception):
     pass
 
 
+class ServiceConfMissing(Exception):
+    pass
+
+
 def configure_for_deployment_mode(mode: str) -> None:
     """
     Places files where Landscape expects to find them for different deployment
@@ -111,6 +115,11 @@ def update_service_conf(updates: dict) -> None:
     `updates` is a mapping of {section => {key => value}}, to be applied
         to the config file.
     """
+    if not os.path.isfile(SERVICE_CONF):
+        # Landscape server will not overwrite this file on install, so we
+        # cannot get the default values if we create it here
+        raise ServiceConfMissing("Landscape server install failed!")
+
     config = ConfigParser()
     config.read(SERVICE_CONF)
 
diff --git a/tests/test_charm.py b/tests/test_charm.py
index 08a4119..55a139b 100644
--- a/tests/test_charm.py
+++ b/tests/test_charm.py
@@ -95,11 +95,8 @@ class TestCharm(unittest.TestCase):
 
         with patches as mocks:
             mocks["apt"].add_package.side_effect = PackageNotFoundError
-            harness.begin_with_initial_hooks()
-
-        status = harness.charm.unit.status
-        self.assertIsInstance(status, BlockedStatus)
-        self.assertEqual(status.message, "Failed to install packages")
+            self.assertRaises(PackageNotFoundError,
+                harness.begin_with_initial_hooks)
 
     def test_install_package_error(self):
         harness = Harness(LandscapeServerCharm)
@@ -116,11 +113,7 @@ class TestCharm(unittest.TestCase):
 
         with patches as mocks:
             mocks["apt"].add_package.side_effect = PackageError("ouch")
-            harness.begin_with_initial_hooks()
-
-        status = harness.charm.unit.status
-        self.assertIsInstance(status, BlockedStatus)
-        self.assertEqual(status.message, "Failed to install packages")
+            self.assertRaises(PackageError, harness.begin_with_initial_hooks)
 
     def test_install_called_process_error(self):
         harness = Harness(LandscapeServerCharm)
@@ -131,11 +124,8 @@ class TestCharm(unittest.TestCase):
         with patch("charm.check_call") as mock:
             with patch("charm.update_service_conf"):
                 mock.side_effect = CalledProcessError(127, Mock())
-                harness.begin_with_initial_hooks()
-
-        status = harness.charm.unit.status
-        self.assertIsInstance(status, BlockedStatus)
-        self.assertEqual(status.message, "Failed to install packages")
+                self.assertRaises(CalledProcessError, 
+                    harness.begin_with_initial_hooks)
 
     def test_install_ssl_cert(self):
         harness = Harness(LandscapeServerCharm)
@@ -188,20 +178,28 @@ class TestCharm(unittest.TestCase):
 
     def test_install_license_file_b64(self):
         harness = Harness(LandscapeServerCharm)
-        harness.update_config({"license_file": "VEhJUyBJUyBBIExJQ0VOU0U="})
+        license_text = "VEhJUyBJUyBBIExJQ0VOU0U"
+        harness.update_config({"license_file": license_text})
         relation_id = harness.add_relation("replicas", "landscape-server")
         harness.update_relation_data(
             relation_id, "landscape-server", {"leader-ip": "test"})
 
         with patch.multiple(
                 "charm",
+                apt=DEFAULT,
+                check_call=DEFAULT,
                 update_service_conf=DEFAULT,
+                prepend_default_settings=DEFAULT,
                 write_license_file=DEFAULT,
         ) as mocks:
             harness.begin_with_initial_hooks()
 
-        mocks["write_license_file"].assert_called_once_with(
-            "VEhJUyBJUyBBIExJQ0VOU0U=", 1000, 1000)
+        mock_write = mocks["write_license_file"]
+        self.assertEqual(len(mock_write.mock_calls), 2)
+        self.assertEqual(mock_write.mock_calls[0].args,
+            (license_text, 1000, 1000))
+        self.assertEqual(mock_write.mock_calls[1].args,
+            (license_text, 1000, 1000))
 
     def test_update_ready_status_not_running(self):
         self.harness.charm.unit.status = WaitingStatus()
diff --git a/tests/test_settings_files.py b/tests/test_settings_files.py
index e16f8f7..2b1321c 100644
--- a/tests/test_settings_files.py
+++ b/tests/test_settings_files.py
@@ -210,9 +210,11 @@ class UpdateServiceConfTestCase(TestCase):
             i += 1
             return retval
 
-        with patch("builtins.open") as open_mock:
-            open_mock.side_effect = return_conf
-            update_service_conf({"test": {"new": "yes"}})
+        with patch("os.path.isfile") as mock_isfile:
+            with patch("builtins.open") as open_mock:
+                mock_isfile.return_value = True
+                open_mock.side_effect = return_conf
+                update_service_conf({"test": {"new": "yes"}})
 
         self.assertEqual(outfile.captured,
                          "[fixed]\nold = no\n\n[test]\nnew = yes\n\n")