Fix HTTP/2 keep-alive channel options

RELEASE_NOTES.md

@@ -2,7 +2,7 @@
 
 ## Summary
 
-<!-- Here goes a general summary of what this release is about -->
+Fix HTTP/2 keep-alive, which was never actually enabled.
 
 ## Upgrading
 
@@ -14,4 +14,12 @@
 
 ## Bug Fixes
 
-<!-- Here goes notable bug fixes that are worth a special mention or explanation -->
+- HTTP/2 keep-alive is now actually enabled. `grpc.keepalive_time_ms` and
+  `grpc.keepalive_timeout_ms` were computed with `timedelta.total_seconds() * 1000`
+  and therefore passed as `float`. gRPC silently ignores channel arguments that are
+  neither `int` nor `str`, so no keep-alive pings were ever sent and a client could
+  only detect a dead stream through its own deadline or a teardown sent by the peer.
+
+  Note that keep-alive pings are answered by the nearest HTTP/2 peer. Where a proxy
+  terminates HTTP/2, successful pings prove that hop is alive, not the backend, so
+  a stream deadline is still needed to detect an orphaned upstream stream.

src/frequenz/client/base/channel.py

@@ -162,23 +162,19 @@ def parse_grpc_uri(
             ("grpc.keepalive_permit_without_calls", 1),
             (
                 "grpc.keepalive_time_ms",
-                (
-                    (
-                        defaults.keep_alive.interval
-                        if options.keep_alive_interval is None
-                        else options.keep_alive_interval
-                    ).total_seconds()
-                    * 1000
+                _to_millis(
+                    defaults.keep_alive.interval
+                    if options.keep_alive_interval is None
+                    else options.keep_alive_interval
                 ),
             ),
             (
                 "grpc.keepalive_timeout_ms",
-                (
+                _to_millis(
                     defaults.keep_alive.timeout
                     if options.keep_alive_timeout is None
                     else options.keep_alive_timeout
-                ).total_seconds()
-                * 1000,
+                ),
             ),
         ]
         if keep_alive
@@ -212,6 +208,22 @@ def parse_grpc_uri(
     return insecure_channel(target, channel_options, interceptors=interceptors)
 
 
+def _to_millis(duration: timedelta) -> int:
+    """Convert a duration to whole milliseconds.
+
+    gRPC channel arguments must be `int` or `str`. grpc-python silently ignores
+    arguments of any other type, so passing a `float` disables the option instead
+    of raising an error.
+
+    Args:
+        duration: The duration to convert.
+
+    Returns:
+        The duration in whole milliseconds.
+    """
+    return int(duration.total_seconds() * 1000)
+
+
 def _to_bool(value: str) -> bool:
     value = value.lower()
     if value in ("true", "on", "1"):

tests/test_channel.py

@@ -280,11 +280,11 @@ def test_parse_uri_ok(  # pylint: disable=too-many-locals
             ("grpc.keepalive_permit_without_calls", 1),
             (
                 "grpc.keepalive_time_ms",
-                (expected_keep_alive_interval.total_seconds() * 1000),
+                int(expected_keep_alive_interval.total_seconds() * 1000),
             ),
             (
                 "grpc.keepalive_timeout_ms",
-                expected_keep_alive_timeout.total_seconds() * 1000,
+                int(expected_keep_alive_timeout.total_seconds() * 1000),
             ),
         ]
         if expected_keep_alive.enabled
@@ -326,6 +326,38 @@ def test_parse_uri_ok(  # pylint: disable=too-many-locals
         )
 
 
+@pytest.mark.parametrize(
+    "uri",
+    [
+        "grpc://api.example.com:443",
+        "grpc://api.example.com:443?keep_alive_interval_s=12.5",
+        "grpc://api.example.com:443?keep_alive_timeout_s=0.75",
+    ],
+)
+def test_keep_alive_option_types(uri: str) -> None:
+    """Test that keep-alive channel arguments are passed as `int`, not `float`.
+
+    gRPC silently ignores channel arguments whose value is neither `int` nor `str`,
+    so a `float` here disables keep-alive without any error. Comparing values only
+    would not catch this, because `60000 == 60000.0`.
+    """
+    with mock.patch(
+        "frequenz.client.base.channel.insecure_channel",
+        return_value=mock.MagicMock(name="mock_channel", spec=Channel),
+    ) as insecure_channel_mock:
+        parse_grpc_uri(uri, defaults=ChannelOptions(ssl=SslOptions(enabled=False)))
+
+    channel_options = insecure_channel_mock.call_args.args[1]
+    assert channel_options is not None
+    keep_alive_args = dict(channel_options)
+    for name in ("grpc.keepalive_time_ms", "grpc.keepalive_timeout_ms"):
+        value = keep_alive_args[name]
+        assert type(value) is int, (  # pylint: disable=unidiomatic-typecheck
+            f"channel argument {name!r} has value {value!r} of type "
+            f"{type(value).__name__}; gRPC only accepts int or str"
+        )
+
+
 @pytest.mark.parametrize("value", ["true", "on", "1", "TrUe", "On", "ON", "TRUE"])
 def test_to_bool_true(value: str) -> None:
     """Test conversion of valid boolean values to True."""