Skip to content
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

nameservers: Use nmcli to update the nameserver and search info #4145

Merged
merged 3 commits into from
May 7, 2024

Conversation

praveenkumar
Copy link
Member

From 4.15, because of ovn-kubernetes the info around nameserver and
search option in the /etc/resolv.conf not match with user expectation.
ovs-configuration service is run on this node which is a dependency
for kubelet and this make changes in the networking by creating bridge
network and restarting the NetworkManager. As soon as NM restart happen,
changes which are done as part of crc vm post start are vanished.

This PR is going to make sure that changes are done using nmcli and
shouldn't removed when NM restart happen. Following steps are done in
this PR

  • Start the ovs-configuration service, even it is enabled it doesn't
    autostart because it only required by kubelet-dependencies.target.
  • ovs-configuration service always create the network named as
    ovs-if-br-ex so we run the nmcli command to update this connection
    and add the nameserver and search option
  • Restart the NM to update the /etc/resolv.conf

Fixes: Issue #4144

pkg/crc/network/nameservers.go Show resolved Hide resolved
pkg/crc/network/nameservers.go Show resolved Hide resolved
pkg/crc/network/nameservers.go Outdated Show resolved Hide resolved
pkg/crc/network/nameservers.go Show resolved Hide resolved
These helper function return string slice which is useful for next
commit to use it with nmcli command.
pkg/crc/network/nameservers.go Outdated Show resolved Hide resolved
pkg/crc/network/nameservers.go Outdated Show resolved Hide resolved
err := sshRunner.CopyDataPrivileged([]byte(resolvFile), "/etc/resolv.conf", 0644)
if err != nil {
return fmt.Errorf("Error creating /etc/resolv on instance: %s", err.Error())
}
return nil
}

func updateNetworkManagerConfig(sshRunner *ssh.Runner, nameservers, searchDomains string) error {
// When ovs-configuration service is running we are sure that name of the connection is ovs-if-br-ex
_, stderr, err := sshRunner.RunPrivileged("Update resolve.conf file", "nmcli", "con", "modify", "ovs-if-br-ex",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolve.conf - typo

Is there a way of programmatically getting the name of the connection to use instead of hardcoding it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way of programmatically getting the name of the connection to use instead of hardcoding it?

I wish, I tried my search foo but didn't find anything. Let me know if you have some pointer, that would be great.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth a nmcli conn show --active ovs-if-br-ex call first to ensure it exists?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it not exist then what would be our fallback? because if it not exist then if will anyway fails when adding the domain and nameserver .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could output a good error message instead of something possibly hard to understand.

pkg/crc/network/nameservers.go Outdated Show resolved Hide resolved
pkg/crc/network/nameservers.go Outdated Show resolved Hide resolved
pkg/crc/network/nameservers.go Outdated Show resolved Hide resolved
pkg/crc/network/nameservers.go Outdated Show resolved Hide resolved
pkg/crc/network/nameservers.go Show resolved Hide resolved
From 4.15, because of ovn-kubernetes the info around nameserver and
search option in the `/etc/resolv.conf` not match with user expectation.
`ovs-configuration` service is run on this node which is a dependency
for kubelet and this make changes in the networking by creating bridge
network and restarting the NetworkManager. As soon as NM restart happen,
changes which are done as part of crc vm post start are vanished.

This PR is going to make sure that changes are done using `nmcli` and
shouldn't removed when NM restart happen. Following steps are done in
this PR
- Start the `ovs-configuration` service, even it is enabled it doesn't
  autostart because it only required by `kubelet-dependencies.target`.
- `ovs-configuration` service always create the network named as
  `ovs-if-br-ex` so we run the nmcli command to update this connection
  and add the nameserver and search option
- Restart the NM to update the /etc/resolv.conf
It is renamed to UpdateResolvFileOnInstance. The resolv.conf file
always present in the VM, this function just update it with user
provided nameserver and our custom dnsmasq instance IP.
Copy link

openshift-ci bot commented May 7, 2024

@praveenkumar: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/security e8f5701 link false /test security

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.

Copy link

openshift-ci bot commented May 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label May 7, 2024
if err != nil {
return fmt.Errorf("failed to update resolv.conf file %s: %v", stderr, err)
}
return sd.Restart("NetworkManager.service")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/etc/resolv.conf update can also be achieved with nmcli conn up ovs-if-br-ex

@praveenkumar praveenkumar merged commit 492c503 into crc-org:main May 7, 2024
23 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants