[warm-reboot] Fix failures of warm reboot on disconnect of ssh session #1529

ghost · 2021-03-26T14:20:26Z

Starting the script in background mode and detaching the process
from terminal session to prevent failures, caused by closing or
sudden disconnecting of terminal session.
Redirecting output of the script to the file to prevent failures,
when the script tries to write an output to file descriptor of
the nonexistent terminal session.
Adding new parameter to script to be able to explicitly run the
script in foreground mode with output to the terminal.
Updating the command reference doc according to the changes.

Signed-off-by: Maksym Belei Maksym_Belei@jabil.com

What I did

Resolves sonic-net/sonic-buildimage#7127
Fixed failures of warm reboot, when the SSH session is being disconnected.
As the script will now be executed in background mode by default, added parameter to explicitly run it as usual, in foreground mode.
Updated command reference according to the changes.

How I did it

By restarting the script in background mode with detaching it from the terminal session.
All the output has redirected to file /var/log/warm-reboot.txt for warm-reboot case, or /var/log/fast-reboot.txt for fast-reboot, depends on REBOOT_TYPE. This will prevent crashes of the script in case, when it will try to write some data to the file descriptor of the disconnected terminal session.

How to verify it

Connect to the switch with SSH;
Execute sudo warm-reboot -v;
See the current progress of warm reboot with cat /var/log/warm-reboot.txt;
Close SSH connection before warm reboot finish;
Warm reboot should finish successfully, in spite of status of the SSH session.

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

The script will be running in background detached mode with output to the file. The related log will be shown in terminal before restarting in background mode:

admin@sonic:~$ sudo warm-reboot
Detaching the process from the terminal session. Redirecting output to /var/log/warm-reboot.txt.

All the usual logs will be written to warm-reboot.txt.

ghost · 2021-03-30T16:09:30Z

retest this please

liat-grozovik · 2021-03-30T18:50:03Z

retest this please

liat-grozovik · 2021-03-30T18:50:22Z

/AzurePipelines run

azure-pipelines · 2021-03-30T18:50:32Z

Azure Pipelines successfully started running 1 pipeline(s).

yxieca · 2021-03-30T19:53:44Z

scripts/fast-reboot

+# re-run the script in background mode with detaching from the terminal session
+if [[ x"${DETACH}" == x"yes" ]]; then
+   echo "Detaching the process from the terminal session. Redirecting output to ${LOG_PATH}."
+   $0 "$@" -d 1>$LOG_PATH 2>$1 & disown %%


It means redirect stderr to stdout. stdout in our case is directed to the file.

I suspect you wanted to do so, you can use 2>&1 or change the whole thing to

$0 "$@" -d &>$LOG_PATH & disown %%

yxieca · 2021-04-01T14:50:57Z

scripts/fast-reboot

@@ -21,6 +21,8 @@ PLATFORM_PLUGIN="${REBOOT_TYPE}_plugin"
 LOG_SSD_HEALTH="/usr/local/bin/log_ssd_health"
 SSD_FW_UPDATE="ssd-fw-upgrade"
 TAG_LATEST=yes
+DETACH=yes


I think -d should mean detach and default should be DETACH=no

Sorry for late response. The idea of the changes is to prevent failures of warm-reboot/fast-reboot in cases when the ssh session is broken/closed. I made detached mode as default and leave possibility to run the scripts in non-detached mode with -d if the user wants. If to make as you propose, the user could forget to add -d parameter, what may lead to failure of the reboot. Due to this, I prefer to leave parameter -d as is. By the way, parameter -t made in similar way, so, I thought that there is no problem to let -d to run the process in non-detached mode, not wise versa.

@maksymbelei95 I don't think changing default behavior is the right thing. This blocking behavior has been used until now in tests and production. Changing the default behavior could have unexpected consequences. For special scenario, it should not be default.

liat-grozovik · 2021-07-02T06:49:00Z

