Reuse inject function from deployment controller into autodetect #792
Conversation
|
Looks like the unit tests are failing. |
rubenvp8510
force-pushed
the
Issue-774
branch
3 times, most recently
from
bbf9640 to
384bb3c
Compare
rubenvp8510
force-pushed
the
Issue-774
branch
13 times, most recently
from
36ad1c2 to
5c89c92
Compare
|
This needs a rebase and will conflict due to #814 |
😢 |
Signed-off-by: Ruben Vargas <[email protected]>
rubenvp8510
force-pushed
the
Issue-774
branch
from
5c89c92 to
a857e40
Compare
| // the reason for this is because the operator might not be running when the deployment is created or updated. | ||
| func (b *Background) detectDeploymentUpdates() error { | ||
| deps := &appsv1.DeploymentList{} | ||
| if err := b.clReader.List(context.Background(), deps); err != nil { |
There was a problem hiding this comment.
Comment based on an error we've seen recently: shouldn't we list it only from the namespaces we are watching?
| instances := &v1.JaegerList{} | ||
| if err := b.clReader.List(context.Background(), instances); err != nil { | ||
| log.WithError(err).Info("failed to retrieve the list of Jaeger instances") | ||
| return nil | ||
| return err |
There was a problem hiding this comment.
Same as above: should probably be limited to the value of WATCH_NAMESPACES
| }, | ||
| }, | ||
| } | ||
| dep.Name = "my-business-container" |
There was a problem hiding this comment.
Why not set on the statement above?
| require.NoError(t, err) | ||
|
|
||
| err = cl.Create(context.TODO(), dep2) | ||
|
|
There was a problem hiding this comment.
Formatting seems a bit odd... Can you get the require.NoError close to the err it's testing?
| err = cl.Create(context.TODO(), jaeger2) | ||
| require.NoError(t, err) | ||
|
|
||
| dep.Annotations = map[string]string{inject.Annotation: jaeger1.Name} |
There was a problem hiding this comment.
This should be part of the definition of dep.
| Name: dep.Name, | ||
| }, persisted) | ||
|
|
||
| reporterEndpoint := fmt.Sprintf("--reporter.grpc.host-port=dns:///%s.%s:14250", service.GetNameForHeadlessCollectorService(jaeger2), jaeger2.Namespace) |
There was a problem hiding this comment.
Keep this assertion, but the official way to inform what's the Jaeger instance being used is via the annotation. So, add an assertion for the annotation that was persisted.
| @@ -3,6 +3,10 @@ package deployment | |||
| import ( | |||
| "context" | |||
|
|
|||
| v1 "github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1" | |||
There was a problem hiding this comment.
Import formatting seems odd.
| @@ -80,33 +81,18 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re | |||
| // Error reading the object - requeue the request. | |||
| return reconcile.Result{}, err | |||
| } | |||
| pods := &v1.JaegerList{} | |||
| err = r.client.List(context.Background(), pods) | |||
There was a problem hiding this comment.
Limit it to the namespaces being watched.
| } else { | ||
| log.WithField("deployment", instance.Name).Info("No suitable Jaeger instances found to inject a sidecar") | ||
| if injected != nil { | ||
| if err := r.client.Update(context.Background(), injected); err != nil { |
There was a problem hiding this comment.
It would probably be better to create a context at the beginning of the reconcile method, and use the same context everywhere down the stream. It will make it easier once we decide to add tracing to this controller, like we added to the other controller.
| @@ -251,3 +251,22 @@ func CleanSidecar(deployment *appsv1.Deployment) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // IfNeeded inject the sidecar on deployment only if it is required | |||
| func IfNeeded(instance *appsv1.Deployment, pods *v1.JaegerList) *appsv1.Deployment { | |||
There was a problem hiding this comment.
I don't think this function name is appropriate. I had to check it twice while doing this review, as I would expect it to return an instance (not nil): either the original instance, or a changed instance, but not nil. The func documentation also doesn't tell that this might return nil, and what it means.
I think it would be better to change the IfNeeded consumers to check Needed instead.
I didn't change anything :) may be this is an old PR and need some updates |
Signed-off-by: Ruben Vargas [email protected]
Fixes #774