[v1.15] bgpv1: Remove disruptive error handling from BGPRouterManager #30735
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
[ upstream commit 1676530 ] Currently, when the BGP Control Plane meet the error during reconciliation, it stops and unregisters the server. This comes at a risk when we accidentally introduce a situation where reconcilers repeatedly return error for a long time. In such a situation, BGP Control Plane repeatedly create/destroy server which may cause a BGP session flapping or frequent route update/withdraw. We have this problem in two places. The first one is in BGPRouterManager.register(). When the initial reconciliation fails, BGP Control Plane stop the server it just creates. In this case, we don't need to stop server. We can register the server to the RouterManager before reconciliation and don't have to unregister it even if the initial reconciliation fails. This is because we have a retry logic now. The reconciliation failure triggers the retry and in the next cycle, the reconciliation will be handled in the BGPRouterManager.reconcile() because the server is already registered. The second problem is in BGPRouterManager.reconcile(). When the reconciliation fails, BGP Control Plane stops the BGP server in failure and unregisters it. Since we have a retry logic, we don't need to stop BGP servers. We can return error and retry later until we recover from the failure state. Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
[ upstream commit 7275198 ] Extend existing reconciler test cases to always execute Reconcile() twice. This simulates the behavior that BGPRouterManager doesn't clear the BGP server on reconciliation failure and the next event occurs and ensures the reconcilers are not relying on that behavior. As a result, existing reconcilers except NeighborReconciler passed the test. The fix for the neighbor reconciler will be followed. Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
[ upstream commit 7684f35 ] There are a lot of repetition of the same fmt.Sprintf in NeighborReconciler that constructs the same neighbor ID. Introduce a helper method to construct neighbor ID. Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
YutaroHayakawa
added
release-note/misc
maintainer-s-little-helper
bot
added
backport/1.15
[ upstream commit 192f37c ] Currenly, NeighborReconciler only reconciles on old vs new configuration differences and doesn't take the actual GoBGP's running state into account. As a result, when the reconciliation of another reconciler fails and CurrentServer.Config is not updated, it tries to repeat the previous reconciliation again. However, when it involves adding the neighbor, it fails, because adding neighbor is not idempotent in GoBGP. To solve this issue, we track the neighbor state in metadata and compare the diff between new config and the state instead of old config. This makes NeighborReconciler idempotent. Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
YutaroHayakawa
force-pushed
the
v1.15-30382
branch
from
0d944a2
to
cf184c5
Compare
YutaroHayakawa
requested review from
a team and
rastislavs
and removed request for
a team
|
/test-backport-1.15 |
rastislavs
approved these changes
Feb 14, 2024
youngnick
approved these changes
Feb 14, 2024
|
Cilium IPsec upgrade: #29987 |
maintainer-s-little-helper
bot
added
the
ready-to-merge
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.
Backport of #30382. No conflict was observed.