Support deployment of agent as a DaemonSet #52
Conversation
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
==========================================
+ Coverage 99.17% 99.26% +0.09%
==========================================
Files 16 16
Lines 603 684 +81
==========================================
+ Hits 598 679 +81
Misses 5 5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewable status: 0 of 17 files reviewed, 1 unresolved discussion (waiting on @jpkrohling and @objectiser)
pkg/deployment/all-in-one.go, line 125 at r1 (raw file):
}, }, InitialDelaySeconds: 1,
Just curious why reducing to 1? Should this be a global config option on the CR?
|
@jpkrohling LGTM, but would probably be a good idea to get someone with more Go experience to review. In terms of the test - it would be good if such a test could be available in a more general location so that it could be shared - but if no obvious place at the moment then I guess it could be placed in this repo for now. |
There was a problem hiding this comment.
Reviewable status: 0 of 17 files reviewed, 1 unresolved discussion (waiting on @jpkrohling)
pkg/deployment/all-in-one.go, line 125 at r1 (raw file):
Previously, objectiser (Gary Brown) wrote…
Just curious why reducing to 1? Should this be a global config option on the CR?
This is meant to reflect the startup time of the application. Jaeger is really fast to start, so, it would show up as "not ready yet" for 5 seconds, when it's really ready for some seconds already.
jpkrohling
force-pushed
the
46-Agent-as-DaemonSet
branch
from
5a507fc to
3f277ed
Compare
I'll create an issue to create such test here in this repo as a temporary measure, but I would like to do that in a follow-up PR. |
|
@objectiser said:
@pavolloffay , would you like to review this one? |
| { | ||
| ContainerPort: 5775, | ||
| HostPort: 5775, | ||
| Name: "zk-compact-trft", |
There was a problem hiding this comment.
Is this populated to k8s objects? If so I would like to see the full names I had to think that means zk-
There was a problem hiding this comment.
It is, but unfortunately, there's a limit of 15 chars for this name. This is the best I could do with 15 chars while keeping it reasonably meaningful.
There was a problem hiding this comment.
that is weird IIRC in resource files we were using full names
There was a problem hiding this comment.
For Container ports? Or for service names? For the ContainerPort object, this is what I see in Kubernetes docs:
If specified, this must be an IANA_SVC_NAME and unique within the pod. Each named port in a pod must have a unique name. Name for the port that can be referred to by services.
Where IANA_SVC_NAME is:
at most 15 characters, matching regex a-z0-9* and it must containts at least one letter [a-z], hypens cannot be adjacent to other hyphens): e.g. "http"
Perhaps you are talking about the "name" directive for the "service" object? Those are DNS_LABELS, which can be longer.
There was a problem hiding this comment.
This https://github.com/jaegertracing/jaeger-kubernetes/blob/master/jaeger-production-template.yml#L78 it looks like a service port name
|
the vertx app looks very simple I guess it only creates a span. We could have it here as a simple go service or even or job (no need for server). |
Will be done as part of #57 . For now, I just used what I already had. |
Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling
force-pushed
the
46-Agent-as-DaemonSet
branch
from
3f277ed to
895f552
Compare
This PR adds support for deploying the agent as DaemonSet.
Note that this PR includes an e2e test that makes use of the Docker image
jaegertracing/vertx-create-span:operator-e2e-tests, whose source can be found here: https://gitlab.com/jpkroehling/vertx-create-span/blob/master/deployment.yaml . I'm not sure how to best proceed with those sources. A suggestion is to make a simple Go application that is similar to that vertx one and build it as part of this project's.Closes #46
Signed-off-by: Juraci Paixão Kröhling [email protected]