release address when acquire a new static address #5658
Conversation
Signed-off-by: Mengxin Liu <[email protected]>
dosubot
bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @oilbeater, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a bug in the IP Address Management (IPAM) system where existing static IP addresses were not correctly released when a new static IP was assigned to the same network interface. The changes introduce robust logic to ensure that previous IP allocations are properly de-allocated, especially in dual-stack environments where addresses for both IPv4 and IPv6 need careful management. This improves the reliability and efficiency of IP address allocation within the system.
Highlights
- Improved Static IP Address Management: The
GetStaticAddressfunction now intelligently releases an existing IP address for a given network interface (nicName) if a new, different static IP is requested for that same interface. This prevents IP address leaks and ensures proper resource management. - Enhanced Dual-Stack IP Handling: For dual-stack configurations, the updated logic ensures that when an IP address of one protocol (e.g., IPv4) is replaced, the existing IP address of the other protocol (e.g., IPv6) for the same
nicNameis preserved and re-associated, maintaining network connectivity. - New Unit Tests for IPAM Logic: Comprehensive unit tests have been added to validate the new behavior, covering single-stack IPv4 and IPv6 scenarios, cases where the same IP is re-requested (no release), and complex dual-stack replacements.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request correctly implements the logic to release an existing static address when a new one is acquired. The accompanying tests cover the new functionality. My review includes two main points: first, a suggestion to refactor duplicated code in subnet.go to improve maintainability. Second, a recommendation to enhance the new test in subnet_test.go to also verify that the MAC address is preserved in a dual-stack scenario, making the test more robust.
| if v4 && s.V4NicToIP[nicName] != nil && !s.V4NicToIP[nicName].Equal(ip) { | ||
| // Preserve IPv6 address if it exists | ||
| var preservedV6IP IP | ||
| var preservedMac string | ||
| if s.V6NicToIP[nicName] != nil { | ||
| preservedV6IP = s.V6NicToIP[nicName] | ||
| preservedMac = s.NicToMac[nicName] | ||
| } | ||
|
|
||
| s.releaseAddr(podName, nicName) | ||
|
|
||
| // Restore IPv6 address if it was preserved | ||
| if preservedV6IP != nil { | ||
| s.V6NicToIP[nicName] = preservedV6IP | ||
| s.V6IPToPod[preservedV6IP.String()] = podName | ||
| if preservedMac != "" { | ||
| s.NicToMac[nicName] = preservedMac | ||
| s.MacToPod[preservedMac] = podName | ||
| } | ||
| } | ||
| } else if v6 && s.V6NicToIP[nicName] != nil && !s.V6NicToIP[nicName].Equal(ip) { | ||
| // Preserve IPv4 address if it exists | ||
| var preservedV4IP IP | ||
| var preservedMac string | ||
| if s.V4NicToIP[nicName] != nil { | ||
| preservedV4IP = s.V4NicToIP[nicName] | ||
| preservedMac = s.NicToMac[nicName] | ||
| } | ||
|
|
||
| s.releaseAddr(podName, nicName) | ||
|
|
||
| // Restore IPv4 address if it was preserved | ||
| if preservedV4IP != nil { | ||
| s.V4NicToIP[nicName] = preservedV4IP | ||
| s.V4IPToPod[preservedV4IP.String()] = podName | ||
| if preservedMac != "" { | ||
| s.NicToMac[nicName] = preservedMac | ||
| s.MacToPod[preservedMac] = podName | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling IPv4 and IPv6 address releases is almost identical, leading to significant code duplication. This makes the code harder to maintain. Consider refactoring this logic into a single block to improve readability and maintainability.
if (v4 && s.V4NicToIP[nicName] != nil && !s.V4NicToIP[nicName].Equal(ip)) ||
(v6 && s.V6NicToIP[nicName] != nil && !s.V6NicToIP[nicName].Equal(ip)) {
var preservedIP IP
var preservedMac string
if v4 {
// Preserve IPv6
preservedIP = s.V6NicToIP[nicName]
} else {
// Preserve IPv4
preservedIP = s.V4NicToIP[nicName]
}
if preservedIP != nil {
preservedMac = s.NicToMac[nicName]
}
s.releaseAddr(podName, nicName)
if preservedIP != nil {
if v4 {
// Restore IPv6
s.V6NicToIP[nicName] = preservedIP
s.V6IPToPod[preservedIP.String()] = podName
} else {
// Restore IPv4
s.V4NicToIP[nicName] = preservedIP
s.V4IPToPod[preservedIP.String()] = podName
}
if preservedMac != "" {
s.NicToMac[nicName] = preservedMac
s.MacToPod[preservedMac] = podName
}
}
}| ip1, _, err := subnet.GetStaticAddress(podName, nicName, firstV4IP, nil, false, true) | ||
| require.NoError(t, err) | ||
| require.Equal(t, "10.0.0.5", ip1.String()) | ||
|
|
||
| // Second allocation - IPv6 for same nicName (should coexist in dual stack) | ||
| firstV6IP, err := NewIP("2001:db8::5") | ||
| require.NoError(t, err) | ||
| ip2, _, err := subnet.GetStaticAddress(podName, nicName, firstV6IP, nil, false, true) | ||
| require.NoError(t, err) | ||
| require.Equal(t, "2001:db8::5", ip2.String()) | ||
|
|
||
| // Verify both IPv4 and IPv6 coexist in dual stack | ||
| require.Equal(t, firstV4IP, subnet.V4NicToIP[nicName], "IPv4 should coexist with IPv6") | ||
| require.Equal(t, firstV6IP, subnet.V6NicToIP[nicName]) | ||
| require.Equal(t, podName, subnet.V4IPToPod["10.0.0.5"]) | ||
| require.Equal(t, podName, subnet.V6IPToPod["2001:db8::5"]) | ||
|
|
||
| // Third allocation - Different IPv4 for same nicName (should replace IPv4 only) | ||
| secondV4IP, err := NewIP("10.0.0.10") | ||
| require.NoError(t, err) | ||
| ip3, _, err := subnet.GetStaticAddress(podName, nicName, secondV4IP, nil, false, true) | ||
| require.NoError(t, err) | ||
| require.Equal(t, "10.0.0.10", ip3.String()) |
There was a problem hiding this comment.
The test DualStack_SameProtocolReplacement correctly verifies that the IPv6 address is preserved when the IPv4 address is replaced. However, it doesn't check if the MAC address is also preserved. In a dual-stack scenario, the MAC address should remain consistent across IP changes for the same NIC. Please add assertions to verify that the MAC address is preserved.
ip1, mac1, err := subnet.GetStaticAddress(podName, nicName, firstV4IP, nil, false, true)
require.NoError(t, err)
require.Equal(t, "10.0.0.5", ip1.String())
require.NotEmpty(t, mac1)
// Second allocation - IPv6 for same nicName (should coexist in dual stack)
firstV6IP, err := NewIP("2001:db8::5")
require.NoError(t, err)
ip2, mac2, err := subnet.GetStaticAddress(podName, nicName, firstV6IP, nil, false, true)
require.NoError(t, err)
require.Equal(t, "2001:db8::5", ip2.String())
require.Equal(t, mac1, mac2)
// Verify both IPv4 and IPv6 coexist in dual stack
require.Equal(t, firstV4IP, subnet.V4NicToIP[nicName], "IPv4 should coexist with IPv6")
require.Equal(t, firstV6IP, subnet.V6NicToIP[nicName])
require.Equal(t, podName, subnet.V4IPToPod["10.0.0.5"])
require.Equal(t, podName, subnet.V6IPToPod["2001:db8::5"])
// Third allocation - Different IPv4 for same nicName (should replace IPv4 only)
secondV4IP, err := NewIP("10.0.0.10")
require.NoError(t, err)
ip3, mac3, err := subnet.GetStaticAddress(podName, nicName, secondV4IP, nil, false, true)
require.NoError(t, err)
require.Equal(t, "10.0.0.10", ip3.String())
require.Equal(t, mac1, mac3)
Pull Request Test Coverage Report for Build 17285001927Details
💛 - Coveralls |
Signed-off-by: Mengxin Liu <[email protected]> (cherry picked from commit 06563e9)
Pull Request
What type of this PR
Examples of user facing changes:
Which issue(s) this PR fixes
Fixes #5654