Add link to openshift console #1142

rubenvp8510 · 2020-07-21T21:50:47Z

Signed-off-by: Ruben Vargas ruben.vp8510@gmail.com

codecov · 2020-07-21T22:00:40Z

Codecov Report

Merging #1142 into master will increase coverage by 0.05%.
The diff coverage is 89.23%.

@@            Coverage Diff             @@
##           master    #1142      +/-   ##
==========================================
+ Coverage   88.22%   88.28%   +0.05%     
==========================================
  Files          86       89       +3     
  Lines        5351     5496     +145     
==========================================
+ Hits         4721     4852     +131     
- Misses        466      474       +8     
- Partials      164      170       +6

Impacted Files	Coverage Δ
pkg/controller/jaeger/jaeger_controller.go	`36.46% <42.85%> (+0.25%)`	⬆️
pkg/controller/jaeger/consolelink.go	`79.48% <79.48%> (ø)`
pkg/consolelink/main.go	`95.00% <95.00%> (ø)`
pkg/inventory/consolelink.go	`100.00% <100.00%> (ø)`
pkg/strategy/all_in_one.go	`91.83% <100.00%> (+0.34%)`	⬆️
pkg/strategy/production.go	`97.36% <100.00%> (+0.07%)`	⬆️
pkg/strategy/strategy.go	`95.79% <100.00%> (+0.26%)`	⬆️
pkg/strategy/streaming.go	`97.10% <100.00%> (+0.04%)`	⬆️
pkg/storage/elasticsearch_dependencies.go	`100.00% <0.00%> (ø)`
... and 4 more

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 b59a4c3...7eba146. Read the comment docs.

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

jpkrohling

Seems to be going in the right direction!

pkg/consolelink/main.go

pkg/consolelink/main_test.go

pkg/consolelink/main.go

pkg/controller/jaeger/consolelink_test.go

pkg/controller/jaeger/jaeger_controller.go

pkg/strategy/all_in_one.go

jpkrohling · 2020-07-22T10:13:58Z

pkg/strategy/all_in_one.go

