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

Unify NetworkManager Connection Names #926

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

M4t7e
Copy link
Contributor

@M4t7e M4t7e commented Aug 5, 2023

This PR is mainly a cosmetic change to unify NetworkManager connection names to make life easier for humans interacting with nmcli.

Before the change:

# nmcli connection show
NAME                UUID                                  TYPE      DEVICE 
cloud-init eth0     1dd9a779-d327-56e1-8454-c65e2556c12c  ethernet  eth0   
Wired connection 1  fdcd1a0a-bb69-3017-a940-96773f29fb8d  ethernet  eth1   
lo                  9e961e02-e328-4a5f-98ed-227fbf4b189d  loopback  lo 

After the change:

# nmcli connection show
NAME  UUID                                  TYPE      DEVICE 
eth0  1dd9a779-d327-56e1-8454-c65e2556c12c  ethernet  eth0   
eth1  2a211c06-56de-35bb-967d-5b0c8378449e  ethernet  eth1   
lo    086d46a4-41f2-4089-81ce-0517216dbe80  loopback  lo  

Copy link
Collaborator

@mysticaltech mysticaltech left a comment

Choose a reason for hiding this comment

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

@M4t7e LGTM, tested and running well huh?

@M4t7e
Copy link
Contributor Author

M4t7e commented Aug 7, 2023

@mysticaltech Yes, I test everything before creating a PR. But tbh, I can only test to some extend because we have a lot of different settings, so I can't test manually all combinations. Agent, Server and Egress Nodes are working fine with this change and I tested it with a few cluster deployments.

The only place where we use nmcli is here:

<<-EOT
NM_CONNECTION=$(nmcli -g GENERAL.CONNECTION device show eth0)
nmcli connection modify "$NM_CONNECTION" \
ipv4.method manual \
ipv4.addresses ${hcloud_floating_ip.agents[each.key].ip_address}/32,${module.agents[each.key].ipv4_address}/32 gw4 172.31.1.1 \
ipv4.route-metric 100 \
&& nmcli connection up "$NM_CONNECTION"
EOT

Here the connection name is obtained by the device name eth0. So no hard-coded connection name is used.

Other places where NetworkManager is touched are here:

  • # Apply new DNS config
    %{if length(var.dns_servers) > 0}
    # Set prepare for manual dns config
    - content: |
    [main]
    dns=none
    path: /etc/NetworkManager/conf.d/dns.conf
    - content: |
    %{for server in var.dns_servers~}
    nameserver ${server}
    %{endfor}
    path: /etc/resolv.conf
    permissions: '0644'
    %{endif}
  • clean_up = <<-EOT
    set -ex
    echo "Second reboot successful, cleaning-up..."
    rm -rf /etc/ssh/ssh_host_*
    echo "Make sure to use NetworkManager"
    touch /etc/NetworkManager/NetworkManager.conf
    sleep 1 && udevadm settle
    EOT

My assumption currently is, that cloud-init will never touch the config again after the very first init. If it would so, only the eth0 connection name is reverted back to cloud-init eth0. eth1 seems to be not managed by cloud-init at all. I tested that as well.

I'm in no hurry with this PR. So if anyone wants to take a look, I'd appreciate that 🙂

PS: A re-run of cloud-init would revert not only the eth0 connection name in NetworkManager back to the old one, but would also remove the config from agents.tf. The floating IP configuration would completely disappear! Maybe in the future we can find a better way to integrate nicely with cloud-init in general rather than overriding what it did on the first pass. Then we could run it again without worrying.

@mysticaltech
Copy link
Collaborator

Thanks for the explanation @M4t7e! Yes, in the future if we can make the whole flow that we currently use more robust to cloud-init it would be great. But normally it shouldn't run again. But if it does, we can run these scripts automatically after each reboot.

Btw, I also do not test every combination, just one test is good enough 🙏

@mysticaltech
Copy link
Collaborator

I will merge it ASAP today or tomorrow.

@mysticaltech mysticaltech changed the base branch from master to staging August 7, 2023 22:59
@mysticaltech mysticaltech merged commit 915a480 into kube-hetzner:staging Aug 7, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants