Handle normalization of host:port addresses in operator upgrade for 1.18 #1033

rubenvp8510 · 2020-04-21T18:49:34Z

Signed-off-by: Ruben Vargas [email protected]

objectiser · 2020-04-23T13:58:50Z

@rubenvp8510 Is this one ready for review? Still has the WIP

rubenvp8510 · 2020-04-23T15:09:56Z

@rubenvp8510 Is this one ready for review? Still has the WIP

@objectiser This is in WIP due the fact that the test fails because latest = 1_17_1, but the rest of the work is ready for review.

pavolloffay · 2020-04-23T15:18:19Z

pkg/upgrade/v1_18_0.go

+	return jaeger, nil
+}
+
+func transformCollectorPorts(jaeger *v1.Jaeger, opts v1.Options, flagMap []deprecationFlagMap) v1.Options {


I would narrow down the scope of params. Jaeger instance is not needed just the logger.

nit: give flagMap a more specific name

+1 good observation, I'll do the changes.

nit: give flagMap a more specific name

Ok, I'll try to give it a better name

pavolloffay · 2020-04-23T15:22:56Z

pkg/upgrade/v1_18_0.go

+		"collector.host-port",
+		"reporter.tchannel.discovery.conn-check-timeout",
+		"reporter.tchannel.discovery.min-peers",
+		"reporter.tchannel.host-port",


Does the operator configure --reporter.grpc.host-port when the channel flags are removed?

Oh no, not at this time.

pkg/upgrade/v1_18_0.go

pavolloffay · 2020-04-24T14:34:19Z

pkg/upgrade/v1_18_0.go

+	return v1.NewOptions(in)
+}
+
+func migrateAgentOptions(jaeger *v1.Jaeger, collectorGrpcPort string) v1.Options {


I like that you moved code for agent to a separate method. Maybe It's better to also move collector migration code to a separate method and method upgrade1_18_0 would call two methods one for agent and one for collector.

pavolloffay · 2020-04-24T14:38:56Z

pkg/upgrade/v1_18_0.go

+)
+
+func upgrade1_18_0(ctx context.Context, client client.Client, jaeger v1.Jaeger) (v1.Jaeger, error) {
+	collectorDeprecatedFlags := []deprecationFlagMap{


Are we using one of these in our code base - either flag or env variable? We are definitely using zipkin (collector.zipkin.http-port) in one or another form.

I think we are using admin-http-port and collector.zipkin.http-port I don't see where we are using the other flags.

pavolloffay · 2020-04-24T14:44:07Z

pkg/upgrade/v1_18_0.go

+			"from": d.from,
+			"to":   d.to,
+		}).Debug("flag value migrated")
+		if val, exists := in[d.to]; exists {


Let's say somebody already created 1.18 CR with new flag e.g. --admin.http.host-port. Then the value would be :host:port.

I am not sure at what time this method is called, so I am not sure if it can happen.

if the CR is already on 1.18 this upgrade method should not be called, this is only called when we have an CR with version < 1.18 (and before call this v1_18_0 function, v1_17_0 should be called.. and before v1_15_0 and so on..)

codecov · 2020-04-28T20:17:05Z

Codecov Report

Merging #1033 into master will increase coverage by 0.24%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1033      +/-   ##
==========================================
+ Coverage   64.34%   64.58%   +0.24%     
==========================================
  Files          83       84       +1     
  Lines        6669     6715      +46     
==========================================
+ Hits         4291     4337      +46     
  Misses       2236     2236              
  Partials      142      142

Impacted Files	Coverage Δ
pkg/upgrade/v1_18_0.go	`100.00% <100.00%> (ø)`

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 db31a0c...88d7cd7. Read the comment docs.

pavolloffay

LGTM, I would just remove those empty lines.

pavolloffay · 2020-04-29T09:38:28Z

pkg/upgrade/v1_18_0.go

+	}
+	opts := migrateDeprecatedOptions(jaeger, jaeger.Spec.Collector.Options, collectorDeprecatedFlags)
+	return transformCollectorPorts(jaeger.Logger(), opts, collectorDeprecatedFlags)
+


nit: remove line

