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

[fix] make VLAN IPs unique across controller pod restarts #544

Merged
merged 4 commits into from
Oct 21, 2024

Conversation

tchinmai7
Copy link
Contributor

What this PR does / why we need it:
With the VLAN clusters, a in-memory map was maintained for each cluster's IPs. When the controller pod restarts, that IP map is lost, and any further cluster scaling activities cause the nodes to get overlapping IPs.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
Testing steps

  1. run make local-deploy off main
  2. Create a cluster with the rke2-vlan flavor
  3. Delete the capl-controller pod
  4. Increase the number of replicas in the md. describe the linodemachines and see that more than one linode has the same vlanIP
  5. run make local-deploy off this branch
  6. Delete one of the problem linodemachines
  7. Repeat the steps and verify that all machines now have a unique IP

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 39.21569% with 31 lines in your changes missing coverage. Please review.

Project coverage is 63.63%. Comparing base (2c91eb4) to head (1d81702).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
util/vlanips.go 50.00% 14 Missing and 6 partials ⚠️
controller/linodemachine_controller_helpers.go 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
- Coverage   64.82%   63.63%   -1.20%     
==========================================
  Files          79       79              
  Lines        4299     5486    +1187     
==========================================
+ Hits         2787     3491     +704     
- Misses       1242     1719     +477     
- Partials      270      276       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

util/vlanips.go Outdated
Comment on lines 68 to 72
if addr.Type == clusterv1.MachineInternalIP {
if ipnet.Contains(net.ParseIP(addr.Address)) {
existingIPs = append(existingIPs, addr.Address)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
if addr.Type == clusterv1.MachineInternalIP {
if ipnet.Contains(net.ParseIP(addr.Address)) {
existingIPs = append(existingIPs, addr.Address)
}
}
if addr.Type == clusterv1.MachineInternalIP && ipnet.Contains(net.ParseIP(addr.Address)) {
existingIPs = append(existingIPs, addr.Address)
}

AshleyDumaine
AshleyDumaine previously approved these changes Oct 21, 2024
Copy link
Member

@AshleyDumaine AshleyDumaine left a comment

Choose a reason for hiding this comment

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

small nit, otherwise LGTM

AshleyDumaine
AshleyDumaine previously approved these changes Oct 21, 2024
@tchinmai7 tchinmai7 merged commit a62d3c8 into main Oct 21, 2024
11 of 13 checks passed
@tchinmai7 tchinmai7 deleted the fix-vlan-ip branch October 21, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants