Service profiles integration tests by dadjeibaah · Pull Request #2638 · linkerd/linkerd2

dadjeibaah · 2019-04-04T21:17:16Z

This PR adds integration tests for service profiles that test profile generation through the tap and Open API flags. This work serves as a baseline for additional work to come listed in issue #2518 .

Related to #2518

l5d-bot · 2019-04-04T21:25:03Z

Integration test results for c4b4f3b: fail 😕
Log output: https://gist.github.com/ba9b6c977e9339bcedc5a361bd70d348

alpeb

Looking good 👍
I added a few comments below.
Also I didn't see the new files emojivoto-injected.yml and Voting.proto being used, mabye they were added by mistake?

alpeb · 2019-04-05T14:23:35Z

test/serviceprofiles/serviceprofiles_test.go

+func TestServiceProfilesFromTap(t *testing.T) {
+	testCases := []testCase{
+		{
+			namespace:   "emojivoto",


Would you mind leveraging TestHelper.GetTestNamespace to generate these namespaces with a prefix? It's not uncommon to have emojivoto/booksapp lying around and this would conflict.
This would imply removing the namespace workload in emojivoto.yml and booksapp.yml, and all the namespace fields in emojivoto.yml.

Also by using a prefix you could leave those namespaces around like the other tests do, for bin/clean-up to take care of, without having to rely on tearDown().

alpeb · 2019-04-05T14:32:49Z

test/serviceprofiles/serviceprofiles_test.go

+
+	for _, tc := range testCases {
+		t.Run(fmt.Sprintf("service profiles from tap:%s", tc.namespace), func(t *testing.T) {
+			cmd := []string{"inject", fmt.Sprintf("testdata/%s", tc.injectYAML)}


Since #2595 got merged, install_test.go is leaving the control plane with auto-injection enabled, so you don't need to explicitly inject things. Although this doesn't create a problem per se, as the auto-injector will just ignore payloads that are already injected...

Confirming that this assumes that the namespaces for these apps have linkerd.io/inject: enabled? I could see value in testing both ways, though either approach would be fine for these tests.

alpeb · 2019-04-05T14:37:23Z

test/serviceprofiles/serviceprofiles_test.go

+	return true
+}
+
+func getRoutes(deployName, namespace string, helper *testutil.TestHelper) ([]string, error) {


The TestHelper is already global, so you don't need to pass it here.

alpeb · 2019-04-05T15:13:23Z

test/serviceprofiles/serviceprofiles_test.go

+		t.Fatalf("routes command failed: %s\n", err)
+	}
+
+	if len(routes) <= 1 {


I guess you could use here assertExpectedRoutes as you do below.

alpeb · 2019-04-05T15:18:14Z

test/serviceprofiles/serviceprofiles_test.go

+	"strings"
+	"testing"
+
+	"github.com/cloudflare/cfssl/log"


Probably VSCode playing tricks 🙂

siggy

looking good, awesome to include tests for swagger, proto, and tap.

these tests are adding nearly 5 minutes on my laptop. i suspect a lot of that is taken up by tearDown(), which will go away per @alpeb's comment.

i think a lot of the rest of the time is spent on deployment, particularly booksapp. consider deploying something smaller with less startup time, or just re-use/modify smoke-test.yaml, which should already be deployed and injected by the time your tests run. also have a look at tap_application.yaml.

$ go test -v ./test/serviceprofiles -integration-tests -linkerd `pwd`/bin/linkerd
=== RUN   TestServiceProfilesFromTap
=== RUN   TestServiceProfilesFromTap/service_profiles_from_tap:emojivoto
=== RUN   TestServiceProfilesFromTap/service_profiles_from_tap:booksapp
--- PASS: TestServiceProfilesFromTap (166.14s)
    --- PASS: TestServiceProfilesFromTap/service_profiles_from_tap:emojivoto (39.31s)
    --- PASS: TestServiceProfilesFromTap/service_profiles_from_tap:booksapp (126.83s)
=== RUN   TestServiceProfilesFromSwagger
--- PASS: TestServiceProfilesFromSwagger (1.00s)
=== RUN   TestServiceProfilesFromProto
--- PASS: TestServiceProfilesFromProto (2.12s)
PASS
ok  	github.com/linkerd/linkerd2/test/serviceprofiles	289.940s

siggy · 2019-04-05T18:52:20Z

test/serviceprofiles/serviceprofiles_test.go

+	cmd := []string{"routes", "--namespace", namespace, deployName}
+	out, stderr, err := helper.LinkerdRun(cmd...)
+	if err != nil {
+		log.Infof("error getting routes: %s\n", stderr)


we typically don't log in these tests, better to just return the error, or call t.Errorf or t.Fatalf.

siggy · 2019-04-05T19:01:32Z

test/serviceprofiles/serviceprofiles_test.go

+
+	for _, tc := range testCases {
+		t.Run(fmt.Sprintf("service profiles from tap:%s", tc.namespace), func(t *testing.T) {
+			cmd := []string{"inject", fmt.Sprintf("testdata/%s", tc.injectYAML)}


Confirming that this assumes that the namespaces for these apps have linkerd.io/inject: enabled? I could see value in testing both ways, though either approach would be fine for these tests.

siggy · 2019-04-05T19:05:39Z

test/serviceprofiles/serviceprofiles_test.go

+				t.Fatalf("routes command failed: %s\n", err.Error())
+			}
+			for _, route := range routes {
+				if len(route) <= 1 {


i think len(route) is testing the length of each route name? should this instead be...

if len(routes) <= 1 { t.Fatalf("Expected routes for service to be greater than or equal to 1 but got %d\n", len(routes)) }

How do we feel about testing routes the way routes_test.go is doing it (i.e. comparing CLI output with golden files)? Not sure which one is better, but at least with routes_test.go, I can open up the golden file to see what the expected output is.

dadjeibaah · 2019-04-05T19:48:29Z

@alpeb @siggy thanks for the review. I also was getting concerned about the amount of time the tests were adding to the test suite. However, the original issue reference emojivoto and booksapp. I can switch you using either smoke_test or tap_application instead, which I agree, would be better.

ihcsim

Thanks for tests!

I think TestServiceProfilesFromTap(), TestServiceProfilesFromSwagger() and TestServiceProfilesFromProto() are essentially very similar, except for the flags passed to the profile command.

TIOLI: Group them as data sources under one TestServiceProfiles() function. I think something like this might work:

diff --git a/test/serviceprofiles/serviceprofiles_test.go b/test/serviceprofiles/serviceprofiles_test.go
index 2b7707fd..314672cd 100644
--- a/test/serviceprofiles/serviceprofiles_test.go
+++ b/test/serviceprofiles/serviceprofiles_test.go
@@ -75,6 +74,65 @@ func TestServiceProfilesFromTap(t *testing.T) {
    t.Fatalf("Expected route details for service to be at most 1 but got %d\n", len(routes))
  }

+    dataSources := []struct {
+      name string
+      args []string
+    }{
+      {
+        name: "tap",
+        args: []string{
+          tc.deployName,
+          "--tap-route-limit",
+          "5",
+          "--tap-duration",
+          "10s",
+        },
+      },
+      {
+        name: "open-api",
+        args: []string{"testdata/authors.swagger"},
+      },
+      {
+        name: "proto",
+        args: []string{"testdata/Emoji.proto"},
+      },
+    }
+
+    for _, dataSource := range dataSources {
+      routes, err := getRoutes(tc.deployName, tc.namespace, TestHelper)
+      if err != nil {
+        t.Fatalf("routes command failed: %s\n", err)
+      }
+
+      if len(routes) > 1 {
+        t.Fatalf("Expected route details for service to be at-most 1 but got %d\n", len(routes))
+      }
+
+      sourceFlag := fmt.Sprintf("--%s", dataSource.name)
+      cmd := []string{"profile", "--namespace", tc.namespace, tc.spName, sourceFlag}
+      cmd = append(cmd, dataSource.args...)
+      out, _, err := TestHelper.LinkerdRun(cmd...)
+      if err != nil {
+        t.Fatalf("profile command failed: %s\n", err.Error())
+      }
+
+      out, err = TestHelper.KubectlApply(out, tc.namespace)
+      if err != nil {
+        t.Fatalf("kubectl apply command failed:\n%s", err)
+      }
+
+      // check that authors now has more than one route
+      routes, err = getRoutes(tc.deployName, tc.namespace, TestHelper)
+      if err != nil {
+        t.Fatalf("routes command failed: %s\n", err)
+      }
+
+      if len(routes) <= 1 {
+        t.Fatalf("Expected route details for service to be greater than 1 but got %d\n", len(routes))
+      }
+    }

ihcsim · 2019-04-05T20:53:45Z

test/serviceprofiles/serviceprofiles_test.go

+				t.Fatalf("routes command failed: %s\n", err.Error())
+			}
+			for _, route := range routes {
+				if len(route) <= 1 {


How do we feel about testing routes the way routes_test.go is doing it (i.e. comparing CLI output with golden files)? Not sure which one is better, but at least with routes_test.go, I can open up the golden file to see what the expected output is.

Signed-off-by: Dennis Adjei-Baah <[email protected]>

l5d-bot · 2019-04-08T17:36:58Z

Integration test results for ccbe530: success 🎉
Log output: https://gist.github.com/549f0b72ef044d41a503a8b04567abfc

Signed-off-by: Dennis Adjei-Baah <[email protected]>

l5d-bot · 2019-04-08T18:04:40Z

Integration test results for ff99ca3: success 🎉
Log output: https://gist.github.com/a56440d6764949a2893b6512da24a5a2

Signed-off-by: Dennis Adjei-Baah <[email protected]>

l5d-bot · 2019-04-08T18:57:36Z

Integration test results for ee5188a: success 🎉
Log output: https://gist.github.com/e5051b3302c632419de44a2f3003e450

ihcsim

Thanks for the changes. Some comments below.

test/serviceprofiles/testdata/tap_application.yaml

test/serviceprofiles/serviceprofiles_test.go

ihcsim · 2019-04-08T21:42:26Z

test/serviceprofiles/serviceprofiles_test.go

+			}
+
+			if !assertExpectedRoutes(tc.expectedRoutes, routes) {
+				t.Fatalf("Expected routes to have prefixes:\n%s\nbut got:\n%s",


We should make this t.Errorf(), so that even if one test case fails, the loop can continue with subsequent test cases.

test/serviceprofiles/serviceprofiles_test.go

Signed-off-by: Dennis Adjei-Baah <[email protected]>

siggy · 2019-04-08T23:21:25Z

test/serviceprofiles/serviceprofiles_test.go

+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.sourceName, func(t *testing.T) {


to fix ci:

tc := tc // pin

siggy · 2019-04-08T23:24:29Z

test/serviceprofiles/testdata/tap_application.yaml

@@ -0,0 +1,197 @@
+# slow_cooker --http-> gateway --grpc-> t1


what about reusing test/tap/testdata/tap_application.yaml?

I was thinking about that too, but then I thought it would be better to have a tap_application.yaml specifically for serviceprofiles. If we want to modify t* services for specific test scenarios. e.g modifying t3 to fail 50% of the time to test out and validate retries.

Makes sense, and I see you just added --percent-failure... mind bumping the bb images to v0.0.5 to match the original version?

siggy · 2019-04-08T23:32:38Z

test/serviceprofiles/serviceprofiles_test.go

+			cmd = append(cmd, tc.args...)
+			out, _, err := TestHelper.LinkerdRun(cmd...)
+			if err != nil {
+				t.Fatalf("profile command failed: %s\n", err.Error())


i'm getting a failure on this test (i'll dig into what's happening):

--- FAIL: TestServiceProfiles (33.18s) --- FAIL: TestServiceProfiles/tap (11.57s) serviceprofiles_test.go:106: profile command failed: exit status 1 --- PASS: TestServiceProfiles/open-api (0.93s)

...mind adding a bit more output to help troubleshoot?

if err != nil { t.Fatalf("'linkerd %s' command failed with %s: %s\n", cmd, err.Error(), out) }

l5d-bot · 2019-04-08T23:41:06Z

Integration test results for 29021e7: fail 😕
Log output: https://gist.github.com/1e79ad2cdbd250b787962861273485c3

siggy · 2019-04-08T23:50:52Z

fwiw i'm getting the same failure locally: https://gist.github.com/l5d-bot/1e79ad2cdbd250b787962861273485c3

Signed-off-by: Dennis Adjei-Baah <[email protected]>

l5d-bot · 2019-04-09T00:27:56Z

Integration test results for f965976: success 🎉
Log output: https://gist.github.com/0c928bb2cef63271cb95899ac21206fe

siggy

lgtm! one more fix but good to go after that 👍 🚢

siggy · 2019-04-09T01:38:02Z

test/serviceprofiles/serviceprofiles_test.go

+			cmd = append(cmd, tc.args...)
+			out, stderr, err := TestHelper.LinkerdRun(cmd...)
+			if err != nil {
+				t.Fatalf("'linkerd %s' command failed with %s: %s\n", cmd, err.Error(), stderr)


this failed on my local system with:

$ go test -v ./test/serviceprofiles -integration-tests -linkerd `pwd`/bin/linkerd === RUN TestServiceProfiles === RUN TestServiceProfiles/tap === RUN TestServiceProfiles/open-api --- FAIL: TestServiceProfiles (35.05s) --- FAIL: TestServiceProfiles/tap (10.43s) serviceprofiles_test.go:102: 'linkerd [profile --namespace l5d-integration-serviceprofile-test t1-svc --tap deploy/t1 --tap-route-limit 5 --tap-duration 10s]' command failed with exit status 1: Error: Tap duration exceeded, try increasing --tap-duration Usage:

i'm guessing it's a timing issue between slow-cooker's sleep 15 at startup, and the profile --tap --tap-duration 10s not seeing any requests.

i think you can mitigate this by increasing --tap-duration to 25s, but also decrease --tap-route-limit to 1, so the command should complete as soon as it sees the first request.

diff --git a/test/serviceprofiles/serviceprofiles_test.go b/test/serviceprofiles/serviceprofiles_test.go index 34803828..c3fadc26 100644 --- a/test/serviceprofiles/serviceprofiles_test.go +++ b/test/serviceprofiles/serviceprofiles_test.go @@ -84,9 +84,9 @@ func TestServiceProfiles(t *testing.T) { tc.args = []string{ tc.deployName, "--tap-route-limit", - "5", + "1", "--tap-duration", - "10s", + "25s", } }

siggy · 2019-04-09T01:39:56Z

test/serviceprofiles/serviceprofiles_test.go

+}
+
+func getRoutes(deployName, namespace string) ([]string, error) {
+	cmd := []string{"routes", "--namespace", namespace, deployName}


not necessarily for this PR, but it may make parsing easier if you add --output json

ihcsim

👍

Running test [serviceprofiles_test.go] 
=== RUN   TestServiceProfiles
=== RUN   TestServiceProfiles/tap
=== RUN   TestServiceProfiles/open-api
--- PASS: TestServiceProfiles (26.97s)
    --- PASS: TestServiceProfiles/tap (10.40s)
    --- PASS: TestServiceProfiles/open-api (0.38s)
PASS
ok      command-line-arguments  27.098s

Signed-off-by: Dennis Adjei-Baah <[email protected]>

l5d-bot · 2019-04-09T17:15:54Z

Integration test results for d1316ef: success 🎉
Log output: https://gist.github.com/d7c15290549c3564fbcb7fa3aee52326

dadjeibaah self-assigned this Apr 4, 2019

dadjeibaah requested review from alpeb, ihcsim, klingerf and siggy April 4, 2019 21:17

alpeb reviewed Apr 5, 2019

View reviewed changes

siggy added area/profiles area/test priority/P1 Planned for Release labels Apr 5, 2019

siggy reviewed Apr 5, 2019

View reviewed changes

ihcsim reviewed Apr 5, 2019

View reviewed changes

Dennis Adjei-Baah added 6 commits April 8, 2019 10:26

add integration tests for service profiles

76d30bb

Signed-off-by: Dennis Adjei-Baah <[email protected]>

generalize service profile tap tests

faf2d6f

Signed-off-by: Dennis Adjei-Baah <[email protected]>

add test for sp with swagger files

5050f81

Signed-off-by: Dennis Adjei-Baah <[email protected]>

add service profile test for proto

a9ef0b9

Signed-off-by: Dennis Adjei-Baah <[email protected]>

code cleanup

0924f0b

Signed-off-by: Dennis Adjei-Baah <[email protected]>

refactor service profile integraton tests

ccbe530

Signed-off-by: Dennis Adjei-Baah <[email protected]>

dadjeibaah force-pushed the dad/sp-integration-tests branch from c4b4f3b to ccbe530 Compare April 8, 2019 17:27

correct golint issue in test file

ff99ca3

Signed-off-by: Dennis Adjei-Baah <[email protected]>

differentiate test runs

ee5188a

Signed-off-by: Dennis Adjei-Baah <[email protected]>

ihcsim reviewed Apr 8, 2019

View reviewed changes

address PR feedback

29021e7

Signed-off-by: Dennis Adjei-Baah <[email protected]>

siggy reviewed Apr 8, 2019

View reviewed changes

fix yaml indentation error

f965976

Signed-off-by: Dennis Adjei-Baah <[email protected]>

siggy approved these changes Apr 9, 2019

View reviewed changes

ihcsim approved these changes Apr 9, 2019

View reviewed changes

increase profile tap duration in tests

d1316ef

Signed-off-by: Dennis Adjei-Baah <[email protected]>

dadjeibaah merged commit c166b1d into master Apr 9, 2019

dadjeibaah deleted the dad/sp-integration-tests branch April 9, 2019 17:16

admc mentioned this pull request Apr 9, 2019

Linkerd 2.3 release test plan #2459

Closed

25 tasks

dadjeibaah mentioned this pull request Apr 11, 2019

add service profile integration tests for service profile metrics #2685

Merged

Conversation

dadjeibaah commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l5d-bot commented Apr 4, 2019

Uh oh!

alpeb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dadjeibaah commented Apr 5, 2019

Uh oh!

ihcsim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l5d-bot commented Apr 8, 2019

Uh oh!

l5d-bot commented Apr 8, 2019

Uh oh!

l5d-bot commented Apr 8, 2019

Uh oh!

ihcsim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l5d-bot commented Apr 8, 2019

Uh oh!

siggy commented Apr 8, 2019

Uh oh!

l5d-bot commented Apr 9, 2019

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dadjeibaah commented Apr 4, 2019 •

edited

Loading