fix(dns): don't emit ClusterDNSDown when clusterDomains empty (unmasks pod probe) - #29
Conversation
…flict with pod probe) When clusterDomains: [] (the hostNetwork default — the agent can't resolve ClusterIP cluster records via Cilium, so the in-agent cluster check is disabled), checkDNSDomains trivially returns true, making the DNS monitor emit an unconditional ClusterDNSDown=False. That masks the pod-network cluster-dns-pod monitor's real ClusterDNSDown=True: two monitors were writing the same condition and the disabled dns-health check would win with a false 'healthy'. Guard the ClusterDNSDown emission on len(ClusterDomains) > 0 so the DNS monitor only owns the condition when it is actually checking cluster domains; otherwise the cluster-dns-pod monitor owns it exclusively. ExternalDNSDown is unaffected. Found live during the 242 re-enable roll: ClusterDNSDown showed reason 'ClusterDNSResolved' (dns-health) instead of 'ClusterDNSResolvedViaPods' (pod monitor). Tests: no ClusterDNSDown when clusterDomains empty (healthy or failing); normal True/False emission when set.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d3f77c1e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // unconditional ClusterDNSDown=False and MASK a real ClusterDNSDown=True reported by | ||
| // the pod-network cluster-dns-pod monitor (which owns this condition when the in-agent | ||
| // check is disabled). So skip the emission entirely when no cluster domains are set. | ||
| if len(m.config.ClusterDomains) > 0 { |
There was a problem hiding this comment.
When clusterDomains: [] and the pod monitor is not enabled/running, this guard stops the DNS monitor from writing ClusterDNSDown at all. Existing NodeDoctorClusterDNSDown=True values are preserved on startup because retiredDNSConditionTypes does not include it (pkg/exporters/kubernetes/condition_manager.go:237), and the Kubernetes exporter only updates conditions present in each status, so an upgrade or config reload from a failing cluster-domain check to disabled cluster checks leaves the old True condition latched indefinitely instead of clearing/removing it. Please either remove/clear the condition when disabling the in-agent check or ensure another owner is configured before suppressing both True and False writes.
Useful? React with 👍 / 👎.
1 participant
Summary
Found live during the task-242 cluster-DNS re-enable roll. Two monitors were writing
ClusterDNSDown: the newcluster-dns-podmonitor (pod-network truth) and the dns-health monitor. WithclusterDomains: [](the hostNetwork default),checkDNSDomainsover zero domains trivially returns true, so dns-health emitted an unconditionalClusterDNSDown=False— which would mask the pod monitor'sTrueon a real cluster-DNS outage. Live symptom: the condition's reason wasClusterDNSResolved(dns-health) instead ofClusterDNSResolvedViaPods(pod monitor).Fix
Guard the
ClusterDNSDownemission inupdateFailureTrackingonlen(ClusterDomains) > 0. The DNS monitor owns the condition only when it is actually checking cluster domains; otherwise thecluster-dns-podmonitor owns it exclusively.ExternalDNSDownand all other behavior unchanged.Tests
clusterDomainsempty → noClusterDNSDownemitted (healthy OR after repeated failures);ExternalDNSDownstill emitted.clusterDomainsset → normalFalse(healthy) /True(past threshold) emission preserved.Required for the 242 re-enable to reliably detect cluster-DNS failures (currently deployed at v1.8.3 rev 16). Ships in v1.8.4.