Skip to content

Remove system neigh DEL operation if SET operation succeeds #2853

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

Merged
prsunny merged 1 commit into from
Jul 18, 2023
Merged

Remove system neigh DEL operation if SET operation succeeds #2853

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions orchagent/neighorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1420,6 +1420,17 @@ void NeighOrch::doVoqSystemNeighTask(Consumer &consumer)
{
SWSS_LOG_NOTICE("VOQ encap index updated for neighbor %s", kfvKey(t).c_str());
it = consumer.m_toSync.erase(it);

/* Remove remaining DEL operation in m_toSync for the same neighbor.
* Since DEL operation is supposed to be executed before SET for the same neighbor
* A remaining DEL after the SET operation means the DEL operation failed previously and should not be executed anymore
*/
auto rit = make_reverse_iterator(it);
Copy link
Copy Markdown
Contributor

@lguohan lguohan Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

sigh, we should have tab space aligned. @arlakshm , @ysmanman . I feel being insulted. @prsunny

Copy link
Copy Markdown
Contributor Author

@ysmanman ysmanman Jul 21, 2023

Choose a reason for hiding this comment

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

sigh, we should have tab space aligned. @arlakshm , @ysmanman . I feel being insulted. @prsunny

I can raise a PR to fix the indentation.

while (rit != consumer.m_toSync.rend() && rit->first == key && kfvOp(rit->second) == DEL_COMMAND)
{
Copy link
Copy Markdown
Contributor

@lguohan lguohan Jul 20, 2023

Choose a reason for hiding this comment

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

i thought m_toSync will only have one entry for each neighbor, i didn't know if we have multiple entry for the same neighbor, just curious.

Copy link
Copy Markdown
Contributor

@lguohan lguohan Jul 20, 2023

Choose a reason for hiding this comment

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

why this problem is voq neighbor specificially? for normal neighbor, why we do not have such problem?

Copy link
Copy Markdown
Contributor Author

@ysmanman ysmanman Jul 21, 2023

Choose a reason for hiding this comment

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

i thought m_toSync will only have one entry for each neighbor, i didn't know if we have multiple entry for the same neighbor, just curious.

m_toSync is type of std::multimap, so it allows multiple actions (like DEL and SET) for the same neighbor.

Copy link
Copy Markdown
Contributor Author

@ysmanman ysmanman Jul 21, 2023

Choose a reason for hiding this comment

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

why this problem is voq neighbor specificially? for normal neighbor, why we do not have such problem?

This problem is not voq specific. The same change/fix exists for local neighbor too. The PR is to apply the same fix for voq.

consumer.m_toSync.erase(next(rit).base());
SWSS_LOG_NOTICE("Removed pending system neighbor DEL operation for %s after SET operation", key.c_str());
}
}
continue;
}
Expand Down Expand Up @@ -1481,6 +1492,7 @@ void NeighOrch::doVoqSystemNeighTask(Consumer &consumer)
else
{
it++;
continue;
}
}
else
Expand All @@ -1489,6 +1501,17 @@ void NeighOrch::doVoqSystemNeighTask(Consumer &consumer)
SWSS_LOG_INFO("System neighbor %s already exists", kfvKey(t).c_str());
it = consumer.m_toSync.erase(it);
}

/* Remove remaining DEL operation in m_toSync for the same neighbor.
* Since DEL operation is supposed to be executed before SET for the same neighbor
* A remaining DEL after the SET operation means the DEL operation failed previously and should not be executed anymore
*/
auto rit = make_reverse_iterator(it);
while (rit != consumer.m_toSync.rend() && rit->first == key && kfvOp(rit->second) == DEL_COMMAND)
{
consumer.m_toSync.erase(next(rit).base());
SWSS_LOG_NOTICE("Removed pending system neighbor DEL operation for %s after SET operation", key.c_str());
}
}
else if (op == DEL_COMMAND)
{
Expand Down