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] fixes Azure/sonic-buildimage#4006 #4008

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dawnbeauty
Copy link

Fixes #4006

  1. [fast-reboot] save fast-reboot state into the db sonic-utilities#780, save fast-reboot state into the db when execute fast-reboot(aligned with Save fast-reboot state into the db #3741)
  2. (This PR)during the syncd service stop stage, sonic-buildimage/files/scripts/syncd.sh could detect the FAST mode by the saved state, and request syncd to trigger fast shutdown.

@yxieca
Copy link
Contributor

yxieca commented Jan 10, 2020

@dawnbeauty this DB entry is designed to be used on the fast-reboot boot up path. The purpose is to control how long do we treat the boot up as 'fast' recovery. This knob is not designed to trigger fast shutdown. The fast-reboot shutdown syncd just like code reboot.

Therefore, this change is not needed.

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.

This change is not needed.

@dawnbeauty
Copy link
Author

@yxieca Yeah, I could change the DB entry accordingly.
But why fast-reboot should not trigger syncd fast shutdown? If so, when should we trigger it?
According to syncd.cpp, warm-reboot would set shutdown type to be SYNCD_RESTART_TYPE_WARM, then the fast-reboot should not set shutdown type to be SYNCD_RESTART_TYPE_FAST ? That completely confuse me. Did I miss sth?

syncd.cpp

#ifdef SAI_SUPPORT_UNINIT_DATA_PLANE_ON_REMOVAL

    if (shutdownType == SYNCD_RESTART_TYPE_FAST || shutdownType == SYNCD_RESTART_TYPE_WARM)
    {
        SWSS_LOG_NOTICE("Fast/warm reboot requested, keeping data plane running");

        sai_attribute_t attr;

        attr.id = SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL;
        attr.value.booldata = false;

        status = sai_switch_api->set_switch_attribute(gSwitchId, &attr);

        if (status != SAI_STATUS_SUCCESS)
        {
            SWSS_LOG_ERROR("Failed to set SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL=false: %s",
                    sai_serialize_status(status).c_str());
        }
    }

#endif

@yxieca
Copy link
Contributor

yxieca commented Jan 17, 2020

If you look at the syncd service script stop() method here: https://github.com/Azure/sonic-buildimage/blob/master/files/scripts/syncd.sh#L154

The syncd stop only distinguish if it is warm shutdown or not.

For fast-reboot, all we need to do is to 'cold' shutdown syncd, which will remove switch, basically detach the ASIC from the CPU. And leave the ASIC forwarding as it is configured. That is all that we need to do for fast-reboot on the shutdown path.

@dawnbeauty
Copy link
Author

According to the comments of SAI_SWITCH_ATTR_UNINIT_DATA_PLANE_ON_REMOVAL (SAIv1.5.1):

/**
     * @brief Uninitialize data plane upon removal of switch object
     *
     * Typical use case for tear down of the host adapter, is to remove the switch ID,
     * which will stop all data and control plane, as leaving data plane open without
     * control can be a security risk.
     * However, on some scenarios, such as fast boot, host adapter would like to set
     * this value to false, call remove switch, and have the data plane still running.
     *
     * @type bool
     * @flags CREATE_AND_SET
     * @default true
     */

The default value of this attr is true. So I think we should explicitly set it to false when trigger fast-reboot to keep dataplane forwarding when remove switch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request incorrect shutdown type when fast reboot
2 participants