No modify annotation when injecting #902

rubenvp8510 · 2020-02-12T00:23:55Z

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

Resolves #899

In the actual code, when Select is called to select a jaeger instance, it modifies the annotation and set the value to the jaeger instance name.

I think this can lead to the scenario when you rename (or recreate) the Jaeger instance and there is no way to map the old name instance to the new one. I would prefer to preserve the value.

pavolloffay · 2020-02-12T07:56:42Z

Let's consider also a scenario when:

deploy a single Jaeger instance in cluster
add app deployment with inject annotation = true
deploy another Jaeger instance.

In this case we should not break existing injected agents.

rubenvp8510 · 2020-02-12T17:04:22Z

@pavolloffay good point, I'll check that case.

But, if you add a four step:
4. delete the first instance

Then the deployment with inject annotation = true, should be injected with the new side-car, right?
Which essentially is the problem this PR is addressing.

pavolloffay · 2020-02-13T08:13:46Z

A good point. @jpkrohling what do you think? It really depends on how we define the behavior.

jpkrohling · 2020-02-13T15:10:56Z

I used to think that true means select a suitable Jaeger instance for me restricted for the first time it found a suitable instance, but I think that we could do better. In the case that @pavolloffay mentioned, the deployment should still be using the first Jaeger instance. And once the first instance is deleted, the deployment should automatically be switched over to the new instance.

We might have all the bits necessary for this already: we have the annotation sidecar.jaegertracing.io/inject and we have a label sidecar.jaegertracing.io/injected. So, we could just leave the inject as the user has defined it originally (true, false, instance-name), and update the label to state what's the instance currently in use. In the step 3 from @pavolloffay's, this value can serve to disambiguate.

rubenvp8510 · 2020-02-13T16:20:26Z

@jpkrohling yes that is what I was thinking, using the label for disambiguate, I think this is the best approach as we avoid touching values defined by the user, (the label is completely handled by the operator).

jpkrohling · 2020-02-14T09:52:46Z

@pavolloffay do you agree with the expected behavior? If so, @rubenvp8510 could you then adapt this PR to what we discussed above, adding tests to confirm that this new behavior is in place?

pavolloffay · 2020-02-14T10:45:01Z

👍 It sounds good to me

pkg/inject/sidecar.go

pkg/controller/namespace/namespace_controller.go

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

rubenvp8510 · 2020-02-27T05:03:20Z

this is ready for another review.

pkg/autodetect/main.go

test/e2e/sidecar_test.go

pavolloffay · 2020-02-27T10:51:16Z

test/e2e/sidecar_test.go

 	cleanupOptions := &framework.CleanupOptions{TestContext: ctx, Timeout: timeout, RetryInterval: retryInterval}

 	jaegerInstanceName := "agent-as-sidecar"
 	j := getJaegerAgentAsSidecarDefinition(jaegerInstanceName, namespace)
