[v1.14] bgpv1: Remove disruptive error handling from BGPRouterManager #30765
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 ] [ backporter's note: v1.14 is missing various refactoring commits on upstream. Needed to adjust some variable names, field names, and so on. ] 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 ] [ backporter's comment: All reconcilers are separated into file in upstream. In v1.14, they are still in the same file. Needed to move test code. ] 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 ] [ backporter's note: Moved neighbor reconciler's logic from individual file to reconciler.go and consolidate neighborReconciler function into NeighborReconciler.Reconcile. ] 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>
[ upstream commit 192f37c ] [ backporter's note: NeighborReconciler doesn't have any metadata infrastructure because we don't have MD5 password feature, so we needed to introduce it in this commit. ] 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>
maintainer-s-little-helper
bot
added
backport/1.14
YutaroHayakawa
requested review from
a team and
rastislavs
and removed request for
a team
YutaroHayakawa
added
area/bgp
release-note/misc
|
/test-backport-1.14 |
|
Cilium Cluster Mesh upgrade, Conformance Cluster Mesh: Image pull timeout |
youngnick
approved these changes
Feb 15, 2024
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. Many conflicts were observed.