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

[bgpcfgd]: Dynamic BBR support #5626

Merged
merged 4 commits into from
Oct 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
{% if CONFIG_DB__DEVICE_METADATA['localhost']['type'] == 'ToRRouter' %}
neighbor PEER_V4 allowas-in 1
neighbor PEER_V4_INT allowas-in 1
{% elif CONFIG_DB__DEVICE_METADATA['localhost']['type'] == 'LeafRouter' %}
{% if CONFIG_DB__BGP_BBR['status'] == 'enabled' %}
neighbor PEER_V4 allowas-in 1
{% endif %}
{% endif %}
{% if CONFIG_DB__DEVICE_METADATA['localhost']['sub_role'] == 'BackEnd' %}
neighbor PEER_V4_INT route-reflector-client
Expand All @@ -24,6 +28,10 @@
{% if CONFIG_DB__DEVICE_METADATA['localhost']['type'] == 'ToRRouter' %}
neighbor PEER_V6 allowas-in 1
neighbor PEER_V6_INT allowas-in 1
{% elif CONFIG_DB__DEVICE_METADATA['localhost']['type'] == 'LeafRouter' %}
{% if CONFIG_DB__BGP_BBR['status'] == 'enabled' %}
neighbor PEER_V6 allowas-in 1
{% endif %}
{% endif %}
{% if CONFIG_DB__DEVICE_METADATA['localhost']['sub_role'] == 'BackEnd' %}
neighbor PEER_V6_INT route-reflector-client
Expand Down
7 changes: 7 additions & 0 deletions files/image_config/constants/constants.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,17 @@ constants:
v6:
- "deny 0::/0 le 59"
- "deny 0::/0 ge 65"
bbr:
enabled: true
peers:
general: # peer_type
db_table: "BGP_NEIGHBOR"
template_dir: "general"
bbr:
PEER_V4:
- ipv4
PEER_V6:
- ipv6
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not use device metadata role to identify if a peer is a tor or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I don't have neighbor metadata in public. That was disabled by multiple requests from the community. Without this change I can implement the change in internal, but not in the public. But we want to test the feature on public too.
  2. I need a way to define, what address families are used by each peer-group for BBR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this adds additional maintaining cost. it is better to check if neighbor metadata is available or not, if it is available, then use it. otherwise, look for bbr definition. do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't agree here, sorry

  1. We need to define mapping between address families and peer-groups. We can't extract this information from the minigraph, it is available only in templates.
  2. Currently bgpcfgd doesn't know anything about the device neighbor metadata, also as T0, T1, T2 and so on. That was requested explicitly by community. So I add an option to depend on the neighbor metadata presence, but I enable that option only for internal image. All other operations with T0, T1 and so on is done inside of the internal templates.
  3. We need to test this functionality using the public image, so we need a way to define this without neighbor meta
  4. Current approach allows us to have T0 peer-groups, which doesn't support BBR.
  5. My goal is to make templates downloadable from external source, like we download acl rules. I think of having a tar archive with templates + metadata file in the archive describing the metadata of the templates.

monitors: # peer_type
enabled: true
db_table: "BGP_MONITORS"
Expand Down
3 changes: 3 additions & 0 deletions src/sonic-bgpcfgd/bgpcfgd/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from .directory import Directory
from .log import log_notice, log_crit
from .managers_allow_list import BGPAllowListMgr
from .managers_bbr import BBRMgr
from .managers_bgp import BGPPeerMgrBase
from .managers_db import BGPDataBaseMgr
from .managers_intf import InterfaceMgr
Expand Down Expand Up @@ -47,6 +48,8 @@ def do_work():
BGPPeerMgrBase(common_objs, "CONFIG_DB", "BGP_PEER_RANGE", "dynamic", False),
# AllowList Managers
BGPAllowListMgr(common_objs, "CONFIG_DB", "BGP_ALLOWED_PREFIXES"),
# BBR Manager
BBRMgr(common_objs, "CONFIG_DB", "BGP_BBR"),
]
runner = Runner()
for mgr in managers:
Expand Down
120 changes: 120 additions & 0 deletions src/sonic-bgpcfgd/bgpcfgd/managers_bbr.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
from swsscommon import swsscommon

from .log import log_err, log_info, log_crit
from .manager import Manager
from .utils import run_command


