watchers: Fix BGP subscriber potentially getting skipped #16341
Merged
Conversation
This file contains 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
christarazi
added
sig/k8s
maintainer-s-little-helper
bot
added
dont-merge/needs-release-note-label
christarazi
force-pushed
the
pr/christarazi/fix-node-labels-bgp
branch
from
e510039
to
d3ad4dc
Compare
christarazi
added
the
release-note/bug
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
|
test-me-please |
joestringer
approved these changes
Jun 2, 2021
|
@joestringer If we had CI coverage with more advanced configuration like setting node-selectors, then we would have very likely caught this. I'll open a PR to add this configuration to the existing test, just so we have regression coverage. |
It is possible for the BGP speaker subscriber to be skipped in a K8s node event if the host endpoint is not yet created. This can happen at the very early stages of Cilium startup, as a K8s node add event is sent to the K8s watchers as one of the first events It is also often sent before any endpoints have been generated. If that happends, then the consequence is that the MetalLB integration is not seeded with the node labels, which can prevent peering with the BGP routers if the user has node-selectors defined in their BGP configuration. In other words, the MetalLB integration would try to match the selectors against empty labels, which will always fail. The short term fix is to move the BGP speaker logic slightly above where the host endpoint logic can return so that it is guaranteed to always be executed. Longer term, we have an issue cilium#15471 to refactor the subscribers so that they are executed separately, rather than bundled into one function like (*K8sWatcher).updateK8sNodeV1(). That would have prevented this bug. Fixes: d8dbb82 ("daemon, bgp, watchers: Implement LB IP announcement via BGP") Fixes: cilium#16340 Signed-off-by: Chris Tarazi <chris@isovalent.com>
christarazi
force-pushed
the
pr/christarazi/fix-node-labels-bgp
branch
from
d3ad4dc
to
b9eb68c
Compare
aanm
approved these changes
Jun 3, 2021
christarazi
added
the
ready-to-merge
|
We have approving reviews and passing CI. Marked as ready to merge. |
christarazi
added a commit
to christarazi/cilium
that referenced
this pull request
Jun 8, 2021
We recently had a regression (cilium#16340) that occurred when the user specified node-selectors in their BGP configmap. The node-selectors were not picked up due to the bug that was fixed in cilium#16341. This commit is to add regression testing for the BGP integration. Signed-off-by: Chris Tarazi <chris@isovalent.com>
aditighag
pushed a commit
that referenced
this pull request
Jun 10, 2021
We recently had a regression (#16340) that occurred when the user specified node-selectors in their BGP configmap. The node-selectors were not picked up due to the bug that was fixed in #16341. This commit is to add regression testing for the BGP integration. Signed-off-by: Chris Tarazi <chris@isovalent.com>
gandro
pushed a commit
to gandro/cilium
that referenced
this pull request
Jun 15, 2021
[ upstream commit 8ba6c28 ] We recently had a regression (cilium#16340) that occurred when the user specified node-selectors in their BGP configmap. The node-selectors were not picked up due to the bug that was fixed in cilium#16341. This commit is to add regression testing for the BGP integration. Signed-off-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
aanm
pushed a commit
that referenced
this pull request
Jun 16, 2021
[ upstream commit 8ba6c28 ] We recently had a regression (#16340) that occurred when the user specified node-selectors in their BGP configmap. The node-selectors were not picked up due to the bug that was fixed in #16341. This commit is to add regression testing for the BGP integration. Signed-off-by: Chris Tarazi <chris@isovalent.com> Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
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.
It is possible for the BGP speaker subscriber to be skipped in a K8s
node event if the host endpoint is not yet created. This can happen at
the very early stages of Cilium startup, as a K8s node add event is sent
to the K8s watchers as one of the first events It is also often sent
before any endpoints have been generated. If that happends, then the
consequence is that the MetalLB integration is not seeded with the node
labels, which can prevent peering with the BGP routers if the user has
node-selectors defined in their BGP configuration. In other words, the
MetalLB integration would try to match the selectors against empty
labels, which will always fail.
The short term fix is to move the BGP speaker logic slightly above where
the host endpoint logic can return so that it is guaranteed to always be
executed. Longer term, we have an issue
#15471 to refactor the
subscribers so that they are executed separately, rather than bundled
into one function like (*K8sWatcher).updateK8sNodeV1(). That would have
prevented this bug.
Fixes: d8dbb82 ("daemon, bgp, watchers: Implement LB IP announcement via
BGP")
Fixes: #16340
Signed-off-by: Chris Tarazi chris@isovalent.com