chore: image bulider followup (#315)

* update TODO comments w/ ISO date

* fix: error log w/ server address

* fix: on start image not yet ready

* fix: lint fixes

* test: increase timeoout

* chore: match arg order

* chore: update status message

pyproject.toml

@@ -15,7 +15,7 @@ omit = [
     # Contains interface for calling repo policy compliance service. Tested in integration test
     # and end to end tests.
     "src/repo_policy_compliance_client.py",
-    # 2024/04/17: These files are pending a major refactor. The refactor includes a RunnerManager
+    # TODO: 2024-04-17: These files are pending a major refactor. The refactor includes a RunnerManager
     # interface class which will include a complete re-organization of the code in these files.
     "src/runner.py",
     "src/runner_manager.py",
@@ -33,7 +33,10 @@ markers = [
 ]
 
 [tool.pylint.'MESSAGES CONTROL']
+# see https://github.com/pydantic/pydantic/issues/1961#issuecomment-759522422
 extension-pkg-whitelist = "pydantic"
+# Ignore W0511 TODO comments because the functions are subject to refactor.
+disable = "W0511"
 
 # Formatting tools configuration
 [tool.black]

requirements.txt

@@ -3,7 +3,7 @@ jinja2
 fabric >=3,<4
 openstacksdk>=3,<4
 ops>=2.8
-pylxd @ git+https://github.com/canonical/pylxd@46b58e61a465a970937cd97bbbf93622f98caa8c
+pylxd @ git+https://github.com/canonical/pylxd
 requests
 typing-extensions
 cryptography <=42.0.5

src/charm.py

@@ -3,7 +3,7 @@
 # Copyright 2024 Canonical Ltd.
 # See LICENSE file for licensing details.
 
-# 2024/03/12 The module contains too many lines which are scheduled for refactoring.
+# TODO: 2024-03-12 The module contains too many lines which are scheduled for refactoring.
 # pylint: disable=too-many-lines
 
 """Charm for creating and managing GitHub self-hosted runner instances."""
@@ -471,6 +471,8 @@ def _on_start(self, _: StartEvent) -> None:
         state = self._setup_state()
 
         if state.instance_type == InstanceType.OPENSTACK:
+            if not self._get_set_image_ready_status():
+                return
             openstack_runner_manager = self._get_openstack_runner_manager(state)
             openstack_runner_manager.reconcile(state.runner_config.virtual_machines)
             self.unit.status = ActiveStatus()
@@ -597,7 +599,7 @@ def _on_config_changed(self, _: ConfigChangedEvent) -> None:  # noqa: C901
                 openstack_runner_manager = self._get_openstack_runner_manager(state)
                 openstack_runner_manager.flush()
                 openstack_runner_manager.reconcile(state.runner_config.virtual_machines)
-                # 2024/04/12: Flush on token changes.
+                # TODO: 2024-04-12: Flush on token changes.
                 self.unit.status = ActiveStatus()
             return
 
@@ -1111,7 +1113,7 @@ def _on_debug_ssh_relation_changed(self, _: ops.RelationChangedEvent) -> None:
             if not self._get_set_image_ready_status():
                 return
             runner_manager = self._get_openstack_runner_manager(state)
-            # 2024/04/12: Should be flush idle.
+            # TODO: 2024-04-12: Should be flush idle.
             runner_manager.flush()
             runner_manager.reconcile(state.runner_config.virtual_machines)
             return
@@ -1131,12 +1133,15 @@ def _on_image_relation_changed(self, _: ops.RelationChangedEvent) -> None:
         state = self._setup_state()
 
         if state.instance_type != InstanceType.OPENSTACK:
+            self.unit.status = BlockedStatus(
+                "Openstack mode not enabled. Please remove the image integration."
+            )
             return
         if not self._get_set_image_ready_status():
             return
 
         runner_manager = self._get_openstack_runner_manager(state)
-        # 2024/04/12: Should be flush idle.
+        # TODO: 2024-04-12: Should be flush idle.
         runner_manager.flush()
         runner_manager.reconcile(state.runner_config.virtual_machines)
         self.unit.status = ActiveStatus()
@@ -1162,7 +1167,8 @@ def _get_openstack_runner_manager(
     ) -> OpenstackRunnerManager:
         """Get OpenstackRunnerManager instance.
 
-        TODO: Combine this with `_get_runner_manager` during the runner manager interface refactor.
+        TODO: 2024-07-09 Combine this with `_get_runner_manager` during the runner manager \
+        interface refactor.
 
         Args:
             state: Charm state.
@@ -1181,7 +1187,8 @@ def _get_openstack_runner_manager(
 
         # Empty image can be passed down due to a delete only case where deletion of runners do not
         # depend on the image ID being available. Make sure that the charm goes to blocked status
-        # in hook where a runner may be created. TODO: This logic is subject to refactoring.
+        # in hook where a runner may be created. TODO: 2024-07-09 This logic is subject to
+        # refactoring.
         image = state.runner_config.openstack_image
         image_id = image.id if image and image.id else ""
         image_labels = image.tags if image and image.tags else []

src/charm_state.py

@@ -1,7 +1,7 @@
 # Copyright 2024 Canonical Ltd.
 #  See LICENSE file for licensing details.
 
-# 2024/06/26 The charm contains a lot of states and configuration. The upcoming refactor will
+# TODO: 2024-06-26 The charm contains a lot of states and configuration. The upcoming refactor will
 # split each/related class to a file.
 # pylint: disable=too-many-lines
 

src/lxd.py

@@ -107,7 +107,7 @@ def write_file(
 
             try:
                 self.push_file(file.name, filepath, mode)
-            # 2024/04/02 - We should define a new error, wrap it and re-raise it.
+            # TODO: 2024-04-02 - We should define a new error, wrap it and re-raise it.
             except LxdError:  # pylint: disable=try-except-raise
                 raise
 
@@ -154,7 +154,7 @@ def read_file(self, filepath: str) -> str:
         with tempfile.NamedTemporaryFile() as file:
             try:
                 self.pull_file(filepath, file.name)
-            # 2024/04/02 - We should define a new error, wrap it and re-raise it.
+            # TODO: 2024-04-02 - We should define a new error, wrap it and re-raise it.
             except LxdError:  # pylint: disable=try-except-raise
                 raise
 

src/metrics/events.py

@@ -191,7 +191,7 @@ def _enable_logrotate() -> None:
             execute_command(
                 [SYSTEMCTL_PATH, "start", LOG_ROTATE_TIMER_SYSTEMD_SERVICE], check_exit=True
             )
-    # 2024/04/02 - We should define a new error, wrap it and re-raise it.
+    # TODO: 2024-04-02 - We should define a new error, wrap it and re-raise it.
     except SubprocessError:  # pylint: disable=try-except-raise
         raise
 

src/metrics/runner.py

@@ -372,7 +372,7 @@ def _extract_metrics_from_storage(metrics_storage: MetricsStorage) -> Optional[R
             metrics_storage=metrics_storage, filename=POST_JOB_METRICS_FILE_NAME
         )
         logger.debug("Post-job metrics for runner %s: %s", runner_name, post_job_metrics)
-    # 2024/04/02 - We should define a new error, wrap it and re-raise it.
+    # TODO: 2024-04-02 - We should define a new error, wrap it and re-raise it.
     except CorruptMetricDataError:  # pylint: disable=try-except-raise
         raise
 

src/openstack_cloud/__init__.py

@@ -120,7 +120,7 @@ def initialize(cloud_config: dict) -> None:
     """
     try:
         valid_config = _validate_cloud_config(cloud_config)
-    # 2024/04/02 - We should define a new error, wrap it and re-raise it.
+    # TODO: 2024-04-02 - We should define a new error, wrap it and re-raise it.
     except OpenStackInvalidConfigError:  # pylint: disable=try-except-raise
         raise
     _write_config_to_disk(valid_config)

src/openstack_cloud/openstack_manager.py

@@ -1,10 +1,10 @@
 # Copyright 2024 Canonical Ltd.
 # See LICENSE file for licensing details.
 
-# 2024/04/11 The module contains too many lines which are scheduled for refactoring.
+# TODO: 2024-04-11 The module contains too many lines which are scheduled for refactoring.
 # pylint: disable=too-many-lines
 
-# 2024/04/22 The module contains duplicate code which is scheduled for refactoring.
+# TODO: 2024-04-22 The module contains duplicate code which is scheduled for refactoring.
 # Lines related to issuing metrics are duplicated:
 #  ==openstack_cloud.openstack_manager:[1320:1337]
 #  ==runner_manager:[383:413]
@@ -576,7 +576,7 @@ def _get_ssh_connection(
                 continue
         raise _SSHError(
             f"No connectable SSH addresses found, server: {server.name}, "
-            "addresses: {server_addresses}"
+            f"addresses: {server_addresses}"
         )
 
     @staticmethod
@@ -649,8 +649,8 @@ def _create_runner(args: _CreateRunnerArgs) -> None:
             instance_config=instance_config,
             runner_env=env_contents,
             pre_job_contents=pre_job_contents,
-            proxies=args.config.charm_state.proxy_config,
             dockerhub_mirror=args.config.dockerhub_mirror,
+            proxies=args.config.charm_state.proxy_config,
         )
         cloud_userdata_str = _generate_cloud_init_userdata(
             templates_env=environment,
@@ -986,7 +986,7 @@ def _remove_one_runner(
                 self._github.delete_runner(self._config.path, github_id)
             except GithubClientError as exc:
                 logger.warning("Failed to remove runner from Github %s, %s", instance_name, exc)
-            # 2024/04/23: The broad except clause is for logging purposes.
+            # TODO: 2024-04-23: The broad except clause is for logging purposes.
             # Will be removed in future versions.
             except Exception:  # pylint: disable=broad-exception-caught
                 logger.critical(
@@ -1108,7 +1108,7 @@ def _remove_openstack_runner(
             logger.warning(
                 "Failed to run runner removal script for %s", server.name, exc_info=True
             )
-        # 2024/04/23: The broad except clause is for logging purposes.
+        # TODO: 2024-04-23: The broad except clause is for logging purposes.
         # Will be removed in future versions.
         except Exception:  # pylint: disable=broad-exception-caught
             logger.critical(
@@ -1119,7 +1119,7 @@ def _remove_openstack_runner(
                 logger.warning("Server does not exist %s", server.name)
         except SDKException as exc:
             logger.error("Something wrong deleting the server %s, %s", server.name, exc)
-        # 2024/04/23: The broad except clause is for logging purposes.
+        # TODO: 2024-04-23: The broad except clause is for logging purposes.
         # Will be removed in future versions.
         except Exception:  # pylint: disable=broad-exception-caught
             logger.critical(
@@ -1172,7 +1172,7 @@ def _run_github_removal_script(
                     result.stderr,
                 )
             return
-        # 2024/04/23: The broad except clause is for logging purposes.
+        # TODO: 2024-04-23: The broad except clause is for logging purposes.
         # Will be removed in future versions.
         except Exception:  # pylint: disable=broad-exception-caught
             logger.critical(

src/runner.py

@@ -739,7 +739,7 @@ def _configure_runner(self) -> None:
         )
         try:
             self._put_file(str(self.runner_script), startup_contents, mode="0755")
-        # 2024/04/02 - We should define a new error, wrap it and re-raise it.
+        # TODO: 2024-04-02 - We should define a new error, wrap it and re-raise it.
         except RunnerFileLoadError:  # pylint: disable=try-except-raise
             raise
         self.instance.execute(["/usr/bin/sudo", "chown", "ubuntu:ubuntu", str(self.runner_script)])

tests/integration/helpers/common.py

@@ -293,7 +293,7 @@ async def deploy_github_runner_charm(
     )
 
     if wait_idle:
-        await model.wait_for_idle(status=ACTIVE, timeout=60 * 30)
+        await model.wait_for_idle(status=ACTIVE, timeout=60 * 40)
 
     return application
 
@@ -454,6 +454,7 @@ async def wait_for(
         else:
             if result := func():
                 return cast(R, result)
+        logger.info("Wait for condition not met, sleeping %s", check_interval)
         time.sleep(check_interval)
 
     # final check before raising TimeoutError.

tests/unit/test_charm.py

@@ -848,7 +848,7 @@ def test__on_image_relation_changed_lxd():
     harness.charm._on_image_relation_changed(MagicMock())
 
     # the unit is in maintenance status since nothing has happened.
-    assert harness.charm.unit.status.name == MaintenanceStatus.name
+    assert harness.charm.unit.status.name == BlockedStatus.name
 
 
 def test__on_image_relation_image_not_ready():