class BBRMgr(Manager):
""" This class initialize "BBR" feature for """
def __init__(self, common_objs, db, table):
"""
Initialize the object
:param common_objs: common object dictionary
:param db: name of the db
:param table: name of the table in the db
"""
super(BBRMgr, self).__init__(
common_objs,
[("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME, "localhost/bgp_asn"),],
db,
table,
)
self.enabled = False
self.bbr_enabled_pgs = {}
self.directory.put(self.db_name, self.table_name, 'status', "disabled")
self.__init()

def set_handler(self, key, data):
""" Implementation of 'SET' command for this class """
if not self.enabled:
log_info("BBRMgr::BBR is disabled. Drop the request")
return True
if not self.__set_validation(key, data):
return True
cmds = self.__set_prepare_config(data['status'])
rv = self.cfg_mgr.push_list(cmds)
if not rv:
log_crit("BBRMgr::can't apply configuration")
return True
Copy link
Collaborator

Choose a reason for hiding this comment

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

always return True?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. False means save it in the queue, and give it back to me next time,
True means - remove it completely.
In this case I don't see any reason the change could be applied without any issue in the future

self.__restart_peers()
return True

def del_handler(self, key):
""" Implementation of 'DEL' command for this class """
log_err("The '%s' table shouldn't be removed from the db" % self.table_name)

def __init(self):
""" Initialize BBRMgr. Extracted from constructor """
if not 'bgp' in self.constants:
log_err("BBRMgr::Disabled: 'bgp' key is not found in constants")
return
if 'bbr' in self.constants['bgp'] and \
'enabled' in self.constants['bgp']['bbr'] and \
self.constants['bgp']['bbr']['enabled']:
self.bbr_enabled_pgs = self.__read_pgs()
Copy link
Collaborator

Choose a reason for hiding this comment

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

"BBR is adding `neighbor PEER_GROUP allowas-in 1' for all BGP peer-groups which points to T0"

can you point me code where you identify if a peer group points to T0 or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I use file constants

if self.bbr_enabled_pgs:
self.enabled = True
self.directory.put(self.db_name, self.table_name, 'status', "enabled")
log_info("BBRMgr::Initialized and enabled")
else:
log_info("BBRMgr::Disabled: no BBR enabled peers")
else:
log_info("BBRMgr::Disabled: not enabled in the constants")

def __read_pgs(self):
"""
Read peer-group bbr settings from constants file
:return: return bbr information from constant peer-group settings
"""
if 'peers' not in self.constants['bgp']:
log_info("BBRMgr::no 'peers' was found in constants")
return {}
res = {}
for peer_name, value in self.constants['bgp']['peers'].items():
if 'bbr' not in value:
continue
for pg_name, pg_afs in value['bbr'].items():
res[pg_name] = pg_afs
return res

def __set_validation(self, key, data):
""" Validate set-command arguments
:param key: key of 'set' command
:param data: data of 'set' command
:return: True is the parameters are valid, False otherwise
"""
if key != 'all':
log_err("Invalid key '%s' for table '%s'. Only key value 'all' is supported" % (key, self.table_name))
return False
if 'status' not in data:
log_err("Invalid value '%s' for table '%s', key '%s'. Key 'status' in data is expected" % (data, self.table_name, key))
return False
if data['status'] != "enabled" and data['status'] != "disabled":
log_err("Invalid value '%s' for table '%s', key '%s'. Only 'enabled' and 'disabled' are supported" % (data, self.table_name, key))
return False
return True

def __set_prepare_config(self, status):
"""
Generate FFR configuration to apply changes
:param status: either "enabled" or "disabled"
:return: list of commands prepared for FRR
"""
bgp_asn = self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME)["localhost"]["bgp_asn"]
cmds = ["router bgp %s" % bgp_asn]
prefix_of_commands = "" if status == "enabled" else "no "
for af in ["ipv4", "ipv6"]:
cmds.append(" address-family %s" % af)
for pg_name in sorted(self.bbr_enabled_pgs.keys()):
if af in self.bbr_enabled_pgs[pg_name]:
cmds.append(" %sneighbor %s allowas-in 1" % (prefix_of_commands, pg_name))
return cmds

def __restart_peers(self):
""" Restart peer-groups which support BBR """
for peer_group in sorted(self.bbr_enabled_pgs.keys()):
Copy link
Collaborator

Choose a reason for hiding this comment

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

why sort here? any reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise my test doesn't know what to expect (or I can use conversion to sets) and compare sets. I'm going to remove it after the batching change

rc, out, err = run_command(["vtysh", "-c", "clear bgp peer-group %s soft in" % peer_group])
if rc != 0:
log_value = peer_group, rc, out, err
log_crit("BBRMgr::Can't restart bgp peer-group '%s'. rc='%d', out='%s', err='%s'" % log_value)
1 change: 1 addition & 0 deletions src/sonic-bgpcfgd/bgpcfgd/managers_bgp.py
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ def add_peer(self, vrf, nbr, data):

kwargs = {
'CONFIG_DB__DEVICE_METADATA': self.directory.get_slot("CONFIG_DB", swsscommon.CFG_DEVICE_METADATA_TABLE_NAME),
'CONFIG_DB__BGP_BBR': self.directory.get_slot('CONFIG_DB', 'BGP_BBR'),
'constants': self.constants,
'bgp_asn': bgp_asn,
'vrf': vrf,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,8 @@
"localhost": {
"type": "ToRRouter"
}
}
}
},
"CONFIG_DB__BGP_BBR": {
"status": "enabled"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,8 @@
"type": "LeafRouter",
"sub_role": "BackEnd"
}
},
"CONFIG_DB__BGP_BBR": {
"status": "disabled"
}
}
}
4 changes: 4 additions & 0 deletions src/sonic-bgpcfgd/tests/swsscommon_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
from mock import MagicMock


swsscommon = MagicMock(CFG_DEVICE_METADATA_TABLE_NAME = "DEVICE_METADATA")
Loading