Fix host DNS settings being edited too soon, and not working with NetworkManager #8575

mac-chaffee · 2022-02-22T21:39:00Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

This PR fixes two bugs:

When the host uses NetworkManager, but isn't Fedora CoreOS (like RHEL), kubespray will currently ignore that fact and just edit /etc/resolv.conf directly. Since FCOS isn't the only OS that runs NetworkManager, I fixed the issue by running the networkmanager-specific tasks if the NetworkManager service is running on the host (rather than checking if the OS is FCOS).
Fixes Issue Configuration of systemd-resolved does not behave as documented in docs/dns-stack.md #8560 for two additional environments: hosts that use NetworkManager, and hosts that use /etc/resolv.conf directly. PR [systemd-resolved] Fix DNS configuration according to docs/dns-stack.… #8561 provides the fix for systemd-resolved, this PR fixes the rest.

Which issue(s) this PR fixes:
Fixes #8560 for hosts other than those using systemd-resolved.

Special notes for your reviewer:

This PR relies on PR #8561 (specifically that line in reset.yml) to fully fix issue #8560 in all environments. But merge order shouldn't matter.

Does this PR introduce a user-facing change?:

Fixed a bug where hosts with NetworkManager enabled were having their /etc/resolv.conf file edited directly instead of through NM.
Fixed a bug where DNS lookup failures would cause reset.yml or scale.yml to error out when resolvconf_mode=host_resolvconf

k8s-ci-robot · 2022-02-22T21:39:08Z

Hi @mac-chaffee. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

mac-chaffee · 2022-02-22T21:41:14Z

roles/kubernetes/preinstall/tasks/0040-set_facts.yml

-    nameserverentries:
-      nameserver {{ ( ( [nodelocaldns_ip] if enable_nodelocaldns else []) + coredns_server|d([]) + nameservers|d([]) + cloud_resolver|d([]) + configured_nameservers|d([])) | unique | join(',nameserver ') }}
+    nameserverentries: |-
+      {{ ( ( [nodelocaldns_ip] if enable_nodelocaldns else []) + coredns_server|d([]) + nameservers|d([]) + cloud_resolver|d([]) + configured_nameservers|d([])) | unique | join(',') }}


This change allows us to specify this complex list of nameservers one time, then re-use the value in both the raw /etc/resolv.conf task and the NetworkManager task.

cristicalin · 2022-02-23T07:50:19Z

This looks great @mac-chaffee, thanks taking case of this.
I was recently bit by the fact that we include the nodelocaldns IP in /etc/resolve.conf too early so it is a much welcomed improvement.

/ok-to-test
/lgtm

/cc @floryut @oomichi

mac-chaffee · 2022-02-23T12:04:29Z

I'd like to run a manual test for at least scale.yml and maybe other plays (unless CI tests those) since those plays cover some extra branches of this code.

/hold

cristicalin · 2022-02-23T13:24:49Z

I'd like to run a manual test for at least scale.yml and maybe other plays (unless CI tests those) since those plays cover some extra branches of this code.

Great idea. It would be good to add this scenario in the CI. We cover upgrades and broken node replacement and there is some incipient ha-scale support but no ci test covering it.

mac-chaffee · 2022-02-23T21:07:21Z

upgrade-cluster.yml

  gather_facts: False
  any_errors_fatal: "{{ any_errors_fatal | default(true) }}"
  environment: "{{ proxy_disable_env }}"
  roles:
    - { role: kubespray-defaults }
-    - { role: kubernetes/preinstall, when: "dns_mode != 'none' and resolvconf_mode == 'host_resolvconf'", tags: resolvconf }
+    - { role: kubernetes/preinstall, when: "dns_mode != 'none' and resolvconf_mode == 'host_resolvconf'", tags: resolvconf, dns_late: true }


Added the extra dns_late: true here to match cluster.yml and the others since this setting allows you to skip much of the preinstall role that doesn't involve updating resolv.conf.

mac-chaffee · 2022-02-23T21:07:24Z

Glad I checked! I had to add @onock's code to the end of scale.yml so that the DNS settings are fully applied at the end of the play. EDIT: nevermind, not true, that code was there already and I just missed it because I was testing on release-2.18.

I also added some comments to help the next poor soul who might have to debug this again :)

mac-chaffee · 2022-02-23T21:08:39Z

/unhold

Signed-off-by: Mac Chaffee <[email protected]>

oomichi · 2022-02-24T19:30:52Z

Nice work to support NetManager systems which are not FCOS also.

/lgtm

cristicalin · 2022-02-26T18:26:49Z

Thanks for this @mac-chaffee !

/approve

k8s-ci-robot · 2022-02-26T18:27:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, mac-chaffee

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:

