add agent dnsPolicy option #1370
Conversation
faceair
force-pushed
the
add-dns-policy
branch
from
d874815 to
954102b
Compare
Codecov Report
@@ Coverage Diff @@
## master #1370 +/- ##
==========================================
+ Coverage 86.72% 86.73% +0.01%
==========================================
Files 91 91
Lines 5056 5060 +4
==========================================
+ Hits 4385 4389 +4
Misses 516 516
Partials 155 155
Continue to review full report at Codecov.
|
| if a.jaeger.Spec.Agent.HostNetwork != nil { | ||
| hostNetwork = *a.jaeger.Spec.Agent.HostNetwork | ||
| if dnsPolicy == "" { | ||
| dnsPolicy = corev1.DNSClusterFirstWithHostNet |
There was a problem hiding this comment.
Do you need to set a default value here? Is that different than if a value simply isn't provided?
There was a problem hiding this comment.
According to the k8s documentation, if we do not set dnsPolicy to DNSClusterFirstWithHostNet, k8s will use ClusterFirst by default, and the daemonset agent will not be able to resolve the service ip of the collector properly.
Error while dialing dial tcp: lookup jaeger-collector.observability.svc.cluster.local on {hostIP}:53: no such host
There was a problem hiding this comment.
Does it mean that DaemonSets aren't working at the moment? I think we have e2e tests exercising this.
There was a problem hiding this comment.
Yes, DaemonSets should not be working now. My company has bypassed this problem by forwarding some dns domains to coredns on the physical machine.
I can try adding the corresponding test case to the e2e test.
There was a problem hiding this comment.
@kevinearls, we do have this, don't we?
@faceair if you could provide an e2e to prove, that would be awesome!
There was a problem hiding this comment.
I modified the existing es2 test case, is this OK?
|
@faceair @jpkrohling We have daemonset_test.go but I have not seen any failures with it. Does this mean there could be a problem with the test? |
|
Would love to see this merged. I'm using the daemonset, and agents are not able to find collector unless I patch with |
faceair
force-pushed
the
add-dns-policy
branch
2 times, most recently
from
ecfc771 to
c2cd1b0
Compare
|
Any progress on merging this? |
|
ping @kevinearls |
|
@obliadp Can you just fix the spelling of "agent-as-sidecar-with-hostnetowk" in test/e2e/sidecar_test.go? |
Signed-off-by: faceair <[email protected]>
Signed-off-by: faceair <[email protected]>
|
@obliadp @faceair @kevinearls @jpkrohling |
|
@faceair are you still interested in this PR? |
|
Is there anything else that needs to be changed in this pr? I've fixed the typo. |
|
@kevinearls could you please review this one again? |
test/e2e/sidecar_test.go
Outdated
| @@ -70,8 +72,8 @@ func (suite *SidecarTestSuite) AfterTest(suiteName, testName string) { | |||
| func (suite *SidecarTestSuite) TestSidecar() { | |||
| cleanupOptions := &framework.CleanupOptions{TestContext: ctx, Timeout: timeout, RetryInterval: retryInterval} | |||
|
|
|||
| firstJaegerInstanceName := "agent-as-sidecar" | |||
| firstJaegerInstance := createJaegerAgentAsSidecarInstance(firstJaegerInstanceName, namespace) | |||
| firstJaegerInstanceName := "agent-as-sidecar-with-hostnetowk" | |||
There was a problem hiding this comment.
nit - "agent-as-sidecar-with-hostnetwork"
|
Thanks for your contribution, @faceair! |
ref: https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#pod-s-dns-policy
It should solve some dns problems, like this #676 (comment)