Do not remove upstream DNS configuraton in cleanup #1280
Conversation
|
/assign @hardys |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flaper87 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 |
openshift-ci
bot
added
the
approved
|
The only problem with doing this unconditionally is some folks expect the dev-scripts changes to be reverted on I wonder if we can somehow detect the resolv.conf isn't NM generated and save it somewhere to restore during host_cleanup.sh? (perhaps looking for presence of |
Yes, this is the most reasonable assumption, but it is not the case already now - i.e. running Honestly, I'm not sure if there is a silver bullet here...
Yes, if |
Right, we're in agreement this is something which needs to be fixed, it's just a question of how :)
I think the majority of folks do have a dedicated machine, but clearly @dustinblack had a reason for pushing his cleanup PR, so I'm trying to find a way to solve your problem without reintroducing his ;) I don't think any of these changes are impossible to revert, we just need to backup and restore the resolv.conf, right? That could even be done unconditionally, since in the NM-managed case it'll just get overwritten, but in the static-config case we'll restore the previous config? |
|
Another point to note is that even when folks have a dedicated machine, sometimes they will want to switch between dev-scripts testing and other setups (upstream metal3-dev-env, maybe assisted-test-infra etc) |
|
@mkowalski I think there is value in revisiting this PR, if you have the bandwidth. |
mkowalski
force-pushed
the
dns-do-not-cleanup
branch
from
f31be61
to
f0ae110
Compare
|
A simple change now should be enough to handle multiple scenarios with as simple logic as possible. I tried to avoid using specific markers in the
|
|
/retest None of them looks like failing because of this change |
|
/retest |
|
/test e2e-metal-ipi-serial-ovn-ipv6 |
mkowalski
force-pushed
the
dns-do-not-cleanup
branch
from
f0ae110
to
262a631
Compare
This PR changes the logic of host cleanup so that the upstream DNS configuration is not removed. This comes from the fact that by default dev-scripts configure NetworkManager to use dnsmasq with DHCP-provided DNS servers and removes the existing /etc/resolv.conf configuration. This causes problems in environments configured without DHCP with DNS servers configured manually without use of the NetworkManager. With this PR user can use `ADDN_DNS` variable to provide their own DNS servers that will be passed to the dnsmasq. At the same time when removing dev-scripts from the system, the backup of the original /etc/resolv.conf will be restored so that the initial system configuration is reverted.
mkowalski
force-pushed
the
dns-do-not-cleanup
branch
from
262a631
to
71929ca
Compare
|
@mkowalski: 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. |
|
/lgtm |
This PR changes the logic of host cleanup so that the upstream DNS
configuration is not removed. This comes from the fact that by default
dev-scripts configure NetworkManager to use dnsmasq with DHCP-provided
DNS servers and removes the existing /etc/resolv.conf configuration.
This causes problems in environments configured without DHCP with DNS
servers configured manually without use of the NetworkManager. With this
PR user can use
ADDN_DNSvariable to provide their own DNS serversthat will be passed to the dnsmasq. At the same time when removing
dev-scripts from the system, the backup of the original /etc/resolv.conf
will be restored so that the initial system configuration is reverted.