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

Make openbmp support redis subscription/population on sonic. #7

Merged
merged 48 commits into from
Nov 12, 2024

Conversation

FengPan-Frank
Copy link
Collaborator

@FengPan-Frank FengPan-Frank commented Apr 9, 2024

Work item tracking
Microsoft ADO (number only):27184382

Make openbmp support redis subscription/population on sonic.

This PR contains below changes:

  1. Use -DENABLE_REDIS=ON to make openbmp support sonic redis population, without -DENABLE_REDIS=ON it keeps its original openbmp functionality/dependency, both of two path verification have been covered by current azure pipeline.
  2. Support BMP_STATE_DB specific table population per users' config via CONFIG_DB, config cli.
  3. Work together with bmpcfgd daemon Add bmpcfgd for monitoring config_db state change. sonic-host-services#121, and the detail workflow is as below:
    3.1 bmpcfgd will be daemon which subscribe config_db table enablement, like bgp_neighbor_table/bgp_rib_in_table/bgp_rib_out_table, once config is changed, bmpcfgd will stop openbmpd, clear bmp related tables, then restart openbmpd
    3.2 openbmpd will be daemon which accept connection from bgp, and populate BMP_STATE_DB for relevant bmp table like BGP_NEIGHBOR, etc.
  4. Integrate with swss-common library.

Server/src/openbmp.cpp Outdated Show resolved Hide resolved
Server/src/openbmp.cpp Outdated Show resolved Hide resolved
LOG_INFO("RedisManager %s is disabled", table.c_str());
return false;
}
std::unique_ptr<swss::Table> stateBMPTable = std::unique_ptr<swss::Table>(new swss::Table(stateDb_.get(), table));
Copy link

@xincunli-sonic xincunli-sonic Nov 1, 2024

Choose a reason for hiding this comment

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

new swss::Table

Consider using std::make_uniqueswss::Table instead of new to create unique pointers. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated, and upgraded into c++14 correspondingly, seems swss-common uses c++14 as well.

/*********************************************************************//**
* Constructor for class
***********************************************************************/
RedisManager::~RedisManager() {
Copy link

@xincunli-sonic xincunli-sonic Nov 1, 2024

Choose a reason for hiding this comment

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

~RedisManager()

The destructor ~RedisManager() is currently empty. Ensure that any dynamically allocated resources are properly managed, or consider removing the destructor if no custom cleanup is required. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no resource to be cleanup now and could be used as place holder.

std::unique_ptr<swss::Table> stateBMPTable = std::unique_ptr<swss::Table>(new swss::Table(stateDb_.get(), table));
std::string fullKey;
for (const auto& key : keys) {
fullKey += key;
Copy link

@xincunli-sonic xincunli-sonic Nov 1, 2024

Choose a reason for hiding this comment

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

+=

Consider using std::ostringstream or std::accumulate to improve readability and efficiency. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated.


DEBUG("RedisManager WriteBMPTable key = %s", fullKey.c_str());

stateBMPTable->set(fullKey, fieldValues);
Copy link

@xincunli-sonic xincunli-sonic Nov 1, 2024

Choose a reason for hiding this comment

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

fieldValues

Suggest to log this values or size of it for future tracebility in complex scenarios #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added more debug log into MsgBusImpl_redis class for more constructive looking, which triggers this WriteBMPTable()

Copy link

@xincunli-sonic xincunli-sonic left a comment

Choose a reason for hiding this comment

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

LGTM

@FengPan-Frank
Copy link
Collaborator Author

/azpw run Azure.sonic-bmp

@mssonicbld
Copy link

/AzurePipelines run Azure.sonic-bmp

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FengPan-Frank
Copy link
Collaborator Author

/AzurePipelines run Azure.sonic-bmp

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FengPan-Frank
Copy link
Collaborator Author

/AzurePipelines run Azure.sonic-bmp

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FengPan-Frank FengPan-Frank mentioned this pull request Nov 8, 2024
@qiluo-msft qiluo-msft merged commit 6b5f200 into sonic-net:master Nov 12, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants