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

[TSA] Reliable TSA: Addressing pizza box issues #19217

Merged
merged 5 commits into from
Jun 19, 2024
Merged

Conversation

fountzou
Copy link
Contributor

@fountzou fountzou commented Jun 5, 2024

Why I did it

Implement HLD https://github.com/skeesara-nokia/SONiC/blob/master/doc/voq/Reliable_TSA.md
Resolves pizza box issues from the original PR #18928 reverted by PR #19173

Work item tracking
  • Microsoft ADO (number only):

How I did it

A new attribute "tsa_enabled" has been added in CHASSIS_APP_DB the value of which changes whenever TSA/TSB is issued in the supervisor (default value is false). bgpcfgd subscribes to CHASSIS_APP_DB to receive updates on the newly added "tsa_enabled" attribute and in conjunction with the CONFIG_DB "tsa_enabled" attribute value, determine the BGP operational state is determined to be in TSA or TSB.

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)

- Why I did it
Implement HLD https://github.com/skeesara-nokia/SONiC/blob/master/doc/voq/Reliable_TSA.md

OB- How I did it
A new attribute "tsa_enabled" has been added in CHASSIS_APP_DB the value of which changes whenever TSA/TSB is issued in the supervisor (default value is false). bgpcfgd subscribes to CHASSIS_APP_DB to receive updates on the newly added "tsa_enabled" attribute and in conjunction with the CONFIG_DB "tsa_enabled" attribute value, determine the BGP operational state is determined to be in TSA or TSB.

Signed-off-by: fountzou <ioannis.fountzoulas@nokia.com>
@fountzou
Copy link
Contributor Author

fountzou commented Jun 5, 2024

Addresses the pizza box issues introduced by the original PR #18928 reverted by PR #19173.

@fountzou
Copy link
Contributor Author

fountzou commented Jun 5, 2024

@abdosi / @arlakshm Please help to review. Thanks!

fountzou and others added 3 commits June 6, 2024 18:25
Signed-off-by: fountzou <ioannis.fountzoulas@nokia.com>
Signed-off-by: fountzou <ioannis.fountzoulas@nokia.com>
@fountzou
Copy link
Contributor Author

fountzou commented Jun 7, 2024

@abdosi @arlakshm Could you please restart the Azure pipeline and review? Thanks!

@judyjoseph judyjoseph requested a review from abdosi June 7, 2024 16:36
@@ -74,6 +76,10 @@ def do_work():
# Device Global Manager
DeviceGlobalCfgMgr(common_objs, "CONFIG_DB", swsscommon.CFG_BGP_DEVICE_GLOBAL_TABLE_NAME),
]

if device_info.get_platform_info().get('switch_type') == "voq":
Copy link
Contributor

@abdosi abdosi Jun 7, 2024

Choose a reason for hiding this comment

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

we need not limit to voq chassis but to any chassis.

Please replace the API to this one: https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-py-common/sonic_py_common/device_info.py#L575

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comment and for pointing out the correct API. Done

ch.connect(ch.CHASSIS_APP_DB, False)
chassis_tsa_status = ch.get(ch.CHASSIS_APP_DB, "BGP_DEVICE_GLOBAL|STATE", 'tsa_enabled')
except Exception:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Please log the message on exception. Helpful for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

if [[ -z "$DEV" ]] && [[ -f /etc/sonic/chassisdb.conf ]]; then
tsa_cfg="$($SONIC_DB_CLI CONFIG_DB HGET "BGP_DEVICE_GLOBAL|STATE" "tsa_enabled")"
if [[ -n "$tsa_cfg" ]]; then
docker exec -i ${DOCKERNAME} $SONIC_DB_CLI CHASSIS_APP_DB HMSET "BGP_DEVICE_GLOBAL|STATE" tsa_enabled ${tsa_cfg}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please capture return code of operation and log message in case of failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed your suggestion. Thanks!

