Migrate to Operator SDK 0.1.0 #116

jpkrohling · 2018-11-19T16:53:30Z

Signed-off-by: Juraci Paixão Kröhling [email protected]

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling · 2018-11-19T16:53:36Z

This change is

codecov · 2018-11-19T16:56:44Z

Codecov Report

Merging #116 into master will decrease coverage by 7.15%.
The diff coverage is 40.31%.

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
- Coverage    99.5%   92.35%   -7.16%     
==========================================
  Files          24       25       +1     
  Lines        1002     1072      +70     
==========================================
- Hits          997      990       -7     
- Misses          5       82      +77

Impacted Files	Coverage Δ
pkg/apis/io/v1alpha1/builder.go	`0% <ø> (ø)`
pkg/controller/jaeger/jaeger_controller.go	`0% <0%> (ø)`
pkg/apis/io/v1alpha1/jaeger_types.go	`100% <100%> (ø)`
pkg/controller/jaeger/controller.go	`100% <100%> (ø)`
pkg/controller/jaeger/all-in-one.go	`100% <100%> (ø)`
pkg/controller/jaeger/production.go	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update effbb55...07d527b. Read the comment docs.

jpkrohling · 2018-11-19T16:59:49Z

Review notes:

I tried to avoid a refactoring as much as I could, as the migration of the SDK itself is already a big change
The new SDK has a package called controller, which conflicts with ours. I'll rename our controllers to something else (suggestions?) in a follow-up PR
There are quite a lot of vendor changes
One e2e-test is failing for me (DaemonSet), but that test at master is also failing to me. Could you check whether it also fails for you? I was pretty sure all the tests worked, so, it might be something specific to my machine that I have to figure out.
I followed their migration guide at https://github.com/operator-framework/operator-sdk/blob/master/doc/migration/v0.1.0-migration-guide.md . The approach is basically: start a new project and copy the operator files over to the new project.
The main change at our side, is at the controller package. The stub package was removed, including its Handler. Now, it works by having a Reconcile method for each observed type. You'll notice we have two controllers, each with its own Reconcile. One is our Jaeger type, and the second is for the Deployment type, to inject an agent sidecar.

jpkrohling · 2018-11-19T17:01:15Z

Still to figure out:

The test failure for the DaemonSet test
The codecov check. We might just bypass it for this PR, as it will probably get back on track once we move our old controller under a new package

objectiser · 2018-11-19T21:47:17Z

@jpkrohling Do the CONTRIBUTING.adoc instructions need updating?

When running make build, I get:

Formatting code...
fatal: No names found, cannot describe anything.
Building...

When running make test I get:

Running end-to-end tests...
2018/11/19 21:44:27 could not find sa manifest: open deploy/sa.yaml: no such file or directory

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling · 2018-11-20T08:42:18Z

Formatting code...
fatal: No names found, cannot describe anything.
Building...

I don't see this, and I'm not sure what might be wrong there. The format target simply runs go fmt. Could you try the following commands, which should be equivalent to make format?

go fmt $(go list ./cmd/... ./pkg/...)

About the other problem:

Running end-to-end tests...
2018/11/19 21:44:27 could not find sa manifest: open deploy/sa.yaml: no such file or directory

Which version of the operator-sdk do you have in the path? I have 0.1.1, but 0.1.0 should be fine as well.

$ operator-sdk --version
operator-sdk version v0.1.1

objectiser · 2018-11-20T09:16:57Z

Still had old operator - once updated:

?   	github.com/jaegertracing/jaeger-operator/pkg/version	[no test files]
Unable to connect to the server: dial tcp 192.168.39.217:8443: connect: no route to host
Unable to connect to the server: dial tcp 192.168.39.217:8443: connect: no route to host
Unable to connect to the server: dial tcp 192.168.39.217:8443: connect: no route to host
Running end-to-end tests...
2018/11/20 09:15:14 failed to set up framework: failed to build the dynamic client: Get https://192.168.39.217:8443/api?timeout=32s: dial tcp 192.168.39.217:8443: connect: no route to host
FAIL	github.com/jaegertracing/jaeger-operator/test/e2e	0.936s
2018/11/20 09:15:14 failed to exec `go test ./test/e2e/... -kubeconfig /home/gbrown/.kube/config -namespacedMan deploy/test/namespace-manifests.yaml -globalMan deploy/test/global-manifests.yaml -root /home/gbrown/repositories/go/src/github.com/jaegertracing/jaeger-operator`: exit status 1
make: *** [Makefile:62: e2e-tests] Error 1

Running the go fmt $(go list ./cmd/... ./pkg/...) command returns no output (or errors).

jpkrohling · 2018-11-20T09:38:07Z

2018/11/20 09:15:14 failed to set up framework: failed to build the dynamic client: Get https://192.168.39.217:8443/api?timeout=32s: dial tcp 192.168.39.217:8443: connect: no route to host

About this one, this is probably because you don't have Minikube running.

Running the go fmt $(go list ./cmd/... ./pkg/...) command returns no output (or errors).

This is intriguing. Are you able to add some debugging to the Makefile, format target? Some echos around the @go fmt command would be a good start.

objectiser · 2018-11-20T10:04:40Z

Sorry not on the ball today - once I started minikube, I get the same error as you (I guess):

Running end-to-end tests...
time="2018-11-20T09:58:47Z" level=info msg="passing &{{Jaeger io.jaegertracing/v1alpha1} {agent-as-daemonset  jaeger-jaeger-group-daemonset-1542707917    0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] nil [] } {allInOne { {map[]} {[] [] map[] {map[] map[]}}} {0  {map[]} {[] [] map[] {map[] map[]}}} {0  {map[]} {[] [] map[] {map[] map[]}}} {DaemonSet  {map[log-level:debug]} {[] [] map[] {map[] map[]}}} { {map[]} {<nil>   }} {<nil>  {[] [] map[] {map[] map[]}}} {[] [] map[] {map[] map[]}}} {}}"
--- FAIL: TestJaeger (70.43s)
    --- FAIL: TestJaeger/jaeger-group (70.40s)
        --- FAIL: TestJaeger/jaeger-group/daemonset (70.40s)
        	client.go:57: resource type ServiceAccount with namespace/name (jaeger-jaeger-group-daemonset-1542707917/jaeger-operator) created
        	client.go:57: resource type Role with namespace/name (jaeger-jaeger-group-daemonset-1542707917/jaeger-operator) created
        	client.go:57: resource type RoleBinding with namespace/name (jaeger-jaeger-group-daemonset-1542707917/jaeger-operator) created
        	client.go:57: resource type Deployment with namespace/name (jaeger-jaeger-group-daemonset-1542707917/jaeger-operator) created
        	jaeger_test.go:57: Initialized cluster resources. Namespace: jaeger-jaeger-group-daemonset-1542707917
        	wait_util.go:45: Waiting for full availability of jaeger-operator deployment (0/1)
        	wait_util.go:51: Deployment available (1/1)
        	client.go:57: resource type Jaeger with namespace/name (jaeger-jaeger-group-daemonset-1542707917/agent-as-daemonset) created
        	wait_util.go:46: Waiting for availability of agent-as-daemonset-agent-daemonset daemonset
        	wait_util.go:46: Waiting for availability of agent-as-daemonset-agent-daemonset daemonset
        	wait_util.go:46: Waiting for availability of agent-as-daemonset-agent-daemonset daemonset
        	wait_util.go:46: Waiting for availability of agent-as-daemonset-agent-daemonset daemonset
        	wait_util.go:46: Waiting for availability of agent-as-daemonset-agent-daemonset daemonset
        	wait_util.go:46: Waiting for availability of agent-as-daemonset-agent-daemonset daemonset
        	wait_util.go:55: Waiting for full availability of agent-as-daemonset-agent-daemonset daemonsets (0/1)
        	wait_util.go:55: Waiting for full availability of agent-as-daemonset-agent-daemonset daemonsets (0/1)
        	wait_util.go:55: Waiting for full availability of agent-as-daemonset-agent-daemonset daemonsets (0/1)
        	wait_util.go:55: Waiting for full availability of agent-as-daemonset-agent-daemonset daemonsets (0/1)
        	wait_util.go:55: Waiting for full availability of agent-as-daemonset-agent-daemonset daemonsets (0/1)
        	wait_util.go:55: Waiting for full availability of agent-as-daemonset-agent-daemonset daemonsets (0/1)
        	wait_util.go:55: Waiting for full availability of agent-as-daemonset-agent-daemonset daemonsets (0/1)
        	daemonset.go:29: timed out waiting for the condition
        	client.go:75: resource type Jaeger with namespace/name (jaeger-jaeger-group-daemonset-1542707917/agent-as-daemonset) successfully deleted
        	client.go:75: resource type Deployment with namespace/name (jaeger-jaeger-group-daemonset-1542707917/jaeger-operator) successfully deleted
        	client.go:75: resource type RoleBinding with namespace/name (jaeger-jaeger-group-daemonset-1542707917/jaeger-operator) successfully deleted
        	client.go:75: resource type Role with namespace/name (jaeger-jaeger-group-daemonset-1542707917/jaeger-operator) successfully deleted
        	client.go:75: resource type ServiceAccount with namespace/name (jaeger-jaeger-group-daemonset-1542707917/jaeger-operator) successfully deleted
FAIL

On the other thing - strange, if I run make format there is not error, but essentially all the build target is doing is calling format before it tries the build - so not clear what is running in between those two steps. Will keep looking.

objectiser · 2018-11-20T10:19:02Z

@jpkrohling It looks like the fatal message is from git describe --tags

objectiser · 2018-11-20T10:29:04Z

pkg/controller/add_deployment.go

@@ -0,0 +1,10 @@
+package controller


Is it a convention in the operator to name the files add_...? Can't they just be deployment.go and jaeger.go?

That's boilerplate code generated by the new operator SDK

objectiser · 2018-11-20T10:41:31Z

pkg/controller/jaeger/jaeger_controller.go

