Skip to content

[Fast-reboot]: Gracefully shutdown syncd in fast-reboot#212

Merged
lguohan merged 4 commits intosonic-net:masterfrom
pavel-shirshov:pavelsh/syncd
Mar 27, 2018
Merged

[Fast-reboot]: Gracefully shutdown syncd in fast-reboot#212
lguohan merged 4 commits intosonic-net:masterfrom
pavel-shirshov:pavelsh/syncd

Conversation

@pavel-shirshov
Copy link
Contributor

@pavel-shirshov pavel-shirshov commented Feb 27, 2018

- What I did
Add a feature to gracefully shutdown syncd

- How I did it
Using syncd_request_shutdown utility

- How to verify it
Run fast-reboot test

- Previous command output (if the output of a command-line utility has changed)

- New command output (if the output of a command-line utility has changed)

-->

@pavel-shirshov pavel-shirshov self-assigned this Feb 27, 2018
lguohan
lguohan reviewed Feb 28, 2018
View reviewed changes
scripts/fast-reboot Outdated
docker kill swss > /dev/null

# Gracefully stop syncd
docker exec -ti syncd /usr/bin/syncd_request_shutdown --cold > /dev/null
Copy link
Contributor

@lguohan lguohan Feb 28, 2018

Choose a reason for hiding this comment

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

will this break the mellanox platform?

Copy link
Contributor Author

@pavel-shirshov pavel-shirshov Feb 28, 2018

Choose a reason for hiding this comment

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

It will not. Currently there're no remove_switch() in syncd.
I'm going to add remove_switch for Broadcom only.

Copy link
Contributor

@yxieca yxieca Mar 13, 2018

Choose a reason for hiding this comment

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

I see. I missed the other change. Sorry.

Regards,
Ying

@pavel-shirshov
Copy link
Contributor Author

pavel-shirshov commented Feb 28, 2018 •
edited
Loading

Please don't merge it.
I've found that:

  1. If I stop swss before syncd. syncd docker goes down too.
  2. otherwise I see cores from swss daemons.
    I need to figure out a better way.

yxieca
yxieca requested changes Mar 10, 2018
View reviewed changes
scripts/fast-reboot Outdated
docker exec -ti syncd /usr/bin/syncd_request_shutdown --cold > /dev/null

# Check it was stopped
until [[ $(docker top syncd | grep -q /usr/bin/syncd) -eq 0 ]]
Copy link
Contributor

@yxieca yxieca Mar 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think you need to s/-q/-c/

Copy link
Contributor

@jleveque jleveque Mar 10, 2018

Choose a reason for hiding this comment

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

Both grep -q and grep -c will return 0 if any matches are found, and will return 1 if no matches are found.

Therefore, either are applicable, however, the condition needs to be changed to either

until [[ $(docker top syncd | grep -q /usr/bin/syncd) -eq 1 ]]

or

while [[ $(docker top syncd | grep -q /usr/bin/syncd) -eq 0 ]]

Copy link
Contributor

@yxieca yxieca Mar 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

$(...) is getting the output of the execution. Therefore, with -q it is always ''.

With -c, the output would be how many lines found. We want to wait until there is 0 lines.

So, it would be prudent to redirect 2&> /dev/null for "docker top syncd" though for fast_reboot scenario, it is highly unlikely that the docker is not running.

Copy link
Contributor

@jleveque jleveque Mar 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, Ying. For some reason I was fixated on the return value, not the stdout output.

yxieca
yxieca approved these changes Mar 13, 2018
View reviewed changes
scripts/fast-reboot Outdated
docker kill swss > /dev/null

# Gracefully stop syncd
docker exec -ti syncd /usr/bin/syncd_request_shutdown --cold > /dev/null
Copy link
Contributor

@yxieca yxieca Mar 13, 2018

Choose a reason for hiding this comment

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

I see. I missed the other change. Sorry.

Regards,
Ying

@lguohan lguohan merged commit c1d0b96 into sonic-net:master Mar 27, 2018
@pavel-shirshov pavel-shirshov deleted the pavelsh/syncd branch March 27, 2018 21:04
zhenggen-xu added a commit to zhenggen-xu/sonic-utilities that referenced this pull request Jun 6, 2018
@zhenggen-xu
Merge branch 'github'
473c9dd 
            RB=1288101
            G=lnos-reviewers
            R=ntrianta,pmao,rmolina,sfardeen,zxu

* github:
  [acl_loader] Support Service ACL binding to multiple services (sonic-net#236)
  [show] Rename 'show session' to 'show mirror session' (sonic-net#235)
  [pfcstat]: create python cli tool to show pfc counters (sonic-net#233)
  [queuestat] add python CLI tool to show queue counters
  [acl-loader] Not to crash upon invalid rule (sonic-net#232)
  Show FDB type in fdbshow/show mac (sonic-net#231)
  [show] add 'show runningconfiguration all' subcommand (sonic-net#230)
  [reboot scripts] remove -t option in docker exec commands (sonic-net#228)
  [reboot] reduce stop service to only stop syncd (sonic-net#223)
  [crm]: Fix failures in CLI show commands (sonic-net#221)
  [Fast-reboot]: Gracefully shutdown syncd in fast-reboot (sonic-net#212)
  add fast-reboot support for nephos platform by stop kernel modules (sonic-net#220)
  [config bgp] Convert user input ipv6 addr to lower case before comparing (sonic-net#218)
  [PFCWD]: set default configuration when enabled by default (sonic-net#213)
  Add fast-reboot support for Aboot based images (sonic-net#214)
  sonic-utilities: Format show vlan config output (sonic-net#210)
  [AAA] Support login(ascii) authentication type (sonic-net#217)
  [sfputil] Adapt new way of getting PLATFORM(sonic-net#216)
  [Fast-Reboot]: Adapt fast-reboot-dump script for SAIv1.2  (sonic-net#211)
  Refactor fast-reboot script. Generate fast-reboot-dumps into configurable directory (sonic-net#208)
  Find correct opennsl module name before stopping it (sonic-net#207)
  [crm]: Add utility for CRM configuration (sonic-net#187)
  [reboot] update reboot script to retrieve platform with new format (sonic-net#206)
  Adapt to config engine change to load platform info properly (sonic-net#205)
  [config] Add qos clear and qos reload support (sonic-net#204)
  Dump default routes from APPL_DB table before fast-reboot (sonic-net#203)
  [acl_loader] Fix a crash issue when appdb is not consistent with cfgdb (sonic-net#202)
  [pfcwd]: add command to set pfcwd polling interval (sonic-net#192)
  [acl-loader] Prevent from hanging if run by non-root user (sonic-net#199)
  [config] Store ConfigDB init indicator boolean value as 1/0 in Redis to be language-agnostic (sonic-net#197)
  Get Vlan Id from SAI Vlan Object if bvid present (sonic-net#196)
  [TACACS+]: Fix aaa show error without configuration (sonic-net#191)
  'config bgp [shutdown|startup] neighbor <hostname>' now affects all sessions for neighbor (sonic-net#195)
  [sonic-clear] add a clear fdb command (sonic-net#186)
mihirpat1 pushed a commit to mihirpat1/sonic-utilities that referenced this pull request Sep 15, 2023
@snider-nokia
Fix QSFP-DD power class mask (sonic-net#212)
221fb8a
stepanblyschak pushed a commit to stepanblyschak/sonic-utilities that referenced this pull request Aug 9, 2025
@mssonicbld
[sfputil debug] Fix issue: do not check output status when CMIS versi…
b7b98ce 
…on is lower than 5.0 (sonic-net#212)

<!--
 Please make sure you've read and understood our contributing guidelines:
 https://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md

 failure_prs.log skip_prs.log Make sure all your commits include a signature generated with `git commit -s` **

 If this is a bug fix, make sure your description includes "closes #xxxx",
 "fixes #xxxx" or "resolves #xxxx" so that GitHub automatically closes the related
 issue when the PR is merged.

 If you are adding/modifying/removing any command or utility script, please also
 make sure to add/modify/remove any unit tests from the tests
 directory as appropriate.

 If you are modifying or removing an existing 'show', 'config' or 'sonic-clear'
 subcommand, or you are adding a new subcommand, please make sure you also
 update the Command Line Reference Guide (doc/Command-Reference.md) to reflect
 your changes.

 Please provide the following information:
-->

#### What I did

OutputStatusTx and OutputStatusRx are only supported since CMIS 5.0. `sfputil debug tx-output` and `sfputil debug rx-output` should check them only if CMIS rev is higher or equal to 5.0.

#### How I did it

1. Use `get_tx_disable` and `get_rx_disable` to check if the corresponding bits have been set by `sfputil debug` command
2. Check the CMIS rev, ignore checking of OutputStatusTx and OutputStatusRx if CMIS rev is below 5.0.

#### How to verify it

1. Manual test
2. unit test

#### Previous command output (if the output of a command-line utility has changed)

#### New command output (if the output of a command-line utility has changed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lguohan lguohan lguohan approved these changes

@yxieca yxieca yxieca approved these changes

+1 more reviewer

@jleveque jleveque jleveque approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@pavel-shirshov pavel-shirshov

Labels

Enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pavel-shirshov @lguohan @jleveque @yxieca