Skip to content

[ycabled] fix exception-handling logic for ycabled#306

Merged
prgeor merged 24 commits intosonic-net:masterfrom
vdahiya12:exception-changes
Nov 11, 2022
Merged

[ycabled] fix exception-handling logic for ycabled#306
prgeor merged 24 commits intosonic-net:masterfrom
vdahiya12:exception-changes

Conversation

@vdahiya12
Copy link
Copy Markdown
Contributor

@vdahiya12 vdahiya12 commented Oct 19, 2022 •
edited
Loading

Signed-off-by: vaibhav-dahiya [email protected]
This PR is changing how exception handling is handled in python threading.
In python3.7 if child thread gets an exception, there is no way of knowing an exception occured, unless a join is called. The problem is resolved in this PR
Also task classes are structured this way

class YcableInfoUpdateTask(threading.Thread):

    def __init__(self):
        threading.Thread.__init__(self)
        self.exc = None
        
   def run(self):
        if self.task_stopping_event.is_set():
            return

        self.task_thread = threading.Thread(target=self.task_worker, args=(y_cable_presence,))
        self.task_thread.start()
        try:
            self.task_worker(self.y_cable_presence)
        except Exception as e:
            helper_logger.log_error("Exception occured at child thread YcableInfoUpdateTask due to {} {}".format(repr(e), traceback.format_exc()))

            self.exc = e

    def join(self):
        threading.Thread.join(self)

        if self.exc:
            raise self.exc

This allows helping catching the exception thrown by a child thread get caught along with traceback.
Main thread has a while loop, which monitors each thread, if not running will kill the whole process and supervisord will bring back the ycabled up.

Description

Motivation and Context

How Has This Been Tested?

Unit-tests and deploying changes on testbed

Additional Information (Optional)

sonic-mgmt tests which pass with this change

/var/src/sonic-mgmt-int/tests/logs/dualtor/test_ipinip.py::test_decap_active_tor
/var/src/sonic-mgmt-int/tests/logs/dualtor/test_ipinip.py::test_decap_standby_tor
/var/src/sonic-mgmt-int/tests/logs/dualtor/test_toggle_mux
/var/src/sonic-mgmt-int/dualtor_io/test_link_failure.py::test_standby_tor_downlink_down_downstream_active
/var/src/sonic-mgmt-int/tests/logs/platform_tests/api/test_chassis.py
In addition ran this script for testing the change on some Gemini devices
as well as switchover regression
time cost distribution

key: time cost

value: counts

i.e. 10:272 means 272 toggles took 0~10ms

{10: 272, 20: 217, 30: 3, 50: 3, 100: 3, 101: 3}

average (ms)

11.204035928143714

sudo cp <fw> /usr/share/sonic/firmware/.

declare -a PORTS='Ethernet0 Ethernet4 Ethernet8 Ethernet12 Ethernet16 Ethernet20 Ethernet40 Ethernet44 Ethernet48 Ethernet52 Ethernet56 Ethernet60 Ethernet64 Ethernet68 Ethernet72 Ethernet76 Ethernet80 Ethernet84 Ethernet104 Ethernet108 Ethernet112 Ethernet116 Ethernet120 Ethernet124'

declare -a PORTS1='Ethernet4'

for port in ${PORTS[@]}
do
  echo -e "\n\nname: $port"
  sudo show muxcable firmware version $port
  sleep 1
  sudo config muxcable firmware download <fw> $port
  sleep 10
  sudo show muxcable firmware version $port

  sudo config muxcable firmware activate $port
  sleep 10
  sudo show muxcable firmware version $port

  show mux status
  sleep 1
  sudo config mux mode auto $port
  sudo config mux mode active $port
  sleep 1
  show mux metrics $port
  sudo config mux mode auto $port
  sudo config mux mode standby $port
  sleep 1
  show mux metrics $port
  sleep 1
  show mux cableinfo $port
  sleep 1
  show mux alivecablestatus $port
  sleep 1
  show mux eyeinfo $port NIC
  show mux eyeinfo $port TORA
  show mux eyeinfo $port TORB
  sleep 1
  show mux pcsstatistics $port NIC
  sleep 1
  show mux pcsstatistics $port TORA
  sleep 1
  show mux pcsstatistics $port TORB
  sleep 1
  show mux fecstatistics $port NIC
  sleep 1
  show mux fecstatistics $port TORA
  sleep 1
  show mux fecstatistics $port TORB
  sleep 1
  show mux get-fec-anlt-speed $port
  sleep 1
  show mux hwmode mux $port
  sleep 1
  show mux hwmode switch $port
  sleep 1
  redis-cli -n 6 hgetall "HW_MUX_CABLE_TABLE|$port"
  sleep 1
  redis-cli -n 6 hgetall "MUX_CABLE_INFO|$port"
  sleep 1
  redis-cli -n 6 hgetall "MUX_CABLE_STATIC_INFO|$port"
  sleep 1
  redis-cli -n 6 hset "TRANSCEIVER_STATUS|$port" "status" "0"
  sleep 1
  sudo config mux mode auto $port
  sudo config mux mode active $port
  sleep 1
  show mux metrics $port
  sudo config mux mode auto $port
  sudo config mux mode standby $port
  sleep 1
  show mux metrics $port
  sleep 1
  show mux cableinfo $port
  sleep 1
  show mux alivecablestatus $port
  sleep 1
  show mux eyeinfo $port NIC
  show mux eyeinfo $port TORA
  show mux eyeinfo $port TORB
  sleep 1
  show mux pcsstatistics $port NIC
  sleep 1
  show mux pcsstatistics $port TORA
  sleep 1
  show mux fecstatistics $port NIC
  sleep 1
  show mux fecstatistics $port TORA
  sleep 1
  show mux fecstatistics $port TORB
  sleep 1
  show mux get-fec-anlt-speed $port
  sleep 1
  show mux hwmode mux $port
  sleep 1
  show mux hwmode switch $port
  sleep 1
  redis-cli -n 6 hgetall "HW_MUX_CABLE_TABLE|$port"
  sleep 1
  redis-cli -n 6 hgetall "MUX_CABLE_INFO|$port"
  sleep 1
  redis-cli -n 6 hgetall "MUX_CABLE_STATIC_INFO|$port"
  sleep 1
  redis-cli -n 6 hset "TRANSCEIVER_STATUS|$port" "status" "1"
  sleep 1
  show mux status
  sleep 1
  sudo config mux mode auto $port
  sudo config mux mode active $port
  sleep 1
  show mux metrics $port
  sudo config mux mode auto $port
  sudo config mux mode standby $port
  sleep 1
  show mux metrics $port
  sleep 1
  show mux cableinfo $port
  sleep 1
  show mux alivecablestatus $port
  sleep 1
  show mux eyeinfo $port NIC
  show mux eyeinfo $port TORA
  show mux eyeinfo $port TORB
  sleep 1
  show mux pcsstatistics $port NIC
  sleep 1
  show mux pcsstatistics $port TORA
  sleep 1
  show mux pcsstatistics $port TORB
  sleep 1
  show mux fecstatistics $port NIC
  sleep 1
  show mux fecstatistics $port TORA
  sleep 1
  show mux fecstatistics $port TORB

done

No exceptions observed, ycabled seems healthy with true thread count

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Oct 19, 2022

This pull request introduces 2 alerts when merging e3cd1a0 into 23b6970 - view on LGTM.com

new alerts:

  • 1 for Too few arguments in formatting call
  • 1 for Syntax error

@vdahiya12
fix all changes
68e9412 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Oct 19, 2022

This pull request introduces 1 alert when merging 68e9412 into 23b6970 - view on LGTM.com

new alerts:

  • 1 for Too few arguments in formatting call

@vdahiya12
fix some typo
cdcdc2e 
Signed-off-by: vaibhav-dahiya <[email protected]>
@vdahiya12
fix UT
aa70dcf 
Signed-off-by: vaibhav-dahiya <[email protected]>
@vdahiya12
fix typo
cc96144 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Oct 20, 2022

This pull request introduces 2 alerts when merging cc96144 into 4ea12cf - view on LGTM.com

new alerts:

  • 2 for Illegal raise

@vdahiya12
fix tests
81cf7ca 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Oct 21, 2022

This pull request introduces 3 alerts when merging 81cf7ca into 4ea12cf - view on LGTM.com

new alerts:

  • 3 for Illegal raise

@vdahiya12
fix
6421d9e 
Signed-off-by: vaibhav-dahiya <[email protected]>
@vdahiya12
fix more UT
5320c94 
Signed-off-by: vaibhav-dahiya <[email protected]>
@vdahiya12
add coverage
b898ffa 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Oct 21, 2022

This pull request introduces 4 alerts when merging b898ffa into 4ea12cf - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
fix UT
2807278 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Oct 21, 2022

This pull request introduces 4 alerts when merging 2807278 into 4ea12cf - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
try LGTM fix
3cfc033 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Oct 21, 2022

This pull request introduces 4 alerts when merging 3cfc033 into 4ea12cf - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
fix LGTM
2c14791 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Oct 21, 2022

This pull request introduces 4 alerts when merging 2c14791 into 4ea12cf - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12 vdahiya12 requested a review from prgeor October 31, 2022 17:10
@vdahiya12
realign
a71b11b 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 1, 2022

This pull request introduces 4 alerts when merging a71b11b into 9ae551f - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
fix spaces
bfc979c 
Signed-off-by: vaibhav-dahiya <[email protected]>
@vdahiya12
fix UT
e92c74f 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 1, 2022

This pull request introduces 4 alerts when merging e92c74f into 9ae551f - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
add all
9745e6a 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 2, 2022

This pull request introduces 4 alerts when merging 9745e6a into 9ae551f - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
fix missing changes
03c43d1 
Signed-off-by: vaibhav-dahiya <[email protected]>
prgeor
prgeor reviewed Nov 7, 2022
View reviewed changes
sonic-ycabled/tests/test_y_cable_helper.py Outdated
asic_index = 0
logical_port_name = "Ethernet0"
mux_tbl = Table("STATE_DB", "Y_CABLE_INFO_TABLE")
rc = post_port_mux_info_to_db(logical_port_name, mux_tbl,'active-standby')
Copy link
Copy Markdown
Collaborator

@prgeor prgeor Nov 7, 2022

Choose a reason for hiding this comment

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

can we add a test to crash at known location and see if we print backtrace ?

Copy link
Copy Markdown
Contributor Author

@vdahiya12 vdahiya12 Nov 8, 2022

Choose a reason for hiding this comment

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

UT is added, fixed

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 7, 2022

This pull request introduces 4 alerts when merging 1ac2a2b into 905874d - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
add exception handling UT
56c48ef 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 8, 2022

This pull request introduces 4 alerts when merging 56c48ef into 905874d - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
fix UT
2e19b16 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 8, 2022

This pull request introduces 4 alerts when merging 2e19b16 into 905874d - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
fix string
e23540c 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 8, 2022

This pull request introduces 4 alerts when merging e23540c into 905874d - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
fix some exceptions
0bd7cfb 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 9, 2022

This pull request introduces 4 alerts when merging 0bd7cfb into 905874d - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12
fix UT
2061ab1 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 9, 2022

This pull request introduces 4 alerts when merging 2061ab1 into 905874d - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@vdahiya12 vdahiya12 requested a review from prgeor November 9, 2022 00:56
prgeor
prgeor reviewed Nov 9, 2022
View reviewed changes
sonic-ycabled/tests/test_ycable.py Outdated

os.environ["YCABLE_UNIT_TESTING"] = "1"

exception_output_expected= """\
Copy link
Copy Markdown
Collaborator

@prgeor prgeor Nov 9, 2022

Choose a reason for hiding this comment

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

will this work if python version changes in future?

Copy link
Copy Markdown
Contributor Author

@vdahiya12 vdahiya12 Nov 9, 2022

Choose a reason for hiding this comment

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

this would not be backwards compatible, thats why I had added some string checks only,
Better to omit this check I think.

sonic-ycabled/tests/test_ycable.py Outdated
assert(expected_exception_start.args == expected_exception_join.args)
assert("NotImplementedError" in str(trace) and "effect" in str(trace))
assert("sonic-ycabled/ycable/ycable_utilities/y_cable_helper.py" in str(trace))
assert("line 3655" in str(trace))
Copy link
Copy Markdown
Collaborator

@prgeor prgeor Nov 9, 2022

Choose a reason for hiding this comment

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

can the line no change if the code changes?

Copy link
Copy Markdown
Contributor Author

@vdahiya12 vdahiya12 Nov 9, 2022

Choose a reason for hiding this comment

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

yes line number will change and assert will have to be updated.
Maybe we need to just focus on the trace lines

@vdahiya12
fix comment
419e742 
Signed-off-by: vaibhav-dahiya <[email protected]>
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 10, 2022

This pull request introduces 4 alerts when merging 419e742 into 905874d - view on LGTM.com

new alerts:

  • 4 for Illegal raise

@prgeor prgeor merged commit e8c5657 into sonic-net:master Nov 11, 2022
@vdahiya12 vdahiya12 mentioned this pull request Nov 11, 2022
Merged
@keboliu keboliu mentioned this pull request Nov 11, 2022
Closed
7 tasks
vdahiya12 added a commit that referenced this pull request Nov 11, 2022
@vdahiya12
[ycabled] fix exception-handling logic for ycabled (#312)
8ec96c0 
Cherry-pick conflict
#306

Signed-off-by: vaibhav-dahiya [email protected]
This PR is changing how exception handling is handled in python threading.
In python3.7 if child thread gets an exception, there is no way of knowing an exception occured, unless a join is called. The problem is resolved in this PR
Also task classes are structured this way

class YcableInfoUpdateTask(threading.Thread):

    def __init__(self):
        threading.Thread.__init__(self)
        self.exc = None
        
   def run(self):
        if self.task_stopping_event.is_set():
            return

        self.task_thread = threading.Thread(target=self.task_worker, args=(y_cable_presence,))
        self.task_thread.start()
        try:
            self.task_worker(self.y_cable_presence)
        except Exception as e:
            helper_logger.log_error("Exception occured at child thread YcableInfoUpdateTask due to {} {}".format(repr(e), traceback.format_exc()))

            self.exc = e

    def join(self):
        threading.Thread.join(self)

        if self.exc:
            raise self.exc

This allows helping catching the exception thrown by a child thread get caught along with traceback.
Main thread has a while loop, which monitors each thread, if not running will kill the whole process and supervisord will bring back the ycabled up.

Description
Motivation and Context
How Has This Been Tested?
Unit-tests and deploying changes on testbed

Additional Information (Optional)
sonic-mgmt tests which pass with this change

/var/src/sonic-mgmt-int/tests/logs/dualtor/test_ipinip.py::test_decap_active_tor
/var/src/sonic-mgmt-int/tests/logs/dualtor/test_ipinip.py::test_decap_standby_tor
/var/src/sonic-mgmt-int/tests/logs/dualtor/test_toggle_mux
/var/src/sonic-mgmt-int/dualtor_io/test_link_failure.py::test_standby_tor_downlink_down_downstream_active
/var/src/sonic-mgmt-int/tests/logs/platform_tests/api/test_chassis.py
@stephenxs stephenxs mentioned this pull request Nov 22, 2022
Merged
7 tasks
yxieca pushed a commit that referenced this pull request Nov 29, 2022
@vdahiya12 @yxieca
[ycabled] fix exception-handling logic for ycabled (#306)
8324c26
lotus-nexthop pushed a commit to lotus-nexthop/sonic-platform-daemons that referenced this pull request Oct 28, 2025
@sujinmkang
Update the return of update_firmware for the image not exist case (so…
95c3aec 
…nic-net#306)

* update the return for update_firmware api's failure case when the image file doesn't exist

* update comments

* update comment

* update comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@prgeor prgeor prgeor approved these changes

Assignees

No one assigned

Labels

Cherry Pick Conflict_202012 Included in 202205 Branch Request for 202205 Branch Y-Cable

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vdahiya12 @prgeor @yxieca