pavolloffay · 2020-04-29T09:39:33Z

pkg/upgrade/v1_18_0.go

+	}
+
+	return v1.NewOptions(opsMap)
+


nit: remove empty line

pavolloffay · 2020-04-29T09:39:44Z

pkg/upgrade/v1_18_0.go

+}
+
+func migrateAgentOptions(jaeger *v1.Jaeger) v1.Options {
+


nit: remove empty line?

pavolloffay · 2020-04-29T09:41:17Z

pkg/upgrade/v1_18_0.go

+	"context"
+	"fmt"
+
+	"sigs.k8s.io/controller-runtime/pkg/client"


this should be joined with the following block.

our import format tool seems to be flaky sometimes.

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 · 2020-05-01T16:51:06Z

I've already rebased this PR. Any other comment?

Thanks

objectiser requested a review from pavolloffay April 23, 2020 15:10

pavolloffay reviewed Apr 23, 2020

View reviewed changes

pavolloffay reviewed Apr 27, 2020

View reviewed changes

rubenvp8510 changed the title ~~WIP - Handle normalization of host:port addresses in operator upgrade for 1.18~~ Handle normalization of host:port addresses in operator upgrade for 1.18 Apr 28, 2020

pavolloffay approved these changes Apr 29, 2020

View reviewed changes

rubenvp8510 force-pushed the normalization-portaddress-TRACING-1111 branch 2 times, most recently from 85914fc to 862f9f7 Compare April 29, 2020 14:00

rubenvp8510 added 8 commits April 29, 2020 12:47

Handle normalization of host:port addresses in operator upgrade for 1.18

02582aa

Signed-off-by: Ruben Vargas <[email protected]>

Enable grpc on agent insteat of tchannel

1b1c253

Signed-off-by: Ruben Vargas <[email protected]>

Address comments, improve readability

2c90f87

Signed-off-by: Ruben Vargas <[email protected]>

Remove specific collector port support for upgrade to 0.18

7240518

Signed-off-by: Ruben Vargas <[email protected]>

Put all collector migrations on a single function

e36820a

Signed-off-by: Ruben Vargas <[email protected]>

Make 1.18 upgrade test pass

4c9c439

Signed-off-by: Ruben Vargas <[email protected]>

Format issues fixed

c4cd6f7

Signed-off-by: Ruben Vargas <[email protected]>

Update upgrade 1.18 tests

88d7cd7

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the normalization-portaddress-TRACING-1111 branch from 862f9f7 to 88d7cd7 Compare April 29, 2020 17:51

pavolloffay merged commit 67dc7d3 into jaegertracing:master May 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle normalization of host:port addresses in operator upgrade for 1.18 #1033

Handle normalization of host:port addresses in operator upgrade for 1.18 #1033

rubenvp8510 commented Apr 21, 2020

objectiser commented Apr 23, 2020

rubenvp8510 commented Apr 23, 2020

pavolloffay Apr 23, 2020

pavolloffay Apr 23, 2020

rubenvp8510 Apr 23, 2020

rubenvp8510 Apr 23, 2020

pavolloffay Apr 23, 2020

rubenvp8510 Apr 23, 2020

pavolloffay Apr 24, 2020

pavolloffay Apr 24, 2020

rubenvp8510 Apr 28, 2020

pavolloffay Apr 24, 2020

rubenvp8510 Apr 28, 2020

codecov bot commented Apr 28, 2020 •

edited

Loading

pavolloffay left a comment

pavolloffay Apr 29, 2020

pavolloffay Apr 29, 2020

pavolloffay Apr 29, 2020

pavolloffay Apr 29, 2020

rubenvp8510 commented May 1, 2020

Handle normalization of host:port addresses in operator upgrade for 1.18 #1033

Handle normalization of host:port addresses in operator upgrade for 1.18 #1033

Conversation

rubenvp8510 commented Apr 21, 2020

objectiser commented Apr 23, 2020

rubenvp8510 commented Apr 23, 2020

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

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2020 • edited Loading

Codecov Report

pavolloffay 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

rubenvp8510 commented May 1, 2020

codecov bot commented Apr 28, 2020 •

edited

Loading