@@ -82,6 +84,9 @@ func newAllInOneStrategy(ctx context.Context, jaeger *v1.Jaeger) S {
 	if viper.GetString("platform") == v1.FlagPlatformOpenShift {
 		if q := route.NewQueryRoute(jaeger).Get(); nil != q {
 			c.routes = append(c.routes, *q)
+			if link := consolelink.Get(jaeger, q); link != nil {


This whole block looks strange, as neither route.NewQueryRoute nor consolelink.Get return nils. I think it can be simplified, can't it? They might even return the values directly instead of pointers.

Well, route.NewQueryRoute(jaeger).Get() could potentially return nil if ingress is not enable.

jaeger-operator/pkg/route/query.go

Line 25 in fd64f15

return nil

In the case of consoleLink Would we want to have a flag for disable/enable it? If not of course I can simplify those lines. :)

If we don't have a query, do we want a console link? I have a feeling that this will not run cleanly in plain Kubernetes. It might work right now because this is shielded at the controller, though.

IMO, if the route is protecting itself by checking whether the platform is openshift, then the console link should do the same.

This is what is happening here, isn't it? If there is no query, (if query returns nil) it won't create a link. Indeed this is checking the platform two lines above. It check the platform, then try to create a queryRoute, if query router is successfully created, then try to create a consoleLink.

pkg/strategy/production.go

… if consoleLink apply fails Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

…the href is updated Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

jpkrohling · 2020-07-23T08:16:30Z

pkg/controller/jaeger/jaeger_controller.go

@@ -336,6 +336,9 @@ func (r *ReconcileJaeger) apply(ctx context.Context, jaeger v1.Jaeger, str strat
 		if err := r.applyRoutes(ctx, jaeger, str.Routes()); err != nil {
 			return jaeger, tracing.HandleError(err, span)
 		}
+		if err := r.applyConsoleLinks(ctx, jaeger, r.updateHref(ctx, jaeger, str.ConsoleLinks())); err != nil {


The controller shouldn't know how to transform the existing console link into desired. In other words, the updateHref logic shouldn't happen here.

The way our operator works, this code here should only call applyConsoleLinks passing str.ConsoleLinks() as parameter, which builds the list of desired console links. The applyConsoleLinks will then get this list over to the inventory, which will compare the existing with the desired and build a map of operations: what should be created, what should be updated, and what should be deleted. The inventory should take care of carrying over the value from the desired to the existing, in case of an update (which is what your updateHref is doing). Take a look at what we are doing with the secret or deployment. The batch job isn't a good example.

pkg/inventory/consolelink.go

pkg/consolelink/main.go

objectiser · 2020-07-23T09:29:55Z

pkg/consolelink/main.go

+			Location:        consolev1.ApplicationMenu,
+			ApplicationMenu: &consolev1.ApplicationMenuSpec{},
+			Link: consolev1.Link{
+				Text: "Jaeger [" + jaeger.Name + "." + jaeger.Namespace + "]",


Can you remove the namespace from the link text, as it will only be shown when viewing that namespace in the console.

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

jpkrohling · 2020-07-31T14:19:38Z

There's something odd. I have two instances in two namespaces, with the same instance name:

$ kubectl get jaegers --all-namespaces
NAMESPACE       NAME       STATUS    VERSION   STRATEGY   STORAGE   AGE
myapp           simplest   Running   1.18.1    allinone   memory    2d2h
observability   simplest   Running   1.18.1    allinone   memory    2m42s

And somehow, the console links got kinda messed up:

$ kubectl get consolelinks --all-namespaces
NAME                                TEXT                URL                                               MENU   AGE
myapp.jaeger-consolelink-simplest   Jaeger [simplest]   https://simplest-observability.apps-crc.testing          9m2s

The link ended showing up in the web console for the myapp namespace, but with the link pointing to the instance at observability.

jpkrohling

See the comment about the scope for console links. The same premise we have for almost/all other objects isn't valid for console links, as it's cluster-scoped (as opposed to "namespaced").

jpkrohling · 2020-07-31T14:21:53Z

pkg/consolelink/main.go

+// RouteAnnotation used to annotate the link with the route name
+var RouteAnnotation = "consolelink.jaegertracing.io/route"
+
+// Get returns an ConsoleLink specification for the current instance


Suggested change

// Get returns an ConsoleLink specification for the current instance

// Get returns a ConsoleLink specification for the current instance

jpkrohling · 2020-07-31T14:29:17Z

pkg/consolelink/main.go

+
+	return &consolev1.ConsoleLink{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      jaeger.Namespace + ".jaeger-consolelink-" + jaeger.Name,


This name could be improved: this is a ConsoleLink object, so, we don't need consolelink in the name. The ConsoleLink resides in a specific namespace, so, the namespace doesn't need to be part of the name either. It's also using dots, which is uncommon for names. Here's how it turns up in a real case: observability.jaeger-consolelink-simplest.

I'd say that you can just use the jaeger.Name, like we have for the Route:

$ kubectl get routes -n observability NAME HOST/PORT PATH SERVICES PORT TERMINATION WILDCARD simplest simplest-observability.apps-crc.testing simplest-query <all> reencrypt None

For reference, here's how the some built-in console links look like:

$ kubectl get consolelinks --all-namespaces NAME TEXT URL MENU AGE observability.jaeger-consolelink-simplest Jaeger [simplest] https://simplest-observability.apps-crc.testing 8m14s openshift-blog OpenShift Blog https://blog.openshift.com 19d openshift-learning-portal Learning Portal https://learn.openshift.com/?ref=webconsole 19d

jpkrohling · 2020-07-31T14:31:36Z

pkg/consolelink/main_test.go

+func TestConsoleLinkGet(t *testing.T) {
+	jaegerName := "TestConsoleLinkJaeger"
+	jaegerNamespace := "TestNS"
+	routerNamer := "TestConsoleLinkRoute"


Suggested change

routerNamer := "TestConsoleLinkRoute"

routerName := "TestConsoleLinkRoute"

jpkrohling · 2020-07-31T14:41:52Z

pkg/controller/jaeger/consolelink.go

+	defer span.End()
+
+	opts := []client.ListOption{
+		client.InNamespace(jaeger.Namespace),


As mentioned in the comment right before this review, I had two instances with the same name in different namespaces. One thing that was weird, is that only one console link was created. Turns out, console links are cluster-scoped, so, this logic here will make one instance remove another instance's console link. You can see this when running make run-debug and have two instances in the cluster:

DEBU[0008] Reconciling Jaeger execution="2020-07-31 14:32:38.767272647 +0000 UTC" instance=simplest namespace=myapp DEBU[0009] creating console link consoleLink=myapp.jaeger-consolelink-simplest instance=simplest namespace=myapp DEBU[0009] deleting console link consoleLink=observability.jaeger-consolelink-simplest instance=simplest namespace= DEBU[0009] Reconciling Jaeger completed execution="2020-07-31 14:32:38.767272647 +0000 UTC" instance=simplest namespace=myapp DEBU[0009] Reconciling Jaeger execution="2020-07-31 14:32:39.084445352 +0000 UTC" instance=simplest namespace=observability DEBU[0009] creating console link consoleLink=observability.jaeger-consolelink-simplest instance=simplest namespace=observability DEBU[0009] deleting console link consoleLink=myapp.jaeger-consolelink-simplest instance=simplest namespace= DEBU[0009] Reconciling Jaeger completed execution="2020-07-31 14:32:39.084445352 +0000 UTC" instance=simplest namespace=observability

Note how the namespace in the "deleting console link" log entries is empty, which was the clue that led me to check the CRD's scope:

$ kubectl get crds consolelinks.console.openshift.io -o jsonpath='{.spec.scope}' Cluster

A couple of things to keep in mind then:

when the operator isn't running in cluster admin mode, it shouldn't even attempt to create a cluster object. Try running the operator with lower permissions, and see what happens

when the user running the operator does have cluster admin rights, the logic in this controller shouldn't filter entries by the namespace. Rather, it should instead check the app.kubernetes.io/instance and app.kubernetes.io/managed-by labels. If the instance in the label isn't the same in the current reconciliation, leave it alone (don't update, don't delete).

I'm ok with 1), We should check for cluster admin before trying to create cluster object.

Is not clear to me, I can fetch all consolelinks but checking for app.kubernetes.io/instance? In the case of two instances with same name in different namespaces, I'll have two of them with the same name... and we will have the same problem... or am I missunderstanding something? I was thinking on adding a new label with the namespace, so I can filter by namespace AND jaeger instance name. Does that make sense?

jpkrohling · 2020-07-31T14:51:22Z

pkg/controller/jaeger/consolelink_test.go

+				Name:      nsnExisting.Name,
+				Namespace: nsnExisting.Namespace,
+				Annotations: map[string]string{
+					consolelink.RouteAnnotation: "my-route-1",


When would this happen? If I'm understanding this correctly, this is testing that a ConsoleLink can point to a route in another namespace. Is that right?

This should test when the route, and jaeger instance names are the same, but they are on different namespaces. We neet to make sure that the associated route is the correct one, the one that is in the same namespace as the jaeger instance.

jpkrohling · 2020-07-31T14:55:53Z

pkg/controller/jaeger/consolelink_test.go

+			},
+		}}).WithRoutes([]osroutev1.Route{{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "my-route-1",


I think this test needs a better description, and a few comments in the code. I spent quite a few minutes trying to figure out why you had host2 for my-route-1 when you specified host1 for my-route-1 before, until I realized that they were two different namespaces :-)

jpkrohling · 2020-07-31T14:56:39Z

pkg/controller/jaeger/jaeger_controller.go

+				jaeger.Logger().WithError(tracing.HandleError(err, span)).Warn("failed to reconcile console links")
+			}
+		} else {
+			jaeger.Logger().WithError(tracing.HandleError(err, span)).Warn("failed to reconcile console links")


Suggested change

jaeger.Logger().WithError(tracing.HandleError(err, span)).Warn("failed to reconcile console links")

jaeger.Logger().WithError(tracing.HandleError(err, span)).Warn("failed to obtain a list of routes to reconcile consolelinks")

…espaces, improve log messages, fix some code style issues Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

objectiser

LGTM - one nit, but just want to test it out (hopefully sometime today).

objectiser · 2020-08-03T10:28:13Z

pkg/consolelink/main.go

@@ -22,10 +22,12 @@ func Get(jaeger *v1.Jaeger, route *routev1.Route) *consolev1.ConsoleLink {

 	return &consolev1.ConsoleLink{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      jaeger.Namespace + ".jaeger-consolelink-" + jaeger.Name,
-			Namespace: jaeger.Namespace,
+			Name:      "jaeger-" + jaeger.Namespace + "-" + jaeger.Name,


nit: Could you create a function to derive the name from the jaeger instance? and then also use from the tests (main_test, but also the various strategy tests).

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 changed the title ~~TRACING-1230 Add link to openshift console~~ [TRACING-1230] Add link to openshift console Jul 21, 2020

Add link to openshift console

5e0607e

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the TRACING-1230-2 branch from 0a2dc74 to 5e0607e Compare July 21, 2020 22:06

jpkrohling changed the title ~~[TRACING-1230] Add link to openshift console~~ Add link to openshift console Jul 22, 2020

jpkrohling reviewed Jul 22, 2020

View reviewed changes

rubenvp8510 added 4 commits July 22, 2020 11:17

Fixed some code style comments, allow reconciliation process continue…

418f6ff

… if consoleLink apply fails Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

Add jaeger instance name to the link text

b1ae803

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

update href on jaeger controller

0c7ea2b

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

return a copy of the consolelink instead of modify the argument when …

f62ed66

…the href is updated Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the TRACING-1230-2 branch from 9f5d27b to f62ed66 Compare July 22, 2020 17:46

add consolelink tests for all strategies

1d8121d

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

objectiser added the Do Not Merge The PR should not be merged yet. label Jul 22, 2020

avoid warning, set namespace on consolelink CR

d266ad8

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

jpkrohling reviewed Jul 23, 2020

View reviewed changes

pkg/inventory/consolelink.go Show resolved Hide resolved

objectiser reviewed Jul 23, 2020

View reviewed changes

rubenvp8510 added 2 commits July 23, 2020 11:03

change consolelink location to NamespaceDashboard

a7f7a61

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

change update href function, move update to strategy

773576e

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

objectiser removed the Do Not Merge The PR should not be merged yet. label Jul 31, 2020

jpkrohling reviewed Jul 31, 2020

View reviewed changes

rubenvp8510 force-pushed the TRACING-1230-2 branch from a5759be to d7189fd Compare August 2, 2020 20:04

Fix console links when creating two jaeger same name in different nam…

6a4a3df

…espaces, improve log messages, fix some code style issues Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the TRACING-1230-2 branch from d7189fd to 6a4a3df Compare August 2, 2020 21:33

Fix some tests, improve readability of others with comments

04e2ac5

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

objectiser reviewed Aug 3, 2020

View reviewed changes

Name function to derivate consolelink name, and use it on tests

7eba146

Signed-off-by: Ruben Vargas <ruben.vp8510@gmail.com>

rubenvp8510 force-pushed the TRACING-1230-2 branch from 8c1704f to 7eba146 Compare August 3, 2020 19:39

objectiser approved these changes Aug 4, 2020

View reviewed changes

objectiser merged commit b24fd72 into jaegertracing:master Aug 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add link to openshift console #1142

Add link to openshift console #1142

rubenvp8510 commented Jul 21, 2020

codecov bot commented Jul 21, 2020 •

edited

Loading

jpkrohling left a comment

jpkrohling Jul 22, 2020

rubenvp8510 Jul 22, 2020

jpkrohling Jul 23, 2020

rubenvp8510 Jul 23, 2020

jpkrohling Jul 23, 2020

objectiser Jul 23, 2020

rubenvp8510 Jul 23, 2020

rubenvp8510 Jul 23, 2020

jpkrohling commented Jul 31, 2020

jpkrohling left a comment

jpkrohling Jul 31, 2020

jpkrohling Jul 31, 2020

jpkrohling Jul 31, 2020

jpkrohling Jul 31, 2020

rubenvp8510 Aug 1, 2020

jpkrohling Jul 31, 2020

rubenvp8510 Aug 2, 2020

jpkrohling Jul 31, 2020

jpkrohling Jul 31, 2020

objectiser left a comment

objectiser Aug 3, 2020

	// Get returns an ConsoleLink specification for the current instance
	// Get returns a ConsoleLink specification for the current instance

	routerNamer := "TestConsoleLinkRoute"
	routerName := "TestConsoleLinkRoute"

	jaeger.Logger().WithError(tracing.HandleError(err, span)).Warn("failed to reconcile console links")
	jaeger.Logger().WithError(tracing.HandleError(err, span)).Warn("failed to obtain a list of routes to reconcile consolelinks")

Add link to openshift console #1142

Add link to openshift console #1142

Conversation

rubenvp8510 commented Jul 21, 2020

codecov bot commented Jul 21, 2020 • edited Loading

Codecov Report

jpkrohling 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

jpkrohling commented Jul 31, 2020

jpkrohling 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

objectiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2020 •

edited

Loading