Merge branch 'feat/openstack-integration' into feat/private-endpoint

src-docs/openstack_manager.md

@@ -16,7 +16,7 @@ Module for handling interactions with OpenStack.
 
 ---
 
-<a href="../src/openstack_cloud/openstack_manager.py#L344"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L345"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>function</kbd> `build_image`
 
@@ -56,7 +56,7 @@ Build and upload an image to OpenStack.
 
 ---
 
-<a href="../src/openstack_cloud/openstack_manager.py#L403"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L404"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>function</kbd> `create_instance_config`
 
@@ -92,7 +92,7 @@ Create an instance config from charm data.
 
 ---
 
-<a href="../src/openstack_cloud/openstack_manager.py#L117"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L118"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>class</kbd> `InstanceConfig`
 The configuration values for creating a single runner instance. 
@@ -131,7 +131,7 @@ __init__(
 
 ---
 
-<a href="../src/openstack_cloud/openstack_manager.py#L192"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L193"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>class</kbd> `ProxyStringValues`
 Wrapper class to proxy values to string. 
@@ -150,7 +150,7 @@ Wrapper class to proxy values to string.
 
 ---
 
-<a href="../src/openstack_cloud/openstack_manager.py#L300"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L301"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>class</kbd> `OpenstackUpdateImageError`
 Represents an error while updating image on Openstack. 
@@ -161,7 +161,7 @@ Represents an error while updating image on Openstack.
 
 ---
 
-<a href="../src/openstack_cloud/openstack_manager.py#L497"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L498"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>class</kbd> `GithubRunnerRemoveError`
 Represents an error removing registered runner from Github. 
@@ -172,7 +172,7 @@ Represents an error removing registered runner from Github.
 
 ---
 
-<a href="../src/openstack_cloud/openstack_manager.py#L505"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L506"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ## <kbd>class</kbd> `OpenstackRunnerManager`
 Runner manager for OpenStack-based instances. 
@@ -185,7 +185,7 @@ Runner manager for OpenStack-based instances.
  - <b>`unit_num`</b>:  The juju unit number. 
  - <b>`instance_name`</b>:  Prefix of the name for the set of runners. 
 
-<a href="../src/openstack_cloud/openstack_manager.py#L514"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L515"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ### <kbd>method</kbd> `__init__`
 
@@ -214,7 +214,7 @@ Construct OpenstackRunnerManager object.
 
 ---
 
-<a href="../src/openstack_cloud/openstack_manager.py#L1320"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L1326"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ### <kbd>method</kbd> `flush`
 
@@ -231,7 +231,7 @@ Flush Openstack servers.
 
 ---
 
-<a href="../src/openstack_cloud/openstack_manager.py#L859"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L865"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ### <kbd>method</kbd> `get_github_runner_info`
 
@@ -248,7 +248,7 @@ Get information on GitHub for the runners.
 
 ---
 
-<a href="../src/openstack_cloud/openstack_manager.py#L1201"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
+<a href="../src/openstack_cloud/openstack_manager.py#L1207"><img align="right" style="float:right;" src="https://img.shields.io/badge/-source-cccccc?style=flat-square"></a>
 
 ### <kbd>method</kbd> `reconcile`
 

src/metrics/runner.py

@@ -146,13 +146,29 @@ def issue_events(
     Returns:
         A set of issued events.
     """
+    # When a job gets picked up directly after spawning, the runner_metrics installed timestamp
+    # might be higher than the pre-job timestamp. This is due to the fact that we issue the runner
+    # installed timestamp for Openstack after waiting with delays for the runner to be ready.
+    # We set the idle_duration to 0 in this case.
+    if (
+        logger.isEnabledFor(logging.WARNING)
+        and runner_metrics.pre_job.timestamp < runner_metrics.installed_timestamp
+    ):
+        logger.info(
+            "Pre-job timestamp %d is before installed timestamp %d for runner %s."
+            " Setting idle_duration to zero",
+            runner_metrics.pre_job.timestamp,
+            runner_metrics.installed_timestamp,
+            runner_metrics.runner_name,
+        )
+    idle_duration = max(runner_metrics.pre_job.timestamp - runner_metrics.installed_timestamp, 0)
     runner_start_event = metric_events.RunnerStart(
         timestamp=runner_metrics.pre_job.timestamp,
         flavor=flavor,
         workflow=runner_metrics.pre_job.workflow,
         repo=runner_metrics.pre_job.repository,
         github_event=runner_metrics.pre_job.event,
-        idle=runner_metrics.pre_job.timestamp - runner_metrics.installed_timestamp,
+        idle=idle_duration,
         queue_duration=job_metrics.queue_duration if job_metrics else None,
     )
     try:

src/openstack_cloud/openstack_manager.py

@@ -49,6 +49,7 @@
 from errors import (
     CreateMetricsStorageError,
     GetMetricsStorageError,
+    GithubApiError,
     GithubClientError,
     GithubMetricsError,
     IssueMetricEventError,
@@ -812,6 +813,11 @@ def _create_runner(args: _CreateRunnerArgs) -> None:
                 raise RunnerCreateError(
                     f"Timeout creating OpenStack runner {instance_config.name}"
                 ) from err
+            except openstack.exceptions.SDKException as err:
+                logger.exception("Failed to create OpenStack runner %s", instance_config.name)
+                raise RunnerCreateError(
+                    f"Failed to create OpenStack runner {instance_config.name}"
+                ) from err
 
             logger.info("Waiting runner %s to come online", instance_config.name)
             OpenstackRunnerManager._wait_until_runner_process_running(conn, instance.name)
@@ -1509,8 +1515,21 @@ def _issue_runner_metrics(self, runner_states: RunnerByHealth) -> IssuedMetricEv
             The stats of issued metric events.
         """
         total_stats: IssuedMetricEventsStats = {}
+        try:
+            online_runners = {
+                runner_info.runner_name
+                for runner_info in self.get_github_runner_info()
+                if runner_info.online
+            }
+        except GithubApiError:
+            logger.exception(
+                "Failed to retrieve set of online runners. Will not issue runner metrics"
+            )
+            return total_stats
+
         for extracted_metrics in runner_metrics.extract(
-            metrics_storage_manager=metrics_storage, ignore_runners=set(runner_states.healthy)
+            metrics_storage_manager=metrics_storage,
+            ignore_runners=set(runner_states.healthy) | online_runners,
         ):
             try:
                 job_metrics = github_metrics.job(
@@ -1546,7 +1565,15 @@ def _issue_reconciliation_metric(
             reconciliation_end_ts: The timestamp of when reconciliation ended.
             runner_states: The states of the runners.
         """
-        github_info = self.get_github_runner_info()
+        try:
+            github_info = self.get_github_runner_info()
+        except GithubApiError:
+            logger.exception(
+                "Failed to retrieve github info for reconciliation metric. "
+                "Will not issue reconciliation metric."
+            )
+            return
+
         online_runners = [runner for runner in github_info if runner.online]
         offline_runner_names = {runner.runner_name for runner in github_info if not runner.online}
         active_runner_names = {runner.runner_name for runner in online_runners if runner.busy}

tests/conftest.py

@@ -124,3 +124,12 @@ def pytest_addoption(parser: Parser):
         help="The Openstack region to authenticate to.",
         default=None,
     )
+    parser.addoption(
+        "--use-existing-app",
+        action="store",
+        help="The existing app to use."
+        "This will skip deployment of the charm and use the existing app."
+        "This option is useful for local testing."
+        "It is expected that the existing app is already integrated with other apps "
+        "like grafana-agent, etc. ",
+    )

tests/unit/metrics/test_runner.py

@@ -537,6 +537,41 @@ def test_issue_events(issue_event_mock: MagicMock):
     )
 
 
+def test_issue_events_pre_job_before_runner_installed(issue_event_mock: MagicMock):
+    """
+    arrange: A runner with pre-job timestamp smaller than installed timestamp.
+    act: Call issue_events.
+    assert: RunnerStart metric is issued with idle set to 0.
+    """
+    runner_name = secrets.token_hex(16)
+    runner_metrics_data = _create_metrics_data(runner_name)
+    runner_metrics_data.pre_job.timestamp = 0
+
+    flavor = secrets.token_hex(16)
+    job_metrics = metrics_type.GithubJobMetrics(
+        queue_duration=3600, conclusion=JobConclusion.SUCCESS
+    )
+    issued_metrics = runner_metrics.issue_events(
+        runner_metrics=runner_metrics_data, flavor=flavor, job_metrics=job_metrics
+    )
+    assert metric_events.RunnerStart in issued_metrics
+    issue_event_mock.assert_has_calls(
+        [
+            call(
+                RunnerStart(
+                    timestamp=runner_metrics_data.pre_job.timestamp,
+                    flavor=flavor,
+                    workflow=runner_metrics_data.pre_job.workflow,
+                    repo=runner_metrics_data.pre_job.repository,
+                    github_event=runner_metrics_data.pre_job.event,
+                    idle=0,
+                    queue_duration=job_metrics.queue_duration,
+                )
+            )
+        ]
+    )
+
+
 def test_issue_events_post_job_before_pre_job(issue_event_mock: MagicMock):
     """
     arrange: A runner with post-job timestamp smaller than pre-job timestamps.

tests/unit/test_openstack_manager.py

@@ -11,6 +11,7 @@
 from invoke import Result
 from openstack.compute.v2.keypair import Keypair
 
+import metrics.storage
 from charm_state import CharmState, ProxyConfig, RepoPolicyComplianceConfig
 from errors import OpenStackError
 from github_type import GitHubRunnerStatus, SelfHostedRunner
@@ -19,7 +20,7 @@
 from metrics.storage import MetricsStorage
 from openstack_cloud import openstack_manager
 from openstack_cloud.openstack_manager import MAX_METRICS_FILE_SIZE
-from runner_type import RunnerByHealth
+from runner_type import RunnerByHealth, RunnerGithubInfo
 
 CLOUD_NAME = "microstack"
 
@@ -689,7 +690,7 @@ def test_reconcile_issue_reconciliation_metrics(
 ):
     """
     arrange: Mock the metrics storage and the ssh connection.
-    act: Reconcile to create a runner.
+    act: Reconcile.
     assert: The expected reconciliation metrics are issued.
     """
     runner_metrics_path = tmp_path / "runner_fs"
@@ -734,6 +735,50 @@ def test_reconcile_issue_reconciliation_metrics(
     )
 
 
+def test_reconcile_ignores_metrics_for_healthy_and_online_runners(
+    openstack_manager_for_reconcile, monkeypatch, tmp_path
+):
+    """
+    arrange: Combination of runner status and github online status.
+    act: Call reconcile.
+    assert: The healthy and online runners are ignored.
+    """
+    runner_metrics_path = tmp_path / "runner_fs"
+    runner_metrics_path.mkdir()
+    ms = MetricsStorage(path=runner_metrics_path, runner_name="test_runner")
+    monkeypatch.setattr(openstack_manager.metrics_storage, "create", MagicMock(return_value=ms))
+    monkeypatch.setattr(openstack_manager.metrics_storage, "get", MagicMock(return_value=ms))
+    openstack_manager_for_reconcile._get_openstack_runner_status = MagicMock(
+        return_value=RunnerByHealth(
+            healthy=("healthy_online", "healthy_offline"),
+            unhealthy=("unhealthy_online", "unhealthy_offline"),
+        )
+    )
+    openstack_manager_for_reconcile.get_github_runner_info = MagicMock(
+        return_value=(
+            RunnerGithubInfo(runner_name="healthy_online", runner_id=0, online=True, busy=True),
+            RunnerGithubInfo(runner_name="unhealthy_online", runner_id=1, online=True, busy=False),
+            RunnerGithubInfo(runner_name="healthy_offline", runner_id=2, online=False, busy=False),
+            RunnerGithubInfo(
+                runner_name="unhealthy_offline", runner_id=3, online=False, busy=False
+            ),
+        )
+    )
+
+    openstack_manager.runner_metrics.extract.return_value = (MagicMock() for _ in range(2))
+    openstack_manager.runner_metrics.issue_events.side_effect = [
+        {metric_events.RunnerStart, metric_events.RunnerStop},
+        {metric_events.RunnerStart},
+    ]
+
+    openstack_manager_for_reconcile.reconcile(quantity=0)
+
+    openstack_manager.runner_metrics.extract.assert_called_once_with(
+        metrics_storage_manager=metrics.storage,
+        ignore_runners={"healthy_online", "healthy_offline", "unhealthy_online"},
+    )
+
+
 def test_repo_policy_config(
     openstack_manager_for_reconcile: openstack_manager.OpenstackRunnerManager,
     monkeypatch: pytest.MonkeyPatch,