Conversation
📝 WalkthroughWalkthroughThe PR refactors the Gravity protocol definitions by splitting gravity.proto into gravity_metrics.proto and gravity_session.proto, introduces an optional ProvisioningProvider interface for provisioning-capable providers, removes the standalone provisioning function, and updates code to handle non-provisioning providers gracefully. Changes
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gravity/grpc_client.go`:
- Around line 1031-1040: In GravityClient.handleUnprovisionRequest, avoid the
silent early return when the provider isn't a provider.ProvisioningProvider;
instead send an error response back to the server with msgID indicating
unprovisioning is unsupported (e.g., call the existing response-send path such
as g.sendUnprovisionResponse(msgID, error) or the common
g.sendErrorResponse/msg-send helper used elsewhere), and log the error via
g.logger before returning so the caller receives a clear failure instead of
hanging; keep the rest of the function (ctx creation and
provisioningProvider.Deprovision call) unchanged.
🧹 Nitpick comments (3)
gravity/grpc_client.go (1)
1382-1390: Missing log message for non-provisioning providers during reconnect.When the provider doesn't implement
ProvisioningProvider, no log message is emitted about attempting to start a new connection. This could make debugging connection issues harder for non-provisioning clients.📝 Proposed fix to add fallback logging
if provisioningProvider, ok := g.provider.(provider.ProvisioningProvider); ok { // Log deployment synchronization intent deploymentCount := len(provisioningProvider.Resources()) if deploymentCount > 0 { g.logger.Info("connection state reset, attempting to start new connection and synchronize %d existing deployments", deploymentCount) } else { g.logger.Info("connection state reset, attempting to start new connection") } + } else { + g.logger.Info("connection state reset, attempting to start new connection") }gravity/proto/gravity_session.proto (2)
232-244: Field naming inconsistency:pausedDurationshould bepaused_duration.Proto3 style guide recommends
snake_casefor field names. Line 241 usespausedDuration(camelCase) while all other fields in this message usesnake_case. If this is intentional for wire compatibility with existing data, consider adding a comment explaining this.📝 Proposed fix for naming consistency
bool paused = 9; - google.protobuf.Duration pausedDuration = 10; + google.protobuf.Duration paused_duration = 10; bool on_demand = 11;Or if wire compatibility is required, add a comment:
bool paused = 9; + // Note: camelCase retained for wire compatibility with existing clients google.protobuf.Duration pausedDuration = 10;
136-140: Consider reserving the deprecated field number.The
hostnamefield (field 2) is documented as deprecated and will be removed. When removing deprecated fields, the field number should be reserved to prevent accidental reuse. Consider adding this when the field is actually removed.message RouteDeploymentRequest { string deployment_id = 1; reserved 2; // hostname - deprecated and removed string virtual_ip = 3; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
gravity/proto/gravity.pb.gois excluded by!**/*.pb.gogravity/proto/gravity_grpc.pb.gois excluded by!**/*.pb.gogravity/proto/gravity_metrics.pb.gois excluded by!**/*.pb.gogravity/proto/gravity_session.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
Makefilegravity/README.mdgravity/grpc_client.gogravity/proto/gravity.protogravity/proto/gravity_metrics.protogravity/proto/gravity_session.protogravity/provider/provider.gogravity/provision.gogravity/simple_benchmark_test.go
💤 Files with no reviewable changes (3)
- gravity/provision.go
- gravity/README.md
- gravity/proto/gravity.proto
🧰 Additional context used
🧬 Code graph analysis (1)
gravity/grpc_client.go (1)
gravity/provider/provider.go (2)
ProvisioningProvider(88-97)DeprovisionReasonUnprovision(68-68)
🪛 Buf (1.64.0)
gravity/proto/gravity_metrics.proto
[error] 3-3: Files with package "gravity" must be within a directory "gravity" relative to root but were in directory "gravity/proto".
(PACKAGE_DIRECTORY_MATCH)
gravity/proto/gravity_session.proto
[error] 7-7: import "gravity_metrics.proto": file does not exist
(COMPILE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
Makefile (1)
51-55: LGTM!The Makefile correctly updates the
gen-protoctarget to compile the new reorganized proto files (gravity_metrics.protoandgravity_session.proto), aligning with the PR objective to unify and reorganize the proto definitions.gravity/provider/provider.go (1)
79-97: LGTM!The interface design cleanly separates concerns:
Providerdefines the minimal contract for all providers (Configure + ProcessInPacket)ProvisioningProviderdefines provisioning-specific capabilities (Deprovision, Resources, SetMetricsCollector)This allows the gRPC client to use type assertions to conditionally access provisioning features, which is the correct pattern for optional interface composition in Go.
gravity/simple_benchmark_test.go (1)
63-90: LGTM!The benchmark correctly migrates from the removed
ControlMessagetype to the newSessionMessagetype, maintaining test coverage for protobuf serialization performance. The variable naming and type usage are consistent with the new proto definitions.gravity/grpc_client.go (2)
622-638: LGTM!The type assertion to
ProvisioningProvidercorrectly guards access toResources(), ensuring that non-provisioning providers work without implementing provisioning-specific methods. TheexistingDeploymentsslice is correctly scoped and populated only when the provider supports provisioning.
855-862: LGTM!The conditional logging for deployment synchronization status is appropriate and only executes when the provider supports provisioning.
gravity/proto/gravity_metrics.proto (1)
1-28: LGTM!The metrics proto file is well-structured with:
- Logical field number ranges for grouping related fields
- Comprehensive metrics coverage (connection, performance, system resources, historical data)
- Appropriate use of nested messages for organization
Regarding the Buf linter warning about
PACKAGE_DIRECTORY_MATCH: this is a common pattern in Go projects where proto files reside in aproto/subdirectory while using a simpler package name. Thego_packageoption correctly specifies the generated code location, so this can be safely ignored if consistent with project conventions.gravity/proto/gravity_session.proto (1)
5-7: Import path works correctly with current directory structure.Both
gravity_metrics.protoandgravity_session.protoexist in the samegravity/proto/directory, so the relative import at line 7 is valid. Any Buf linter warning about the file not existing is likely due to running the linter from the repository root without proper proto path configuration. If needed, configurebuf.yamlto include thegravity/proto/directory in the proto path.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| func (g *GravityClient) handleUnprovisionRequest(msgID string, request *pb.UnprovisionRequest) { | ||
| provisioningProvider, ok := g.provider.(provider.ProvisioningProvider) | ||
| if !ok { | ||
| return | ||
| } | ||
| g.logger.Debug("received unprovision request: deployment_id=%s", request.DeploymentId) | ||
|
|
||
| // Call provider to deprovision the deployment | ||
| ctx := context.WithoutCancel(g.context) | ||
| err := g.provider.Deprovision(ctx, request.DeploymentId, provider.DeprovisionReasonUnprovision) | ||
| err := provisioningProvider.Deprovision(ctx, request.DeploymentId, provider.DeprovisionReasonUnprovision) |
There was a problem hiding this comment.
Silent early return may cause server to hang waiting for response.
When the provider doesn't implement ProvisioningProvider, the function returns without sending any response to the server. The server sent an UnprovisionRequest and likely expects a response (success or error). This could cause the server to timeout or hang.
Consider sending an error response indicating the provider doesn't support unprovisioning:
🛠️ Proposed fix to send error response
func (g *GravityClient) handleUnprovisionRequest(msgID string, request *pb.UnprovisionRequest) {
provisioningProvider, ok := g.provider.(provider.ProvisioningProvider)
if !ok {
+ g.logger.Debug("received unprovision request but provider does not support provisioning")
+ // Send error response to server
+ response := &pb.ProtocolResponse{
+ Id: msgID,
+ Event: "unprovision",
+ Success: false,
+ Error: "provider does not support provisioning",
+ }
+ responseMsg := &pb.SessionMessage{
+ Id: msgID,
+ MessageType: &pb.SessionMessage_Response{
+ Response: response,
+ },
+ }
+ if len(g.streamManager.controlStreams) > 0 && g.streamManager.controlStreams[0] != nil {
+ g.streamManager.controlStreams[0].Send(responseMsg)
+ }
return
}📝 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.
| func (g *GravityClient) handleUnprovisionRequest(msgID string, request *pb.UnprovisionRequest) { | |
| provisioningProvider, ok := g.provider.(provider.ProvisioningProvider) | |
| if !ok { | |
| return | |
| } | |
| g.logger.Debug("received unprovision request: deployment_id=%s", request.DeploymentId) | |
| // Call provider to deprovision the deployment | |
| ctx := context.WithoutCancel(g.context) | |
| err := g.provider.Deprovision(ctx, request.DeploymentId, provider.DeprovisionReasonUnprovision) | |
| err := provisioningProvider.Deprovision(ctx, request.DeploymentId, provider.DeprovisionReasonUnprovision) | |
| func (g *GravityClient) handleUnprovisionRequest(msgID string, request *pb.UnprovisionRequest) { | |
| provisioningProvider, ok := g.provider.(provider.ProvisioningProvider) | |
| if !ok { | |
| g.logger.Debug("received unprovision request but provider does not support provisioning") | |
| // Send error response to server | |
| response := &pb.ProtocolResponse{ | |
| Id: msgID, | |
| Event: "unprovision", | |
| Success: false, | |
| Error: "provider does not support provisioning", | |
| } | |
| responseMsg := &pb.SessionMessage{ | |
| Id: msgID, | |
| MessageType: &pb.SessionMessage_Response{ | |
| Response: response, | |
| }, | |
| } | |
| if len(g.streamManager.controlStreams) > 0 && g.streamManager.controlStreams[0] != nil { | |
| g.streamManager.controlStreams[0].Send(responseMsg) | |
| } | |
| return | |
| } | |
| g.logger.Debug("received unprovision request: deployment_id=%s", request.DeploymentId) | |
| // Call provider to deprovision the deployment | |
| ctx := context.WithoutCancel(g.context) | |
| err := provisioningProvider.Deprovision(ctx, request.DeploymentId, provider.DeprovisionReasonUnprovision) |
🤖 Prompt for AI Agents
In `@gravity/grpc_client.go` around lines 1031 - 1040, In
GravityClient.handleUnprovisionRequest, avoid the silent early return when the
provider isn't a provider.ProvisioningProvider; instead send an error response
back to the server with msgID indicating unprovisioning is unsupported (e.g.,
call the existing response-send path such as g.sendUnprovisionResponse(msgID,
error) or the common g.sendErrorResponse/msg-send helper used elsewhere), and
log the error via g.logger before returning so the caller receives a clear
failure instead of hanging; keep the rest of the function (ctx creation and
provisioningProvider.Deprovision call) unchanged.
There was a problem hiding this comment.
the provider advertises it's capability in the config so we trust them to expect this to never be called. Good point though we may want to, in the future, validate the provider interface against the capabilities
There was a problem hiding this comment.
No description provided.
1 participant
Summary by CodeRabbit
Release Notes
Breaking Changes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.