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.

Sign up for GitHub

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

Jump to bottom

[mellanox|ffb] use system level warm reboot for Mellanox fastfast boot #400

Merged
merged 6 commits into from
Jan 7, 2019
Merged

[mellanox|ffb] use system level warm reboot for Mellanox fastfast boot #400

merged 6 commits into from
Jan 7, 2019

Conversation

stepanblyschak
Copy link
Contributor

@stepanblyschak stepanblyschak commented Dec 11, 2018 •
edited
Loading

use common warm boot flow for mellanox fastfast boot.

Signed-off-by: Stepan Blyschak [email protected]

@stepanblyschak stepanblyschak mentioned this pull request Dec 11, 2018
Merged
@qiluo-msft qiluo-msft requested a review from kcudnik December 11, 2018 19:35
qiluo-msft
qiluo-msft reviewed Dec 11, 2018
View reviewed changes
syncd/syncd.cpp Outdated
@@ -3683,56 +3679,77 @@ int syncd_main(int argc, char **argv)
FlexCounter::removeAllCounters();
stopNotificationsProcessingThread();

sai_attribute_t attr;
/* FIXME: Temprary W/A */
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Temprary [](start = 26, length = 8)

Typo #Closed

qiluo-msft
qiluo-msft reviewed Dec 11, 2018
View reviewed changes
syncd/syncd.cpp Outdated
sai_attribute_t attr;
/* FIXME: Temprary W/A */
char* platform = getenv("platform");
if (platform && (strncmp(platform, "mellanox", 8) == 0))
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

8 [](start = 63, length = 1)

refactor to remove the magic number #Closed

Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if this is good idea to get plafrom like that

qiluo-msft
qiluo-msft reviewed Dec 11, 2018
View reviewed changes
syncd/syncd.cpp Outdated

/* FIXME: W/A */
char* platform = getenv("platform");
if (platform && (strncmp(platform, "mellanox", 8) == 0))
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

8 [](start = 59, length = 1)

magic number #Closed

qiluo-msft
qiluo-msft reviewed Dec 11, 2018
View reviewed changes
syncd/syncd.cpp Outdated
@@ -3683,56 +3679,77 @@ int syncd_main(int argc, char **argv)
FlexCounter::removeAllCounters();
stopNotificationsProcessingThread();

sai_attribute_t attr;
/* FIXME: Temprary W/A */
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

W/A [](start = 35, length = 3)

Could you elaborate why this is a W/A, and what is your future plan? #Closed

qiluo-msft
qiluo-msft reviewed Dec 11, 2018
View reviewed changes
syncd/syncd.cpp Outdated
@@ -3771,6 +3788,15 @@ int syncd_main(int argc, char **argv)
{
SWSS_LOG_NOTICE("Warm Reboot requested, keeping data plane running");


/* FIXME: W/A */
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

FIXME: W/A [](start = 15, length = 10)

future plan? #Closed

qiluo-msft
qiluo-msft previously requested changes Dec 11, 2018
View reviewed changes
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comments

kcudnik
kcudnik reviewed Dec 12, 2018
View reviewed changes
syncd/syncd.cpp Outdated
@@ -1626,6 +1626,26 @@ void InspectAsic()
}
}

void mlnxFastFastConfigDone()
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we have platform specific code in syncd ?

kcudnik
kcudnik reviewed Dec 12, 2018
View reviewed changes
syncd/syncd.cpp Outdated
* TODO: Mellanox SAI should perform issu start on
* SAI_SWITCH_ATTR_PRE_SHUTDOWN
*/
int rc = system("/usr/bin/issu --start");
Copy link
Collaborator

Choose a reason for hiding this comment

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

why call some other binary here?
also this code looks similar to one below

if you want to use this please export entire warm section to separate method, since this is already too much code inside this while loop

qiluo-msft
qiluo-msft reviewed Dec 15, 2018
View reviewed changes
syncd/Makefile.am Outdated
@@ -37,6 +37,10 @@ syncd_CPPFLAGS += -DSAITHRIFT=yes
syncd_LDADD += -lrpcserver -lthrift
endif

if sonic_asic_platform_mellanox
syncd_CPPFLAGS += -DUNINIT_DATA_PLANE_ON_REMOVAL
Copy link
Contributor

Choose a reason for hiding this comment

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

UNINIT_DATA_PLANE_ON_REMOVAL [](start = 20, length = 28)

suggest macro name: SAI_SUPPORT_UNINIT_DATA_PLANE_ON_REMOVAL
to indicate this is a feature, not a specific action.

The UNINIT_DATA_PLANE_ON_REMOVAL may indicate that you want to destroy data plane on removal for Mellanox ASIC.

[mellanox|ffb] rename 'SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL' …
c19f585 
…-> 'SAI_SUPPORT_UNINIT_DATA_PLANE_ON_REMOVAL' to avoid overlapping with SAI attribute

Signed-off-by: Stepan Blyschak <[email protected]>
@liat-grozovik
Copy link
Collaborator

As all comments were related to code that is currently removed i see no reason why not to approve and merge.
@qiluo-msft and @kcudnik can you please review again and approve?

lguohan
lguohan approved these changes Jan 7, 2019
View reviewed changes
Copy link
Contributor

@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.

:shipit:

@lguohan lguohan dismissed qiluo-msft’s stale review January 7, 2019 18:00

issue addressed.

@lguohan lguohan merged commit 60f97c3 into sonic-net:master Jan 7, 2019
volodymyrsamotiy pushed a commit to volodymyrsamotiy/sonic-sairedis that referenced this pull request Jul 30, 2019
@stepanblyschak
[mellanox|ffb] use system level warm reboot for Mellanox fastfast boot (
445a608 
sonic-net#400)

* [mellanox|ffb] use system level warm reboot for Mellanox fastfast boot

Signed-off-by: Stepan Blyschak <[email protected]>

* [mellanox|ffb] remove platform check code

Signed-off-by: Stepan Blyschak <[email protected]>

* [mellanox|ffb] rename function to onApplyViewInFastFastBoot, check for error status

Signed-off-by: Stepan Blyschak <[email protected]>

* [mellanox|ffb] Set UNINIT_DATA_PLANE_ON_REMOVAL in warm shutdown case as well

Signed-off-by: Stepan Blyschak <[email protected]>

* [mellanox|ffb] build mellanox with -DSAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL

Signed-off-by: Stepan Blyschak <[email protected]>

* [mellanox|ffb] rename 'SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL' -> 'SAI_SUPPORT_UNINIT_DATA_PLANE_ON_REMOVAL' to avoid overlapping with SAI attribute

Signed-off-by: Stepan Blyschak <[email protected]>
pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
@stepanblyschak @lguohan
[mellanox|ffb] use system level warm reboot for Mellanox fastfast boot (
63f6a11 
sonic-net#400)

* [mellanox|ffb] use system level warm reboot for Mellanox fastfast boot

Signed-off-by: Stepan Blyschak <[email protected]>

* [mellanox|ffb] remove platform check code

Signed-off-by: Stepan Blyschak <[email protected]>

* [mellanox|ffb] rename function to onApplyViewInFastFastBoot, check for error status

Signed-off-by: Stepan Blyschak <[email protected]>

* [mellanox|ffb] Set UNINIT_DATA_PLANE_ON_REMOVAL in warm shutdown case as well

Signed-off-by: Stepan Blyschak <[email protected]>

* [mellanox|ffb] build mellanox with -DSAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL

Signed-off-by: Stepan Blyschak <[email protected]>

* [mellanox|ffb] rename 'SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL' -> 'SAI_SUPPORT_UNINIT_DATA_PLANE_ON_REMOVAL' to avoid overlapping with SAI attribute

Signed-off-by: Stepan Blyschak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiluo-msft qiluo-msft qiluo-msft left review comments

@lguohan lguohan lguohan approved these changes

@kcudnik kcudnik kcudnik approved these changes

Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stepanblyschak @liat-grozovik @lguohan @qiluo-msft @kcudnik @stcheng