#Enforce CHASSIS_APP_DB.tsa_enabled to be in sync with BGP_DEVICE_GLOBAL.STATE.tsa_enabled
if [[ -f /etc/sonic/chassisdb.conf ]]; then
tsa_cfg="$(sonic-db-cli CONFIG_DB HGET "BGP_DEVICE_GLOBAL|STATE" "tsa_enabled")"
sonic-db-cli CHASSIS_APP_DB HMSET "BGP_DEVICE_GLOBAL|STATE" tsa_enabled ${tsa_cfg}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please capture return code of operation and log message in case of failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks for your comment

Copy link
Contributor

@abdosi abdosi left a comment

Choose a reason for hiding this comment

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

Comments added.

Signed-off-by: fountzou <ioannis.fountzoulas@nokia.com>
@fountzou fountzou requested a review from abdosi June 10, 2024 13:42
Copy link
Contributor

@abdosi abdosi left a comment

Choose a reason for hiding this comment

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

LGTM

@abdosi
Copy link
Contributor

abdosi commented Jun 11, 2024

@lguohan : Can you help with merge of this.

@mlok-nokia
Copy link
Contributor

@lguohan Hi Guohan, Please help to merge this PR. Thanks

@mlok-nokia
Copy link
Contributor

@rlhui Hi Rita, Please help to merge this PR. Thanks

@rlhui rlhui merged commit 99e0e1a into sonic-net:master Jun 19, 2024
20 checks passed
switch_type=`sonic-db-cli CONFIG_DB hget 'DEVICE_METADATA|localhost' 'switch_type'`
TSA_CHASSIS_STATE=false

if [[ $switch_type == 'voq' ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please change this to include chassis-packet or change the check to look for type == "SpineRouter"?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fountzou : Please address this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for comment @tjchadaga @abdosi . Fixed -- kindly see #19527

sonic-cfggen -a "$CONFIG_DB_TSA_STATE_UPDATE" -w
echo "Chassis Mode: Normal -> Maintenance"
logger -t TSA -p user.info "Chassis Mode: Normal -> Maintenance"
fi
echo "Please execute \"rexec all -c 'sudo config save -y'\" to preserve System mode in Maintenance after reboot\
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be changed to just 'sudo config save' instead of rexec all now?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fountzou : Please address this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for comment @tjchadaga @abdosi . Fixed -- kindly see #19527

sonic-cfggen -a "$CONFIG_DB_TSA_STATE_UPDATE" -w
echo "Chassis Mode: Maintenance -> Normal"
logger -t TSB -p user.info "Chassis Mode: Maintenance -> Normal"
fi
echo "Please execute \"rexec all -c 'sudo config save -y'\" to preserve System mode in Normal state after reboot\
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here

Copy link
Contributor

Choose a reason for hiding this comment

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

@fountzou : Please address this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for comment @tjchadaga @abdosi . Fixed -- kindly see #19527

@yejianquan
Copy link
Contributor

yejianquan commented Jul 10, 2024

Hi @fountzou , please resolve the conflict to 202405 and raise a seperate PR

@fountzou
Copy link
Contributor Author

Hi @yejianquan. Sure, working on that -- thank you

arun1355492 pushed a commit to arun1355492/sonic-buildimage that referenced this pull request Jul 26, 2024
* [TSA] Reliable TSA: Addressing pizza box issues

- Why I did it
Implement HLD https://github.com/skeesara-nokia/SONiC/blob/master/doc/voq/Reliable_TSA.md

OB- How I did it
A new attribute "tsa_enabled" has been added in CHASSIS_APP_DB the value of which changes whenever TSA/TSB is issued in the supervisor (default value is false). bgpcfgd subscribes to CHASSIS_APP_DB to receive updates on the newly added "tsa_enabled" attribute and in conjunction with the CONFIG_DB "tsa_enabled" attribute value, determine the BGP operational state is determined to be in TSA or TSB.

Signed-off-by: fountzou <ioannis.fountzoulas@nokia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants