✨ Add ExtraDialOpts field to GRPCDialer - #230
Conversation
Allow consumers to pass additional grpc.DialOption values (e.g. grpc.WithConnectParams, grpc.WithAuthority) that GRPCDialer does not configure directly. Options are appended last in Dial() so they take precedence on conflict. Zero value preserves existing behavior. Signed-off-by: Dmitrii Andreev <dandreev@redhat.com>
WalkthroughAdds an ChangesExtraDialOpts support in GRPCDialer
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cloudevents/generic/options/grpc/options_test.go (1)
123-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen this beyond a non-nil smoke test.
Line 149 leaves
TLSConfignil, so this only covers the insecure branch, andgrpc.NewClientreturning a non-nil connection does not verify that the extra option was actually applied. Please add at least one TLS case and one observable assertion for an injected option so the new contract is protected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cloudevents/generic/options/grpc/options_test.go` around lines 123 - 165, The current TestDialExtraDialOpts in GRPCDialer only checks that Dial() returns a non-nil connection on the insecure path, so it doesn’t prove ExtraDialOpts are applied or cover the TLS branch. Extend the test to include a TLSConfig-backed case and add an observable assertion tied to an injected dial option (for example by validating the dialer behavior changes when a known option is supplied) so the contract in GRPCDialer.Dial is actually protected.pkg/cloudevents/generic/options/grpc/options.go (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that these options are forwarded to
grpc.NewClient.
ExtraDialOptsaccepts anygrpc.DialOption, but this code passes them throughgrpc.NewClient, which does not honor the entire dial-option surface. A short field comment calling that out would prevent callers from assuming unsupported options will take effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cloudevents/generic/options/grpc/options.go` at line 35, Update the `ExtraDialOpts` field comment in the `Options` type to explicitly say these `grpc.DialOption` values are forwarded to `grpc.NewClient`, and that not every dial option is supported or honored there. Keep the note short and colocated with `ExtraDialOpts` so callers understand the limitation when using `grpc.NewClient`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/cloudevents/generic/options/grpc/options_test.go`:
- Around line 123-165: The current TestDialExtraDialOpts in GRPCDialer only
checks that Dial() returns a non-nil connection on the insecure path, so it
doesn’t prove ExtraDialOpts are applied or cover the TLS branch. Extend the test
to include a TLSConfig-backed case and add an observable assertion tied to an
injected dial option (for example by validating the dialer behavior changes when
a known option is supplied) so the contract in GRPCDialer.Dial is actually
protected.
In `@pkg/cloudevents/generic/options/grpc/options.go`:
- Line 35: Update the `ExtraDialOpts` field comment in the `Options` type to
explicitly say these `grpc.DialOption` values are forwarded to `grpc.NewClient`,
and that not every dial option is supported or honored there. Keep the note
short and colocated with `ExtraDialOpts` so callers understand the limitation
when using `grpc.NewClient`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 873f54bc-b2d4-4374-8d8d-975d8311c547
📒 Files selected for processing (2)
pkg/cloudevents/generic/options/grpc/options.gopkg/cloudevents/generic/options/grpc/options_test.go
|
/approve |
|
/assign @tesshuflower |
|
@qiujian16 This looks pretty self-contained to me, but I don't seem to be able to approve - would you mind taking a look? |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kuudori, qiujian16, tesshuflower The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
this is weird. I think your id is in the owner list |
ac9666c
into
open-cluster-management-io:main
…overy (#219) ## What Fix ~95s gRPC recovery delay after Maestro restart. Three changes: 1. **Cap gRPC reconnect backoff at 5s** — configure `grpc.WithConnectParams` via new `ExtraDialOpts` field on OCM SDK's `GRPCDialer` ([upstream PR merged](open-cluster-management-io/sdk-go#230)). Reduces worst-case reconnect delay from 120s to 5s. 2. **Bounded retry for transient gRPC errors** — retry `Create`/`Get`/`Patch` ManifestWork calls up to 3 times with exponential backoff (1s, 2s, 4s) on `codes.Unavailable`. Uses `k8s.io/apimachinery/pkg/util/wait.ExponentialBackoffWithContext`. Retry wraps raw `workClient` calls inside each method (before `apperrors.MaestroError` wraps the error, preserving gRPC status codes). 3. **Log warnings in AlwaysAck** — structured warning with event ID, type, and error details before discarding errors. Makes silent publish failures visible in adapter logs. ## Why Maestro restart causes gRPC connection to enter `TRANSIENT_FAILURE` with grpc-go default `MaxDelay=120s`. HTTP path recovers instantly (per-request), but gRPC publish path stays in backoff. The adapter silently drops every failed publish via `AlwaysAck`, so operators see "no progress" instead of "Maestro publish failing". Breaks tier2-nightly `maestro_unavailability` recovery test (120s window). ## Testing - Unit tests for `isTransientGRPCError` (6 cases) and `retryOnTransientGRPC` (5 scenarios: success, retry-success, max-attempts, non-transient-fast-fail, context-cancellation) - All existing `AlwaysAck` tests updated for new logger parameter - `make test` + `make lint` pass ## Test plan - [ ] `make test` — unit tests pass - [ ] `make lint` — 0 issues - [ ] `make build` — binary builds - [ ] Verify tier2-nightly `maestro_unavailability` test passes 5x consistently
3 participants
Summary
Add
ExtraDialOpts []grpc.DialOptionfield toGRPCDialer, allowing consumers to pass additional gRPC dial options thatGRPCDialerdoes not configure directly (e.g.grpc.WithConnectParams,grpc.WithAuthority).Options are appended last in
Dial()— both TLS and insecure code paths — so caller-supplied options take precedence on conflict. Zero value (nil) preserves existing behavior.Use case: configuring
grpc.WithConnectParamsto cap connection-level reconnect backoff (grpc-go defaults toMaxDelay=120s, which causes multi-minute recovery delays after server restarts).Related issue(s)
N/A — additive feature, no breaking changes.
Summary by CodeRabbit