Skip to content

Commit

Permalink
sanity: optional gRPC keepalive
Browse files Browse the repository at this point in the history
The gRPC defaults for a client do not work for a gRPC server that also
uses the defaults (grpc/grpc-go#3171) which
caused connection drops when a CSI driver is slow to respond and the
client starts sending keepalive pings too quickly
(kubernetes-csi#238).

The sanity package no longer uses gRPC keepalive. Instead, custom test
suites can enable that via some new configuration options if needed
and when it is certain that the CSI driver under test can handle it.
  • Loading branch information
pohly committed Nov 12, 2019
1 parent fdb220c commit eeed144
Show file tree
Hide file tree
Showing 35 changed files with 53 additions and 9,543 deletions.
2 changes: 1 addition & 1 deletion driver/mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func (m *MockCSIDriver) Nexus() (*grpc.ClientConn, error) {
}

// Create a client connection
m.conn, err = utils.Connect(m.Address())
m.conn, err = utils.Connect(m.Address(), grpc.WithInsecure())
if err != nil {
return nil, err
}
Expand Down
16 changes: 14 additions & 2 deletions pkg/sanity/sanity.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,19 @@ type TestConfig struct {
// is empty, it must provide both the controller and node service.
Address string

// DialOptions specifies the options that are to be used
// when connecting to Address. The default is grpc.WithInsecure().
// A dialer will be added for Unix Domain Sockets.
DialOptions []grpc.DialOption

// ControllerAddress optionally provides the gRPC endpoint of
// the controller service.
ControllerAddress string

// ControllerDialOptions specifies the options that are to be used
// for ControllerAddress.
ControllerDialOptions []grpc.DialOption

// SecretsFile is the filename of a .yaml file which is used
// to populate CSISecrets which are then used for calls to the
// CSI driver.
Expand Down Expand Up @@ -175,6 +184,9 @@ func NewTestConfig() TestConfig {
RemovePathCmdTimeout: 10 * time.Second,
TestVolumeSize: 10 * 1024 * 1024 * 1024, // 10 GiB
IDGen: &DefaultIDGenerator{},

DialOptions: []grpc.DialOption{grpc.WithInsecure()},
ControllerDialOptions: []grpc.DialOption{grpc.WithInsecure()},
}
}

Expand Down Expand Up @@ -240,7 +252,7 @@ func (sc *TestContext) Setup() {
sc.Conn.Close()
}
By("connecting to CSI driver")
sc.Conn, err = utils.Connect(sc.Config.Address)
sc.Conn, err = utils.Connect(sc.Config.Address, sc.Config.DialOptions...)
Expect(err).NotTo(HaveOccurred())
sc.connAddress = sc.Config.Address
} else {
Expand All @@ -253,7 +265,7 @@ func (sc *TestContext) Setup() {
sc.ControllerConn = sc.Conn
sc.controllerConnAddress = sc.Config.Address
} else {
sc.ControllerConn, err = utils.Connect(sc.Config.ControllerAddress)
sc.ControllerConn, err = utils.Connect(sc.Config.ControllerAddress, sc.Config.ControllerDialOptions...)
Expect(err).NotTo(HaveOccurred())
sc.controllerConnAddress = sc.Config.ControllerAddress
}
Expand Down
2 changes: 1 addition & 1 deletion test/driver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func TestSimpleDriver(t *testing.T) {
defer s.Stop()

// Setup a connection to the driver
conn, err := utils.Connect(s.Address())
conn, err := utils.Connect(s.Address(), grpc.WithInsecure())
if err != nil {
t.Errorf("Error: %s", err.Error())
}
Expand Down
11 changes: 1 addition & 10 deletions utils/grpcutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,10 @@ import (

"google.golang.org/grpc"
"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/keepalive"
)

// Connect address by grpc
func Connect(address string) (*grpc.ClientConn, error) {
dialOptions := []grpc.DialOption{
grpc.WithInsecure(),
}
func Connect(address string, dialOptions ...grpc.DialOption) (*grpc.ClientConn, error) {
u, err := url.Parse(address)
if err == nil && (!u.IsAbs() || u.Scheme == "unix") {
dialOptions = append(dialOptions,
Expand All @@ -41,11 +37,6 @@ func Connect(address string) (*grpc.ClientConn, error) {
return net.DialTimeout("unix", u.Path, timeout)
}))
}
// This is necessary when connecting via TCP and does not hurt
// when using Unix domain sockets. It ensures that gRPC detects a dead connection
// in a timely manner.
dialOptions = append(dialOptions,
grpc.WithKeepaliveParams(keepalive.ClientParameters{PermitWithoutStream: true}))

conn, err := grpc.Dial(address, dialOptions...)
if err != nil {
Expand Down
Loading

0 comments on commit eeed144

Please sign in to comment.