fix: Specify compute api dynamically (#591)

* specify compute api dynamically

* specify max nova compute api to be 2.91

* update changelog

* lint

* remove test_runner_manager_openstack.py

Author: cbartz

docs/changelog.md

@@ -2,6 +2,11 @@
 
 This changelog documents user-relevant changes to the GitHub runner charm.
 
+## 2025-07-09
+
+- Specify max supported nova compute api to be 2.91. This fixes an issue where the charm could fail
+ due to a bug on the OpenStack side: https://bugs.launchpad.net/nova/+bug/2095364
+
 ### 2025-06-30
 - New configuration options aproxy-exclude-addresses and aproxy-redirect-ports for allowing aproxy to redirect arbitrary TCP traffic
 - Added prometheus metrics to the GitHub runner manager application.

github-runner-manager/src/github_runner_manager/openstack_cloud/openstack_cloud.py

@@ -39,6 +39,11 @@
 
 _SSH_TIMEOUT = 30
 _TEST_STRING = "test_string"
+# Max nova compute we support is 2.91, because
+# - 2.96 has a bug with server list  https://bugs.launchpad.net/nova/+bug/2095364
+# - 2.92 requires public key to be set in the keypair, which is not supported by the app
+#        https://docs.openstack.org/api-ref/compute/#import-or-create-keypair
+_MAX_NOVA_COMPUTE_API_VERSION = "2.91"
 
 SecurityRuleDict = dict[str, Any]
 
@@ -153,33 +158,6 @@ def exception_handling_wrapper(*args: P.args, **kwargs: P.kwargs) -> T:
     return exception_handling_wrapper
 
 
-@contextmanager
-def _get_openstack_connection(credentials: OpenStackCredentials) -> Iterator[OpenstackConnection]:
-    """Create a connection context managed object, to be used within with statements.
-
-    Using the context manager ensures that the connection is properly closed after use.
-
-    Args:
-        credentials: The OpenStack authorization information.
-
-    Yields:
-        An openstack.connection.Connection object.
-    """
-    # api documents that keystoneauth1.exceptions.MissingRequiredOptions can be raised but
-    # I could not reproduce it. Therefore, no catch here for such exception.
-    with openstack.connect(
-        auth_url=credentials.auth_url,
-        project_name=credentials.project_name,
-        username=credentials.username,
-        password=credentials.password,
-        region_name=credentials.region_name,
-        user_domain_name=credentials.user_domain_name,
-        project_domain_name=credentials.project_domain_name,
-    ) as conn:
-        conn.authorize()
-        yield conn
-
-
 class OpenstackCloud:
     """Client to interact with OpenStack cloud.
 
@@ -237,7 +215,7 @@ def launch_instance(
         instance_id = runner_identity.instance_id
         metadata = runner_identity.metadata
 
-        with _get_openstack_connection(credentials=self._credentials) as conn:
+        with self._get_openstack_connection() as conn:
             security_group = OpenstackCloud._ensure_security_group(conn, ingress_tcp_ports)
             keypair = self._setup_keypair(conn, runner_identity.instance_id)
             meta = metadata.as_dict()
@@ -283,7 +261,7 @@ def get_instance(self, instance_id: InstanceID) -> OpenstackInstance | None:
         """
         logger.info("Getting openstack server with %s", instance_id)
 
-        with _get_openstack_connection(credentials=self._credentials) as conn:
+        with self._get_openstack_connection() as conn:
             server: OpenstackServer = conn.get_server(name_or_id=instance_id.name)
             if server is not None:
                 return OpenstackInstance(server, self.prefix)
@@ -298,7 +276,7 @@ def delete_instance(self, instance_id: InstanceID) -> None:
         """
         logger.info("Deleting openstack server with %s", instance_id)
 
-        with _get_openstack_connection(credentials=self._credentials) as conn:
+        with self._get_openstack_connection() as conn:
             self._delete_instance(conn, instance_id)
 
     def _delete_instance(self, conn: OpenstackConnection, instance_id: InstanceID) -> None:
@@ -400,7 +378,7 @@ def get_instances(self) -> tuple[OpenstackInstance, ...]:
         """
         logger.info("Getting all openstack servers managed by the charm")
 
-        with _get_openstack_connection(credentials=self._credentials) as conn:
+        with self._get_openstack_connection() as conn:
             instance_list = list(self._get_openstack_instances(conn))
             server_names = set(server.name for server in instance_list)
 
@@ -417,7 +395,7 @@ def get_instances(self) -> tuple[OpenstackInstance, ...]:
     @_catch_openstack_errors
     def cleanup(self) -> None:
         """Cleanup unused key files and openstack keypairs."""
-        with _get_openstack_connection(credentials=self._credentials) as conn:
+        with self._get_openstack_connection() as conn:
             instances = self._get_openstack_instances(conn)
             exclude_keyfiles_set = {
                 self._get_key_path(InstanceID.build_from_name(self.prefix, server.name))
@@ -650,6 +628,86 @@ def _ensure_security_group(
             )
         return security_group
 
+    @contextmanager
+    def _get_openstack_connection(self) -> Iterator[OpenstackConnection]:
+        """Create a connection context managed object, to be used within with statements.
+
+        Using the context manager ensures that the connection is properly closed after use.
+
+        Yields:
+            An openstack.connection.Connection object.
+        """
+        # api documents that keystoneauth1.exceptions.MissingRequiredOptions can be raised but
+        # I could not reproduce it. Therefore, no catch here for such exception.
+
+        with openstack.connect(
+            auth_url=self._credentials.auth_url,
+            project_name=self._credentials.project_name,
+            username=self._credentials.username,
+            password=self._credentials.password,
+            region_name=self._credentials.region_name,
+            user_domain_name=self._credentials.user_domain_name,
+            project_domain_name=self._credentials.project_domain_name,
+            compute_api_version=self._max_compute_api_version,
+        ) as conn:
+            conn.authorize()
+            yield conn
+
+    @functools.cached_property
+    def _max_compute_api_version(self) -> str:
+        """Determine the maximum compute API version supported by the client.
+
+        The sdk does not support versions greater than 2.95, so we need to ensure that the
+        maximum version returned by the OpenStack cloud is not greater than that.
+        https://bugs.launchpad.net/nova/+bug/2095364
+
+        Returns:
+            The maximum compute API version to use for the client.
+        """
+        max_version = self._determine_max_compute_api_version_by_cloud()
+        if self._version_greater_than(max_version, _MAX_NOVA_COMPUTE_API_VERSION):
+            logger.warning(
+                "The maximum compute API version %s is greater than the supported version %s. "
+                "Using the maximum supported version.",
+                max_version,
+                _MAX_NOVA_COMPUTE_API_VERSION,
+            )
+            return _MAX_NOVA_COMPUTE_API_VERSION
+        return max_version
+
+    def _determine_max_compute_api_version_by_cloud(self) -> str:
+        """Determine the maximum compute API version supported by the OpenStack cloud.
+
+        Returns:
+            The maximum compute API version as a string.
+        """
+        with openstack.connect(
+            auth_url=self._credentials.auth_url,
+            project_name=self._credentials.project_name,
+            username=self._credentials.username,
+            password=self._credentials.password,
+            region_name=self._credentials.region_name,
+            user_domain_name=self._credentials.user_domain_name,
+            project_domain_name=self._credentials.project_domain_name,
+        ) as conn:
+            version_endpoint = conn.compute.get_endpoint()
+            resp = conn.session.get(version_endpoint)
+            return resp.json()["version"]["version"]
+
+    def _version_greater_than(self, version1: str, version2: str) -> bool:
+        """Compare two OpenStack API versions.
+
+        Args:
+            version1: The first version to compare.
+            version2: The second version to compare.
+
+        Returns:
+            True if version1 is greater than version2, False otherwise.
+        """
+        return tuple(int(x) for x in version1.split(".")) > tuple(
+            int(x) for x in version2.split(".")
+        )
+
 
 def get_missing_security_rules(
     security_group: OpenstackSecurityGroup, ingress_tcp_ports: list[int] | None

github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py

@@ -19,6 +19,7 @@
 
 from github_runner_manager.errors import OpenStackError, SSHError
 from github_runner_manager.openstack_cloud.openstack_cloud import (
+    _MAX_NOVA_COMPUTE_API_VERSION,
     _MIN_KEYPAIR_AGE_IN_SECONDS_BEFORE_DELETION,
     _TEST_STRING,
     DEFAULT_SECURITY_RULES,
@@ -302,3 +303,92 @@ def mock_ssh_connection(*_args, **_kwargs) -> MagicMock:
             pass
 
     assert "No connectable SSH addresses found" in str(err.value)
+
+
+@pytest.mark.parametrize(
+    "max_compute_api_version, expected_version",
+    [
+        pytest.param(
+            "2.110",
+            _MAX_NOVA_COMPUTE_API_VERSION,
+            id="higher version but string is lexically smaller",
+        ),
+        pytest.param(
+            "2.92", _MAX_NOVA_COMPUTE_API_VERSION, id="one higher version than supported"
+        ),
+        pytest.param("2.91", _MAX_NOVA_COMPUTE_API_VERSION, id="highest supported version"),
+        pytest.param("2.90", "2.90", id="one version lower than supported"),
+        pytest.param("2.1", "2.1", id="really low version"),
+    ],
+)
+def test_get_openstack_connection_sets_max_compute_api(
+    openstack_cloud,
+    monkeypatch: pytest.MonkeyPatch,
+    max_compute_api_version: str,
+    expected_version: str,
+):
+    """
+    arrange: Setup get_compute_api to return different max versions.
+    act: Get OpenStack connection using context manager
+    assert: The max compute API version is set to be < 2.95.
+    """
+    monkeypatch.setattr(
+        openstack_cloud,
+        "_determine_max_compute_api_version_by_cloud",
+        MagicMock(return_value=max_compute_api_version),
+    )
+    openstack_connect_mock = MagicMock()
+    monkeypatch.setattr(
+        "github_runner_manager.openstack_cloud.openstack_cloud.openstack.connect",
+        openstack_connect_mock,
+    )
+
+    with openstack_cloud._get_openstack_connection():
+        pass
+
+    assert openstack_connect_mock.call_args[1]["compute_api_version"] == expected_version
+
+
+def test_determine_max_api_version(
+    openstack_cloud: OpenstackCloud, monkeypatch: pytest.MonkeyPatch
+):
+    """
+    arrange: Mock the OpenStack connection to return a specific max API version.
+    act: Call _determine_max_compute_api_version.
+    assert: The returned version is as expected.
+    """
+    mock_connection = MagicMock()
+    monkeypatch.setattr(
+        "github_runner_manager.openstack_cloud.openstack_cloud.openstack.connect",
+        MagicMock(return_value=mock_connection),
+    )
+    mock_connection.__enter__.return_value = mock_connection
+
+    endpoint_resp_json = {
+        "version": {
+            "id": "v2.1",
+            "status": "CURRENT",
+            "version": "2.96",
+            "min_version": "2.1",
+            "updated": "2013-07-23T11:33:21Z",
+            "links": [
+                {"rel": "self", "href": "http://172.16.1.204/openstack-nova/v2.1/"},
+                {"rel": "describedby", "type": "text/html", "href": "http://docs.openstack.org/"},
+            ],
+            "media-types": [
+                {
+                    "base": "application/json",
+                    "type": "application/vnd.openstack.compute+json;version=2.1",
+                }
+            ],
+        }
+    }
+    endpoint_resp = MagicMock()
+    endpoint_resp.json.return_value = endpoint_resp_json
+
+    session_mock = MagicMock()
+    mock_connection.session = session_mock
+    session_mock.get = MagicMock(return_value=endpoint_resp)
+
+    max_version = openstack_cloud._determine_max_compute_api_version_by_cloud()
+    assert max_version == "2.96"

tests/integration/helpers/openstack.py

@@ -145,10 +145,10 @@ async def ensure_charm_has_runner(self, app: Application) -> None:
         Args:
             app: The GitHub Runner Charm app to create the runner for.
         """
-        await OpenStackInstanceHelper._set_app_runner_amount(app, 1)
+        await OpenStackInstanceHelper.set_app_runner_amount(app, 1)
 
     @staticmethod
-    async def _set_app_runner_amount(app: Application, num_runners: int) -> None:
+    async def set_app_runner_amount(app: Application, num_runners: int) -> None:
         """Reconcile the application to a runner amount.
 
         Args:

tests/integration/test_charm_runner.py

@@ -47,13 +47,13 @@ async def test_check_runner(app: Application, instance_helper: OpenStackInstance
     act: Run check_runner action.
     assert: Action returns result with one runner.
     """
-    await instance_helper.ensure_charm_has_runner(app)
+    await instance_helper.set_app_runner_amount(app, 2)
 
     action = await app.units[0].run_action("check-runners")
     await action.wait()
 
     assert action.status == "completed"
-    assert action.results["online"] == "1"
+    assert action.results["online"] == "2"
     assert action.results["offline"] == "0"
     assert action.results["unknown"] == "0"
 
@@ -68,17 +68,15 @@ async def test_flush_runner_and_resource_config(
     instance_helper: OpenStackInstanceHelper,
 ) -> None:
     """
-    arrange: A working application with one runner.
+    arrange: A working application with two runners.
     act:
-        1. Run Check_runner action. Record the runner name for later.
-        2. Nothing.
-        3. Change the virtual machine resource configuration.
-        4. Run flush_runner action.
-        5. Dispatch a workflow to make runner busy and call flush_runner action.
+        1. Run Check_runner action. Record the runner names for later.
+        2. Flush runners.
+        3. Dispatch a workflow to make runner busy and call flush_runner action.
 
     assert:
-        1. One runner exists.
-        2. Check the resource matches the configuration.
+        1. Two runner exists.
+        2. Runners are recreated.
         3. The runner is not flushed since by default it flushes idle.
 
     Test are combined to reduce number of runner spawned.