-
Notifications
You must be signed in to change notification settings - Fork 348
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.
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
Add support for injecting jaeger agent in annotated statefulsets. #78
Conversation
fcb3a91
to
9f64d5f
Compare
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
==========================================
+ Coverage 88.55% 96.45% +7.89%
==========================================
Files 70 30 -40
Lines 3119 1466 -1653
==========================================
- Hits 2762 1414 -1348
+ Misses 244 40 -204
+ Partials 113 12 -101
Continue to review full report at Codecov.
|
Let me know once it's ready to be reviewed. |
@jpkrohling request for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @annanay25)
a discussion (no related file):
I like the idea of getting the automatic sidecar injection to work for stateful sets, and I'm thankful for your efforts!
I think the approach from this PR won't scale when we think about other Pod container types. A better plan would be to make the current code more generic, to accept sdk.Object
instead of Deployment
. This way, a good part of the code could be reused for all types. Custom behavior can be achieved through private, specialized functions.
I have a couple of other things that should be considered before merging:
- Documentation -- the README would be outdated with this PR
- e2e test -- we already have one for the current feature, under
test/e2e/sidecar.go
. You could rename it tosidecar-deployment.go
and create another one forsidecar-statefulset.go
. Don't worry about duplication of code here in the tests: in most cases, it's actually desirable to have duplicated code here, so that the tests are very explicit about what they assume
pkg/inject/sidecar.go, line 74 at r1 (raw file):
// Return a container for sidecar injection func Container(jaeger *v1alpha1.Jaeger) v1.Container {
I would prefer to keep this private, leaving the Sidecar
function as the single entry point to this kind of feature. You could move the existing Inject
function to, say, injectIntoDeployment
and create a new function named Inject(sdk.Object, *v1alpha1.Jaeger)
. The sdk.Object
is then type-checked and the appropriate specialized injectXXX
is called. Your new feature would then be inside a injectIntoStatefulSet
.
pkg/statefulset/agent.go, line 1 at r1 (raw file):
package statefulset
This is mostly a duplicate of what we have under the inject
package, sidecar.go
source file. It would probably not be hard to change that to accommodate StatefulSets.
pkg/stub/handler.go, line 92 at r1 (raw file):
} } case *appsv1.StatefulSet:
This also seems like a good duplication of the logic for the Deployment
part. It would probably be possible to make the same code work for both cases, especially if the Needed
and Select
functions were changed to accept a sdk.Object
instead of Deployment
.
Thanks for the comprehensive review! I understand that we need to write sidecar injection for multiple pod types, I am looking into making the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package statefulset
has been deleted, and its functionality written within package inject
.
Functions Sidecar
, Needed
, Select
have been modified to handle sdk.Object
.
Todo:
- Add tests
- Update README
@annanay25 , let me know once this is ready for a new review round. Currently, the unit tests are failing, so, I assume it's not ready yet. |
@jpkrohling I was a little confused as to how to proceed here - #78 (comment) Your thoughts? |
Sorry, I missed that part. I just replied with a suggestion that might work. |
Heads up: I'm starting to work on #66, which will probably conflict with this PR. Please, wait a couple of days to work on this issue, as I'm not sure how much of it would be reusable there. |
Understood. I'll hold this off, until later. Thanks! |
The migration to the new operator SDK is now complete. Please, rebase and update this PR. |
8bb98a0
to
0e28d44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR.
Todo: Tests for StatefulSet
@jpkrohling the case statement grouping as implemented here - https://github.com/jaegertracing/jaeger-operator/pull/78/files#diff-f88c99835d4b2acda532e36123761825R91 - is not evaluating the |
ebd9569
to
e3a81ee
Compare
@jpkrohling I updated this patch, but there's a bunch of replicated code in the inject package which I'm not sure how to workaround. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r2, 1 of 5 files at r3, 3 of 4 files at r4.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @annanay25 and @jpkrohling)
pkg/inject/sidecar.go, line 44 at r4 (raw file):
logrus.Debugf("Skipping sidecar injection for instance %v", o.Name) } else { decorate(o)
Can't you collapse this into the case above? Like you did for the deployment_controller.go
?
pkg/inject/sidecar.go, line 52 at r4 (raw file):
// Needed determines whether a pod needs to get a sidecar injected or not func Needed(Name string, Annotations map[string]string, Containers []v1.Container) bool {
I'm curious as to why the var names are capitalized here. Also, it would probably be better to receive an obj
like the other functions, do a type check and log in case the type isn't a supported type (Deployment/Statefulset)
pkg/inject/sidecar.go, line 70 at r4 (raw file):
// Select a suitable Jaeger from the JaegerList for the given Pod, or nil of none is suitable func Select(Annotations map[string]string, availableJaegerPods *v1alpha1.JaegerList) *v1alpha1.Jaeger {
Same question as above
pkg/inject/sidecar.go, line 120 at r4 (raw file):
func decorate(obj runtime.Object) { switch o := obj.(type) { case *appsv1.Deployment:
Is the code for Deployment
exactly like the one for StatefulSet
? If not, please leave a comment explaining the difference. If they are the same, then collapse the two cases as you did before in the deployment_controller.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @annanay25 and @jpkrohling)
pkg/inject/sidecar.go, line 44 at r4 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Can't you collapse this into the case above? Like you did for the
deployment_controller.go
?
That doesn't seem to work, and apparently we need to perform complete type checking at least once.
switch instance := obj.(type) {
case *appsv1.Deployment:
case *appsv1.StatefulSet:
decorate(instance)
^ seems to fail for me.
pkg/inject/sidecar.go, line 52 at r4 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
I'm curious as to why the var names are capitalized here. Also, it would probably be better to receive an
obj
like the other functions, do a type check and log in case the type isn't a supported type (Deployment/Statefulset)
Capitalisation was erroneous, will change this. You had suggested earlier that it would be better to pass only the necessary variables to functions like Needed
and Select
- #78 (comment)
pkg/inject/sidecar.go, line 120 at r4 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Is the code for
Deployment
exactly like the one forStatefulSet
? If not, please leave a comment explaining the difference. If they are the same, then collapse the two cases as you did before in thedeployment_controller.go
.
They're the same, but some type checking constraints are forcing me to publish the functions this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @annanay25 and @jpkrohling)
pkg/inject/sidecar.go, line 44 at r4 (raw file):
Previously, annanay25 (Annanay Agarwal) wrote…
That doesn't seem to work, and apparently we need to perform complete type checking at least once.
switch instance := obj.(type) { case *appsv1.Deployment: case *appsv1.StatefulSet: decorate(instance)
^ seems to fail for me.
This seems to work:
func Sidecar(obj runtime.Object, jaeger *v1alpha1.Jaeger) {
deployment.NewAgent(jaeger) // we need some initialization from that, but we don't actually need the agent's instance here
switch o := obj.(type) {
case *appsv1.Deployment:
case *appsv1.StatefulSet:
if jaeger == nil || o.Annotations[Annotation] != jaeger.Name {
logrus.Debugf("Skipping sidecar injection for instance %v", o.Name)
} else {
decorate(o)
logrus.Debugf("Injecting sidecar for pod %v", o.Name)
o.Spec.Template.Spec.Containers = append(o.Spec.Template.Spec.Containers, container(jaeger))
}
}
}
pkg/inject/sidecar.go, line 52 at r4 (raw file):
Previously, annanay25 (Annanay Agarwal) wrote…
Capitalisation was erroneous, will change this. You had suggested earlier that it would be better to pass only the necessary variables to functions like
Needed
andSelect
- #78 (comment)
I'd have to go back to see why I suggested that. Was it perhaps because you'd need to pass a Go generic object (interface{}
), instead of Kubernetes'? It's certainly better to pass name/annotations/containers instead of a generic Go object, but I'd rather make it accept a Kubernetes runtime object if possible.
pkg/inject/sidecar.go, line 120 at r4 (raw file):
Previously, annanay25 (Annanay Agarwal) wrote…
They're the same, but some type checking constraints are forcing me to publish the functions this way.
Looks like this might work:
func decorate(obj runtime.Object) {
switch o := obj.(type) {
case *appsv1.Deployment:
case *appsv1.StatefulSet:
if app, found := o.Spec.Template.Labels["app"]; found {
// Append the namespace to the app name. Using the DNS style "<app>.<namespace>""
// which also matches with the style used in Istio.
if len(o.Namespace) > 0 {
app += "." + o.Namespace
} else {
app += ".default"
}
for i := 0; i < len(o.Spec.Template.Spec.Containers); i++ {
if !hasEnv(envVarServiceName, o.Spec.Template.Spec.Containers[i].Env) {
o.Spec.Template.Spec.Containers[i].Env = append(o.Spec.Template.Spec.Containers[i].Env, v1.EnvVar{
Name: envVarServiceName,
Value: app,
})
}
if !hasEnv(envVarPropagation, o.Spec.Template.Spec.Containers[i].Env) {
o.Spec.Template.Spec.Containers[i].Env = append(o.Spec.Template.Spec.Containers[i].Env, v1.EnvVar{
Name: envVarPropagation,
Value: "jaeger,b3",
})
}
}
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @annanay25 and @jpkrohling)
pkg/inject/sidecar.go, line 44 at r4 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
This seems to work:
func Sidecar(obj runtime.Object, jaeger *v1alpha1.Jaeger) { deployment.NewAgent(jaeger) // we need some initialization from that, but we don't actually need the agent's instance here switch o := obj.(type) { case *appsv1.Deployment: case *appsv1.StatefulSet: if jaeger == nil || o.Annotations[Annotation] != jaeger.Name { logrus.Debugf("Skipping sidecar injection for instance %v", o.Name) } else { decorate(o) logrus.Debugf("Injecting sidecar for pod %v", o.Name) o.Spec.Template.Spec.Containers = append(o.Spec.Template.Spec.Containers, container(jaeger)) } } }
When I make this change, the following happens:
➜ jaeger-operator git:(statefulsets-sidecar) ✗ make test
Running unit tests...
? github.com/jaegertracing/jaeger-operator/cmd [no test files]
? github.com/jaegertracing/jaeger-operator/cmd/manager [no test files]
ok github.com/jaegertracing/jaeger-operator/pkg/account 0.289s coverage: 100.0% of statements
? github.com/jaegertracing/jaeger-operator/pkg/apis [no test files]
ok github.com/jaegertracing/jaeger-operator/pkg/apis/io/v1alpha1 0.396s coverage: 17.0% of statements
? github.com/jaegertracing/jaeger-operator/pkg/cmd/start [no test files]
? github.com/jaegertracing/jaeger-operator/pkg/cmd/version [no test files]
ok github.com/jaegertracing/jaeger-operator/pkg/config/sampling 0.278s coverage: 94.1% of statements
ok github.com/jaegertracing/jaeger-operator/pkg/config/ui 0.356s coverage: 94.1% of statements
? github.com/jaegertracing/jaeger-operator/pkg/controller [no test files]
? github.com/jaegertracing/jaeger-operator/pkg/controller/deployment [no test files]
ok github.com/jaegertracing/jaeger-operator/pkg/controller/jaeger 3.567s coverage: 64.3% of statements
ok github.com/jaegertracing/jaeger-operator/pkg/cronjob 0.341s coverage: 93.5% of statements
ok github.com/jaegertracing/jaeger-operator/pkg/deployment 0.288s coverage: 100.0% of statements
ok github.com/jaegertracing/jaeger-operator/pkg/ingress 0.205s coverage: 100.0% of statements
--- FAIL: TestInjectSidecar (0.00s)
sidecar_test.go:33:
Error Trace: sidecar_test.go:33
Error: "[{ [] [] [] [] [] {map[] map[]} [] [] nil nil nil nil %!s(bool=false) %!s(bool=false) %!s(bool=false)}]" should have 2 item(s), but has 1
Test: TestInjectSidecar
panic: runtime error: index out of range [recovered]
panic: runtime error: index out of range
goroutine 11 [running]:
testing.tRunner.func1(0xc42016a960)
/usr/local/go/src/testing/testing.go:742 +0x29d
panic(0x1753be0, 0x1c9e3f0)
/usr/local/go/src/runtime/panic.go:502 +0x229
github.com/jaegertracing/jaeger-operator/pkg/inject.TestInjectSidecar(0xc42016a960)
/Users/annanay.a/Desktop/git/go/src/github.com/jaegertracing/jaeger-operator/pkg/inject/sidecar_test.go:34 +0x2e0
testing.tRunner(0xc42016a960, 0x1865c88)
/usr/local/go/src/testing/testing.go:777 +0xd0
created by testing.(*T).Run
/usr/local/go/src/testing/testing.go:824 +0x2e0
FAIL github.com/jaegertracing/jaeger-operator/pkg/inject 0.293s
ok github.com/jaegertracing/jaeger-operator/pkg/route 0.405s coverage: 100.0% of statements
ok github.com/jaegertracing/jaeger-operator/pkg/service 0.275s coverage: 100.0% of statements
ok github.com/jaegertracing/jaeger-operator/pkg/storage 0.252s coverage: 100.0% of statements
ok github.com/jaegertracing/jaeger-operator/pkg/strategy 0.265s coverage: 100.0% of statements
ok github.com/jaegertracing/jaeger-operator/pkg/util 0.212s coverage: 100.0% of statements
? github.com/jaegertracing/jaeger-operator/pkg/version [no test files]
make: *** [unit-tests] Error 1
... because it finds the body of case *appsv1.Deployment:
empty and breaks out without doing anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @annanay25 and @jpkrohling)
pkg/inject/sidecar.go, line 44 at r4 (raw file):
Previously, annanay25 (Annanay Agarwal) wrote…
When I make this change, the following happens:
➜ jaeger-operator git:(statefulsets-sidecar) ✗ make test Running unit tests... ? github.com/jaegertracing/jaeger-operator/cmd [no test files] ? github.com/jaegertracing/jaeger-operator/cmd/manager [no test files] ok github.com/jaegertracing/jaeger-operator/pkg/account 0.289s coverage: 100.0% of statements ? github.com/jaegertracing/jaeger-operator/pkg/apis [no test files] ok github.com/jaegertracing/jaeger-operator/pkg/apis/io/v1alpha1 0.396s coverage: 17.0% of statements ? github.com/jaegertracing/jaeger-operator/pkg/cmd/start [no test files] ? github.com/jaegertracing/jaeger-operator/pkg/cmd/version [no test files] ok github.com/jaegertracing/jaeger-operator/pkg/config/sampling 0.278s coverage: 94.1% of statements ok github.com/jaegertracing/jaeger-operator/pkg/config/ui 0.356s coverage: 94.1% of statements ? github.com/jaegertracing/jaeger-operator/pkg/controller [no test files] ? github.com/jaegertracing/jaeger-operator/pkg/controller/deployment [no test files] ok github.com/jaegertracing/jaeger-operator/pkg/controller/jaeger 3.567s coverage: 64.3% of statements ok github.com/jaegertracing/jaeger-operator/pkg/cronjob 0.341s coverage: 93.5% of statements ok github.com/jaegertracing/jaeger-operator/pkg/deployment 0.288s coverage: 100.0% of statements ok github.com/jaegertracing/jaeger-operator/pkg/ingress 0.205s coverage: 100.0% of statements --- FAIL: TestInjectSidecar (0.00s) sidecar_test.go:33: Error Trace: sidecar_test.go:33 Error: "[{ [] [] [] [] [] {map[] map[]} [] [] nil nil nil nil %!s(bool=false) %!s(bool=false) %!s(bool=false)}]" should have 2 item(s), but has 1 Test: TestInjectSidecar panic: runtime error: index out of range [recovered] panic: runtime error: index out of range goroutine 11 [running]: testing.tRunner.func1(0xc42016a960) /usr/local/go/src/testing/testing.go:742 +0x29d panic(0x1753be0, 0x1c9e3f0) /usr/local/go/src/runtime/panic.go:502 +0x229 github.com/jaegertracing/jaeger-operator/pkg/inject.TestInjectSidecar(0xc42016a960) /Users/annanay.a/Desktop/git/go/src/github.com/jaegertracing/jaeger-operator/pkg/inject/sidecar_test.go:34 +0x2e0 testing.tRunner(0xc42016a960, 0x1865c88) /usr/local/go/src/testing/testing.go:777 +0xd0 created by testing.(*T).Run /usr/local/go/src/testing/testing.go:824 +0x2e0 FAIL github.com/jaegertracing/jaeger-operator/pkg/inject 0.293s ok github.com/jaegertracing/jaeger-operator/pkg/route 0.405s coverage: 100.0% of statements ok github.com/jaegertracing/jaeger-operator/pkg/service 0.275s coverage: 100.0% of statements ok github.com/jaegertracing/jaeger-operator/pkg/storage 0.252s coverage: 100.0% of statements ok github.com/jaegertracing/jaeger-operator/pkg/strategy 0.265s coverage: 100.0% of statements ok github.com/jaegertracing/jaeger-operator/pkg/util 0.212s coverage: 100.0% of statements ? github.com/jaegertracing/jaeger-operator/pkg/version [no test files] make: *** [unit-tests] Error 1
... because it finds the body of
case *appsv1.Deployment:
empty and breaks out without doing anything.
Sorry, my bad. I should have added a comma and then would realize that it won't work anyway:
https://golang.org/ref/spec#Type_switches
My main problem with the current code is that it won't scale nicely for all types that include a PodSpec/PodTemplateSpec, like batches, jobs, daemon sets, ... How about creating a callback-type of function, that accepts a list of containers? Or perhaps you can come up with a way to have the Sidecar
/decorate
to accept a PodTemplateSpec
? That's pretty much all you need to inject a sidecar, no?
Completely agree. We need a better way to do this. I think we need more than the
|
That's fine, you can just pass the namespace then. Is that all you'd need? PodTemplateSpec + Namespace? |
I think there are quite a few merge conflicts now. Would you like to fix them, or should I close it? |
I'm going to take a stab at this soon :) |
Signed-off-by: Annanay <[email protected]>
Signed-off-by: Annanay <[email protected]>
Signed-off-by: Annanay <[email protected]>
e3a81ee
to
9a1e78f
Compare
Signed-off-by: Annanay <[email protected]>
Signed-off-by: Annanay <[email protected]>
@jpkrohling could you tell me if the functions look good? I'll correct the tests accordingly :) |
Looks like the build is failing:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r5.
Reviewable status: 1 of 3 files reviewed, 12 unresolved discussions (waiting on @annanay25 and @jpkrohling)
pkg/controller/deployment/deployment_controller.go, line 91 at r5 (raw file):
switch o := *obj.(type) { case *appsv1.Deployment:
Sorry for bringing this up again, but could you try to use v1.Object
from https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/meta.go#L33 ?, where v1
is "k8s.io/apimachinery/pkg/apis/meta/v1"
I recently found this one and it might solve this type of problem.
Signed-off-by: Annanay <[email protected]>
Signed-off-by: Annanay <[email protected]>
Signed-off-by: Annanay <[email protected]>
|
I mean, instead of having almost the same code for |
@annanay25, I'm closing this one, as we are currently working on the webhook approach (see #171), which would make this obsolete. If you still want to work on this before we move on with that other approach, feel free to reopen. |
Attempts to resolve #62.
Need to check test coverage.