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
Bug 1919271: [on-prem] NM resolve prepender: support appending a nameserver for systemd-resolved #2359
Bug 1919271: [on-prem] NM resolve prepender: support appending a nameserver for systemd-resolved #2359
Conversation
@vrutkovs: This pull request references Bugzilla bug 1919271, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In 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 kubernetes/test-infra repository. |
/test okd-e2e-vsphere |
/retest |
/retest |
36d4f95
to
d494572
Compare
/test okd-e2e-vsphere |
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.
Code looks fine, I'm not sure if the Only leave the first 3 nameservers in /etc/resolv.conf
moving into the $NAMESERVER_IP
condition affects anything. It'd be good for the metal team to have the final lgtm
/var/run/NetworkManager/resolv.conf > /etc/resolv.tmp | ||
source /etc/os-release | ||
if [[ "$NAME" == "Fedora" ]]; then | ||
echo "[Resolve]" > /etc/systemd/resolved.conf.d/kni.conf |
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.
https://www.freedesktop.org/software/systemd/man/resolved.conf.html
I think we should use something like 60-kni.conf
to make ordering easier.
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.
Good idea, updated
…lved FCOS 33 doesn't use NM to manage resolv.conf. Instead it uses systemd-resolved. The script should not replace /etc/resolv.conf in this case, but instead create a dropin for systemd-resolved
d494572
to
558b245
Compare
Nameservers would be cleaned up on RHCOS when new interface is up and NAMESERVER_IP is resolved (previously we'd clean those regardless of NAMESERVER_IP result). I don't think this would regress in any corner case really |
/test e2e-openstack |
holding for on-prem reviews /hold |
/assign @bcrochet @EmilienM @jcpowermac @yboaron |
-e "/Generated by/c# Generated by KNI resolv prepender NM dispatcher script\nsearch $DOMAIN\nnameserver $NAMESERVER_IP" \ | ||
/var/run/NetworkManager/resolv.conf > /etc/resolv.tmp | ||
source /etc/os-release | ||
if [[ "$NAME" == "Fedora" ]]; then |
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.
What sets the $NAME variable? Don't you also need to check the version of Fedora?
Perhaps a better check would be to look for the use of systemd-resolved, so that there's no surprise when RHCOS switches to 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.
What sets the $NAME variable?
Its being set after source /etc/os-release
Don't you also need to check the version of Fedora?
We could add this later, but OKD 4.6 uses F33+, so systemd-resolved would certainly be used
Perhaps a better check would be to look for the use of systemd-resolved
That was my initial approach, but its not clear if there is a nice way to do that - check that /etc/resolv.conf is a symlink? systemd-resolved service is running?
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.
I think the most robust is systemctl is-active --quiet systemd-resolved
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.
For some reason that didn't work. I'll have another look and follow up on that later
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.
Yeah, don't worry too much about it. I think we'll know where to look if suddenly DNS stops working.
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.
I think checking for the link should be fine.
/lgtm I think this is good to go |
-e "/Generated by/c# Generated by KNI resolv prepender NM dispatcher script\nsearch $DOMAIN\nnameserver $NAMESERVER_IP" \ | ||
/var/run/NetworkManager/resolv.conf > /etc/resolv.tmp | ||
source /etc/os-release | ||
if [[ "$NAME" == "Fedora" ]]; then |
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.
I think checking for the link should be fine.
if [[ "$NAME" == "Fedora" ]]; then | ||
echo "[Resolve]" > /etc/systemd/resolved.conf.d/60-kni.conf | ||
echo "DNS=$NAMESERVER_IP" >> /etc/systemd/resolved.conf.d/60-kni.conf | ||
echo "Domains=$DOMAIN" >> /etc/systemd/resolved.conf.d/60-kni.conf |
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.
Wouldn't it be nicer to have a single heredoc or an echo with "\n" instead of three separate echo?
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.
The most cleanest way would be creating this file unconditionally - and it would be caught up in RHEL when it does the switch. WDYT?
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.
I don't have a preference how to write the file, seems echo with '\n' would the cleanest option. I don't mind rewriting it if you insist
/lgtm |
/lgtm Seems like everyone is in agreement on the functionality of this. If we want to modify how things get done we can always do that in a followup. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cybertron, Fedosin, iamemilio, jcpowermac, mandre, vrutkovs, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
/retest Please review the full test history for this PR and help us cut down flakes. |
13 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
Skipping optional tests |
/retest Please review the full test history for this PR and help us cut down flakes. |
@vrutkovs: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
@vrutkovs: All pull requests linked via external trackers have merged: Bugzilla bug 1919271 has been moved to the MODIFIED state. In 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 kubernetes/test-infra repository. |
FCOS 33 doesn't use NM to manage resolv.conf. Instead it uses systemd-resolved.
The script should not replace /etc/resolv.conf in this case, but instead create a dropin for systemd-resolved
4.6 cherry-pick: #2356