~~OWNERS~~ [cristicalin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

floryut · 2022-03-16T11:00:04Z

Registering the error here more as a reminder than a "you have to fix" but since this PR the nightly fedora job is failing
https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/2142124125
(same job before the change https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/2139329846)

mac-chaffee · 2022-03-17T04:32:52Z

Dang, I'm away right now but I'll be back on Monday to give it a proper look.

…NM (kubernetes-sigs#8575) Signed-off-by: Mac Chaffee <[email protected]>

vielmetti · 2022-04-28T20:15:24Z

@mac-chaffee have you been able to take a look at this, as referenced above? thanks

mac-chaffee · 2022-04-28T20:19:15Z

@vielmetti I haven't been able to reproduce that issue. Since I can't trigger the CI to run again or tweak the log level or SSH into the host, I'm a little stuck on what to do unfortunately :(

I've been using a version of kubespray which includes this change for two upgrades so far with no issue (on RHEL), so it might be a Fedora-specific issue.

…NM (kubernetes-sigs#8575) Signed-off-by: Mac Chaffee <[email protected]>

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 22, 2022

k8s-ci-robot requested review from cristicalin and holmsten February 22, 2022 21:39

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 22, 2022

mac-chaffee commented Feb 22, 2022

View reviewed changes

k8s-ci-robot requested review from floryut and oomichi February 23, 2022 07:50

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 23, 2022

k8s-ci-robot assigned cristicalin Feb 23, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2022

cristicalin mentioned this pull request Feb 23, 2022

[systemd-resolved] Fix DNS configuration according to docs/dns-stack.… #8561

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2022

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2022

mac-chaffee commented Feb 23, 2022

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2022

Fix host DNS config 1) being edited too soon and 2) not working with NM

95fd296

Signed-off-by: Mac Chaffee <[email protected]>

k8s-ci-robot assigned oomichi Feb 24, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 24, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2022

k8s-ci-robot merged commit b554246 into kubernetes-sigs:master Feb 26, 2022

rtsp pushed a commit to rtsp/kubespray that referenced this pull request Apr 12, 2022

Fix host DNS config 1) being edited too soon and 2) not working with …

80e20c5

…NM (kubernetes-sigs#8575) Signed-off-by: Mac Chaffee <[email protected]>

sakuraiyuta pushed a commit to sakuraiyuta/kubespray that referenced this pull request Apr 16, 2022

Fix host DNS config 1) being edited too soon and 2) not working with …

9602c3c

…NM (kubernetes-sigs#8575) Signed-off-by: Mac Chaffee <[email protected]>

oomichi mentioned this pull request May 28, 2022

Release Proposal v2.19 #8885

Closed

rptaylor mentioned this pull request Nov 28, 2022

frontier-squid: individually addressable pods for CVMFS sharding sciencebox/charts#97

Closed

mac-chaffee deleted the fix-8560 branch January 20, 2023 00:15

LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jun 29, 2023

Fix host DNS config 1) being edited too soon and 2) not working with …

b7ef0ff

…NM (kubernetes-sigs#8575) Signed-off-by: Mac Chaffee <[email protected]>

LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Oct 22, 2023

Fix host DNS config 1) being edited too soon and 2) not working with …

17ba5ff

…NM (kubernetes-sigs#8575) Signed-off-by: Mac Chaffee <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix host DNS settings being edited too soon, and not working with NetworkManager #8575

Fix host DNS settings being edited too soon, and not working with NetworkManager #8575

mac-chaffee commented Feb 22, 2022

k8s-ci-robot commented Feb 22, 2022

mac-chaffee Feb 22, 2022

cristicalin commented Feb 23, 2022

mac-chaffee commented Feb 23, 2022

cristicalin commented Feb 23, 2022

mac-chaffee Feb 23, 2022

mac-chaffee commented Feb 23, 2022 •

edited

Loading

mac-chaffee commented Feb 23, 2022

oomichi commented Feb 24, 2022

cristicalin commented Feb 26, 2022

k8s-ci-robot commented Feb 26, 2022

floryut commented Mar 16, 2022 •

edited

Loading

mac-chaffee commented Mar 17, 2022

vielmetti commented Apr 28, 2022

mac-chaffee commented Apr 28, 2022

Fix host DNS settings being edited too soon, and not working with NetworkManager #8575

Fix host DNS settings being edited too soon, and not working with NetworkManager #8575

Conversation

mac-chaffee commented Feb 22, 2022

k8s-ci-robot commented Feb 22, 2022

mac-chaffee Feb 22, 2022

Choose a reason for hiding this comment

cristicalin commented Feb 23, 2022

mac-chaffee commented Feb 23, 2022

cristicalin commented Feb 23, 2022

mac-chaffee Feb 23, 2022

Choose a reason for hiding this comment

mac-chaffee commented Feb 23, 2022 • edited Loading

mac-chaffee commented Feb 23, 2022

oomichi commented Feb 24, 2022

cristicalin commented Feb 26, 2022

k8s-ci-robot commented Feb 26, 2022

floryut commented Mar 16, 2022 • edited Loading

mac-chaffee commented Mar 17, 2022

vielmetti commented Apr 28, 2022

mac-chaffee commented Apr 28, 2022

mac-chaffee commented Feb 23, 2022 •

edited

Loading

floryut commented Mar 16, 2022 •

edited

Loading