add upgrade tests #76

ObiWahn · 2018-03-26T07:30:53Z

No description provided.

ewoutp

See comments

ewoutp · 2018-03-26T12:34:05Z

tests/test_util.go

+func arangoDeploymentHealthy(t *testing.T, deployment *api.ArangoDeployment, k8sClient kubernetes.Interface) {
+	// Create a database client
+	ctx := context.Background()
+	DBClient := mustNewArangodDatabaseClient(ctx, k8sClient, deployment, t)


Prefer to keep the dbclient as an argument passed to this function.

ewoutp · 2018-03-26T12:34:16Z

tests/test_util.go

@@ -297,3 +297,69 @@ func removeSecret(cli kubernetes.Interface, secretName, ns string) error {
 	}
 	return nil
 }
+
+func arangoDeploymentHealthy(t *testing.T, deployment *api.ArangoDeployment, k8sClient kubernetes.Interface) {


Add comment.

ewoutp · 2018-03-26T12:35:06Z

tests/test_util.go

@@ -297,3 +297,69 @@ func removeSecret(cli kubernetes.Interface, secretName, ns string) error {
 	}
 	return nil
 }
+
+func arangoDeploymentHealthy(t *testing.T, deployment *api.ArangoDeployment, k8sClient kubernetes.Interface) {


Name is now clean to me.
Suggest to rename to waitUntilArangoDeploymentHealthy

ewoutp · 2018-03-26T12:36:17Z

tests/test_util.go

@@ -297,3 +297,69 @@ func removeSecret(cli kubernetes.Interface, secretName, ns string) error {
 	}
 	return nil
 }
+
+func arangoDeploymentHealthy(t *testing.T, deployment *api.ArangoDeployment, k8sClient kubernetes.Interface) {


To be in line with other waitUntil... functions, this function must return an error in case something is wrong.
You can (read should) then remove the t *testing.T argument.

ewoutp · 2018-03-26T12:37:44Z

tests/upgrade_test.go

+	"github.com/arangodb/kube-arangodb/pkg/util"
+)
+
+// TODO - environments (provided from outside)


I don't understand. What is there TODO?

Perhaps you can replace this by a comment, saying that the kubernetes deployment environments are controlled outside the tests and are therefor not a variable in these tests.

ewoutp · 2018-03-26T12:39:18Z

tests/upgrade_test.go

+		t.Fatalf("Deployment not running in time: %v", err)
+	}
+
+	arangoDeploymentHealthy(t, deployment, k8sClient)


Comment please

ewoutp · 2018-03-26T12:40:48Z

tests/upgrade_test.go

+	arangoDeploymentHealthy(t, deployment, k8sClient)
+
+	ctx := context.Background()
+	DBClient := mustNewArangodDatabaseClient(ctx, k8sClient, deployment, t)


I think this test must be done on all servers.
The current implementation of the test can succeed even f not all servers are actually upgraded.

Also, you put this test in an op := func() error {... with a Retry: "Wait until all versions are correct"
Reason: It could be that the change of image is saved, the cluster is still healthy and the version check test
starts before any upgrading has actually taken place.

and change signature to match other helper functions

ewoutp

See comments

ewoutp · 2018-03-27T07:30:46Z

tests/test_util.go

@@ -231,9 +236,30 @@ func waitUntilVersionUp(cli driver.Client, allowNoLeaderResponse ...bool) error
 		return maskAny(noLeaderErr)
 	}

+	if predicate != nil {


Move the predicate call inside the op function.
I expect the function to wait until the version response is valid, including any predicate I may pass in.

ewoutp · 2018-03-27T07:31:15Z

tests/test_util.go

+}
+
+// creates predicate to be used in waitUntilVersionUp
+func createEqualVersionsPredicateFromString(version_string string) func(driver.VersionInfo) error {


Avoid underscores in names in go.
Use camelCasing

ewoutp · 2018-03-27T07:32:53Z

tests/test_util.go

 	return nil
 }

+// creates predicate to be used in waitUntilVersionUp
+func createEqualVersionsPredicate(info driver.VersionInfo) func(driver.VersionInfo) error {


I would expect this function to have a version driver.Version argument, since that is the only thing you're comparing.
Will automatically make createEqualVersionsPredicateFromString obsolete.

ewoutp · 2018-03-27T07:34:00Z

tests/test_util.go

@@ -297,3 +323,72 @@ func removeSecret(cli kubernetes.Interface, secretName, ns string) error {
 	}
 	return nil
 }
