Skip to content

vultr: remove check_route check#2151

Merged
TheRealFalcon merged 2 commits into
canonical:mainfrom
zimbatm:remove-vultr-check-route
May 25, 2023
Merged

vultr: remove check_route check#2151
TheRealFalcon merged 2 commits into
canonical:mainfrom
zimbatm:remove-vultr-check-route

Conversation

@zimbatm
Copy link
Copy Markdown
Contributor

@zimbatm zimbatm commented Apr 30, 2023

Proposed Commit Message

vultr: remove check_route check

The heuristic is assuming that the URL will contain an IP, and that the
route explicitly lists that IP (eg: 0.0.0.0/0 should match but doesn't).
In order for the heuristic to be 100% reliable, it would have to
replicate exactly what the system is doing both in terms of DNS and
route resolution.

Because the HTTP request below is already exercising the python and
system resolution, it is simpler to just remove this check and lean on
the HTTP request to provide the answer if the network is up or not.

Additional Context

Test Steps

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@zimbatm zimbatm force-pushed the remove-vultr-check-route branch from 987b7f8 to 6df2a19 Compare April 30, 2023 10:23
@zimbatm zimbatm changed the title remove vultr check route vultr: remove check_route check Apr 30, 2023
@zimbatm
Copy link
Copy Markdown
Contributor Author

zimbatm commented Apr 30, 2023

I might have encountered this issue because the routes set by #2125 are different than dhclient.

Copy link
Copy Markdown
Collaborator

@igalic igalic left a comment

Choose a reason for hiding this comment

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

👍

@zimbatm
Copy link
Copy Markdown
Contributor Author

zimbatm commented May 6, 2023

Is there anything I can do to help this PR move forward?

@TheRealFalcon
Copy link
Copy Markdown
Contributor

Thanks for the PR @zimbatm .
I noticed you signed the CLA, but haven't yet added yourself to the CLA signers file. As part of this PR, please also add your name (alphabetically) to the CLA signers file. The full details are described in the last bullet point of the documentation.

@eb3095 Would you mind reviewing this?

@github-actions
Copy link
Copy Markdown

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

@github-actions github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label May 24, 2023
@TheRealFalcon TheRealFalcon removed the stale-pr Pull request is stale; will be auto-closed soon label May 24, 2023
@TheRealFalcon TheRealFalcon self-assigned this May 24, 2023
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

I think this one has waited long enough. The changes here look good. Worst case scenario looks like we'll get a couple extra DHCP error log messages.

There's still a linting error to fix and the cla signers file will need to be rebased, but then we should be good to go on this one!

zimbatm added 2 commits May 25, 2023 15:09
The heuristic is assuming that the URL will contain an IP, and that the
route explicitly lists that IP (eg: 0.0.0.0/0 should match but doesn't).
In order for the heuristic to be 100% reliable, it would have to
replicate exactly what the system is doing both in terms of DNS and
route resolution.

Because the HTTP request below is already exercising the python nd
system resolution, it is simpler to just remove this check and lean on
the HTTP request to provide the answer if the network is up or not.
@zimbatm zimbatm force-pushed the remove-vultr-check-route branch from a2f4447 to 3c62b73 Compare May 25, 2023 13:09
@zimbatm
Copy link
Copy Markdown
Contributor Author

zimbatm commented May 25, 2023

thanks, fixed!

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@TheRealFalcon TheRealFalcon merged commit 4f0f439 into canonical:main May 25, 2023
wegank pushed a commit to NixOS/nixpkgs that referenced this pull request Jun 19, 2023
Keep support for udhcpc (waiting for upstream PR:
canonical/cloud-init#4190).

Vultr patch has been merged upstream
(canonical/cloud-init#2151).
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.

3 participants