Scan deployments for agent injection #454

rubenvp8510 · 2019-05-30T02:36:46Z

No description provided.

pavolloffay · 2019-06-03T09:23:27Z

pkg/inject/sidecar.go

+		if Needed(dep) {
+			if belongsToJaegerInstance(dep, jaeger, len(pods.Items) == 1) {
+				// a suitable jaeger instance was found! let's inject a sidecar pointing to it then
+				log.WithFields(log.Fields{


I think you can call logger on jaeger instance jager.Log()

pavolloffay · 2019-06-03T09:23:36Z

pkg/inject/sidecar.go

@@ -177,3 +177,34 @@ func hasEnv(name string, vars []corev1.EnvVar) bool {
 	}
 	return false
 }
+
+// RequireInjections deployments


a better comment?

pavolloffay · 2019-06-03T09:25:42Z

pkg/controller/jaeger/jaeger_controller.go

+	return nil
+}
+
+func (r *ReconcileJaeger) scanDeployments(jaeger v1.Jaeger) error {


What does the func do? Does it scan for something or does it add sidecars?

The name is wrong, not intuitive at all, sorry

It suppose to add the sidecar for deployments that require that. But I think I found a better way for do this.

jpkrohling · 2019-07-02T12:52:43Z

@rubenvp8510, are you still interested in working on this one? We might change in the future to use the webhook approach, which would render this one here obsolete, but this is indeed fixing a bug we have right now, IIRC.

jpkrohling · 2019-07-23T11:44:14Z

We now have an auto-detect package (pkg/autodetect) with a process that runs periodically in the background. If you decide to continue working on this one, it would be good to use that feature.

rubenvp8510 · 2019-07-24T04:57:12Z

@jpkrohling Yes I want to update this PR, I'll check the autodetect stuff and update this.

Thanks!

rubenvp8510 · 2019-07-31T04:29:36Z

@jpkrohling I'm not totally convinced of using auto-detect feature on this, I updated the code, now the actual code uses that feature. but I need to get all deployments and scan for updates/injections. Not sure if it's a better way to do it, I have the problem that I cannot query on annotations.

jpkrohling · 2019-09-19T15:07:34Z

@rubenvp8510, wasn't this implemented in another PR? Should this one be closed?

rubenvp8510 · 2019-09-19T16:14:46Z

@jpkrohling this is another issue, I think you are confusing this PR with #453, which is the opposite.

This is about to inject the agent on existing deployments, the other is about deletion. Not sure what is the decision regarding of this PR, Do we want to proceed in this way ?or are we going to work more on webhook admission controller?

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

jpkrohling

Had a comment about reusing some code. Other than that, LGTM.

jpkrohling · 2019-10-24T07:57:05Z

pkg/autodetect/main.go

@@ -77,6 +77,54 @@ func (b *Background) Stop() {
 	b.ticker.Stop()
 }

+func (b *Background) requireUpdates(deps *appsv1.DeploymentList) []*appsv1.Deployment {
+	instances := &v1.JaegerList{}


Could you leave a comment in the func, saying that this is related to the Deployment controller, but that this here exists only because the Jaeger Operator might not be up and running when the deployment was created/updated/annotated?

jpkrohling · 2019-10-24T07:59:23Z

pkg/autodetect/main.go

+		return nil
+	}
+	requireUpdates := make([]*appsv1.Deployment, 0)
+	for i := 0; i < len(deps.Items); i++ {


The code within this loop is pretty much the same as the existing controller, isn't it:

jaeger-operator/pkg/controller/deployment/deployment_controller.go

Lines 84 to 111 in b6211a4

if inject.Needed(instance) {

pods := &v1.JaegerList{}

opts := []client.ListOption{}

err := r.client.List(context.Background(), pods, opts...)

if err != nil {

log.WithError(err).Error("failed to get the available Jaeger pods")

return reconcile.Result{}, err

}

jaeger := inject.Select(instance, pods)

if jaeger != nil && jaeger.GetDeletionTimestamp() == nil {

// a suitable jaeger instance was found! let's inject a sidecar pointing to it then

// Verified that jaeger instance was found and is not marked for deletion.

log.WithFields(log.Fields{

"deployment": instance.Name,

"namespace": instance.Namespace,

"jaeger": jaeger.Name,

"jaeger-namespace": jaeger.Namespace,

}).Info("Injecting Jaeger Agent sidecar")

instance = inject.Sidecar(jaeger, instance)

if err := r.client.Update(context.Background(), instance); err != nil {

log.WithField("deployment", instance).WithError(err).Error("failed to update")

return reconcile.Result{}, err

}

} else {

log.WithField("deployment", instance.Name).Info("No suitable Jaeger instances found to inject a sidecar")

}

}

In that case, it would probably make sense to externalize that into its own func, and call it from both places (this here and the controller shown above)

jpkrohling · 2019-10-24T08:00:26Z

pkg/inject/sidecar.go

@@ -62,6 +62,17 @@ func Sidecar(jaeger *v1.Jaeger, dep *appsv1.Deployment) *appsv1.Deployment {
 	return dep
 }

+// UpdateSideCar modify the deployment side car with the latest parameters if it's required.
+func UpdateSideCar(jaeger *v1.Jaeger, dep *appsv1.Deployment) bool {


s/SideCar/Sidecar

jpkrohling · 2019-11-15T15:06:06Z

@rubenvp8510 do you want to merge this, and address the remaining comments in a follow-up PR?

rubenvp8510 · 2019-11-15T15:10:43Z

@rubenvp8510 do you want to merge this, and address the remaining comments in a follow-up PR?

Yes please! :)

jpkrohling · 2019-11-15T15:12:31Z

Merged. Please, create an issue to track the remaining points.

rubenvp8510 force-pushed the Issue-442 branch 2 times, most recently from 9fb00b9 to 875263a Compare May 30, 2019 17:16

pavolloffay reviewed Jun 3, 2019

View reviewed changes

jpkrohling mentioned this pull request Jul 24, 2019

Cannot use HorizontalPodAutoscaling: injected agent lacks resource defs #160

Closed

rubenvp8510 force-pushed the Issue-442 branch 2 times, most recently from 3adaea1 to 03ee9fb Compare July 31, 2019 04:22

rubenvp8510 force-pushed the Issue-442 branch 2 times, most recently from 075fee2 to f79e48c Compare July 31, 2019 20:38

rubenvp8510 force-pushed the Issue-442 branch from f79e48c to 983680d Compare October 24, 2019 02:51

Scan deployments for agent injection

72d05d8

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

rubenvp8510 force-pushed the Issue-442 branch from 983680d to 72d05d8 Compare October 24, 2019 03:20

jpkrohling reviewed Oct 24, 2019

View reviewed changes

jpkrohling merged commit 89c439e into jaegertracing:master Nov 15, 2019

rubenvp8510 mentioned this pull request Nov 15, 2019

Reuse inject update loop from deployment controller into autodetect. #774

Closed

This was referenced Jan 17, 2020

Duplicated "Injecting sidecar" message #855

Closed

Remove deployment updates from autodetect loop #869

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scan deployments for agent injection #454

Scan deployments for agent injection #454

rubenvp8510 commented May 30, 2019

pavolloffay Jun 3, 2019

pavolloffay Jun 3, 2019

pavolloffay Jun 3, 2019

rubenvp8510 Jun 3, 2019

jpkrohling commented Jul 2, 2019

jpkrohling commented Jul 23, 2019

rubenvp8510 commented Jul 24, 2019

rubenvp8510 commented Jul 31, 2019

jpkrohling commented Sep 19, 2019

rubenvp8510 commented Sep 19, 2019

jpkrohling left a comment

jpkrohling Oct 24, 2019

jpkrohling Oct 24, 2019

jpkrohling Oct 24, 2019

jpkrohling commented Nov 15, 2019

rubenvp8510 commented Nov 15, 2019

jpkrohling commented Nov 15, 2019

	if inject.Needed(instance) {
	pods := &v1.JaegerList{}
	opts := []client.ListOption{}
	err := r.client.List(context.Background(), pods, opts...)
	if err != nil {
	log.WithError(err).Error("failed to get the available Jaeger pods")
	return reconcile.Result{}, err
	}

	jaeger := inject.Select(instance, pods)
	if jaeger != nil && jaeger.GetDeletionTimestamp() == nil {
	// a suitable jaeger instance was found! let's inject a sidecar pointing to it then
	// Verified that jaeger instance was found and is not marked for deletion.
	log.WithFields(log.Fields{
	"deployment": instance.Name,
	"namespace": instance.Namespace,
	"jaeger": jaeger.Name,
	"jaeger-namespace": jaeger.Namespace,
	}).Info("Injecting Jaeger Agent sidecar")
	instance = inject.Sidecar(jaeger, instance)
	if err := r.client.Update(context.Background(), instance); err != nil {
	log.WithField("deployment", instance).WithError(err).Error("failed to update")
	return reconcile.Result{}, err
	}
	} else {
	log.WithField("deployment", instance.Name).Info("No suitable Jaeger instances found to inject a sidecar")
	}
	}

Scan deployments for agent injection #454

Scan deployments for agent injection #454

Conversation

rubenvp8510 commented May 30, 2019

pavolloffay Jun 3, 2019

Choose a reason for hiding this comment

pavolloffay Jun 3, 2019

Choose a reason for hiding this comment

pavolloffay Jun 3, 2019

Choose a reason for hiding this comment

rubenvp8510 Jun 3, 2019

Choose a reason for hiding this comment

jpkrohling commented Jul 2, 2019

jpkrohling commented Jul 23, 2019

rubenvp8510 commented Jul 24, 2019

rubenvp8510 commented Jul 31, 2019

jpkrohling commented Sep 19, 2019

rubenvp8510 commented Sep 19, 2019

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling Oct 24, 2019

Choose a reason for hiding this comment

jpkrohling Oct 24, 2019

Choose a reason for hiding this comment

jpkrohling Oct 24, 2019

Choose a reason for hiding this comment

jpkrohling commented Nov 15, 2019

rubenvp8510 commented Nov 15, 2019

jpkrohling commented Nov 15, 2019