Skip to content

Conversation

tgraf
Copy link
Member

@tgraf tgraf commented Apr 20, 2020

This restores the exact node IP behavior unless remote-node identities are enabled. It also fixes two bugs in separate commits that have been discovered by the unit tests added as part of this work.

@tgraf tgraf added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Apr 20, 2020
@tgraf tgraf requested a review from a team April 20, 2020 11:02
@tgraf tgraf requested review from a team as code owners April 20, 2020 11:02
@tgraf tgraf force-pushed the pr/tgraf/legacy-node-ip-mode branch 2 times, most recently from 6933a66 to 1167f64 Compare April 20, 2020 15:11
@tgraf tgraf requested a review from a team as a code owner April 20, 2020 15:11
@coveralls
Copy link

coveralls commented Apr 20, 2020

Coverage Status

Coverage increased (+0.01%) to 44.685% when pulling bc33662 on pr/tgraf/legacy-node-ip-mode into 84abbf8 on master.

@tgraf tgraf force-pushed the pr/tgraf/legacy-node-ip-mode branch from 1167f64 to 94183a4 Compare April 20, 2020 23:38
@tgraf
Copy link
Member Author

tgraf commented Apr 20, 2020

test-me-please

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I pondered for a bit whether users who unintentionally picked up this behaviour in v1.7.2 (and populated local ipcaches) would require any additional manual intervention to ensure that the entries are cleaned up during upgrade. My conclusion was that this is not necessary since we will recreate the ipcache map during startup, and the new logic will kick in at that point. 👍

Minor nit below.

@tgraf tgraf requested a review from aanm April 21, 2020 08:53
@tgraf tgraf force-pushed the pr/tgraf/legacy-node-ip-mode branch from 94183a4 to 5a0be55 Compare April 21, 2020 08:54
@tgraf
Copy link
Member Author

tgraf commented Apr 21, 2020

test-me-please

tgraf added 3 commits April 21, 2020 18:27
Commit bbfabcf added all IPs of a node to the ipcache to unify the
behavior of the identity representing a node. While the fix is correct,
it results in unexpected behavior change for some users. Revert the
behavior and only do so if remote-node identities are enabled which is
the new opt-in behavior starting with Cilium 1.7.

Fixes: bbfabcf ("node: Assign remote node identity to all node IPs")

Signed-off-by: Thomas Graf <thomas@cilium.io>
…added

Even though some IPs have been excluded from being added to the ipcache.
The same IPs have not been excluded when deleting thus resulting in an
attempt to remove IPs in the ipcache which clearly were not owned by the
node resource.

Signed-off-by: Thomas Graf <thomas@cilium.io>
The IPs were added but never deleted

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf force-pushed the pr/tgraf/legacy-node-ip-mode branch from 5a0be55 to bc33662 Compare April 21, 2020 16:30
@tgraf
Copy link
Member Author

tgraf commented Apr 21, 2020

test-me-please

@tgraf tgraf merged commit 5370abe into master Apr 21, 2020
@tgraf tgraf deleted the pr/tgraf/legacy-node-ip-mode branch April 21, 2020 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants