Update instances status using client.Status().update interface #1253

rubenvp8510 · 2020-10-14T03:18:19Z

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

codecov · 2020-10-14T03:23:27Z

Codecov Report

Merging #1253 into master will decrease coverage by 0.10%.
The diff coverage is 46.66%.

@@            Coverage Diff             @@
##           master    #1253      +/-   ##
==========================================
- Coverage   87.43%   87.32%   -0.11%     
==========================================
  Files          90       90              
  Lines        4933     4946      +13     
==========================================
+ Hits         4313     4319       +6     
- Misses        457      464       +7     
  Partials      163      163

Impacted Files	Coverage Δ
pkg/upgrade/upgrade.go	`55.68% <38.46%> (-2.22%)`	⬇️
pkg/controller/jaeger/jaeger_controller.go	`35.63% <100.00%> (+0.37%)`	⬆️

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 8b07b3f...ef5d183. Read the comment docs.

jpkrohling

This is missing a test. If you can reproduce the problem with a simple unit test, that's optimal, otherwise, an e2e test is required.

jpkrohling · 2020-10-14T07:22:50Z

pkg/controller/jaeger/jaeger_controller.go

+	}
+
+	// set the status version to the updated instance version
+	instance.Status.Version = updated.Status.Version


I don't think this is the right place to do this... We either do this during the instance creation, or after an upgrade.

This is missing a test. If you can reproduce the problem with a simple unit test, that's optimal, otherwise, an e2e test is required.

I'll add an e2e test, we have unit tests in place for the upgrade process, even with those tests we didn't detect the issue before.

jpkrohling · 2020-10-14T07:24:56Z

pkg/upgrade/upgrade.go

@@ -72,6 +72,15 @@ func ManagedInstances(ctx context.Context, c client.Client, reader client.Reader
 					"namespace": jaeger.Namespace,
 				}).WithError(err).Error("failed to store the upgraded instance")
 				tracing.HandleError(err, span)
+			} else {


It's more readable if you invert the situation: "if err == nil { success } else { failure }". We use "if err != nil" only when that's the only code branch.

Also, you might want to change from Update to Patch.

ok, that make sense, about changing Update to Patch I would prefer to do it in a separate PR if it's required.

jpkrohling

@kevinearls, could you review this one as well?

jpkrohling · 2020-10-19T08:48:57Z

pkg/upgrade/upgrade.go

-			if err := c.Update(ctx, &jaeger); err != nil {
+			if err := c.Update(ctx, &jaeger); err == nil {
+				jaeger.Status.Version = version
+				if err := c.Status().Update(ctx, &jaeger); err != nil {


s/Update/Patch/ ?

I can do the change but What would be the benefit of doing patch for this case?

Nevermind, I did the change.

jpkrohling · 2020-10-19T08:49:30Z

test/e2e/upgrade_test.go

+	"testing"
+)
+
+const EnvUpdateVersionKey = "UPDATE_TEST_VERSION"


Those two consts can be part of a single const () declaration.

jpkrohling · 2020-10-19T08:50:40Z

test/e2e/upgrade_test.go

+func TestOperatorUpgrade(t *testing.T) {
+
+	upgradeTestVersion := os.Getenv(EnvUpdateVersionKey)
+	t.Log(upgradeTestVersion)


Add some context to this log entry. Like, Attempting to upgrade to version ...

This log should not be here, was part of my debugging, but yeah a log message could be useful.

jpkrohling · 2020-10-19T08:51:45Z

test/e2e/upgrade_test.go

+	image := deployment.Spec.Template.Spec.Containers[0].Image
+	image = strings.Replace(image, "latest", upgradeTestTag, 1)
+	deployment.Spec.Template.Spec.Containers[0].Image = image
+	fw.Client.Update(context.Background(), deployment)


s/Update/Patch/?

jpkrohling · 2020-10-19T08:52:05Z

test/e2e/upgrade_test.go

+
+func TestOperatorUpgrade(t *testing.T) {
+
+	upgradeTestVersion := os.Getenv(EnvUpdateVersionKey)


What happens when the env var is empty?

It fails :)

When? With which message? Is it easy to understand what's going on?

is not, I'll add a validation here to make it more clear.

kevinearls · 2020-10-19T10:05:10Z

test/e2e/upgrade_test.go

+	}
+	fw = framework.Global
+	createdJaeger := &v1.Jaeger{}
+	key := types.NamespacedName{Name: "my-jaeger", Namespace: ctx.GetID()}


@rubenvp8510 I realize this is a short test, but I'd prefer if you followed the example of the other tests, used stretchr suites, and put all of this setup code in a SetupSuite(). That would make it consist with all of the other E2E tests

jpkrohling

Looks good to me, but the final review is on @kevinearls :-)

pkg/upgrade/upgrade.go

jpkrohling · 2020-10-20T12:20:49Z

pkg/upgrade/upgrade.go

+					log.WithFields(log.Fields{
+						"instance":  jaeger.Name,
+						"namespace": jaeger.Namespace,
+					}).WithError(err).Error("failed update status of the upgraded instance")


Suggested change

}).WithError(err).Error("failed update status of the upgraded instance")

}).WithError(err).Error("failed to update the status of the upgraded instance")

kevinearls · 2020-10-20T12:58:38Z

test/e2e/upgrade_test.go

+	require.Regexp(t, versionRegexp, upgradeTestVersion,
+		"Invalid upgrade version, need to specify a version to upgrade in format X.Y.Z")
+
+	ctx, err := prepare(t)


@rubenvp8510 A couple of more small things, which are more for consistently with other tests than correctness.

Can you move everything from the ""ctx, err := prepare()" down to "fw = framework.Global" into a SetupSuite() method, making sure to remove the "defer ctx.Cleanup() call.

Then add a TearDownSuite() which should look like this one: https://github.com/jaegertracing/jaeger-operator/blob/master/test/e2e/elasticsearch_test.go#L56-L58 handleSuiteTearDown will take care of the ctx.Cleanup()

rubenvp8510 · 2020-10-20T14:16:23Z

I've already done all requested changes :=)

Thanks for the review!

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

rubenvp8510 · 2020-10-27T14:17:44Z

Any other comments on this PR? :)

mergify bot assigned rubenvp8510 Oct 14, 2020

mergify bot requested a review from jpkrohling October 14, 2020 03:18

jpkrohling reviewed Oct 14, 2020

View reviewed changes

rubenvp8510 force-pushed the fix-upgrade branch 6 times, most recently from a8475a4 to 1eeb75d Compare October 15, 2020 22:26

jpkrohling reviewed Oct 19, 2020

View reviewed changes

jpkrohling requested a review from kevinearls October 19, 2020 08:52

kevinearls suggested changes Oct 19, 2020

View reviewed changes

rubenvp8510 force-pushed the fix-upgrade branch 2 times, most recently from cd4dd1e to 0a2e476 Compare October 19, 2020 18:47

rubenvp8510 requested review from jpkrohling and kevinearls October 20, 2020 11:53

jpkrohling reviewed Oct 20, 2020

View reviewed changes

kevinearls suggested changes Oct 20, 2020

View reviewed changes

rubenvp8510 requested a review from kevinearls October 20, 2020 15:19

kevinearls approved these changes Oct 20, 2020

View reviewed changes

rubenvp8510 force-pushed the fix-upgrade branch from f42f014 to 9c977e6 Compare October 21, 2020 11:37

rubenvp8510 requested a review from jpkrohling October 21, 2020 12:01

rubenvp8510 added 6 commits October 22, 2020 06:54

Update instances status using client.Status().update interface

6d1e50b

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

Upgrade e2e tests

ec23b98

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

Minor changes to upgrade code

1780a03

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

Use test suites for upgrade test

7fe151c

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

Change update calls to patch on jaeger upgrade process

bbdbed6

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

Move test initialization to SetupTest

ef5d183

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

rubenvp8510 force-pushed the fix-upgrade branch from 9c977e6 to ef5d183 Compare October 22, 2020 11:56

jpkrohling approved these changes Oct 27, 2020

View reviewed changes

jpkrohling merged commit e5f2540 into jaegertracing:master Oct 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update instances status using client.Status().update interface #1253

Update instances status using client.Status().update interface #1253

rubenvp8510 commented Oct 14, 2020

codecov bot commented Oct 14, 2020 •

edited

Loading

jpkrohling left a comment

jpkrohling Oct 14, 2020

rubenvp8510 Oct 14, 2020 •

edited

Loading

jpkrohling Oct 14, 2020

rubenvp8510 Oct 14, 2020

jpkrohling left a comment

jpkrohling Oct 19, 2020

rubenvp8510 Oct 19, 2020

rubenvp8510 Oct 19, 2020

jpkrohling Oct 19, 2020

jpkrohling Oct 19, 2020

rubenvp8510 Oct 19, 2020

jpkrohling Oct 19, 2020

jpkrohling Oct 19, 2020

rubenvp8510 Oct 19, 2020

jpkrohling Oct 19, 2020

rubenvp8510 Oct 19, 2020

kevinearls Oct 19, 2020

jpkrohling left a comment

jpkrohling Oct 20, 2020

kevinearls Oct 20, 2020

rubenvp8510 commented Oct 20, 2020

rubenvp8510 commented Oct 27, 2020


		func TestOperatorUpgrade(t *testing.T) {

		upgradeTestVersion := os.Getenv(EnvUpdateVersionKey)

	}).WithError(err).Error("failed update status of the upgraded instance")
	}).WithError(err).Error("failed to update the status of the upgraded instance")

Update instances status using client.Status().update interface #1253

Update instances status using client.Status().update interface #1253

Conversation

rubenvp8510 commented Oct 14, 2020

codecov bot commented Oct 14, 2020 • edited Loading

Codecov Report

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

rubenvp8510 commented Oct 20, 2020

rubenvp8510 commented Oct 27, 2020

codecov bot commented Oct 14, 2020 •

edited

Loading

rubenvp8510 Oct 14, 2020 •

edited

Loading