+
+func waitUntilArangoDeploymentHealthy(deployment *api.ArangoDeployment, DBClient driver.Client, k8sClient kubernetes.Interface, versionString ...string) error {


Add comment describing what the function does.

Replace versionString ...string with requiredVersion driver.Version.
If that is a non-empty string, create the predicate.

Reason: I think we may want to add other checks in the future and you can only have 1 optional argument.

ewoutp · 2018-03-27T07:36:16Z

tests/test_util.go

+		singles := members.Single
+		agents := members.Agents
+
+		if len(singles) != 2 || len(agents) != 3 {


Don't like these constants.
Check against a spec.

ewoutp · 2018-03-27T07:36:55Z

tests/test_util.go

+			}
+		}
+
+		if goodResults < 1 || noLeaderResults > 1 {


Don't like the last 1 constant. Check against a spec.

ewoutp · 2018-03-27T07:38:48Z

tests/upgrade_test.go

+	deploymentClient := kubeArangoClient.MustNewInCluster()
+
+	// Prepare deployment config
+	deploymentName := "test-upgrade-" + string(mode) + "-" + string(engine)


This looks a bit complicated.

deploymentName := strings.Replace(fmt.Sprintf("test-upgrade-%s-%s-%sto%s", mode, engine, fromVersion, toVersion), ".", "-", -1)

ewoutp · 2018-03-27T08:00:54Z

tests/test_util.go

@@ -297,3 +310,72 @@ func removeSecret(cli kubernetes.Interface, secretName, ns string) error {
 	}
 	return nil
 }
+
+func waitUntilArangoDeploymentHealthy(deployment *api.ArangoDeployment, DBClient driver.Client, k8sClient kubernetes.Interface, versionString string) error {


Still needs a comment

[ci LONG=1] [ci TESTOPTIONS="-test.run TestUpgrade*"]

ewoutp

LGTM, please merge it

ObiWahn added the 9 WIP label Mar 26, 2018

ObiWahn force-pushed the tests/upgrade branch 6 times, most recently from 0d536b2 to 05f8536 Compare March 26, 2018 10:28

ObiWahn added the 9 Review Required label Mar 26, 2018

ewoutp self-requested a review March 26, 2018 12:32

ewoutp suggested changes Mar 26, 2018

View reviewed changes

ObiWahn force-pushed the tests/upgrade branch 2 times, most recently from 8f46730 to 6fa02ea Compare March 26, 2018 13:02

ObiWahn added 7 commits March 26, 2018 15:05

add upgrade tests

20dba8f

make use of new parameter for waitUntilVersionUp

50c80ed

create waitForArangoDeploymentHealthy helper

3f39ed9

fix use format string correctly

021a427

add version check

d2d5f84

fix deployment name

b461b88

if copy and paste error

c6187c3

ObiWahn force-pushed the tests/upgrade branch from 6fa02ea to c6187c3 Compare March 26, 2018 13:08

ObiWahn added 3 commits March 27, 2018 07:03

change versions

4aa6e49

rename arangoDeploymentHealthy -> waitUntilArangoDeploymentHealthy

91c210e

and change signature to match other helper functions

add predicate to waitUntilVersionUP

08ef440

ObiWahn force-pushed the tests/upgrade branch from 01909e5 to 08ef440 Compare March 27, 2018 06:57

add predicate helper

3976718

ObiWahn force-pushed the tests/upgrade branch from 70a4c53 to 3976718 Compare March 27, 2018 07:14

ObiWahn added 2 commits March 27, 2018 09:26

integrate version predicate into existing test

ee24887

fix format strings (use more explicit types instead of generic value)

3b44250

ewoutp suggested changes Mar 27, 2018

View reviewed changes

ewoutp reviewed Mar 27, 2018

View reviewed changes

ObiWahn force-pushed the tests/upgrade branch from 100813c to eef306d Compare March 27, 2018 08:14

add small fixes

61122a8

ObiWahn force-pushed the tests/upgrade branch from eef306d to 61122a8 Compare March 27, 2018 08:26

update versions

bec0925

[ci LONG=1] [ci TESTOPTIONS="-test.run TestUpgrade*"]

ObiWahn force-pushed the tests/upgrade branch from a11ebed to bec0925 Compare March 27, 2018 09:20

disable upgrade tests to 3.4 until 3.4 is available on hub.docker.com

d5a9574

ObiWahn force-pushed the tests/upgrade branch from af81617 to d5a9574 Compare March 27, 2018 10:58

add missing uniuri

f1e29f7

ObiWahn force-pushed the tests/upgrade branch from 897f875 to f1e29f7 Compare March 27, 2018 18:04

ewoutp approved these changes Mar 27, 2018

View reviewed changes

add downgrade tests

bab052f

ObiWahn requested a review from neunhoef March 28, 2018 07:15

ewoutp approved these changes Mar 28, 2018

View reviewed changes

ObiWahn merged commit e5905ba into master Mar 28, 2018

ObiWahn deleted the tests/upgrade branch March 28, 2018 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add upgrade tests #76

add upgrade tests #76

ObiWahn commented Mar 26, 2018

ewoutp left a comment

ewoutp Mar 26, 2018

ewoutp Mar 26, 2018

ewoutp Mar 26, 2018

ewoutp Mar 26, 2018

ewoutp Mar 26, 2018

ewoutp Mar 26, 2018

ewoutp Mar 26, 2018

ewoutp Mar 26, 2018

ewoutp left a comment

ewoutp Mar 27, 2018

ewoutp Mar 27, 2018

ewoutp Mar 27, 2018

ewoutp Mar 27, 2018

ewoutp Mar 27, 2018

ewoutp Mar 27, 2018

ewoutp Mar 27, 2018

ewoutp Mar 27, 2018

ewoutp Mar 27, 2018

ewoutp left a comment

add upgrade tests #76

add upgrade tests #76

Conversation

ObiWahn commented Mar 26, 2018

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

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

ewoutp left a comment

Choose a reason for hiding this comment