Skip to content

Revert PR#718 and PR#727#744

Closed
yxieca wants to merge 2 commits intosonic-net:masterfrom
yxieca:revert
Closed

Revert PR#718 and PR#727#744
yxieca wants to merge 2 commits intosonic-net:masterfrom
yxieca:revert

Conversation

@yxieca
Copy link
Copy Markdown
Contributor

@yxieca yxieca commented Jan 10, 2019

What I did
Reverting PR #718 and PR #727.

Why I did it
These 2 changes caused VLAN interface becoming ready for about 20 seconds later. This delay caused fast-reboot to fail, and triggered a failure in warm-reboot test.

How I verified it
After reverting the change, fast-reboot test is passing. And warm-reboot test don't see data plane disruption anymore.

@jipanyang
Copy link
Copy Markdown
Contributor

What is the root cause of ready delay for VLAN interface?

@yxieca
Copy link
Copy Markdown
Contributor Author

yxieca commented Jan 11, 2019

Not sure. I only isolated to these 2 changes. Didn't have time to look deeper yet.

@stcheng
Copy link
Copy Markdown
Contributor

stcheng commented Jan 11, 2019

If this PR #748 is merged, can we avoid reverting?

@stepanblyschak
Copy link
Copy Markdown
Contributor

@stcheng Actually @yxieca tried #748 but still observed issues with fast/warm reboot. So we agreed to revert this as a quick fix for fast/warm reboot. However, #748 still is an independent fix

sai_object_id_t m_egress_acl_table_group_id = 0;
vlan_members_t m_vlan_members;
sai_port_oper_status_t m_oper_status = SAI_PORT_OPER_STATUS_UNKNOWN;
sai_port_oper_status_t m_oper_status;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This brings back undefined behavior, e.g:
For LAG, VLAN m_oper_status field is never set, so there is a chance that NeighOrch will invalidate neighbor on LAG or VLAN in next hop group in case m_oper_status equals SAI_PORT_OPER_STATUS_DOWN.

@yxieca yxieca closed this Jan 16, 2019
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023
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.

4 participants