NE-2414: Use trafficDistribution: PreferSameNode for openshift-dns Service - #457
Conversation
|
@danwinship: This pull request references NE-2414 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
@danwinship: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/approve Leaving the lgtm for @Miciah |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rikatz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
following up from slack: this makes the setting of topology aware hints in the existing code redundant, since |
|
/assign |
|
/assign @rikatz |
|
Verified using cluster bot Also able to run topology aware hint test case ocp-63553 successfully |
|
@melvinjoseph86: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
The failure in the `DNS should answer A and AAAA queries for a dual-stack service` test on metal jobs was triggered by the recent merge of PR openshift#457 (`NE-2414: Use trafficDistribution: PreferSameNode for openshift-dns Service`). ### The Root Cause In `pkg/operator/controller/controller_dns_service.go`, the operator compares the `current` DNS Service against the `expected` (static) asset using a function called `serviceChanged()`. To prevent unnecessary updates, the comparison correctly ignores dynamic dual-stack fields. However, when PR openshift#457 added `trafficDistribution: PreferSameNode` to the static asset, it caused `expected` and `current` to differ on existing clusters, legitimately triggering a Service update. When an update is triggered, the operator blindly replaces the spec (`updated.Spec = expected.Spec`) and then tries to manually preserve dynamic fields that the API server or other controllers manage. It only preserved `ClusterIP`, dropping `ClusterIPs`, `IPFamilies`, and `IPFamilyPolicy`. ### The Impact Because `expected.Spec` (from the static YAML) has no IP families defined, the operator pushes a Service update that wipes out the secondary IPv6 Cluster IP. The `openshift-dns` service is abruptly converted back to a single-stack IPv4 service, which permanently breaks the `DNS should answer A and AAAA queries for a dual-stack service` test on metal (dual-stack) jobs. ### The Fix This explicitly preserves the dual-stack fields (`ClusterIPs`, `IPFamilies`, `IPFamilyPolicy`) during an update. It also improves the dual-stack test assertions to verify these fields are preserved field-for-field. Jira: https://issues.redhat.com/browse/OCPBUGS-78053
The failure in the `DNS should answer A and AAAA queries for a dual-stack service` test on metal jobs was triggered by the recent merge of PR openshift#457 (`NE-2414: Use trafficDistribution: PreferSameNode for openshift-dns Service`). ### The Root Cause In `pkg/operator/controller/controller_dns_service.go`, the operator compares the `current` DNS Service against the `expected` (static) asset using a function called `serviceChanged()`. To prevent unnecessary updates, the comparison correctly ignores dynamic dual-stack fields. However, when PR openshift#457 added `trafficDistribution: PreferSameNode` to the static asset, it caused `expected` and `current` to differ on existing clusters, legitimately triggering a Service update. When an update is triggered, the operator blindly replaces the spec (`updated.Spec = expected.Spec`) and then tries to manually preserve dynamic fields that the API server or other controllers manage. It only preserved `ClusterIP`, dropping `ClusterIPs`, `IPFamilies`, and `IPFamilyPolicy`. ### The Impact Because `expected.Spec` (from the static YAML) has no IP families defined, the operator pushes a Service update that wipes out the secondary IPv6 Cluster IP. The `openshift-dns` service is abruptly converted back to a single-stack IPv4 service, which permanently breaks the `DNS should answer A and AAAA queries for a dual-stack service` test on metal (dual-stack) jobs. ### The Fix This explicitly preserves the dual-stack fields (`ClusterIPs`, `IPFamilies`, `IPFamilyPolicy`) during an update. It also improves the dual-stack test assertions to verify these fields are preserved field-for-field. Jira: https://issues.redhat.com/browse/OCPBUGS-78085
DNS queries from pods on primary user-defined networks (UDNs) scatter randomly across dns-default pods on all nodes instead of being handled by the local node's dns-default pod. UDN pods reach the dns-default service (172.30.0.10) through a path that crosses network boundaries: UDN pod -> UDN cluster router -> management port -> default network -> OVN load balancer -> dns-default pod. The OVN load balancer on the default network treats dns-default as a standard ClusterIP service and distributes traffic across all backend pods cluster-wide. Setting internalTrafficPolicy to Local restricts the EndpointSlice to contain only the node-local dns-default backend. Since dns-default runs as a DaemonSet with a pod on every node, this is safe and guarantees that DNS queries are always handled by the local pod. The existing trafficDistribution: PreferSameNode (added in PR openshift#457) provides a soft hint for same-node preference but does not guarantee locality for cross-network UDN traffic. internalTrafficPolicy: Local provides the hard constraint needed. Jira: https://issues.redhat.com/browse/OCPBUGS-55179 Signed-off-by: Parikshit Khedekar <pkhedeka@redhat.com>
ovn-kubernetes and our build of kube-proxy both currently special-case openshift-dns to give it "prefer same node" semantics. As of k8s 1.34 / OCP 4.22, we can do this in a standard way by adding "
trafficDistribution: PreferSameNode" to theopenshift-dnsservice.(
trafficDistributionitself is GA as of k8s 1.33,PreferSameNodeis Beta in 1.34 and already GA upstream in 1.35.)(ovn-k probably won't implement
trafficDistributionin 4.22, but merging this now will let us get rid of a patch for kube-proxy in openshift/kubernetes.)