Repository: ovn-org/libovsdb
Pull request #142 (2021-06-02 10:46:34)

Add interface for libovsdb connection with timeout
I am submitting this for now in an attempt to get a discussion going. 

libovsdb does currently not provide an interface for a connection with a specified timeout. Any connection attempted will thus use the default timeout specified by the operating system. Clients using libovsdb could however benefit from being able to specify a timeout, because they implicitly and incorrectly sometimes assume one. For example, go-ovn implicitly and incorrectly assumes that there is a timeout specified, seeing as how it assume that it should be able to retry a failed connection attempt 500 milliseconds later, see: https://github.com/eBay/go-ovn/blob/master/client.go#L360. In the case of go-ovn that will not be the case seeing as how the connection will block for the default timeout set by the operating system. 

The first step is to implement a interface in libovsdb allowing a timeout to be specified, the next would be to have clients modify their usage of that and explicitly set it if they are making assumptions based on it. 

This is a problem which has been seen on clusters using go-ovn / libovsdb, see: https://bugzilla.redhat.com/show_bug.cgi?id=1966280#c2 

coveralls: ## Pull Request Test Coverage Report for [Build 899064881](https://coveralls.io/builds/40236239)

* **4** of **10**   **(40.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage decreased (**-0.07%**) to **69.501%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [client/client.go](https://coveralls.io/builds/40236239/source?filename=client%2Fclient.go#L57) | 4 | 10 | 40.0%
<!-- | **Total:** | **4** | **10** | **40.0%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/40236239/badge)](https://coveralls.io/builds/40236239) |
| :-- | --: |
| Change from base [Build 882729464](https://coveralls.io/builds/40088700): |  -0.07% |
| Covered Lines: | 1839 |
| Relevant Lines: | 2646 |

---
##### 💛  - [Coveralls](https://coveralls.io)

dave-tucker: Thanks @alexanderConstantinescu
I agree that having a default timeout and the ability to provide a user-defined one is needed.

I've been thinking about the `Connect` API recently since the discussion in #129 and I'd rather not add another variant.

I'd like to change the `Connect` API to something like:

```go
Connect(endpoints string, ...Options)
```

And we'd provide options like:
```go
WithTLSConfg(cfg tls.Config)
WithTimeout(t time.Duration)
WithCache()
WithLeaderOnly()
```

[Here's](https://github.com/google/go-containerregistry/blob/main/pkg/v1/remote/options.go) an example of how this pattern is implemented.

This means that the `Connect` API can remain ~static and we can easily add more functionality and options as required.

@amorenoz wdyt?

Once we have an agreement on a way forward, I could quickly whip up a PR to move the TLSConfig and then you could add the timeout in the same manner.
alexanderConstantinescu: > Thanks @alexanderConstantinescu
> I agree that having a default timeout and the ability to provide a user-defined one is needed.
> 
> I've been thinking about the `Connect` API recently since the discussion in #129 and I'd rather not add another variant.
> 
> I'd like to change the `Connect` API to something like:
> 
> ```go
> Connect(endpoints string, ...Options)
> ```
> 
> And we'd provide options like:
> 
> ```go
> WithTLSConfg(cfg tls.Config)
> WithTimeout(t time.Duration)
> WithCache()
> WithLeaderOnly()
> ```
> 
> [Here's](https://github.com/google/go-containerregistry/blob/main/pkg/v1/remote/options.go) an example of how this pattern is implemented.
> 
> This means that the `Connect` API can remain ~static and we can easily add more functionality and options as required.
> 
> @amorenoz wdyt?
> 
> Once we have an agreement on a way forward, I could quickly whip up a PR to move the TLSConfig and then you could add the timeout in the same manner.

I completely agree. Your proposal definitely leads to a cleaner API. 

Ping me on the PR once you have one, and if you decide to not implement the timeout portion: I could add it. I don't have any objections to you implementing all parameters you think libovsdb should accept, though.  

I am leaving the PR open until everyone (well, you and Adrian I guess) has agreed on the way forward.   
amorenoz: I like the idea. Thanks @alexanderConstantinescu for the proposal.
About the API proposal, is there a way to enforce a mandatory option (e.g: `DBModel`)? or will those be left as standard arguments (i.e: not the variadic `...Options`)?
vtolstov: I'm prefer to have context at the first place. This is go idiomatic and easy to control like calling cancel.
May be add ConnectContext to not break exiting clients and in feature deprecated connect and replace it ?
dave-tucker: @vtolstov that's a good idea. As we're still pre 1.0 I'm not too worried about breaking `Connect`...

@alexanderConstantinescu how do you feel about implementing this with a context?

Without my proposed changes that would mean that `Connect` would look like this:
```
Connect(ctx context.Context, endpoints string, tlsConfig tls.Config)
```
Then you could create a [`net.Dialer`](https://golang.org/pkg/net/#Dialer) and use `DialContext`.

Obviously that doesn't provide a default timeout (other than those specified in the OS itself), but maybe that's ok?
vtolstov: i think that we can combine to something like
Connect(ctx context.Context, opts...Option)
via opts we can pass endpoints, timeout, tls.Config and much more in feature if we need
alexanderConstantinescu: OK, so if I understand correctly there's a consensus gravitating towards having something like:

```
Connect(ctx context.Context, endpoints string, opts...Option)
```
And then like @dave-tucker said, provide options of the likes:
```
WithTLSConfg(cfg tls.Config)
WithTimeout(t time.Duration)
WithCache()
WithLeaderOnly()
```
I feel like endpoints aren't options really, but more a required argument and hence it would make since to provide through `Connect` arguments. This touches what @amorenoz said:

> About the API proposal, is there a way to enforce a mandatory option (e.g: DBModel)?

Not sure if I've synthesized our conversation correctly? 

> Obviously that doesn't provide a default timeout (other than those specified in the OS itself), but maybe that's ok?

Yes, I am not for a default timeout  - I just ended up writing one in this PR to have something "working". What is really required is that clients can specify their own timeout, and not get bitten by implicit assumptions. 
dave-tucker: Endpoints are somewhat optional as we fallback to a default Unix socket if an empty string is provided. The only mandatory arg is the DBModel today.

@alexanderConstantinescu if you want to rework this to use context, I'll review and it should be good to merge. 

I opened a PR for the Options piece that can merge after this.
alexanderConstantinescu: > Endpoints are somewhat optional as we fallback to a default Unix socket if an empty string is provided. The only mandatory arg is the DBModel today.
> 
> @alexanderConstantinescu if you want to rework this to use context, I'll review and it should be good to merge.
> 
> I opened a PR for the Options piece that can merge after this.

Looking into this more deeply and trying to implement it, I am realizing this API might not very well suited when using `tls.DialWithDialer`. There doesn't seem to be a way to "TLS dial" with context. In fact, `DialWithDialer` [uses it's own context](https://github.com/golang/go/blob/dev.boringcrypto.go1.16/src/crypto/tls/tls.go#L115). I could do something like:

```go
func Connect(ctx context.Context, endpoints string, database *model.DBModel, tlsConfig *tls.Config) (*OvsdbClient, error) {
	var c net.Conn
	var dialer net.Dialer
	var err error
	var u *url.URL

	for _, endpoint := range strings.Split(endpoints, ",") {
		if u, err = url.Parse(endpoint); err != nil {
			return nil, err
		}
		// u.Opaque contains the original endPoint with the leading protocol stripped
		// off. For example: endPoint is "tcp:127.0.0.1:6640" and u.Opaque is "127.0.0.1:6640"
		host := u.Opaque
		if len(host) == 0 {
			host = defaultTCPAddress
		}
		switch u.Scheme {
		case UNIX:
			path := u.Path
			if len(path) == 0 {
				path = defaultUnixAddress
			}
			c, err = dialer.DialContext(ctx, u.Scheme, path)
		case TCP:
			c, err = dialer.DialContext(ctx, u.Scheme, host)
		case SSL:
			dialer.Cancel = ctx.Done()
			c, err = tls.DialWithDialer(&dialer, "tcp", host, tlsConfig)
		default:
			err = fmt.Errorf("unknown network protocol %s", u.Scheme)
		}

		if err == nil {
			return newRPC2Client(c, database)
		}
	}

	return nil, fmt.Errorf("failed to connect to endpoints %q: %v", endpoints, err)
}
```

But I am not sure about providing `tls.DialWithDialer` a dialer with `Cancel` set which refers to context X, and have the TLS library implementation combine that with its privately generated context...but maybe that should work?
alexanderConstantinescu: Doh, this won't pass CI :1st_place_medal: 

```
run golangci-lint
  Running [/home/runner/golangci-lint-1.39.0-linux-amd64/golangci-lint run --out-format=github-actions] in [] ...
  Error: SA1019: dialer.Cancel has been deprecated since Go 1.7: Use DialContext instead.  (staticcheck)
```

from [here](https://github.com/golang/go/blob/master/src/net/dial.go#L89)


alexanderConstantinescu: Ended up doing this: https://github.com/golang/go/issues/18482#issuecomment-269873780

Patch:
diff --git a/client/client.go b/client/client.go
index 2a51f8b..2131f36 100644
--- a/client/client.go
+++ b/client/client.go
@@ -1,6 +1,7 @@
 package client
 
 import (
+	"context"
 	"crypto/tls"
 	"encoding/json"
 	"fmt"
@@ -48,8 +49,9 @@ const (
 
 // Connect to ovn, using endpoint in format ovsdb Connection Methods
 // If address is empty, use default address for specified protocol
-func Connect(endpoints string, database *model.DBModel, tlsConfig *tls.Config) (*OvsdbClient, error) {
+func Connect(ctx context.Context, endpoints string, database *model.DBModel, tlsConfig *tls.Config) (*OvsdbClient, error) {
 	var c net.Conn
+	var dialer net.Dialer
 	var err error
 	var u *url.URL
 
@@ -69,11 +71,14 @@ func Connect(endpoints string, database *model.DBModel, tlsConfig *tls.Config) (
 			if len(path) == 0 {
 				path = defaultUnixAddress
 			}
-			c, err = net.Dial(u.Scheme, path)
+			c, err = dialer.DialContext(ctx, u.Scheme, path)
 		case TCP:
-			c, err = net.Dial(u.Scheme, host)
+			c, err = dialer.DialContext(ctx, u.Scheme, host)
 		case SSL:
-			c, err = tls.Dial("tcp", host, tlsConfig)
+			dialer := tls.Dialer{
+				Config: tlsConfig,
+			}
+			c, err = dialer.DialContext(ctx, "tcp", host)
 		default:
 			err = fmt.Errorf("unknown network protocol %s", u.Scheme)
 		}
diff --git a/cmd/stress/stress.go b/cmd/stress/stress.go
index 34c6b72..0f6173e 100644
--- a/cmd/stress/stress.go
+++ b/cmd/stress/stress.go
@@ -1,6 +1,7 @@
 package main
 
 import (
+	"context"
 	"flag"
 	"fmt"
 	"log"
@@ -45,7 +46,7 @@ var (
 )
 
 func run() {
-	ovs, err := client.Connect(*connection, dbModel, nil)
+	ovs, err := client.Connect(context.Background(), *connection, dbModel, nil)
 	if err != nil {
 		log.Fatal(err)
 	}
diff --git a/example/play_with_ovs/play_with_ovs.go b/example/play_with_ovs/play_with_ovs.go
index 74753dd..54c8bb5 100644
--- a/example/play_with_ovs/play_with_ovs.go
+++ b/example/play_with_ovs/play_with_ovs.go
@@ -1,6 +1,7 @@
 package main
 
 import (
+	"context"
 	"flag"
 	"fmt"
 	"log"
@@ -101,7 +102,7 @@ func main() {
 		log.Fatal("Unable to create DB model ", err)
 	}
 	// By default libovsdb connects to 127.0.0.0:6400.
-	ovs, err := client.Connect(*connection, dbmodel, nil)
+	ovs, err := client.Connect(context.Background(), *connection, dbmodel, nil)
 
 	// If you prefer to connect to OVS in a specific location :
 	// ovs, err := client.Connect("tcp:192.168.56.101:6640", nil)


Test patch:
diff --git a/client/ovs_integration_test.go b/client/ovs_integration_test.go
index 66f3f85..5160528 100644
--- a/client/ovs_integration_test.go
+++ b/client/ovs_integration_test.go
@@ -2,6 +2,7 @@ package client
 
 import (
 	"bytes"
+	"context"
 	"log"
 	"os"
 	"testing"
@@ -71,14 +72,14 @@ func TestConnectIntegration(t *testing.T) {
 	go func() {
 		// Use Convenience params. Ignore failure even if any
 
-		_, err := Connect(cfg.Addr, defDB, nil)
+		_, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 		if err != nil {
 			log.Println("Couldnt establish OVSDB connection with Defult params. No big deal")
 		}
 	}()
 
 	go func() {
-		ovs, err := Connect(cfg.Addr, defDB, nil)
+		ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 		if err != nil {
 			connected <- false
 		} else {
@@ -106,7 +107,7 @@ func TestListDbsIntegration(t *testing.T) {
 		t.Skip()
 	}
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -141,7 +142,7 @@ func TestGetSchemasIntegration(t *testing.T) {
 		t.Skip()
 	}
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -169,7 +170,7 @@ func TestInsertTransactIntegration(t *testing.T) {
 	}
 	SetConfig()
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -227,7 +228,7 @@ func TestDeleteTransactIntegration(t *testing.T) {
 		t.Skip()
 	}
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -272,7 +273,7 @@ func TestMonitorIntegration(t *testing.T) {
 		t.Skip()
 	}
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -290,7 +291,7 @@ func TestNotifyIntegration(t *testing.T) {
 	}
 	SetConfig()
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -321,7 +322,7 @@ func TestRemoveNotifyIntegration(t *testing.T) {
 	}
 	SetConfig()
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -364,7 +365,7 @@ func TestTableSchemaValidationIntegration(t *testing.T) {
 	}
 	SetConfig()
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -389,7 +390,7 @@ func TestColumnSchemaInRowValidationIntegration(t *testing.T) {
 	}
 	SetConfig()
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -415,7 +416,7 @@ func TestColumnSchemaInMultipleRowsValidationIntegration(t *testing.T) {
 	}
 	SetConfig()
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -447,7 +448,7 @@ func TestColumnSchemaValidationIntegration(t *testing.T) {
 		t.Skip()
 	}
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -472,7 +473,7 @@ func TestMonitorCancelIntegration(t *testing.T) {
 	}
 	SetConfig()
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}
@@ -504,7 +505,7 @@ func TestInsertDuplicateTransactIntegration(t *testing.T) {
 	}
 	SetConfig()
 
-	ovs, err := Connect(cfg.Addr, defDB, nil)
+	ovs, err := Connect(context.Background(), cfg.Addr, defDB, nil)
 	if err != nil {
 		t.Fatalf("Failed to Connect. error: %s", err)
 	}