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

[Fast-reboot] Set flex counters delay indicator to prevent flex counters enablement after fast-reboot #1768

Merged
merged 6 commits into from
Sep 2, 2021
Merged

[Fast-reboot] Set flex counters delay indicator to prevent flex counters enablement after fast-reboot #1768

merged 6 commits into from
Sep 2, 2021

Conversation

shlomibitton
Copy link
Contributor

Signed-off-by: Shlomi Bitton shlomibi@nvidia.com

What I did

Set flex counters delay indicator to prevent flex counters enablement after fast-reboot.

How I did it

Modify config DB json file with 'true' status for delay of flex counters indicator.

How to verify it

Run fast-reboot and observe counters are created only when enable_counters script is called, even if the tables are present in config DB.

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)

Shlomi Bitton and others added 3 commits August 17, 2021 09:16
Copy link
Contributor

@yxieca yxieca left a comment

Choose a reason for hiding this comment

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

Please move the change to fast-reboot script itself.

Move the logic to fast-reboot script
@shlomibitton
Copy link
Contributor Author

Please move the change to fast-reboot script itself.

@yxieca can you please check now?

yxieca
yxieca previously approved these changes Aug 18, 2021
@liat-grozovik
Copy link
Collaborator

@shlomibitton is it needed for 202106?
@nazariig could u please refer to the comments?
@vaibhavhd can you please help with review?

@shlomibitton
Copy link
Contributor Author

@shlomibitton is it needed for 202106?
@nazariig could u please refer to the comments?
@vaibhavhd can you please help with review?

@liat-grozovik Yes we should, thanks.

@@ -670,6 +671,18 @@ then
systemctl stop "$service_name"
fi

if [[ "$REBOOT_TYPE" = "fast-reboot" ]]; then
CONFIG_DB_FILE=/etc/sonic/config_db.json
Copy link
Contributor

Choose a reason for hiding this comment

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

The CONFIG_DB_FILE constant can be moved to the top of this file. So that it can be reused at other places. There is an existing usage here:
https://github.com/Azure/sonic-utilities/blob/master/scripts/fast-reboot#L516

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, can you check please?

if [[ COUNTERPOLL_DELAY_RC -ne 0 ]]; then
error "Failed to delay counterpoll. Exit code: $COUNTERPOLL_DELAY_RC"
unload_kernel
exit "${EXIT_COUNTERPOLL_DELAY_FAILURE}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think exiting here is incorrect. At this point all the critical services are killed/stopped, and we have committed to reboot (no matter what):
https://github.com/Azure/sonic-utilities/blob/6fd06755ab3762888cb26512c5aebccbcf3cbcf9/scripts/fast-reboot#L557

Moreover, exiting here would leave DUT in unusable state, as the trap handlers are also removed and no restoration will be performed:
https://github.com/Azure/sonic-utilities/blob/6fd06755ab3762888cb26512c5aebccbcf3cbcf9/scripts/fast-reboot#L561

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this whole logic should move above the set +e, so that if this indeed fails, we have a way to recover DUT from there, and not have any impact.

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, can you check please?

…re occur.

Declare config_db.json file location
@qiluo-msft qiluo-msft merged commit 8d16eb5 into sonic-net:master Sep 2, 2021
judyjoseph pushed a commit that referenced this pull request Sep 2, 2021
…ers enablement after fast-reboot (#1768)

#### What I did
Set flex counters delay indicator to prevent flex counters enablement after fast-reboot.

#### How I did it
Modify config DB json file with 'true' status for delay of flex counters indicator.

#### How to verify it
Run fast-reboot and observe counters are created only when enable_counters script is called, even if the tables are present in config DB.
@shlomibitton shlomibitton deleted the shlomi_fast_reboot_indication_master branch September 5, 2021 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants