protocol: client: enable builtin yamux client support#201
protocol: client: enable builtin yamux client support#201egernst merged 1 commit intokata-containers:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #201 +/- ##
==========================================
+ Coverage 31.71% 32.91% +1.19%
==========================================
Files 10 10
Lines 1463 1580 +117
==========================================
+ Hits 464 520 +56
- Misses 936 980 +44
- Partials 63 80 +17
Continue to review full report at Codecov.
|
protocols/client/client_test.go
Outdated
|
|
||
| cliFunc := func(sock string, success bool, expect string) { | ||
| cli, err := NewAgentClient(sock) | ||
| cli, err := NewAgentClient(sock, false) |
There was a problem hiding this comment.
Is there no way to unit test the yamux option?
There was a problem hiding this comment.
surely there is. I'll add UT for it.
sboeuf
left a comment
There was a problem hiding this comment.
LGTM, but please address @jodh-intel comment about unit testing before merging.
|
@jodh-intel UT added. PTAL. |
|
Thanks @bergwolf - |
|
@jodh-intel CI test timeout is too low. Can we increase it? The UT actually includes two |
|
Hi @bergwolf - yes, we could change the timeout... ... but 10 seconds is a long time. How about doing this instead?: diff --git a/protocols/client/client.go b/protocols/client/client.go
index ffaded3..b092524 100644
--- a/protocols/client/client.go
+++ b/protocols/client/client.go
@@ -26,9 +26,11 @@ import (
const (
unixSocketScheme = "unix"
vsockSocketScheme = "vsock"
- dialTimeout = 5 * time.Second
)
+// defined as a var to allow the tests to modify it
+var dialTimeout = 5 * time.Second
+
// AgentClient is an agent gRPC client connection wrapper for agentgrpc.AgentServiceClient
type AgentClient struct {
agentgrpc.AgentServiceClient
diff --git a/protocols/client/client_test.go b/protocols/client/client_test.go
index b7789b3..633da23 100644
--- a/protocols/client/client_test.go
+++ b/protocols/client/client_test.go
@@ -12,6 +12,7 @@ import (
"os"
"strings"
"testing"
+ "time"
"github.com/hashicorp/yamux"
"github.com/stretchr/testify/assert"
@@ -106,6 +107,13 @@ func agentClientTest(t *testing.T, sock string, success, enableYamux bool, expec
}
func TestNewAgentClient(t *testing.T) {
+ savedDialTimeout := dialTimeout
+ dialTimeout = 1 * time.Second
+
+ defer func() {
+ dialTimeout = savedDialTimeout
+ }()
+
mock, waitCh, err := startMockServer(t, false)
assert.Nil(t, err, "failed to start mock server: %s", err)
@@ -129,6 +137,13 @@ func TestNewAgentClient(t *testing.T) {
}
func TestNewAgentClientWithYamux(t *testing.T) {
+ savedDialTimeout := dialTimeout
+ dialTimeout = 1 * time.Second
+
+ defer func() {
+ dialTimeout = savedDialTimeout
+ }()
+
mock, waitCh, err := startMockServer(t, true)
assert.Nil(t, err, "failed to start mock server: %s", err) |
When enableYamux is set, dial the grpc server with builtin yamux translation support. Fixes: kata-containers#200 Signed-off-by: Peng Tao <bergwolf@gmail.com>
|
@jodh-intel thanks for the suggestion. I've added it. PTAL. |
|
Hi @bergwolf - Nice - much neater your way! ;) lgtm |
When enableYamux is set, dial the grpc server with builtin yamux
translation support.
Fixes: #200