Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "antctl get bgppeers" agent command #6689

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Atish-iaf
Copy link
Contributor

Add antctl get bgppeers agent command to print current status of all BGP peers
of effective BGP policy applied on the local Node.

For #6209

@Atish-iaf Atish-iaf marked this pull request as ready for review September 24, 2024 09:46
@Atish-iaf Atish-iaf added area/component/antctl Issues or PRs releated to the command line interface component area/transit/bgp Issues or PRs related to BGP support. labels Sep 24, 2024
Signed-off-by: Kumar Atish <kumar.atish@broadcom.com>
var bgpPeersResp []apis.BGPPeerResponse
for _, peer := range peers {
bgpPeersResp = append(bgpPeersResp, apis.BGPPeerResponse{
Peer: peer.Address,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge the address and the port here? E.g., 192.168.77.201:179


peers, err := bq.GetBGPPeerStatus()
if err != nil {
if err.Error() == "failed to get bgp peers: there is no effective bgp policy applied to the Node" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a brittle way to do this and not idiomatic. You need to define a custom error and check it by name here. There are a few examples of this in our code base, such as ErrExternalIPPoolNotFound in the ExternalIPPool controller.

if c.bgpPolicyState == nil {
return nil, fmt.Errorf("failed to get bgp peers: there is no effective bgp policy applied to the Node")
}
peers, err := c.bgpPolicyState.bgpServer.GetPeers(context.TODO())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GetBGPPeerStatus function should take a context as parameter, which you can can forward here. There is no reason to use context.TODO() here.

if c.bgpPolicyState == nil {
return nil, fmt.Errorf("failed to get bgp peers: there is no effective bgp policy applied to the Node")
}
peers, err := c.bgpPolicyState.bgpServer.GetPeers(context.TODO())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

holding a read lock while calling this function may be bit overkill. In the case of gobgp at least, all these operations are asynchronous (so there can be some delay, hence the context parameter) and all the server functions can be called concurrently as far as I know.

so the following would probably be correct:

getBgpServer := func() bgp.Interface {
        c.bgpPolicyStateMutex.RLock()
	defer c.bgpPolicyStateMutex.RUnlock()
        if c.bgpPolicyState == nil {
		return nil
	}
	return c.bgpPolicyState.bgpServer
}

bgpServer := getBgpServer()
if bgpServer == nil {
	// return error
}
peers, err := bgpServer.GetPeers(ctx)

This way we don't hold the read lock for a potentially expensive async operation.
That means that we have to guarantee that all bgp.Interface implementations (at the moment we only have gobgp) support concurrent function calls. Probably worth double checking that this assumption holds for gobgp, but it seems to me that all API calls get serialized to a channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/component/antctl Issues or PRs releated to the command line interface component area/transit/bgp Issues or PRs related to BGP support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants