-
Notifications
You must be signed in to change notification settings - Fork 217
Add IPv6 support in NetworkInterface feature #3957
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
base: main
Are you sure you want to change the base?
Conversation
69dc31e
to
9ba90cd
Compare
lisa/features/ipv6.py
Outdated
from lisa.feature import Feature | ||
|
||
|
||
class IPv6(Feature): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add it to NetworkInterface, instead of a new feature. After that, we can deprecate the use_ipv6
on Azure runbook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please review the updated implementation? I am still testing it, just want to make sure the approach is correct.
I couldn't figure out how to add Ipv4/ipv6 in initialize environment.
I also have not yet added Ipv4 as default support for existing platforms other than Azure
unsupported_os=[BSD, Windows], | ||
), | ||
) | ||
def perf_tcp_ipv6_ntttcp_sriov(self, result: TestResult) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason to add perf test cases for IPv6? The IP version shouldn't have a significant performance impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LISA currently do not have coverage for any IPv6 scenario. Hence, I thought performance is a good place to start with.
I also expect that the perf numbers can be potentially different for ipv4 and ipv6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a performance concern with IPv6. Instead, you can add a functional test case to verify whether the network can communicate using IPv6.
9ba90cd
to
0ff61e7
Compare
b00fb8f
to
e35839a
Compare
e35839a
to
dd659fb
Compare
2c0a409
to
45b495d
Compare
@@ -709,6 +709,11 @@ class NetworkDataPath(str, Enum): | |||
Sriov = "Sriov" | |||
|
|||
|
|||
class IPVersion(str, Enum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it reuse IpProtocol.ipv6
in other places, instead of creating a new enum?
@@ -2274,13 +2274,27 @@ def get_primary_ip_addresses( | |||
nic_name = get_matched_str(network_interface.id, NIC_NAME_PATTERN) | |||
nic = network_client.network_interfaces.get(resource_group_name, nic_name) | |||
if nic.primary: | |||
# Check if IPv6 is actually enabled on the NIC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which setting determines whether IpProtocol.ipv6 is enabled?
@@ -2274,13 +2274,27 @@ def get_primary_ip_addresses( | |||
nic_name = get_matched_str(network_interface.id, NIC_NAME_PATTERN) | |||
nic = network_client.network_interfaces.get(resource_group_name, nic_name) | |||
if nic.primary: | |||
# Check if IPv6 is actually enabled on the NIC | |||
# if its enabled, use it as internal ip. | |||
ipv6_enabled = any( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between this value and use_ipv6
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the problem I was having.
use_ipv6 is currently passed by runbook. This flow makes both internal address and public address as ipv6.
My requirement changes to
- Ipv4 as public
- Ipv6 as internal
- IPv6 requirement is a feature rather than runbook parameter.
Can you suggest a better approach for the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I found four instances of
use_ipv6
inRemoteNode
,AzurePlatformSchema
,AzureArmParameter
, andNodeContext
, but onlyAzurePlatformSchema
andAzureArmParameter
appear to be used. Please review the code history to understand the purpose of the other two. - To support the scenario you mentioned, please update the code to define
use_ipv6_public
anduse_ipv6_internal
, both defaulting toFalse
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, let me rework the PR
# All Azure VMs support synthetic and SRIOV networking | ||
# All Azure VMs can support both IPv4 and IPv6 via ARM template configuration | ||
return schema.NetworkInterfaceOptionSettings( | ||
data_path=search_space.SetSpace( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is this method used? There wasn’t such a method, but the SR-IOV and synthetic settings worked correctly.
@@ -1199,6 +1199,12 @@ def _create_deployment_parameters( | |||
) | |||
arm_parameters.use_ipv6 = self._azure_runbook.use_ipv6 | |||
|
|||
# Override IPv6 setting if environment specifically requires IPv6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overrides of requirements should be defined in the runbook, and the runbook requirements are merged with the test requirements in lisa_runner
. The Azure Platform shouldn't modify it, as that would disrupt the flow.
@@ -346,6 +346,17 @@ def get_ip_address(self, nic_name: str) -> str: | |||
assert "ip_addr" in matched, f"not find ip address for nic {nic_name}" | |||
return matched["ip_addr"] | |||
|
|||
def get_ipv6_address(self, nic_name: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a parameter to the get_ip_address
method instead of creating a new method.
ip=lagscope_server_ip | ||
if lagscope_server_ip is not None | ||
else server.internal_address | ||
ip=( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would lagscope_server_ip
be empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good question.
The definition of perf_ntttcp has parameter " lagscope_server_ip: Optional[str] = None,"
I'll get back to you
Add IPv6 feature by utilizing the existing use_ipv6 runbook parameter.
IPv6 Features enables IPv6 support in VNet, Subnet and enable IPv6 on the nic interface.
The Primary IP of the VM continues to be IPv4, hence no change is required in host that is running LISA. The communication between the VMs uses IPv6.