-
Notifications
You must be signed in to change notification settings - Fork 880
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
add oracle ipv6 single stack imds functionality #5785
base: main
Are you sure you want to change the base?
Conversation
3d1941e
to
f207b2a
Compare
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.
This isn't a full review, but I'm commenting on what I think are the big issues to be addressed.
I think you may be trying to make the EphemeralIPNetwork
class do more than it needs to. The original purpose of the connectivity url was to see if we could connect to the IMDS BEFORE setting up our ephemeral connection. If we have connectivity due to something outside of cloud-init (i.e., klibc in initramfs on OCI), then cloud-init doesn't need to do the ephemeral network setup. However, you've modified it to attempt to setup the v6 address, and then check for connectivity afterwards. I don't think there's value added by adding the connectivity check after. If we've setup a connection (either ipv4, ipv6, or both), we can assume we have the connectivity we need.
Additionally, the context manager in EphemeralIPNetwork
is now attempting to signal to the caller (via ipv6_reached_at_url
) if it should be using ipv6 or not. This isn't really the job of EphemeralIPNetwork though, and it's currently preventing v4 from being setup if v6 works, and preventing v6 from being setup if no v6 callback was provided. This is a fairly large change in behavior that would break EC2 as implemented.
I think that the only changes that were needed to ephemeral.py
are around allowing adding support for multiple connectivity urls and updating EphemeralIPNetwork
to do the connectivity check early in the context manager (see my inline comment for more details).
IPV4_METADATA_ROOT, | ||
IPV6_METADATA_ROOT, | ||
] | ||
METADATA_ROOTS = [ |
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.
Unused?
@@ -189,18 +225,30 @@ def _get_data(self): | |||
) | |||
|
|||
with network_context: | |||
fetched_metadata = read_opc_metadata( | |||
if network_context.ipv6_reached_at_url: |
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.
This isn't gonna work when network_context
is util.nullcontext()
cloudinit/net/ephemeral.py
Outdated
@@ -404,13 +404,37 @@ def __init__( | |||
interface, | |||
ipv6: bool = False, | |||
ipv4: bool = True, | |||
connectivity_urls_data: Optional[List[Dict[str, Any]]] = None, | |||
ipv6_connectivity_check_callback: Optional[Callable] = 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 reason we need a separate parameter here? Why can't the v6 address be folded into the connectivity_urls_data
?
|
||
# if we have connectivity to imds, then skip ephemeral network setup | ||
if self.perform_dhcp_setup: # and not available_urls: | ||
# TODO: ask james: this obviously fails on ipv6 single stack only |
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 this comment still relevant?
start_time = time.monotonic() | ||
instance_url, instance_response = wait_for_url( | ||
urls, | ||
url_that_worked, instance_response = wait_for_url( |
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.
Rather than passing metadata_patterns
that is ipv4 or ipv6 specific into this function, I think instead we should set urls
here to be all valid metadata urls. We can then add connect_synchronously=False
to this wait_for_url
call that implements the happy eyeballs algorithm we talked about before. This function will then return the working url that we can use for all future calls.
16d2927
to
042cca9
Compare
042cca9
to
12c26f4
Compare
12c26f4
to
697b4bd
Compare
Proposed Commit Message
N/A
We rebasing 😎
Additional Context
This is to enable future functionality on the Oracle Cloud and will have no immediate ramifications or generally available use cases for the meantime.
Test Steps
These changes were manually tested on both jammy (22.04) and noble (24.04), on Oracle Platform Ubuntu images for testing ipv4 and dual stack, and then on custom built images for ipv6 single stack testing. For this manual validation, the following things were checked:
The new oracle single-stack ipv6 integration tests was run against an existing instance with the following command:
Merge type