@yxieca can you please further review and see if all comments were addressed and it can be merged?

stepanblyschak · 2021-09-02T10:52:36Z

@maksymbelei95 @liat-grozovik @yxieca What is needed in order to have this fix merged?

liat-grozovik · 2021-09-02T11:52:26Z

it LGTM. i would suggest to add unit test if possible. @yxieca ?

stepanblyschak · 2021-09-15T07:34:24Z

@maksymbelei95 Could you please resolve the conflict?

* Starting the script in background mode and detaching the process from terminal session to prevent failures, caused by closing or sudden disconnecting of terminal session. * Redirecting output of the script to the file to prevent failures, when the script tries to write an output to file descriptor of the nonexistant terminal session. * Adding new parameter to script to be able to explicitly run the script in foreground mode with output to the terminal. * Updating the command reference doc according to the changes. Signed-off-by: Maksym Belei <Maksym_Belei@jabil.com>

pshulik · 2021-10-20T06:28:17Z

@yxieca, Could you please review the latest merge

yxieca · 2021-10-22T00:24:42Z

@yxieca, Could you please review the latest merge

I still see detach becomes default behavior. I think the default behavior should be the original behavior: blocking.

yxieca

Please don't change default behavior.

MaratGubaiev · 2021-11-02T09:40:32Z

Could anybody please rerun the tests?
@yxieca, @liat-grozovik

liat-grozovik · 2021-11-02T09:51:46Z

/azp run

azure-pipelines · 2021-11-02T09:51:56Z

Azure Pipelines successfully started running 1 pipeline(s).

dprital · 2021-11-25T10:11:58Z

@yxieca I see that this PR was approved by you. Can you please merge it ?

#1529) Starting the script in background mode and detaching the process from terminal session to prevent failures, caused by closing or sudden disconnecting of terminal session. Redirecting output of the script to the file to prevent failures, when the script tries to write an output to file descriptor of the nonexistent terminal session. Adding new parameter to script to be able to explicitly run the script in foreground mode with output to the terminal. Updating the command reference doc according to the changes. - What I did Resolves sonic-net/sonic-buildimage#7127 Fixed failures of warm reboot, when the SSH session is being disconnected. As the script will now be executed in background mode by default, added parameter to explicitly run it as usual, in foreground mode. Updated command reference according to the changes. - How I did it By restarting the script in background mode with detaching it from the terminal session. All the output has redirected to file /var/log/warm-reboot.txt for warm-reboot case, or /var/log/fast-reboot.txt for fast-reboot, depends on REBOOT_TYPE. This will prevent crashes of the script in case, when it will try to write some data to the file descriptor of the disconnected terminal session. - How to verify it 1. Connect to the switch with SSH; 2. Execute sudo warm-reboot -v; 3. See the current progress of warm reboot with cat /var/log/warm-reboot.txt; 4. Close SSH connection before warm reboot finish; Warm reboot should finish successfully, in spite of status of the SSH session. - New command output (if the output of a command-line utility has changed) The script will be running in background detached mode with output to the file. The related log will be shown in terminal before restarting in background mode: admin@sonic:~$ sudo warm-reboot Detaching the process from the terminal session. Redirecting output to /var/log/warm-reboot.txt. All the usual logs will be written to warm-reboot.txt.

ghost force-pushed the fix_warmboot_ssh branch from 9439573 to 726a54c Compare March 26, 2021 14:21

ghost marked this pull request as ready for review March 26, 2021 14:23

ghost mentioned this pull request Mar 26, 2021

[warm-reboot] warm-reboot breaks if ssh session in which it was started drops sonic-net/sonic-buildimage#7127

Closed

liat-grozovik requested a review from yxieca March 30, 2021 18:49

yxieca reviewed Mar 30, 2021

View reviewed changes

yxieca reviewed Apr 1, 2021

View reviewed changes

ghost force-pushed the fix_warmboot_ssh branch from 726a54c to f44771e Compare April 12, 2021 06:37

