Allow grpc option tweaking, i.e. keepalive settings#359
Conversation
WalkthroughThe changes introduce customizable gRPC options in the documentation and codebase. A new YAML section with a Changes
Sequence Diagram(s)sequenceDiagram
participant ClientConfig
participant Lease
participant Streams
participant GRPC
ClientConfig->>Lease: request_lease_async(grpcOptions)
Lease->>Streams: connect_router_stream(endpoint, token, tls_config, grpc_options)
Streams->>GRPC: aio_secure_channel(target, credentials, grpc_options)
GRPC->>GRPC: _override_default_grpc_options(grpc_options)
GRPC-->>Streams: secure channel established
Streams-->>Lease: router stream connected
Lease-->>ClientConfig: lease established
sequenceDiagram
participant ExporterConfig
participant Exporter
participant Streams
participant GRPC
ExporterConfig->>Exporter: serve(grpcOptions)
Exporter->>Streams: connect_router_stream(endpoint, token, tls_config, grpc_options)
Streams->>GRPC: aio_secure_channel(target, credentials, grpc_options)
GRPC->>GRPC: _override_default_grpc_options(grpc_options)
GRPC-->>Streams: secure channel established
Streams-->>Exporter: stream connected
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
mangelajo
force-pushed
the
grpc-keepalive-settings
branch
from
347ba22 to
7478daa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/config/client.py (1)
51-51: Added gRPC configuration options to ClientConfig.The
grpcOptionsfield allows users to configure gRPC parameters through client configuration. Note that the type allows forNonevalues, which differs from other implementations in the codebase.Consider using a consistent approach for nullable vs. non-nullable fields across the codebase. Since you're using
default_factory=dict, theNonecase might be unnecessary.- grpcOptions: dict[str, Any] | None = Field(default_factory=dict) + grpcOptions: dict[str, Any] = Field(default_factory=dict)packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
29-29: Type inconsistency in gRPC options.The
grpc_optionsattribute is typed asdict[str, str]here, but asdict[str, Any]inLeaseclass. This inconsistency might cause issues if gRPC options need non-string values.Consider using a consistent type definition across the codebase:
- grpc_options: dict[str, str] = field(default_factory=dict) + grpc_options: dict[str, Any] = field(default_factory=dict)This would require adding
from typing import Anyto the imports.packages/jumpstarter/jumpstarter/common/streams.py (1)
35-35: Missing type hint for gRPC options.The function signature has been updated to include the
grpc_optionsparameter, but no type hint is provided.Consider adding a type hint for the
grpc_optionsparameter for consistency and better code documentation:- async def connect_router_stream(endpoint, token, stream, tls_config, grpc_options): + async def connect_router_stream(endpoint, token, stream, tls_config, grpc_options: dict[str, Any]):This would require adding
from typing import Anyto the imports.
🛑 Comments failed to post (2)
packages/jumpstarter/jumpstarter/common/grpc.py (2)
47-61:
⚠️ Potential issueCritical error: Debugging code will prevent normal execution.
There's a
raise ValueError(options)statement on line 57 that will always throw an exception, breaking the function. Additionally, print statements after this line are unreachable.def _override_default_grpc_options(grpc_options: dict[str, Any] | None) -> Sequence[Tuple[str, Any]]: defaults = ( ("grpc.lb_policy_name", "round_robin"), # we keep a low keepalive time to avoid idle timeouts on cloud load balancers ("grpc.keepalive_time_ms", 20000), ("grpc.keepalive_timeout_ms", 5000), ("grpc.http2.max_pings_without_data", 5), ("grpc.keepalive_permit_without_calls", 1), ) options = dict(defaults, **(grpc_options or {})) - raise ValueError(options) - print("grpc_options", grpc_options) - print(options) return tuple(options.items())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def _override_default_grpc_options(grpc_options: dict[str, Any] | None) -> Sequence[Tuple[str, Any]]: defaults = ( ("grpc.lb_policy_name", "round_robin"), # we keep a low keepalive time to avoid idle timeouts on cloud load balancers ("grpc.keepalive_time_ms", 20000), ("grpc.keepalive_timeout_ms", 5000), ("grpc.http2.max_pings_without_data", 5), ("grpc.keepalive_permit_without_calls", 1), ) options = dict(defaults, **(grpc_options or {})) return tuple(options.items())
56-56: 🛠️ Refactor suggestion
Potential issue with dictionary construction.
The current dictionary construction approach with tuple input might not work as expected. Consider using
dict()conversion first.- options = dict(defaults, **(grpc_options or {})) + options = dict(defaults) + options.update(grpc_options or {})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.options = dict(defaults) options.update(grpc_options or {})
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/common/grpc.py (2)
56-57: Consider adding validation for critical gRPC options.While the implementation correctly merges default and custom options, there's no validation for potentially problematic values. Consider adding checks for critical options to prevent configuration issues.
def _override_default_grpc_options(grpc_options: dict[str, Any] | None) -> Sequence[Tuple[str, Any]]: defaults = ( ("grpc.lb_policy_name", "round_robin"), # we keep a low keepalive time to avoid idle timeouts on cloud load balancers ("grpc.keepalive_time_ms", 20000), ("grpc.keepalive_timeout_ms", 5000), ("grpc.http2.max_pings_without_data", 5), ("grpc.keepalive_permit_without_calls", 1), ) + # Merge default options with user-provided options options = dict(defaults, **(grpc_options or {})) + + # Validate critical options + if "grpc.keepalive_time_ms" in grpc_options and grpc_options["grpc.keepalive_time_ms"] < 1000: + raise ConfigurationError("grpc.keepalive_time_ms must be at least 1000ms") + return tuple(options.items())
38-58: Consider adding documentation for the new parameter.The function lacks docstring documentation for the new
grpc_optionsparameter. Consider adding documentation to help users understand what options can be passed and their format.def aio_secure_channel(target: str, credentials: grpc.ChannelCredentials, grpc_options: dict[str, Any] | None): + """ + Create a secure gRPC channel with customizable options. + + Args: + target: The server address to connect to. + credentials: The credentials to use for the connection. + grpc_options: Optional dictionary of gRPC channel options to override defaults. + Keys should be gRPC option names (e.g., "grpc.keepalive_time_ms"). + + Returns: + A secure gRPC channel. + """ return grpc.aio.secure_channel( target, credentials, options=_override_default_grpc_options(grpc_options), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/source/cli/clients.md(1 hunks)docs/source/introduction/exporters.md(1 hunks)packages/jumpstarter/jumpstarter/client/lease.py(3 hunks)packages/jumpstarter/jumpstarter/common/grpc.py(2 hunks)packages/jumpstarter/jumpstarter/common/streams.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(3 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/exporter/exporter.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/source/introduction/exporters.md
- packages/jumpstarter/jumpstarter/config/client_config_test.py
- docs/source/cli/clients.md
- packages/jumpstarter/jumpstarter/client/lease.py
- packages/jumpstarter/jumpstarter/common/streams.py
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter/jumpstarter/config/exporter.py
- packages/jumpstarter/jumpstarter/exporter/exporter.py
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
🔇 Additional comments (5)
packages/jumpstarter/jumpstarter/common/grpc.py (5)
6-6: Appropriate import additions.The additional imports from typing are necessary for the new type annotations in the updated function signatures.
38-40: Good signature update with proper type annotations.The function signature now correctly includes the new
grpc_optionsparameter with appropriate type annotations. Usingdict[str, Any] | Noneallows for both passing custom options and defaulting to None when not needed.
44-44: Properly handles the new options parameter.The implementation now correctly passes the processed options to the underlying gRPC channel.
47-58: Well-implemented helper function for gRPC options.The
_override_default_grpc_optionsfunction is a clean implementation that:
- Defines sensible defaults for gRPC
- Reduces keepalive time to 20000ms (from 350000ms) to avoid idle timeouts
- Correctly merges custom options with defaults
- Returns options in the format expected by gRPC
The change in keepalive time aligns with the PR objective to allow tweaking these settings.
50-51: Improved keepalive time for better connection stability.Reducing the keepalive time from 350000ms to 20000ms (as indicated in the AI summary) is a good improvement to prevent cloud load balancers from dropping idle connections. The comment clearly explains the reasoning.
|
Also missing a call site at |
yup, I just found from E2E, thanks :D |
mangelajo
force-pushed
the
grpc-keepalive-settings
branch
from
7478daa to
100f1ea
Compare
|
@NickCao let's see if it's all good now. |
NickCao
force-pushed
the
grpc-keepalive-settings
branch
from
100f1ea to
f9dbbad
Compare
2 participants
Summary by CodeRabbit
grpcConfigandgrpcOptionsparameters for both clients and exporters to expand configuration capabilities.grpcConfigandgrpcOptionsparameters for both clients and exporters.grpcOptionsfield.