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

Warm reboot: Don't load json conifg like copp, ininip, ports and switch again upon… #1939

Merged

Conversation

jipanyang
Copy link
Collaborator

… swss warm start

Signed-off-by: Jipan Yang jipan.yang@alibaba-inc.com

- What I did
Some of the configurations in copp, ininip, ports and switch are create only, they should be applied once only.
For swss warm start, same configurations already exist in appDB and have been restored.

- How I did it
Skip swss json load upon warm start.

- How to verify it
cold start,
enable warm start, start again.

- Description for the changelog

… swss warm start

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>
if [ "$WARM_START" == "true" ]; then
exit 0
fi

Copy link
Collaborator

Choose a reason for hiding this comment

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

what if we have modified any of the json?

Copy link
Collaborator Author

@jipanyang jipanyang Aug 16, 2018

Choose a reason for hiding this comment

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

This is a good question. At restore phase, you get whatever the previous configurations are.

Should anything be changed for any configuration in the json files, probably there should be specific script, or program implemented for the delta update. It is similar to configDB data change.

I doubt this script is the right place to deal with each possible change. Maybe the long term solution is to eliminate these json files and make them part of the configDB.

Copy link
Collaborator

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

need to address the json change scenario.

@wendani
Copy link
Contributor

wendani commented Aug 17, 2018

retest this please

@jipanyang
Copy link
Collaborator Author

Related to issues:
sonic-net/sonic-swss#582
sonic-net/sonic-swss#583

…red at system reboot

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>
@jipanyang jipanyang changed the title Don't load json conifg like copp, ininip, ports and switch again upon… Warm reboot: Don't load json conifg like copp, ininip, ports and switch again upon… Aug 21, 2018
exit 0
fi

WARM_START=`redis-cli -n 4 hget "WARM_RESTART|swss" enable`
Copy link
Collaborator

Choose a reason for hiding this comment

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

WARM_RESTART|swss [](start = 33, length = 17)

In the scope of this PR, what is the use case for

  1. WARM_RESTART|system == false
  2. WARM_RESTART|swss == true

Intuitively, this module should care only the swss one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are two scenarios:
<1> Individual SWSS docker restart.
either system == true or swss == true, warm start will be performed.

<2> system restart.
It is to be supported. But we may hit issue when system == false, swss == true. data would have been flushed at system init. SWSS docker has to perform extra check to make sure data exits for state restore.

if [ "$WARM_START" == "true" ]; then
# We have to make sure db data has not been flushed.
WARM_START=`redis-cli -n 6 hget "WARM_RESTART_TABLE|orchagent" restart_count`
if [ -n "$WARM_START" ] && [ "$WARM_START" != "0" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

[](start = 2, length = 2)

This indentation is not necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok. will update it

WARM_START=`redis-cli -n 6 hget "WARM_RESTART_TABLE|orchagent" restart_count`
if [ -n "$WARM_START" ] && [ "$WARM_START" != "0" ]; then
exit 0
fi
Copy link
Collaborator

@qiluo-msft qiluo-msft Aug 21, 2018

Choose a reason for hiding this comment

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

fi [](start = 4, length = 2)

else, should we treat it as 'logic error' and return error code immediately? Your code fallbacks to cold start, which may hide bugs in other components. #Closed

Copy link
Collaborator Author

@jipanyang jipanyang Aug 21, 2018

Choose a reason for hiding this comment

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

This is to handle the case of system cold boot while swss docker warm enabled.
swss docker warm restart knob is only supposed to handle the scenario of individual docker warm restart. #Resolved

WARM_START=`redis-cli -n 4 hget "WARM_RESTART|swss" enable`
if [ "$WARM_START" == "true" ]; then
# We have to make sure db data has not been flushed.
WARM_START=`redis-cli -n 6 hget "WARM_RESTART_TABLE|orchagent" restart_count`
Copy link
Collaborator

Choose a reason for hiding this comment

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

WARM_START [](start = 2, length = 10)

Don't reuse variable names, especially in embedded scopes.

…at update

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>
@lguohan lguohan merged commit 3f049d3 into sonic-net:master Sep 7, 2018
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.

5 participants