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

Configure multicard interfaces #1952

Closed
wants to merge 1 commit into from

Conversation

nkvetsinski
Copy link
Contributor

Issue #, if available:

Description of changes:
In the AMI we force systemd-networkd to only manage the primary interface. In this commit I'm adding a script that will configure ip addresses and routes for interfaces that are not managed by systemd-networkd.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Testing Done

I verified that:

  • Pod networking is working as expected (pods on local and remote machines can communicate)
  • Non zero card interfaces on remote machines can communicate. I tested this with iperf, starting a server on one pod and binding it to non zero card interface. On another pod (running on different machine), using iperf client (bound to non zero card interface). Running tcpdump on both machines and verifying traffic is flowing thru the correct interfaces.

See this guide for recommended testing for PRs. Some tests may not apply. Completing tests and providing additional validation steps are not required, but it is recommended and may reduce review time and time to merge.

@nkvetsinski nkvetsinski marked this pull request as ready for review September 4, 2024 20:50
Comment on lines +3 to +14
# EKS already forces amazon-ec2-net-utils to manage the primary ENI only (0/0):
# https://github.com/awslabs/amazon-eks-ami/pull/1738
#
# When VPC CNI is installed it creates another ENI from network card with index 0 (0/1).
# VPC CNI will continue adding additional ENIs until max number of interfaces for the intance
# is reached (0/2, 0/3, etc).
#
# This script configures IP and routing for Elastic Network Interfaces that are part of
# non-zero indexed EC2 network cards (1/0, 1/1, 2/1, 3/1, etc). The way we find out whether
# we need to configure the interface is by checking IMDS:
# /latest/meta-data/network/interfaces/macs/${mac_address}/network-card/
# This script will skip any interfaces that are part of the 0 indexed card.
Copy link
Member

@cartermckinnon cartermckinnon Sep 4, 2024

Choose a reason for hiding this comment

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

A+++++ comment, thx.

I'm wondering if we can leverage systemd-networkd here instead of setting all the routes up ourselves. We modify the default networkd config so that it only matches the primary ENI (by using PermanentMACAddress). Can we add more MAC's there for the other interfaces on other cards? Or create additional networkd configs for those interfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we add the mac addresses to the current drop-in file, the default routes for them will end up in the main route table, which I think it's not a good idea.

If we want to leverage systemd-networkd we'll have to create separate .network file for the non zero cards populate the mac addresses and configure the policy routing. I was thinking about that too, but figured it is probably easier/clear to setup everything with iproute package commands. I'm fine with changing direction, also asked somebody more familiar with networking to take a look, let's see what their take will be too :)

Copy link
Member

@cartermckinnon cartermckinnon Sep 5, 2024

Choose a reason for hiding this comment

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

it is probably easier/clear to setup everything with iproute package commands

I would much rather defer to networkd 😂 we have to phone a friend every time we touch this script

Copy link
Member

Choose a reason for hiding this comment

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

+1 to seeing what we can do to avoid adding this into our AMIs for AL2023. I think we need to figure out how amazon-ec2-net-utils can support management of just the primary ENI and let others (like VPC CNI) handle all secondary ENI management.

We're also going through this much effort to bring up network interfaces that aren't on network card 0, but those interfaces aren't actually used by K8s pods in any case since we only assign IPs from network card 0 right? This is something we've got to chase with the VPC CNI to make it network card aware, but until then I wonder how much value this PR brings us other than consistency with our AL2 AMI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 to seeing what we can do to avoid adding this into our AMIs for AL2023. I think we need to figure out how amazon-ec2-net-utils can support management of just the primary ENI and let others (like VPC CNI) handle all secondary ENI manageme

We already "tell" ec2-net-utils to manage the primary ENI, by using drop-in. When checking with VPC CNI people I was told that there is backlog item about handling non primary interfaces, but I didn't get anything more than that. I can ask further.

We're also going through this much effort to bring up network interfaces that aren't on network card 0, but those interfaces aren't actually used by K8s pods in any case since we only assign IPs from network card 0 right?

Potential use case is if someone runs the pod in the host network namespace and specifically binds their app to one of non zero card interfaces.

I wonder how much value this PR brings us other than consistency with our AL2 AMI.

My main goal was parity with AL2 AMI.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to seeing what we can do to avoid adding this into our AMIs for AL2023. I think we need to figure out how amazon-ec2-net-utils can support management of just the primary ENI and let others (like VPC CNI) handle all secondary ENI management

Making VPC CNI aware of network cards other than card 0 is going to be a much bigger change and currently not prioritized by the networking team. We went ahead with same approach as AL2 AMI to not block the AL2023 AMI on this.
cc: @jayanthvn on setting up routes via iproute vs using networkd by creating separate .network files

Copy link
Contributor

Choose a reason for hiding this comment

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

The CNI plugin will not manage the ENI setup and routes for the primary ENIs on non-zero network cards, even if the CNI plugin manages the secondary ENIs on the non-zero network cards.

else
echo "handling interface $if_name"

if_ip_addr=$(imds "/latest/meta-data/network/interfaces/macs/$trimmed_mac/local-ipv4s" | head -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any plans to support IPv6?

@M00nF1sh
Copy link
Member

M00nF1sh commented Sep 10, 2024

  1. I prefer to use drop-ins to configure those non-zero network cards whenever possible. As I think it's better to align with EC2's original behavior of AL2023 for those ENIs not managed by vpc-cni as much as possible instead of align with original EKS AL2 behavior.
  2. do we need a support flag in node-adm to conditionally enable/disable this behavior? as it's a drift from current EKS AL2023 behavior

@nkvetsinski nkvetsinski reopened this Sep 10, 2024
@nkvetsinski
Copy link
Contributor Author

  1. I prefer to use drop-ins to configure those non-zero network cards whenever possible. As I think it's better to align with EC2's original behavior of AL2023 for those ENIs not managed by vpc-cni as much as possible instead of align with original EKS AL2 behavior.

    1. do we need a support flag in node-adm to conditionally enable/disable this behavior? as it's a drift from current EKS AL2023 behavior

We synced on the side, Yang is in favor of creating separate .network files for the interfaces. I want to get an alignment with @cartermckinnon whether we need this in nodeadm. My take is that it will be easier to do it as shell script and since it's not a super critical part of node bootstrap I'd rather not add it nodeadm workflow.

@cartermckinnon
Copy link
Member

cartermckinnon commented Sep 11, 2024

Thanks @M00nF1sh, agree.

@nkvetsinski We already modify the existing networkd config in nodeadm, so I think this should go there as well (plus it's more easily unit tested vs a shell script)

@nkvetsinski
Copy link
Contributor Author

I'm going to open a new PR for this guys, since we're changing the approach.

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.

6 participants