[v1.12] Author backport of WG Node handler and dependent PR #25928
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ upstream commit d8a0be6 ] [backporter notes: 1. some conflicts due to moved files 2. had to mildly adapt the fakeDatapath to keep a ptr to fakeNodeHandler instead of the interface] Return pointer to implementing struct instead of implemented interface from the constructor, as is commonly considered idiomatic Go. The constructor is already in a linux-specific package. To ensure that we really do implement the interface we want, add a static type check in form of a variable assignment. As a side-effect, this allows us to drop a number of type assertions in tests. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
[ upstream commit 8294624 ] [backporter notes: conflicts due to moved files/import paths] The NodeHandler interface is too large, as can be seen in various implementations which are only implementing a subset of methods. This patch splits off the NodeID handling part, and removes the stub methods from noop implementations. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
[ upstream commit 1aa06c6 ] [ backporter notes: conflicts due to moved files ] The NodeHandler interface still contains two different concepts. Remove the NodeNeighbor discovery/handling from it, and delete the different stub implementations. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
[ upstream commit c8598f8 ] [ backporter notes: 1. different way of subscribing to NodeManager 2. conflicts due to moved files/import paths ] The reason the WireGuard agent node event handling was contained within the linuxNodeHandler code was routing, which is no longer the case. In addition, entangling the two leads to a deadlock, as diagnosed in GitHub issue cilium#24574. This patch thus implements NodeHandler for the WireGuard agent, and subscribes to the NodeManager itself. That way, the wait cycle of the deadlock is broken, as the linuxNodeHandler doesn't acquire the IPCache lock while holding its lock. From the perspective of the agent, the invocations of the callbacks change insofar that previously, only once the linuxNodeHandler considered itself "initialised" it would forward node events. Specifically, this excluded the initial sync of nodes performed on subscribe. However, I didn't see a reason to specifically replicate this behaviour. Suggested-by: Sebastian Wicki <sebastian@isovalent.com> Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
|
/test-backport-1.12 Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed: Click to show.Test NameFailure OutputJenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.9/16/ If it is a flake and a GitHub issue doesn't already exist to track it, comment Then please upload the Jenkins artifacts to that issue. |
gandro
approved these changes
Jun 6, 2023
michi-covalent
approved these changes
Jun 9, 2023
|
test-1.16-4.9 looks like it hit a variant of #24839 |
|
/test-1.16-4.9 |
|
Marked as a release blocker, since this is a regression fix (see #25419 (comment)). |
This was referenced Jun 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Once this PR is merged, you can update the PR labels via:
or with