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

Improve the retry attempts, error conditions, and IPv6 attempts for fetching ec2 metadata from IMDS #314427

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hjkatz
Copy link

@hjkatz hjkatz commented May 24, 2024

Description of changes

Why: NixOS/amis#137

Things done

  • Add ipv6 awareness to the script
  • Add additional retry options to all curl commands
  • Add exit conditions for failures during boot
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels May 24, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label May 24, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels May 25, 2024
@hjkatz hjkatz changed the title Improve the retry attempts and error conditions for fetching ec2 metadata from IMDS Improve the retry attempts, error conditions, and IPv6 attempts for fetching ec2 metadata from IMDS May 28, 2024
token=""

# first test ipv4
token=$(
Copy link
Member

Choose a reason for hiding this comment

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

This will add a 10 second delay to booting machines in an IPv6 subnet. I don't think that is desirable.

I think we need to rethink this script instead of patching this in. This is getting very complex

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. While implementing this I think there should be a separate trigger or method for identifying which connectivity is preferred, then use that connectivity explicitly.

I'm curious if there's a way we could query the interfaces attached and see if any ipv6 address is available, if so, then we try that first.

But I still think it's odd that dhcp grants ipv6 before ipv4...

What ideas do you have for this script? I'm happy to help implement

Copy link
Member

Choose a reason for hiding this comment

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

But I still think it's odd that dhcp grants ipv6 before ipv4...

It's just a race condition. Sometimes you get the IPv4 lease sometimes the IPv6 lease first

Copy link
Author

Choose a reason for hiding this comment

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

The script could try ipv4/6 back and forth until it finds a working token rather than trying all at once. i.e. we move the retry logic out of curl's opts and into the bash function.

@FliegendeWurst FliegendeWurst added the awaiting_changes (old Marvin label, do not use) label Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12. first-time contribution This PR is the author's first one; please be gentle! awaiting_changes (old Marvin label, do not use)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants