bgpv2: Add a knob to disable CRD status reporting #35976
Merged
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
b4a17f1 to
f732c49
Compare
Status reporting (status.conditions in the ClusterConfig and PeerConfig or status in the NodeConfig) are useful for troubleshooting. Thus, it is recommended to enable it. However, if there's an issue like high API server load, we may want to disable it. This commit introduces a Helm value to control it. The default is enabled. We'll introduce an actual agent/operator options in the subsequent commits. Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
Add a new agent/operator option enable-bgp-control-plane-status-report that enables/disables status reporting (status.conditions on the CiliumBGPClusterConfig and CiliumBGPPeerConfig, status on the CiliumBGPNodeConfig). It is enabled by default, but users can disable it to reduce the API server load. So far, we only provide a knob to enable/disable all status reporting. In the future, we may want to provide more fine-grained knob such as enabling/disabling status reporting per resource. However, as a starting point, we'll provide it with the simplest way. Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
When users disable the status reporting on the running cluster, the previously reported status won't be updated anymore. That means the stale status report persists. To avoid this, cleanup the status fields when the status reporting is disabled. Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
f732c49 to
ece2183
Compare
|
/test |
gandro
reviewed
Nov 18, 2024
There was a problem hiding this comment.
Code-wise, this looks good to me.
We used to have a similar feature for CNPs, that we eventually turned off by default and then removed completely because it suffered from the same scalability issues.
I do wonder if we should also disable this feature by default.
gandro
approved these changes
Nov 18, 2024
There was a problem hiding this comment.
Discussed offline. Turns out the scalability issues are less pronounced with the BGP status compared to the old CNP status I mentioned. So keeping this enabled by default seems like a reasonable tradeoff.
In the CNP case, we used to edit global CNP from multiple nodes while they are watched by all nodes. On the other hand, the BGP resources only have a single writer.
tommyp1ckles
approved these changes
Nov 20, 2024
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.
We recently introduced several CRD status reporting features (#33411, #35517, #35527, #35650). They are useful for troubleshooting, but there's a trade-off between API server load. For the large cluster with many nodes or many BGP resources may experience a high API server load. In this PR, we introduce a knob to disable BGPv2 CRD status reporting for such an environment.