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
node/manager: Utilize set.SliceSubsetOf in ipcache deletion #25180
node/manager: Utilize set.SliceSubsetOf in ipcache deletion #25180
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
07da8ce
to
f0426a6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
d56bd82
to
47fcc78
Compare
/test |
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.
LGTM overall, I've just left a suggestion to improve SliceSubsetOf
.
47fcc78
to
5e126fd
Compare
This will be used in upcoming commits when the main set can be nil. Signed-off-by: Chris Tarazi <chris@isovalent.com>
This cleans up the code to have proper diffing logic when delete node IPs from the ipcache. Previously, the logic had a potential for a bug because it assumed that oldIPs and newIPs were sorted. FWIW, no bug has been reported in regards to the previous code fixed by this commit, so it's theoretical for now. In the end, this is a defensive change to prevent it from happening. Additionally, this commit will ease the changes that will be introduced in cilium#23208. Signed-off-by: Chris Tarazi <chris@isovalent.com>
5e126fd
to
c56683b
Compare
/test |
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.
LGTM 🚀
Marking ready to merge as we have approving reviews and the Travis failure is due to images missing, but it's not required as the integrations test job (test the same thing AFAIK) succeeded. |
Copying the more relevant commit for ease of review: