pkg/test: add in-cluster/in-image testing support by AlexNPavel · Pull Request #469 · operator-framework/operator-sdk

AlexNPavel · 2018-09-10T22:35:12Z

Issue #435

shawn-hurley · 2018-09-11T15:56:55Z

 `,
 		Run: buildFunc,
 	}
+	buildCmd.Flags().BoolVarP(&enableTests, "enable-tests", "e", false, "Enable in-cluster testing by adding test binary to the image")


Are keeping the default false for now and plan on changing it? I think eventually this should be a default of true? Thoughts?

For now, we will keep it as default false. The SDK does not create any default tests for operators and the build would fail in that case. This is why we need a user to confirm that they have tests that they want to include. The test binary is also 34 MB, which may be undesirable to include by default.

shawn-hurley · 2018-09-11T16:06:35Z

-	dbcmd.Env = append(os.Environ(), fmt.Sprintf("IMAGE=%v", image))
+	intermediateImageName := image
+	if enableTests {
+		if namespacedManBuild == "" {


I wonder if pulling this code block into its own function may make this more readable?

Issue operator-framework#435

The cluster test has defers that need to run on error, which would not run if cmdError is used due to its use of os.Exit(). Instead, we can just return an error, which allows the defer functions to run.

This adds tests for the new incluster image testing mode as well as fixes some bugs that this test exposed

We don't need to do any extra operations anymore if a user does not supply a namespaced manifest path, so we can just use a default string

hasbro17 · 2018-09-24T03:13:40Z

+    command: ["/go-test.sh"]
+    resources:
+      requests:
+        cpu: 1


I don't think we should specify resource requests by default. This could prevent the pod from being scheduled onto a node if there isn't enough cpu.
By not setting a default, the pod will either take on the default request and limit enforced by the namespace, or use as much as is available.

hasbro17 · 2018-09-24T03:16:21Z

+func renderTestManifest(image string) error {
+	namespacedBytes, err := ioutil.ReadFile(namespacedManBuild)
+	if err != nil {
+		log.Fatalf("could not read rbac manifest: %v", err)


It's not rbac manifest(at least not in the test cluster case).
log.Fatalf("could not read namespaced manifest: %v", err)

hasbro17 · 2018-09-24T03:20:56Z

+		log.Fatalf("could not read rbac manifest: %v", err)
+	}
+	if err = generator.RenderTestYaml(cmdutil.GetConfig(), image); err != nil {
+		cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("failed to generate deploy/test-pod.yaml: (%v)", err))


You can use log.Fatalf() instead of cmdError.ExitWithError(). It's the exact same thing except it returns a specific error code.
We're going to remove cmdError.ExitWithError since returning error codes isn't as useful as we thought initially, and we never specified more than two codes.

hasbro17 · 2018-09-24T03:25:22Z

+	if err = generator.RenderTestYaml(cmdutil.GetConfig(), image); err != nil {
+		cmdError.ExitWithError(cmdError.ExitError, fmt.Errorf("failed to generate deploy/test-pod.yaml: (%v)", err))
+	}
+	return verifyDeploymentImage(namespacedBytes, image)


The only error renderTestManifest() returns is actually just treated as a warning that's just printed out.

genWarning := renderTestManifest(image) if genWarning != nil { fmt.Printf("%s\n", genWarning) }

For the other two errors it just fails fast with log.Fatal.
So change renderTestManifest() to not return an error.
And print the warning message from verifyDeploymentImage() either here or outside this function by just checking the error returned directly from it.

hasbro17 · 2018-09-24T03:46:21Z


 echo "building container ${IMAGE}..."
-docker build -t "${IMAGE}" -f tmp/build/Dockerfile .
+docker build -t "${IMAGE}" -f tmp/build/Dockerfile . --build-arg NAMESPACEDMAN=$NAMESPACEDMAN


We're no longer using this docker-build.sh script anymore right? We're just calling docker build directly in operator-sdk build.
So we should probably not generate this anymore.

hasbro17 · 2018-09-24T05:19:28Z

 		return err
 	}
-	return renderWriteFile(filepath.Join(buildDir, dockerfile), "tmp/build/Dockerfile", dockerFileTmpl, dTd)
+	return writeFileAndPrint(filepath.Join(buildDir, goTest), buf.Bytes(), os.FileMode(int(0755)))


Use the constant defaultExecFileMode .

defaultExecMode is currently set to 0744, which doesn't work for this setup. Should I change defaultExecMode to 0755?

hasbro17 · 2018-09-24T05:56:39Z

+	inCluster := false
+	if *kubeconfigPath == "incluster" {
+		// Work around https://github.com/kubernetes/kubernetes/issues/40973
+		// See https://github.com/coreos/etcd-operator/issues/731#issuecomment-283804819


nit: We can remove the etcd-operator issue link since we're linking to the upstream issue in the line above.

hasbro17 · 2018-09-24T06:05:21Z

+				ImagePullPolicy: pullPolicy,
+				Command:         []string{"/go-test.sh"},
+				Env: []v1.EnvVar{{
+					Name:      "TEST_NAMESPACE",


Define this as a constant somewhere. In pkg/test probably.

hasbro17 · 2018-09-24T06:07:46Z

 	err = wait.PollImmediate(time.Second, time.Second*10, func() (done bool, err error) {
-		err = Global.DynamicClient.List(goctx.TODO(), &dynclient.ListOptions{Namespace: "default"}, obj)
+		if Global.InCluster {
+			err = Global.DynamicClient.List(goctx.TODO(), &dynclient.ListOptions{Namespace: os.Getenv("TEST_NAMESPACE")}, obj)


We should verify that TEST_NAMESPACE is not empty in the incluster case above, before using it here and in ctx.GetNamespace().

mhrivnak · 2018-09-24T14:04:33Z

+	}
+	testCmd.Flags().StringVarP(&testNamespace, "namespace", "n", "default", "Namespace to run tests in")
+	testCmd.Flags().StringVarP(&kubeconfigCluster, "kubeconfig", "k", defaultKubeConfig, "Kubeconfig path")
+	testCmd.Flags().StringVarP(&imagePullPolicy, "imagePullPolicy", "i", "Always", "Set test pod image pull policy. Allowed values: Always, Never")


One more nit here: Is there a reason to ask people to capitalize the words "always" and "never"? It's somewhat unusual for a CLI.

The reason was that they are capitalized in the config file. I could change it, or maybe use strings.ToLower(...) and support both.

We've decided to support both upper and lowercase

hasbro17 · 2018-09-24T23:15:26Z

+		log.Fatalf("failed to generate deploy/test-pod.yaml: (%v)", err)
+	}
+	// the error from verifyDeploymentImage is just a warning, not fatal error
+	fmt.Printf("%v", verifyDeploymentImage(namespacedBytes, image))


Does this print <nil> every time if there's no error returned by verifyDeploymentImage()?

Yes it does; I somehow missed that. I'll put a nil check there

hasbro17 · 2018-09-25T00:16:43Z

 	f := framework.Global
 	ctx := f.NewTestCtx(t)
 	defer ctx.Cleanup(t)
+	operatorYAML, err := ioutil.ReadFile("deploy/operator.yaml")


Check the returned error.

hasbro17 · 2018-09-25T00:21:17Z

LGTM after nit. Great work.
We'll need to update the test-framework docs after this.
https://github.com/operator-framework/operator-sdk/blob/master/doc/test-framework/writing-e2e-tests.md
https://github.com/operator-framework/operator-sdk/blob/master/doc/sdk-cli-reference.md#test

AlexNPavel force-pushed the incluster-test branch 2 times, most recently from 749d6e0 to 0cbc804 Compare September 11, 2018 00:25

shawn-hurley requested changes Sep 11, 2018

View reviewed changes

AlexNPavel added 4 commits September 12, 2018 14:03

pkg/test: add in-cluster/in-image testing support

b25c0d1

Issue operator-framework#435

*: change the way the dockerfiles are used

c0a67a7

commands/.../build.go: remove unused flag

923505c

commands/.../build.go: split some test stuff into new function

7cfeb77

AlexNPavel force-pushed the incluster-test branch from 0cbc804 to 7cfeb77 Compare September 12, 2018 21:19

AlexNPavel changed the title ~~pkg/test: add in-cluster/in-image testing support~~ [WIP] pkg/test: add in-cluster/in-image testing support Sep 12, 2018

AlexNPavel mentioned this pull request Sep 12, 2018

Operator Image Testing #435

Closed

AlexNPavel added 4 commits September 12, 2018 15:51

commands,templates: change test manifest handling

afffc38

commands/.../test: create new test subcommands

e401771

.travis.yml: update travis.yml to match test subcommands

046725f

commands/.../test/cluster.go: fix error capitalization

aa34ac0

AlexNPavel force-pushed the incluster-test branch from 6a1309b to aa34ac0 Compare September 13, 2018 16:32

AlexNPavel added 8 commits September 13, 2018 09:50

commands/.../test/*: fix subcommands

becee99

commands/.../test/*: make the tests work and improve output

2790687

commands/.../test/cluster.go: return errors instead of cmderror

49fc1c1

The cluster test has defers that need to run on error, which would not run if cmdError is used due to its use of os.Exit(). Instead, we can just return an error, which allows the defer functions to run.

*: add tests for incluster test image and fix bugs

d6f22e3

This adds tests for the new incluster image testing mode as well as fixes some bugs that this test exposed

Merge branch 'master' into incluster-test

732e049

commands/.../test/cluster.go: uncomment deferred cleanup

2441947

commands/.../build.go: shorten code a bit

73ea6e2

We don't need to do any extra operations anymore if a user does not supply a namespaced manifest path, so we can just use a default string

commands/.../build.go: more code shortening

4f303c7

AlexNPavel force-pushed the incluster-test branch from 389d16a to 4f303c7 Compare September 14, 2018 19:04

commands/.../build.go: change intermediateImageName to baseImageName

128f935

shawn-hurley approved these changes Sep 14, 2018

View reviewed changes

commands/.../test/cluster.go: clean up error messages

b4d18dc

AlexNPavel changed the title ~~[WIP] pkg/test: add in-cluster/in-image testing support~~ pkg/test: add in-cluster/in-image testing support Sep 14, 2018

AlexNPavel requested review from fanminshi and hasbro17 September 14, 2018 20:55

commands/.../build.go: make unmarshalling error fatal

50eb65c

AlexNPavel force-pushed the incluster-test branch from c152125 to 50eb65c Compare September 21, 2018 23:19

hasbro17 mentioned this pull request Sep 21, 2018

test-framework: allow running local tests in a predefined namespace #518

Closed

shawn-hurley approved these changes Sep 22, 2018

View reviewed changes

hasbro17 reviewed Sep 24, 2018

View reviewed changes

mhrivnak reviewed Sep 24, 2018

View reviewed changes

AlexNPavel added 6 commits September 24, 2018 12:26

commands,pkg: improvements suggested by haseeb

5102bb3

pkg/generator: remove docker_build.sh generation

d722d6e

Merge branch 'master' into incluster-test

b7900f9

pkg/generator/generator.go: change defaultExecFileMode to 0755

81be3d2

commands/.../test/cluster: support lowercase pull policy

ad18ed6

commands/.../test: use structs for test flags/vars

49ab763

hasbro17 reviewed Sep 24, 2018

View reviewed changes

commands/.../build: add nil error check

7947e47

hasbro17 reviewed Sep 25, 2018

View reviewed changes

test/e2e/memcached_test.go: check error from readfile

3e1dfcb

AlexNPavel mentioned this pull request Sep 25, 2018

Update Docs With Incluster Testing Support #526

Merged

AlexNPavel merged commit fe6d2a3 into operator-framework:master Sep 25, 2018

AlexNPavel deleted the incluster-test branch September 25, 2018 18:33

This was referenced Sep 26, 2018

Move build.sh into the sdk binary #541

Closed

commands/operator-sdk: Add CLI support for Ansible Operator #486

Merged

Conversation

AlexNPavel commented Sep 10, 2018

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!

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!

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!

hasbro17 commented Sep 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants