Skip to content

ICCP Bridgeport removal failure when MCLAG interface Bridgeport exist #2535

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

Open
prasanna-cls wants to merge 3 commits into
base: master
Choose a base branch
from
Open

ICCP Bridgeport removal failure when MCLAG interface Bridgeport exist #2535

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ac6b35
In the MCLAG scenario, if the ICCP peer link bridge port is being del…
prasanna-cls Nov 18, 2022
e84c562
updated comments.
prasanna-cls Nov 22, 2022
852f32f
comments.
prasanna-cls Dec 2, 2022
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
7 changes: 4 additions & 3 deletions orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4984,6 +4984,10 @@ bool PortsOrch::removeBridgePort(Port &port)
gFdbOrch->flushFDBEntries(port.m_bridge_port_id, SAI_NULL_OBJECT_ID);
SWSS_LOG_INFO("Flush FDB entries for port %s", port.m_alias.c_str());

/* Notify about bridgeport deletion */
Copy link
Copy Markdown
Collaborator

@prsunny prsunny Dec 6, 2022

Choose a reason for hiding this comment

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

This could cause race-conditions. The idea of notifying other subscribers is to let them know BridgePort deletion is complete. This flow doesn't honor that. Please propose a different solution.

Copy link
Copy Markdown
Contributor Author

@prasanna-cls prasanna-cls Dec 7, 2022

Choose a reason for hiding this comment

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

Hi,
From SAI point of view, the ISOLATION_GROUP_TABLE:MCLAG_ISO_GRP object depends upon the bridge port as a dependent object.
For this reason, when SWSS creates a bridge port, MCLAG is notified after the bridge port is created in SAI.
Similarly, during the bridge port deletion scenario, MCLAG has to be notified (to clean-up ISOLATION_GROUP) , before SAI deletes that bridge port.

And the MCLAG(for ISOLATION_GROUP_TABLE) is the only subscriber listening to this bridge port changes. and this changes are done as part of handling MCLAG in this pull request.

do you suggest to have a different message type, something like "SUBJECT_TYPE_BRIDGE_PORT_REMOVAL_HANDLE" that can be called before sai_bridge_api->remove_bridge_port() , instead of "SUBJECT_TYPE_BRIDGE_PORT_CHANGE", which could be retained to be called after SAI brdige port remove ?

Copy link
Copy Markdown

@skannan-sonic skannan-sonic Jan 7, 2023

Choose a reason for hiding this comment

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

Hi @prsunny, @prasanna-cls, can we fix this issue as follows

This way there is no change in notification and Isolation group also removed.

diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp [9/1964]
index 376328f..804f356 100755
--- a/orchagent/portsorch.cpp
+++ b/orchagent/portsorch.cpp
@@ -4,6 +4,7 @@
#include "neighorch.h"
#include "gearboxutils.h"
#include "vxlanorch.h"
+#include "isolationgrouporch.h"
#include "directory.h"
#include "subintf.h"

@@ -49,6 +50,7 @@ extern NeighOrch *gNeighOrch;
extern CrmOrch *gCrmOrch;
extern BufferOrch *gBufferOrch;
extern FdbOrch *gFdbOrch;
+extern IsoGrpOrch gIsoGrpOrch;
extern Directory<Orch> gDirectory;
extern sai_system_port_api_t *sai_system_port_api;
extern string gMySwitchType;
@@ -4984,6 +4986,9 @@ bool PortsOrch::removeBridgePort(Port &port)
gFdbOrch->flushFDBEntries(port.m_bridge_port_id, SAI_NULL_OBJECT_ID);
SWSS_LOG_INFO("Flush FDB entries for port %s", port.m_alias.c_str());

  • PortUpdate update = { port, false };

  • gIsoGrpOrch->update (SUBJECT_TYPE_BRIDGE_PORT_CHANGE, static_cast<void *>(&update));

  • /* Remove bridge port */
    status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id);
    if (status != SAI_STATUS_SUCCESS)
    @@ -5000,7 +5005,6 @@ bool PortsOrch::removeBridgePort(Port &port)
    port.m_bridge_port_id = SAI_NULL_OBJECT_ID;

    /* Remove bridge port */

  • PortUpdate update = { port, false };
    notify(SUBJECT_TYPE_BRIDGE_PORT_CHANGE, static_cast<void *>(&update));

    SWSS_LOG_NOTICE("Remove bridge port %s from default 1Q bridge", port.m_alias.c_str());

Copy link
Copy Markdown
Contributor Author

@prasanna-cls prasanna-cls Jan 12, 2023

Choose a reason for hiding this comment

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

Yes @kselvaraj2 , this looks ok to me. @prsunny is this approach fine ?

Copy link
Copy Markdown
Contributor Author

@prasanna-cls prasanna-cls Jan 12, 2023

Choose a reason for hiding this comment

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

@prsunny can I update the review with the approach suggested by @kselvaraj2 ?

PortUpdate update = { port, false };
notify(SUBJECT_TYPE_BRIDGE_PORT_CHANGE, static_cast<void *>(&update));

/* Remove bridge port */
status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id);
if (status != SAI_STATUS_SUCCESS)
Expand All @@ -4999,9 +5003,6 @@ bool PortsOrch::removeBridgePort(Port &port)
saiOidToAlias.erase(port.m_bridge_port_id);
port.m_bridge_port_id = SAI_NULL_OBJECT_ID;

/* Remove bridge port */
PortUpdate update = { port, false };
notify(SUBJECT_TYPE_BRIDGE_PORT_CHANGE, static_cast<void *>(&update));

SWSS_LOG_NOTICE("Remove bridge port %s from default 1Q bridge", port.m_alias.c_str());

Expand Down