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 bmpcfgd for monitoring config_db state change. #18940

Merged
merged 21 commits into from
Sep 19, 2024

Conversation

FengPan-Frank
Copy link
Contributor

@FengPan-Frank FengPan-Frank commented May 11, 2024

Why I did it

sonic-net/SONiC#1621, need dedicated daemon to monitor config_db and manage openbmpd state.

Work item tracking
  • Microsoft ADO (number only):27511095

How I did it

Use swss-common lib to monitor config_db change.

How to verify it

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@lguohan lguohan added the BMP Bgp Monitor Container label May 18, 2024
@FengPan-Frank FengPan-Frank requested a review from qiluo-msft May 22, 2024 13:50
@FengPan-Frank
Copy link
Contributor Author

FengPan-Frank commented May 23, 2024

/azp run Azure.sonic-buildimage

Copy link

Command 'Azure.sonic-buildimage' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

Copy link

Commenter does not have sufficient privileges for PR 18940 in repo sonic-net/sonic-buildimage

@FengPan-Frank FengPan-Frank requested a review from qiluo-msft May 29, 2024 13:30
subprocess.call(["service", "openbmpd", "start"])


class BMPCfgDaemon:
Copy link
Contributor

Choose a reason for hiding this comment

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

BMPCfgDaemon

For MultiASIC:

This Daemon is running in localhost only? I recall the design mentioned we will have multiple BMP_STATE_DB per asic? If so, I suppose we should have individual cfgd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This daemon should be running in each bmp container, each bmp container will have 1 bmpcfgd + 1 openbmpd.

Copy link
Contributor

@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

self.config_db.connect(wait_for_init=True, retry_on=True)
self.bmpcfg = BMPCfg(self.state_db_conn)
except KeyError as error:
self.log_error(
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 6, 2024

Choose a reason for hiding this comment

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

log_error

Seems like a critical error. Do you want to swallow (continue) or keep throwing? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated without try/catch.


class BMPCfgDaemon:
def __init__(self):
try:
Copy link
Collaborator

@qiluo-msft qiluo-msft Sep 6, 2024

Choose a reason for hiding this comment

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

try

Which lines have the possibility to throw KeyError? Why it is KeyError instead of some RedisXXX exception type? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rethink about this, we should not need try/catch in init flow, any exception should terminate and propagate the call stack, updated code.

@qiluo-msft qiluo-msft merged commit e106909 into sonic-net:master Sep 19, 2024
23 checks passed
aidan-gallagher pushed a commit to aidan-gallagher/sonic-buildimage that referenced this pull request Nov 16, 2024
#### Why I did it
sonic-net/SONiC#1621, need dedicated daemon to monitor config_db and manage openbmpd state.
#### How I did it
Use swss-common lib to monitor config_db change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BMP Bgp Monitor Container
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants