Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix passing default grpc call options in Kubernetes client #18360

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

serathius
Copy link
Member

@serathius serathius commented Jul 24, 2024

Totally slipped past me, only found this because K8s has integration tests that expect client side grpc error for size of request. I didn't catch it as they are not located in apiserver package, but in controlplane. When iterating I assumed that all relevant tests are in apiserver package.

cc @ahrtr

Comment on lines 49 to 50
kv pb.KVClient
callOpts []grpc.CallOption
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that it's better to reuse clientv3.KV.

etcd/client/v3/kv.go

Lines 93 to 104 in 9a6c9ae

type kv struct {
remote pb.KVClient
callOpts []grpc.CallOption
}
func NewKV(c *Client) KV {
api := &kv{remote: RetryKVClient(c)}
if c != nil {
api.callOpts = c.callOpts
}
return api
}

You can make change something like below. But you also need to make further change because clientv3.KV has slight different interface methods as pb.KVClient.

$ git diff
diff --git a/client/v3/kubernetes/client.go b/client/v3/kubernetes/client.go
index 4ca0e5fe8..67a5b50cf 100644
--- a/client/v3/kubernetes/client.go
+++ b/client/v3/kubernetes/client.go
@@ -31,7 +31,7 @@ func New(cfg clientv3.Config) (*Client, error) {
        }
        kc := &Client{
                Client: c,
-               kv:     clientv3.RetryKVClient(c),
+               kv:     clientv3.NewKV(c),
        }
        kc.Kubernetes = kc
        return kc, nil
@@ -40,7 +40,7 @@ func New(cfg clientv3.Config) (*Client, error) {
 type Client struct {
        *clientv3.Client
        Kubernetes Interface
-       kv         pb.KVClient
+       kv         clientv3.KV
 }
 
 var _ Interface = (*Client)(nil)

@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 27 lines in your changes missing coverage. Please review.

Project coverage is 68.80%. Comparing base (fe796ab) to head (ff24c6b).

Current head ff24c6b differs from pull request most recent head 2bcaed1

Please upload reports for the commit 2bcaed1 to get more accurate results.

Files with missing lines Patch % Lines
client/v3/kubernetes/client.go 0.00% 27 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
Files with missing lines Coverage Δ
client/v3/kubernetes/client.go 0.00% <0.00%> (ø)

... and 18 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18360      +/-   ##
==========================================
+ Coverage   68.77%   68.80%   +0.03%     
==========================================
  Files         420      420              
  Lines       35489    35470      -19     
==========================================
- Hits        24407    24406       -1     
+ Misses       9650     9625      -25     
- Partials     1432     1439       +7     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe796ab...2bcaed1. Read the comment docs.

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

It's much simpler now! Thanks

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, serathius

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@serathius serathius merged commit c0291f8 into etcd-io:main Aug 30, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants