Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Match pod dnsPolicy to hostNetwork config #691

Merged
merged 4 commits into from
Feb 25, 2022
Merged

Match pod dnsPolicy to hostNetwork config #691

merged 4 commits into from
Feb 25, 2022

Conversation

gai6948
Copy link
Contributor

@gai6948 gai6948 commented Feb 6, 2022 •
edited by pavolloffay
Loading

Resolves #690

I added a simple check to make the pods' DNSPolicy matching the hostNetwork option

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 6, 2022 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@gai6948 gai6948 marked this pull request as ready for review February 6, 2022 13:01
@gai6948 gai6948 requested review from a team and jpkrohling February 6, 2022 13:01
pavolloffay
pavolloffay reviewed Feb 7, 2022
View reviewed changes
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test to validate this.

pkg/collector/utils.go
)

func getDnsPolicy(otelcol v1alpha1.OpenTelemetryCollector) corev1.DNSPolicy {
dnsPolicy := corev1.DNSClusterFirst
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be rather "Default" ?

https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/#pod-s-dns-policy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found this section on that page:

Note: "Default" is not the default DNS policy. If dnsPolicy is not explicitly specified, then "ClusterFirst" is used.

@gai6948
Copy link
Contributor Author

gai6948 commented Feb 8, 2022

Actually I don't know if hostNetwork=true makes sense for deployment and statefulsets, if it isn't perhaps we only need to test on daemonsets and not needing to duplicate such simple tests

@pavolloffay
Copy link
Member

@gai6948 the CI failed

@pavolloffay
Copy link
Member

Actually I don't know if hostNetwork=true makes sense for deployment and statefulsets, if it isn't perhaps we only need to test on daemonsets and not needing to duplicate such simple tests

This is why we should understand why people want to use hostNetwork

@gai6948
Copy link
Contributor Author

gai6948 commented Feb 11, 2022

Actually I don't know if hostNetwork=true makes sense for deployment and statefulsets, if it isn't perhaps we only need to test on daemonsets and not needing to duplicate such simple tests

This is why we should understand why people want to use hostNetwork

My use case for hostNetwork is for my application pod to connect to daemonset running on same host

@pavolloffay
Copy link
Member

@gai6948 the CI is still failing? Please fix it to move this forward.

@VineethReddy02
Copy link
Contributor

@gai6948 FYI, the host network tests are failing for details on tests, here are refs

https://github.com/open-telemetry/opentelemetry-operator/runs/5274662525?check_suite_focus=true#step:7:280

https://github.com/open-telemetry/opentelemetry-operator/runs/5274662525?check_suite_focus=true#step:7:306

@gai6948
Copy link
Contributor Author

gai6948 commented Feb 22, 2022

The tests should pass now

@pavolloffay pavolloffay merged commit 4dfffb8 into open-telemetry:main Feb 25, 2022
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
@gai6948
Match pod dnsPolicy to hostNetwork config (open-telemetry#691)
79b9475 
* feat: match pod dnspolicy to hostNetwork

* feat: add dnspolicy support to deployment and sts

* added tests for hostNetwork DNSpolicy

* use hostnetwork spec in deployment and statefulset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavolloffay pavolloffay pavolloffay approved these changes

@jpkrohling jpkrohling Awaiting requested review from jpkrohling

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Uses "ClusterFirstWithHostNet" DNS policy when hostNetwork is set to true
3 participants
@gai6948 @pavolloffay @VineethReddy02