-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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.
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
Fix coredns NodeHosts on dual-stack clusters #9584
Conversation
* Add both dual-stack addresses to the node hosts file * Add hostname to hosts file as alias for node name to ensure consistent resolution Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9584 +/- ##
===========================================
- Coverage 49.44% 27.75% -21.70%
===========================================
Files 151 154 +3
Lines 13471 13558 +87
===========================================
- Hits 6661 3763 -2898
- Misses 5467 9008 +3541
+ Partials 1343 787 -556
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
nodeIPv4 = "" | ||
nodeIPv6 = "" | ||
} else if nodeIPv4 == "" && nodeIPv6 == "" { | ||
logrus.Errorf("No InternalIP addresses found for node " + nodeName) | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to changes here, but shouldn't we return the error instead of just logging it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, returning an error from a handler causes wrangler to reenque the handler and run it again, with the expectation that there is an external problem that can be retried. In this case there is nothing to retry - this state cannot be handled, we just need to wait for another update where the node does have IPs set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks for the explanation Brad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Proposed Changes
Types of Changes
bugfix
Verification
kubectl get configmap -n kube-system coredns -o template --template '{{.data.NodeHosts}}'
Testing
Linked Issues
User-Facing Change
Further Comments