ghost requested a review from yxieca April 12, 2021 09:55

NGorb-jabil force-pushed the fix_warmboot_ssh branch from f44771e to 3b7e183 Compare October 18, 2021 11:01

yxieca requested changes Oct 22, 2021

View reviewed changes

MaratGubaiev and others added 2 commits October 26, 2021 17:39

-D : detached mode - closing terminal will not cause stopping reboot

d898699

Merge branch 'master' into fix_warmboot_ssh

2fbdc52

yxieca approved these changes Nov 2, 2021

View reviewed changes

stepanblyschak approved these changes Nov 25, 2021

View reviewed changes

liat-grozovik merged commit e63f47e into sonic-net:master Nov 25, 2021

aravindmani-1 mentioned this pull request Dec 8, 2021

[submodule] Update sonic-utilities submodule sonic-net/sonic-buildimage#9467

Closed

5 tasks

DavidZagury mentioned this pull request Dec 8, 2021

[Submodule][sonic-utilities] Update sonic-utilities submodule sonic-net/sonic-buildimage#9469

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[warm-reboot] Fix failures of warm reboot on disconnect of ssh session #1529

[warm-reboot] Fix failures of warm reboot on disconnect of ssh session #1529

ghost commented Mar 26, 2021 •

edited by ghost

Loading

ghost commented Mar 30, 2021

liat-grozovik commented Mar 30, 2021

liat-grozovik commented Mar 30, 2021

azure-pipelines bot commented Mar 30, 2021

yxieca Mar 30, 2021

ghost Mar 30, 2021

yxieca Apr 1, 2021

ghost Apr 12, 2021

yxieca Apr 1, 2021

ghost Apr 12, 2021

yxieca Sep 2, 2021

liat-grozovik commented Jul 2, 2021

stepanblyschak commented Sep 2, 2021

liat-grozovik commented Sep 2, 2021 •

edited

Loading

stepanblyschak commented Sep 15, 2021

pshulik commented Oct 20, 2021

yxieca commented Oct 22, 2021

yxieca left a comment

MaratGubaiev commented Nov 2, 2021

liat-grozovik commented Nov 2, 2021

azure-pipelines bot commented Nov 2, 2021

dprital commented Nov 25, 2021

[warm-reboot] Fix failures of warm reboot on disconnect of ssh session #1529

[warm-reboot] Fix failures of warm reboot on disconnect of ssh session #1529

Conversation

ghost commented Mar 26, 2021 • edited by ghost Loading

What I did

How I did it

How to verify it

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

ghost commented Mar 30, 2021

liat-grozovik commented Mar 30, 2021

liat-grozovik commented Mar 30, 2021

azure-pipelines bot commented Mar 30, 2021

yxieca Mar 30, 2021

Choose a reason for hiding this comment

ghost Mar 30, 2021

Choose a reason for hiding this comment

yxieca Apr 1, 2021

Choose a reason for hiding this comment

ghost Apr 12, 2021

Choose a reason for hiding this comment

yxieca Apr 1, 2021

Choose a reason for hiding this comment

ghost Apr 12, 2021

Choose a reason for hiding this comment

yxieca Sep 2, 2021

Choose a reason for hiding this comment

liat-grozovik commented Jul 2, 2021

stepanblyschak commented Sep 2, 2021

liat-grozovik commented Sep 2, 2021 • edited Loading

stepanblyschak commented Sep 15, 2021

pshulik commented Oct 20, 2021

yxieca commented Oct 22, 2021

yxieca left a comment

Choose a reason for hiding this comment

MaratGubaiev commented Nov 2, 2021

liat-grozovik commented Nov 2, 2021

azure-pipelines bot commented Nov 2, 2021

dprital commented Nov 25, 2021

ghost commented Mar 26, 2021 •

edited by ghost

Loading

liat-grozovik commented Sep 2, 2021 •

edited

Loading