+	defer func() {
+		if !agentAsSideCarDeleted {


Why is this check needed? The variable is intialized and set in the middle of the tests. Isn't the deterministic at the end of the test?

I extended this test to test the case addressed by this PR.

At some point in this test I create a new instance and delete the "agent-as-sidecar" Jaeger instance. If I don't do this check the test fails because it tries to delete an non-exist instance at the end.

I also got confused by this, but I think I understand what's going on: if the test fails midway of the execution, this instance will be lingering around, which is why you need the conditional undeploy.

It might be better to change the undeployJaegerInstance to return a bool indicating whether it was able to find and remove the instance. In most cases, the caller wouldn't care about whether the Jaeger has been undeployed correctly, I suppose, but in the cases where it does care, it could just call require.True(t, undeployJaegerInstance(j)).

cc @kevinearls

@jpkrohling agreed, I think we can change that to return a bool and let the caller decide if they care about failure.

@jpkrohling @kevinearls @pavolloffay I'll do the suggested change but Is it ok to maintain this in a single test? or better to split?

@rubenvp8510 I think it would be better to split.

pavolloffay · 2020-02-27T10:52:49Z

pkg/inject/sidecar.go

-
-	if jaeger == nil || (dep.Annotations[Annotation] != jaeger.Name && dep.Annotations[AnnotationLegacy] != jaeger.Name) {
+	labelValue, hasLabel := dep.Labels[Label]
+	if jaeger == nil || (hasLabel && labelValue != jaeger.Name) {


Can be the hasLabel omitted? e.g. if jaeger == nil || (labelValue != jaeger.Name)

I don't think it can be omitted.

The logic is this: if deployment doesn't have a label, that means that it needs to be injected. If it has a label, we need to check if the label is equal to jaeger instance. The latest only applies in the case of an update, while the former is for new deployments. Remember, now inject.Select function doesn't have side effects, nor modify the deployment.

If we don't check for hasLabel some sidecar tests will fail. For instance, TestInjectSidecar will fail because it will try to inject a sidecar in a deployment that doesn't have any label (the label is added when the sidecar is injected). If we check only labelValue != jaeger.Name the deployment will never be injected. The first time, when it doesn't have a label, labelValue will be an empty string.

On the other hand, if we check for hasLabel we will inject the side car the first time. Of course I can do a labelValue != "", but I would prefer to use the value returned by go maps for key existence.

Does that makes sense?

I'm not a huge fan of this conditional. How about something like this?

if jaeger == nil { logFields.Trace("no Jaeger instance found, skipping sidecar injection") return dep } if val, ok := dep.Labels[Label]; ok && val != jaeger.Name { logFields.Trace("deployment is assigned to a different Jager instance, skipping sidecar injection") return dep } decorate(dep) logFields.Debug("injecting sidecar") dep.Spec.Template.Spec.Containers = append(dep.Spec.Template.Spec.Containers, container(jaeger, dep)) setInjectedInstanceLabel(dep, jaeger)

One more thing I wanted to check: this changes the behavior of the Sidecar function, so that it won't check the Annotation at all anymore. Therefore, this test would be outdated:

jaeger-operator/pkg/inject/sidecar_test.go

Lines 33 to 40 in 4424e8a

func TestInjectSidecar(t *testing.T) {

jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestInjectSidecar"})

dep := dep(map[string]string{Annotation: jaeger.Name}, map[string]string{})

dep = Sidecar(jaeger, dep)

assert.Len(t, dep.Spec.Template.Spec.Containers, 2)

assert.Contains(t, dep.Spec.Template.Spec.Containers[1].Image, "jaeger-agent")

assert.Len(t, dep.Spec.Template.Spec.Containers[0].Env, 0)

}

Removing the Annotation would still get this test passing, which proves the change in behavior. If this is desired, then please change the existing tests, including the one above, but make sure that the contract with the consumers isn't broken: people annotating their deployments have to get the same behavior as before.

True, I need to update the above test, (and I need to review if I need to update more). The contract with the consumer isn't broken, the only difference is that now the annotation is only used for select the instance (in the select function) and not for inject. but If I'm not wrong or miss something, that is transparent for the consumer.

I'm not a huge fan of this conditional. How about something like this?

if jaeger == nil { logFields.Trace("no Jaeger instance found, skipping sidecar injection") return dep } if val, ok := dep.Labels[Label]; ok && val != jaeger.Name { logFields.Trace("deployment is assigned to a different Jager instance, skipping sidecar injection") return dep } decorate(dep) logFields.Debug("injecting sidecar") dep.Spec.Template.Spec.Containers = append(dep.Spec.Template.Spec.Containers, container(jaeger, dep)) setInjectedInstanceLabel(dep, jaeger)

Is essentially the same, but more separated, and with better log messages :) I'll do the change.

I find it easier to read and understand, but if you disagree, feel free to keep the current version.

I indeed think your suggestion is a good idea, improves a lot the readability.

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

rubenvp8510 · 2020-03-09T13:41:15Z

@jpkrohling please review :)

jpkrohling

There's a clarification needed around the change in Sidecar behavior. All other requests are optional.

jpkrohling · 2020-03-10T10:26:11Z

pkg/inject/sidecar.go

-
-	if jaeger == nil || (dep.Annotations[Annotation] != jaeger.Name && dep.Annotations[AnnotationLegacy] != jaeger.Name) {
+	labelValue, hasLabel := dep.Labels[Label]
+	if jaeger == nil || (hasLabel && labelValue != jaeger.Name) {


I'm not a huge fan of this conditional. How about something like this?

if jaeger == nil { logFields.Trace("no Jaeger instance found, skipping sidecar injection") return dep } if val, ok := dep.Labels[Label]; ok && val != jaeger.Name { logFields.Trace("deployment is assigned to a different Jager instance, skipping sidecar injection") return dep } decorate(dep) logFields.Debug("injecting sidecar") dep.Spec.Template.Spec.Containers = append(dep.Spec.Template.Spec.Containers, container(jaeger, dep)) setInjectedInstanceLabel(dep, jaeger)

jpkrohling · 2020-03-10T10:29:52Z

pkg/inject/sidecar.go

@@ -80,6 +73,15 @@ func Needed(dep *appsv1.Deployment, ns *corev1.Namespace) bool {
 	return !HasJaegerAgent(dep)
 }

+func setInjectedInstanceLabel(target *appsv1.Deployment, jaeger *v1.Jaeger) {


In new code, we are trying to avoid changing the parameters we receive, returning new values instead. Like:

func setInjectedInstanceLabel(target *appsv1.Deployment, jaeger *v1.Jaeger) *appsv1.Deployment { ret := *target ... return &ret }

In this particular case, I would actually not split it in a separate function, as it this is very simple and, apparently, needed only in the Sidecar function.

I don't remember why I split this.. I think it was due some codeclimate issues. I'll check but seems to not be necessary

jpkrohling · 2020-03-10T10:33:05Z

test/e2e/sidecar_test.go

 	cleanupOptions := &framework.CleanupOptions{TestContext: ctx, Timeout: timeout, RetryInterval: retryInterval}

 	jaegerInstanceName := "agent-as-sidecar"
 	j := getJaegerAgentAsSidecarDefinition(jaegerInstanceName, namespace)
+	defer func() {
+		if !agentAsSideCarDeleted {


I also got confused by this, but I think I understand what's going on: if the test fails midway of the execution, this instance will be lingering around, which is why you need the conditional undeploy.

It might be better to change the undeployJaegerInstance to return a bool indicating whether it was able to find and remove the instance. In most cases, the caller wouldn't care about whether the Jaeger has been undeployed correctly, I suppose, but in the cases where it does care, it could just call require.True(t, undeployJaegerInstance(j)).

cc @kevinearls

jpkrohling · 2020-03-10T10:53:19Z

pkg/inject/sidecar.go

-
-	if jaeger == nil || (dep.Annotations[Annotation] != jaeger.Name && dep.Annotations[AnnotationLegacy] != jaeger.Name) {
+	labelValue, hasLabel := dep.Labels[Label]
+	if jaeger == nil || (hasLabel && labelValue != jaeger.Name) {


One more thing I wanted to check: this changes the behavior of the Sidecar function, so that it won't check the Annotation at all anymore. Therefore, this test would be outdated:

jaeger-operator/pkg/inject/sidecar_test.go

Lines 33 to 40 in 4424e8a

func TestInjectSidecar(t *testing.T) {

jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestInjectSidecar"})

dep := dep(map[string]string{Annotation: jaeger.Name}, map[string]string{})

dep = Sidecar(jaeger, dep)

assert.Len(t, dep.Spec.Template.Spec.Containers, 2)

assert.Contains(t, dep.Spec.Template.Spec.Containers[1].Image, "jaeger-agent")

assert.Len(t, dep.Spec.Template.Spec.Containers[0].Env, 0)

}

Removing the Annotation would still get this test passing, which proves the change in behavior. If this is desired, then please change the existing tests, including the one above, but make sure that the contract with the consumers isn't broken: people annotating their deployments have to get the same behavior as before.

pkg/inject/sidecar_test.go

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

codecov · 2020-03-11T22:20:58Z

Codecov Report

Merging #902 into master will increase coverage by 0.36%.
The diff coverage is 74.24%.

@@            Coverage Diff             @@
##           master     #902      +/-   ##
==========================================
+ Coverage   64.04%   64.40%   +0.36%     
==========================================
  Files          79       82       +3     
  Lines        6427     6535     +108     
==========================================
+ Hits         4116     4209      +93     
- Misses       2173     2181       +8     
- Partials      138      145       +7

Impacted Files	Coverage Δ
pkg/inject/sidecar.go	`95.09% <53.84%> (-1.75%)`	⬇️
pkg/upgrade/upgrade.go	`55.35% <71.42%> (+4.29%)`	⬆️
pkg/autodetect/main.go	`86.87% <80.55%> (-1.72%)`	⬇️
pkg/controller/jaeger/clusterrolebinding.go	`78.37% <100.00%> (+1.90%)`	⬆️
pkg/strategy/production.go	`94.73% <0.00%> (-5.27%)`	⬇️
pkg/apis/jaegertracing/v1/options.go	`88.00% <0.00%> (-1.66%)`	⬇️
pkg/kafka/streaming.go	`100.00% <0.00%> (ø)`
pkg/upgrade/v1_15_0.go	`100.00% <0.00%> (ø)`
pkg/deployment/agent.go	`100.00% <0.00%> (ø)`
... and 7 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 7531c6b...5aee6a8. Read the comment docs.

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

rubenvp8510 · 2020-03-12T01:31:15Z

@jpkrohling

I updated the sidecar tests, but as you mentioned on one of your comments, the annotation is irrelevant for those tests (except the inject.Select test), and this is intentional.

This is because this PR is not validating for annotation on inject.Sidecar, now it only injects the Jaeger instance passed as an argument without checking for annotations, of course, it does another checks, like if the instance not nil and if the deploy doesn't have one sidecar already. We can assume a previously selected jaeger instance by inject.Select , which is still using the annotator for the selection. Does that makes sense?

Summary: This PR uses the annotations only for select the correct Jaeger instance. and use label for check if the deployment already have an agent and which Jaeger instance is the owner.

From the consumer/user perspective it will be the same, the deployments with annotations will be injected.

jpkrohling

LGTM. One point still seems to be pending (split of the tests), but @kevinearls and @rubenvp8510 can come to an agreement about doing it in this PR or in a follow-up PR, in which case an issue has to be created before.

kevinearls · 2020-03-13T16:00:00Z

@jpkrohling @rubenvp8510 I'd be ok splitting the tests in a separate PR.

jpkrohling · 2020-03-16T14:38:48Z

I'm merging this, but @rubenvp8510, please create an issue to track the splitting of the test.

rubenvp8510 force-pushed the Issue-899 branch 2 times, most recently from 4f86b64 to a8cff50 Compare February 12, 2020 03:16

rubenvp8510 force-pushed the Issue-899 branch 8 times, most recently from f2d94b4 to f1b93fa Compare February 19, 2020 01:29

rubenvp8510 requested review from jpkrohling and pavolloffay and removed request for jpkrohling February 19, 2020 03:52

pavolloffay reviewed Feb 19, 2020

View reviewed changes

pkg/inject/sidecar.go Outdated Show resolved Hide resolved

pkg/controller/namespace/namespace_controller.go Outdated Show resolved Hide resolved

rubenvp8510 force-pushed the Issue-899 branch 5 times, most recently from cd184b4 to 22af302 Compare February 20, 2020 02:57

rubenvp8510 added 2 commits February 26, 2020 20:20

No modify annotation when injecting

6eb19fd

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

Use label to check if the sidecar is already injected

55d5042

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

rubenvp8510 force-pushed the Issue-899 branch from 22af302 to d60c42f Compare February 27, 2020 02:20

rubenvp8510 force-pushed the Issue-899 branch 2 times, most recently from 05c5457 to 399c303 Compare February 27, 2020 03:47

rubenvp8510 requested a review from pavolloffay February 27, 2020 05:03

pavolloffay reviewed Feb 27, 2020

View reviewed changes

Adding tests

4424e8a

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

rubenvp8510 force-pushed the Issue-899 branch 4 times, most recently from 2078004 to 4424e8a Compare February 28, 2020 05:30

rubenvp8510 requested a review from pavolloffay February 28, 2020 13:08

jpkrohling reviewed Mar 10, 2020

View reviewed changes

Improve readability of sidecar.Inject conditional

d54bf94

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

rubenvp8510 added 2 commits March 11, 2020 16:37

Change undeployJaegerInstance to not fail on error

f88d25b

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

Update sidecar tests

5aee6a8

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

jpkrohling approved these changes Mar 13, 2020

View reviewed changes

jpkrohling merged commit 9e2cffa into jaegertracing:master Mar 16, 2020

	func TestInjectSidecar(t *testing.T) {
	jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestInjectSidecar"})
	dep := dep(map[string]string{Annotation: jaeger.Name}, map[string]string{})
	dep = Sidecar(jaeger, dep)
	assert.Len(t, dep.Spec.Template.Spec.Containers, 2)
	assert.Contains(t, dep.Spec.Template.Spec.Containers[1].Image, "jaeger-agent")
	assert.Len(t, dep.Spec.Template.Spec.Containers[0].Env, 0)
	}

No modify annotation when injecting #902

No modify annotation when injecting #902

Conversation

rubenvp8510 commented Feb 12, 2020 • edited by pavolloffay Loading

pavolloffay commented Feb 12, 2020

rubenvp8510 commented Feb 12, 2020

pavolloffay commented Feb 13, 2020

jpkrohling commented Feb 13, 2020

rubenvp8510 commented Feb 13, 2020

jpkrohling commented Feb 14, 2020

pavolloffay commented Feb 14, 2020

rubenvp8510 commented Feb 27, 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

Choose a reason for hiding this comment

rubenvp8510 Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 commented Mar 9, 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

codecov bot commented Mar 11, 2020 • edited Loading

Codecov Report

rubenvp8510 commented Mar 12, 2020

jpkrohling left a comment

Choose a reason for hiding this comment

kevinearls commented Mar 13, 2020

jpkrohling commented Mar 16, 2020

rubenvp8510 commented Feb 12, 2020 •

edited by pavolloffay

Loading

rubenvp8510 Mar 10, 2020 •

edited

Loading

codecov bot commented Mar 11, 2020 •

edited

Loading