Load balancing tests #218

dhly-etc · 2018-07-11T23:23:48Z

Needs significant testing and probably fixes, additional code.

This PR now imports arangodb/go-driver#139

ewoutp

Needs several changes, but still I'm impressed for a go novice.

ewoutp · 2018-07-16T05:40:08Z

deps/github.com/arangodb/go-driver/context.go

@@ -52,11 +52,12 @@ const (
 keyIsSystem ContextKey = "arangodb-isSystem"
 keyIgnoreRevs ContextKey = "arangodb-ignoreRevs"
 keyEnforceReplicationFactor ContextKey = "arangodb-enforceReplicationFactor"
- keyConfigured ContextKey = "arangodb-configured"
+ keyConfigured   ContextKey = "arangodb-configured"


The go-driver is vendored in this repo. Never change it here, without a PR in the go-driver repo first.

VST is controlled by the type of Connection, no need to add anything to the context for that.

ewoutp · 2018-07-16T05:44:05Z

pkg/util/arangod/client.go

+ if err != nil {
+ return nil, maskAny(err)
+ }
+ // Create client


From this point on the code paths are the same. Only create conn in the if/else block.
Use var conn driver.Connection before the if/else block.
Then change conn, err := ... to conn, err = ...

ewoutp · 2018-07-16T05:45:07Z

pkg/util/arangod/client.go

- if err != nil {
- return nil, maskAny(err)
+ config := driver.ClientConfig{}
+ vst := driver.mustUseVST(context)


I would not use the context for this, but add an argument to the function.
To avoid changing all uses of this function, use vst ...bool and then if len(vst) > 0 && vst[0] { ...

ewoutp · 2018-07-16T05:47:41Z

pkg/util/arangod/client.go

+func createArangodVSTConfigForDNSNames(ctx context.Context, cli corev1.CoreV1Interface, apiObject *api.ArangoDeployment, dnsNames []string) (http.ConnectionConfig, error) {
+ scheme := "vst"
+ if apiObject != nil && apiObject.Spec.IsSecure() {
+ // TODO set security for transport


Secure connection is essential, since almost all kube-arangodb deployments use TLS.
To use a secure VST connection, set scheme = "https".

ewoutp · 2018-07-16T05:48:23Z

pkg/util/arangod/client.go

@@ -200,6 +218,22 @@ func createArangodHTTPConfigForDNSNames(ctx context.Context, cli corev1.CoreV1In
 return connConfig, nil
 }

+// createArangodVSTConfigForDNSNames creates a go-driver VST connection config for a given DNS names.
+func createArangodVSTConfigForDNSNames(ctx context.Context, cli corev1.CoreV1Interface, apiObject *api.ArangoDeployment, dnsNames []string) (http.ConnectionConfig, error) {
+ scheme := "vst"


Should be "http". There is no "vst" scheme in the go-driver, just a VST connection that re-uses http/https schemes.

ewoutp · 2018-07-16T05:49:06Z

pkg/util/arangod/client.go

@@ -186,7 +204,7 @@ func createArangodClientForDNSName(ctx context.Context, cli corev1.CoreV1Interfa
 func createArangodHTTPConfigForDNSNames(ctx context.Context, cli corev1.CoreV1Interface, apiObject *api.ArangoDeployment, dnsNames []string) (http.ConnectionConfig, error) {
 scheme := "http"
 transport := sharedHTTPTransport
- if apiObject != nil && apiObject.Spec.IsSecure() {
+ if apiObject != nil && apiObject.Spec.IsSecure() {


Likely you did not use go fmt.

ewoutp · 2018-07-16T05:52:14Z

tests/load_balancer_test.go

+
+
+// TestLoadBalancerCursorVST tests cursor forwarding with load-balanced VST
+func TestLoadBalancerCursorVST(t *testing.T) {


Set the use of VST explicitly, not via context.

I would "duplicate" this test for VST & HTTP. To do so, create a helper function that contains almost all of the test and from TestLoadBalancerCursorVST and TestLoadBalancerCursorHTTP call that function with 1 argument specifying VST/HTTP.

ewoutp · 2018-07-16T05:54:45Z

tests/load_balancer_test.go

+
+ // Create a database client
+ ctx := context.Background()
+ client := mustNewArangodDatabaseClient(ctx, kubecli, apiObject, t)


Extend this call to include VST/HTTP. (again as optional argument vst ...bool)

dhly-etc · 2018-07-16T11:57:31Z

pkg/util/arangod/client.go

+ if apiObject != nil && apiObject.Spec.IsSecure() {
+ // TODO set security for transport
+ }
+ transport := vst.NewTransport(/* TODO configure transport */)


@ewoutp I haven't found examples of how to configure the transport correctly for secure vs. non-secure. Do you have a link to somewhere this is done?

https://github.com/arangodb/go-driver/blob/master/vst/connection.go#L96

that line is what makes this work

ewoutp · 2018-07-16T12:30:33Z

tests/load_balancer_test.go

+
+ // Setup context alternatives
+ contexts := []queryTestContext{
+ queryTestContext{driver.WithUseVST(nil, true), false},


remove WithUseVST

ewoutp · 2018-07-19T05:47:44Z

pkg/util/arangod/client.go

 DualStack: true,
 }).DialContext,
 MaxIdleConns: 100,
- IdleConnTimeout: 90 * time.Second,
+ IdleConnTimeout: 100 * time.Millisecond,


These are shared instances, so you're now changings this for ALL. That is not good.

ewoutp · 2018-07-19T05:48:27Z

tests/load_balancer_test.go

+}
+
+// tests cursor forwarding with load-balanced conn.
+func LoadBalancingCursorSubtest(t *testing.T, useVst bool) {


This function should start with a lower-case letter.
Uppercase is used to export a member outside its package, which is not needed (and wanted) here.

ewoutp · 2018-07-19T05:49:27Z

tests/load_balancer_test.go

+}
+
+func wasForwarded(r *driver.Response) bool {
+ h := (*r).Header("x-arango-request-served-by")


driver.Response is already an interface. Change r *driver.Response to r driver.Response.

ewoutp · 2018-07-19T05:51:04Z

tests/load_balancer_test.go

+ // Close upon exit of the function
+ defer cursor.Close()
+ }
+ if test.ExpectSuccess {


This entire file needs go fmt. Since most editors do that automatically, any future change would cause a huge diff if you do not format now.

ewoutp · 2018-07-19T05:51:44Z

tests/test_util.go

+}
+
+// CreateArangodDatabaseVSTClient creates a go-driver client for accessing the entire cluster (or single server) via VST
+func CreateArangodDatabaseVSTClient(ctx context.Context, cli corev1.CoreV1Interface, apiObject *api.ArangoDeployment) (driver.Client, error) {


start with lowercase letter. Should not be exported.

ewoutp · 2018-07-19T05:53:47Z

tests/load_balancer_test.go

+
+func wasForwarded(r *driver.Response) bool {
+ h := (*r).Header("x-arango-request-served-by")
+ return h != ""


API question. Is this header only set if the request is forwarded?
The header key seems to suggest that it is always there.
So I expected that you want to look for different values for this header instead of non-empty values.

Currently it is only set if the request is served by a different server than the one the request was sent to. You are correct that this is less than intuitive, so I'll think a bit about a better name.

ewoutp · 2018-07-19T05:54:34Z

tests/load_balancer_test.go

+ t.Error("HasMore returned false, but ReadDocument returns a document")
+ }
+ result = append(result, doc.Elem().Interface())
+ if (wasForwarded(&r)) {


In go, if expressions do not need brackets. A go fmt will remove them automatically.

ewoutp · 2018-07-19T06:07:41Z

tests/load_balancer_test.go

+ depl := newDeployment("test-lb-" + uniuri.NewLen(4))
+ depl.Spec.Mode = api.NewMode(api.DeploymentModeCluster)
+ image := "dhly/arangodb:3.3.11-local"
+ depl.Spec.Image = &image


This works, but in order to be consistent with the rest of the tests, use:
depl.Spec.Image = util.NewString("...")

dhly-etc and others added 2 commits July 11, 2018 09:26

Initial empty test file.

eb57d89

First attempt to configure a VST connection.

acb88ba

dhly-etc added the 9 WIP label Jul 11, 2018

ghost assigned dhly-etc Jul 11, 2018

ghost added the 2 - Working label Jul 11, 2018

ewoutp suggested changes Jul 16, 2018

View reviewed changes

dhly-etc commented Jul 16, 2018

View reviewed changes

Dan Larkin-York added 2 commits July 16, 2018 08:20

Incorporate changes from review.

013c87b

Formatting fixes.

14042ea

ewoutp reviewed Jul 16, 2018

View reviewed changes

Dan Larkin-York added 7 commits July 16, 2018 08:32

Remove bad fn call.

3d47a47

Fix transport.

20d9bf6

Fix some compilation issues.

53738d6

Moved client changes to test code.

85ac0a0

Added placeholder checks for headers and fixed VST issue.

7c9b8fd

Further adjustments to use correct image and check header.

6601334

Lowered keep-alive to trigger load-balancing.

9cbb4da

ewoutp reviewed Jul 19, 2018

View reviewed changes

Formatted files

d90fd0e

ghost assigned ewoutp Jul 19, 2018

ewoutp reviewed Jul 19, 2018

View reviewed changes

ewoutp and others added 3 commits July 19, 2018 09:16

Imported updated go-driver

e7e3747

Adjustments to timeouts and image.

7a86092

Merged in master, various small fixes

a87fea7

ewoutp added 2 commits August 6, 2018 15:47

Revert changelog changes

3c74e96

Ensure BOTH forwarded as well as not-forwarded requests

289e776

ewoutp merged commit 8c9fee8 into master Aug 6, 2018

ewoutp deleted the feature/load-balancing-tests branch August 6, 2018 14:03

ghost removed the 2 - Working label Aug 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load balancing tests #218

Load balancing tests #218

dhly-etc commented Jul 11, 2018 •

edited by ewoutp

Loading

ewoutp left a comment

ewoutp Jul 16, 2018

ewoutp Jul 16, 2018

ewoutp Jul 16, 2018

ewoutp Jul 16, 2018

ewoutp Jul 16, 2018

ewoutp Jul 16, 2018

ewoutp Jul 16, 2018

ewoutp Jul 16, 2018

dhly-etc Jul 16, 2018

ewoutp Jul 16, 2018

ewoutp Jul 16, 2018

ewoutp Jul 19, 2018

ewoutp Jul 19, 2018

ewoutp Jul 19, 2018

ewoutp Jul 19, 2018

ewoutp Jul 19, 2018

ewoutp Jul 19, 2018

dhly-etc Jul 19, 2018

ewoutp Jul 19, 2018

ewoutp Jul 19, 2018



		// TestLoadBalancerCursorVST tests cursor forwarding with load-balanced VST
		func TestLoadBalancerCursorVST(t *testing.T) {

Load balancing tests #218

Load balancing tests #218

Conversation

dhly-etc commented Jul 11, 2018 • edited by ewoutp Loading

ewoutp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhly-etc commented Jul 11, 2018 •

edited by ewoutp

Loading