+}
+
+func (r *ReconcileJaeger) handleDependencies(ctrl Controller) error {
+	for _, dep := range ctrl.Dependencies() {


Can the dependencies be launched concurrently - rather than waiting on each one in turn?

Yes, I believe so. Would you open an issue for that? This is an existing code that just got moved here.

jpkrohling · 2018-11-20T11:10:58Z

@jpkrohling It looks like the fatal message is from git describe --tags

That's interesting. What I get is this:

$ git describe --tags
v1.8.0-3-g4722d4f

(which means that I'm at 4722d4f, 3 commits ahead of the v1.8.0 tag)

jpkrohling · 2018-11-20T11:12:15Z

once I started minikube, I get the same error as you (I guess):

You did not see this error with the previous version, right?

objectiser · 2018-11-20T11:13:27Z

No I didn't see that error before.

objectiser · 2018-11-20T11:35:20Z

test/e2e/jaeger_test.go

 	if err != nil {
 		t.Fatalf("failed to add custom resource scheme to framework: %v", err)
 	}

 	t.Run("jaeger-group", func(t *testing.T) {
-		t.Run("my-jaeger", JaegerAllInOne)
-		t.Run("my-other-jaeger", JaegerAllInOne)
+		// t.Run("my-jaeger", JaegerAllInOne)


Don't forget to uncomment these :)

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

objectiser · 2018-11-20T11:38:52Z

@jpkrohling Just went back to the current master and tried running the test, and now seeing it. So not sure when that started failing.

jpkrohling · 2018-11-20T11:40:27Z

Just went back to the current master and tried running the test, and now seeing it. So not sure when that started failing.

I'm almost sure I ran the tests before I started working on this task. I'll try to get it fixed today.

jpkrohling · 2018-11-20T12:58:43Z

git bisect tells me that:

13d7cc5db1b3b9effbd262e9f0e77d8a1b76d139 is the first bad commit

I'll open an issue and handle this test failure in a different PR.

Migrate to Operator SDK 0.1.0

6a63d41

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling requested a review from objectiser November 19, 2018 16:53

Uncomment target dependencies for e2e-tests

4722d4f

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

objectiser reviewed Nov 20, 2018

View reviewed changes

Uncomment e2e tests

07d527b

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling mentioned this pull request Nov 20, 2018

Add tests for new code from the operator SDK #119

Closed

objectiser approved these changes Nov 20, 2018

View reviewed changes

jpkrohling merged commit ee056b7 into jaegertracing:master Nov 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Operator SDK 0.1.0 #116

Migrate to Operator SDK 0.1.0 #116

jpkrohling commented Nov 19, 2018

jpkrohling commented Nov 19, 2018

codecov bot commented Nov 19, 2018 •

edited

Loading

jpkrohling commented Nov 19, 2018

jpkrohling commented Nov 19, 2018 •

edited

Loading

objectiser commented Nov 19, 2018

jpkrohling commented Nov 20, 2018

objectiser commented Nov 20, 2018

jpkrohling commented Nov 20, 2018

objectiser commented Nov 20, 2018

objectiser commented Nov 20, 2018

objectiser Nov 20, 2018

jpkrohling Nov 20, 2018 •

edited

Loading

objectiser Nov 20, 2018

jpkrohling Nov 20, 2018

objectiser Nov 20, 2018

jpkrohling commented Nov 20, 2018

jpkrohling commented Nov 20, 2018

objectiser commented Nov 20, 2018

objectiser Nov 20, 2018

objectiser commented Nov 20, 2018

jpkrohling commented Nov 20, 2018

jpkrohling commented Nov 20, 2018

Migrate to Operator SDK 0.1.0 #116

Migrate to Operator SDK 0.1.0 #116

Conversation

jpkrohling commented Nov 19, 2018

jpkrohling commented Nov 19, 2018

codecov bot commented Nov 19, 2018 • edited Loading

Codecov Report

jpkrohling commented Nov 19, 2018

jpkrohling commented Nov 19, 2018 • edited Loading

objectiser commented Nov 19, 2018

jpkrohling commented Nov 20, 2018

objectiser commented Nov 20, 2018

jpkrohling commented Nov 20, 2018

objectiser commented Nov 20, 2018

objectiser commented Nov 20, 2018

objectiser Nov 20, 2018

Choose a reason for hiding this comment

jpkrohling Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

objectiser Nov 20, 2018

Choose a reason for hiding this comment

jpkrohling Nov 20, 2018

Choose a reason for hiding this comment

objectiser Nov 20, 2018

Choose a reason for hiding this comment

jpkrohling commented Nov 20, 2018

jpkrohling commented Nov 20, 2018

objectiser commented Nov 20, 2018

objectiser Nov 20, 2018

Choose a reason for hiding this comment

objectiser commented Nov 20, 2018

jpkrohling commented Nov 20, 2018

jpkrohling commented Nov 20, 2018

codecov bot commented Nov 19, 2018 •

edited

Loading

jpkrohling commented Nov 19, 2018 •

edited

Loading

jpkrohling Nov 20, 2018 •

edited

Loading