Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sidecar not removed #1508

Merged
merged 7 commits into from
Jul 27, 2021
Merged

Sidecar not removed #1508

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
03a975b
first approach for removing sidecar when deploy annotation is set to …
mfz85 Jul 10, 2021
24ca36f
fixed missing condition test on sidecar/needed when deploy annotation…
mfz85 Jul 14, 2021
a5bdd69
removeSidecar refactored as instance method
mfz85 Jul 15, 2021
87c5672
Merge branch 'jaegertracing:master' into sidecar_not_removed
mfz85 Jul 15, 2021
fe4d02a
Merge branch 'master' into sidecar_not_removed
jpkrohling Jul 21, 2021
768aa97
inject.desired refactor
mfz85 Jul 24, 2021
70af62e
Merge branch 'master' into sidecar_not_removed
jpkrohling Jul 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions pkg/controller/deployment/deployment_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re

if !inject.Desired(dep, ns) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add this cleaning logic on the else statement of the if inject.Needed(dep, ns) {

Here: https://github.com/jaegertracing/jaeger-operator/pull/1508/files#diff-c553184997fa4c9f63e697af2f39eb2e6fb43eeff64a0ef66c9e955f61347b02R172

// sidecar isn't desired for this deployment, skip remaining of the reconciliation
mfz85 marked this conversation as resolved.
Show resolved Hide resolved
hasAgent, _ := inject.HasJaegerAgent(dep)
if hasAgent {
removeSideCar(r, dep, ctx)
}
return reconcile.Result{}, nil
}

Expand Down Expand Up @@ -170,6 +174,25 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re
return reconcile.Result{}, nil
}

func removeSideCar(r *ReconcileDeployment, dep *appsv1.Deployment, ctx context.Context) {
mfz85 marked this conversation as resolved.
Show resolved Hide resolved
jaegerInstance, hasLabel := dep.Labels[inject.Label]
if hasLabel {
mfz85 marked this conversation as resolved.
Show resolved Hide resolved
log.WithFields(log.Fields{
"deployment": dep.Name,
"namespace": dep.Namespace,
"jaeger": jaegerInstance,
}).Info("Removing Jaeger Agent sidecar")
patch := client.MergeFrom(dep.DeepCopy())
inject.CleanSidecar(jaegerInstance, dep)
if err := r.client.Patch(ctx, dep, patch); err != nil {
log.WithFields(log.Fields{
"deploymentName": dep.Name,
"deploymentNamespace": dep.Namespace,
}).WithError(err).Error("error cleaning orphaned deployment")
}
}
}

func (r *ReconcileDeployment) syncOnJaegerChanges(event handler.MapObject) []reconcile.Request {
reconciliations := []reconcile.Request{}
nss := map[string]corev1.Namespace{} // namespace cache
Expand Down
4 changes: 2 additions & 2 deletions pkg/inject/sidecar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,10 @@ func Desired(dep *appsv1.Deployment, ns *corev1.Namespace) bool {
"namespace": dep.Namespace,
"deployment": dep.Name,
})
_, depExist := dep.Annotations[Annotation]
annotationValue, depExist := dep.Annotations[Annotation]
_, nsExist := ns.Annotations[Annotation]

if depExist {
if depExist && !strings.EqualFold(annotationValue, "false") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the annotation is at the namespace level? Isn't this logic already present in the inject.Desired?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inject.Desired just test if the annotation is present, but if the annotation exists with a "false" value it still returns "true".

Should i validate the same condition for the namespace?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confident we have a logic somewhere that determines whether a deployment should have a sidecar based on annotations from both the deployment itself and the namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's present in "inject.select", but doesn't it make more sense for this to be validated in "inject.desired" ?, it is a bit confusing that desired returns "true" when it is not really desired.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Would you be able to create an issue to refactor that? Also, it would be great to have an e2e test to assert the behavior of this PR here. It doesn't have to be done as part of this PR, though.

I'd still like to get @rubenvp8510's review before merging, but looks good to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Would you be able to create an issue to refactor that? Also, it would be great to have an e2e test to assert the behavior of this PR here. It doesn't have to be done as part of this PR, though.

count on it

Copy link
Collaborator

@rubenvp8510 rubenvp8510 Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This covers both cases, because inject.Needed is used when we modify the namespace to determine if the deployments that belongs to namespace needs an injection. Same case for individual deployments.

Except that we already have the cleaning logic on namespace but not on individual deployments, this is what this PR is adding.,

I think this modification to the desired behaviour makes sense , but it is a little bit confusing the difference between needed and desired, Indeed inject.Needed calls inject.Desired. so I think is enough to expose Needed and use it on the controllers. Desired could be renamed to desired (lowercase so don't expose outside package) and use only Needed on the contgrollers.

Copy link
Collaborator

@rubenvp8510 rubenvp8510 Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I think we shouldn't use inject.Select for validating these cases, inject.Select is just when we are make sure we need to inject something but we need to find which instance we need to inject (and if it exists)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the refactor is done, can you review @rubenvp8510 ?

logger.Debug("annotation present on deployment")
return true
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/inject/sidecar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,11 @@ func TestSidecarNeeded(t *testing.T) {
ns: ns(map[string]string{Annotation: "true"}),
needed: false,
},
{
dep: dep(map[string]string{Annotation: "false"}, map[string]string{}),
ns: ns(map[string]string{}),
needed: false,
},
}
for _, test := range tests {
t.Run(fmt.Sprintf("dep:%s, ns: %s", test.dep.Annotations, test.ns.Annotations), func(t *testing.T) {
Expand Down