Fix VLAN member tagging_mode updates for existing members

cfgmgr/vlanmgr.cpp

@@ -245,8 +245,11 @@ bool VlanMgr::addHostVlanMember(int vlan_id, const string &port_alias, const str
     //               /sbin/bridge vlan del vid 1 dev {{ port_alias }} &&
     //               /sbin/bridge vlan add vid {{vlan_id}} dev {{port_alias}} {{tagging_mode}}"
     ostringstream cmds, inner;
+    // The default vid removal is best-effort: on a tagging_mode change of an existing
+    // member this runs a second time when vid 1 is already gone, and a hard failure here
+    // would throw before APPL_DB is updated, dropping the mode change end-to-end.
     inner << IP_CMD " link set " << shellquote(port_alias) << " master " DOT1Q_BRIDGE_NAME " && "
-      BRIDGE_CMD " vlan del vid " DEFAULT_VLAN_ID " dev " << shellquote(port_alias) << " && "
+      "( " BRIDGE_CMD " vlan del vid " DEFAULT_VLAN_ID " dev " << shellquote(port_alias) << " || true ) && "
       BRIDGE_CMD " vlan add vid " + std::to_string(vlan_id) + " dev " << shellquote(port_alias) << " " + tagging_cmd;
     cmds << BASH_CMD " -c " << shellquote(inner.str());
 
@@ -542,6 +545,24 @@ bool VlanMgr::isVlanMemberStateOk(const string &vlanMemberKey)
     return false;
 }
 
+// Checks STATE_DB so a previously applied tagging_mode is still known right after a vlanmgrd restart
+bool VlanMgr::isVlanMemberModeApplied(const string &vlanMemberKey, const string &tagging_mode)
+{
+    vector<FieldValueTuple> temp;
+
+    if (m_stateVlanMemberTable.get(vlanMemberKey, temp))
+    {
+        for (const auto &fv : temp)
+        {
+            if (fvField(fv) == "tagging_mode" && fvValue(fv) == tagging_mode)
+            {
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
 /*
  * members is grouped in format like
  * "Ethernet1,Ethernet2,Ethernet3,Ethernet4,Ethernet5,Ethernet6,
@@ -630,21 +651,6 @@ void VlanMgr::doVlanMemberTask(Consumer &consumer)
        // TODO:  store port/lag/VLAN data in local data structure and perform more validations.
         if (op == SET_COMMAND)
         {
-             if (isVlanMemberStateOk(kfvKey(t)))
-             {
-                SWSS_LOG_DEBUG("%s already set", kfvKey(t).c_str());
-                m_vlanMemberReplay.erase(kfvKey(t));
-                it = consumer.m_toSync.erase(it);
-                continue;
-             }
-
-            /* Don't proceed if member port/lag is not ready yet */
-            if (!isMemberStateOk(port_alias) || !isVlanStateOk(vlan_alias))
-            {
-                SWSS_LOG_DEBUG("%s not ready, delaying", kfvKey(t).c_str());
-                it++;
-                continue;
-            }
             string tagging_mode = "untagged";
 
             for (auto i : kfvFieldsValues(t))
@@ -664,6 +670,23 @@ void VlanMgr::doVlanMemberTask(Consumer &consumer)
                 continue;
             }
 
+            // Skip only duplicates. Checked against STATE_DB, not the in-memory m_PortVlanMember cache
+            if (isVlanMemberModeApplied(kfvKey(t), tagging_mode))
+            {
+                SWSS_LOG_DEBUG("%s already set", kfvKey(t).c_str());
+                m_vlanMemberReplay.erase(kfvKey(t));
+                it = consumer.m_toSync.erase(it);
+                continue;
+            }
+
+            /* Don't proceed if member port/lag is not ready yet */
+            if (!isMemberStateOk(port_alias) || !isVlanStateOk(vlan_alias))
+            {
+                SWSS_LOG_DEBUG("%s not ready, delaying", kfvKey(t).c_str());
+                it++;
+                continue;
+            }
+
             if (addHostVlanMember(vlan_id, port_alias, tagging_mode))
             {
                 key = VLAN_PREFIX + to_string(vlan_id);
@@ -674,6 +697,8 @@ void VlanMgr::doVlanMemberTask(Consumer &consumer)
                 vector<FieldValueTuple> fvVector;
                 FieldValueTuple s("state", "ok");
                 fvVector.push_back(s);
+                FieldValueTuple tm("tagging_mode", tagging_mode);
+                fvVector.push_back(tm);
                 m_stateVlanMemberTable.set(kfvKey(t), fvVector);
 
                 m_vlanMemberReplay.erase(kfvKey(t));
@@ -891,6 +916,8 @@ void VlanMgr::doVlanPacVlanMemberTask(Consumer &consumer)
                 vector<FieldValueTuple> fvVector1;
                 FieldValueTuple s1("state", "ok");
                 fvVector.push_back(s1);
+                FieldValueTuple tm("tagging_mode", tagging_mode);
+                fvVector.push_back(tm);
                 m_stateVlanMemberTable.set(kfvKey(t), fvVector);
             }
         }
@@ -944,6 +971,8 @@ void VlanMgr::addPortToVlan(const std::string& membername, const std::string& vl
         vector<FieldValueTuple> fvVector1;
         FieldValueTuple s2("state", "ok");
         fvVector1.push_back(s2);
+        FieldValueTuple s3("tagging_mode", tagging_mode);
+        fvVector1.push_back(s3);
         key = VLAN_PREFIX + to_string(vlan_id);
         key += '|';
         key += membername;

cfgmgr/vlanmgr.h

@@ -46,6 +46,7 @@ class VlanMgr : public Orch
     bool isVlanStateOk(const std::string &alias);
     bool isVlanMacOk();
     bool isVlanMemberStateOk(const std::string &vlanMemberKey);
+    bool isVlanMemberModeApplied(const std::string &vlanMemberKey, const std::string &tagging_mode);
     void doVlanPacPortTask(Consumer &consumer);
     void doVlanPacFdbTask(Consumer &consumer);
     void doVlanPacVlanMemberTask(Consumer &consumer);

orchagent/portsorch.cpp

@@ -6075,10 +6075,13 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer)
                 continue;
             }
 
-            /* Duplicate entry */
+            /* Existing member: apply tagging_mode change in place, or no-op if unchanged */
             if (vlan.m_members.find(port_alias) != vlan.m_members.end())
             {
-                it = consumer.m_toSync.erase(it);
+                if (setVlanMemberTaggingMode(vlan, port, tagging_mode))
+                    it = consumer.m_toSync.erase(it);
+                else
+                    it++;
                 continue;
             }
 
@@ -7692,6 +7695,18 @@ bool PortsOrch::getVlanByVlanId(sai_vlan_id_t vlan_id, Port &vlan)
     return false;
 }
 
+static sai_vlan_tagging_mode_t taggingModeStrToSai(const string &tagging_mode)
+{
+    if (tagging_mode == "untagged")
+        return SAI_VLAN_TAGGING_MODE_UNTAGGED;
+    else if (tagging_mode == "tagged")
+        return SAI_VLAN_TAGGING_MODE_TAGGED;
+    else if (tagging_mode == "priority_tagged")
+        return SAI_VLAN_TAGGING_MODE_PRIORITY_TAGGED;
+    assert(false);
+    return SAI_VLAN_TAGGING_MODE_TAGGED;
+}
+
 bool PortsOrch::addVlanMember(Port &vlan, Port &port, string &tagging_mode, string end_point_ip)
 {
     SWSS_LOG_ENTER();
@@ -7721,15 +7736,8 @@ bool PortsOrch::addVlanMember(Port &vlan, Port &port, string &tagging_mode, stri
     attrs.push_back(attr);
 
 
-    sai_vlan_tagging_mode_t sai_tagging_mode = SAI_VLAN_TAGGING_MODE_TAGGED;
+    sai_vlan_tagging_mode_t sai_tagging_mode = taggingModeStrToSai(tagging_mode);
     attr.id = SAI_VLAN_MEMBER_ATTR_VLAN_TAGGING_MODE;
-    if (tagging_mode == "untagged")
-        sai_tagging_mode = SAI_VLAN_TAGGING_MODE_UNTAGGED;
-    else if (tagging_mode == "tagged")
-        sai_tagging_mode = SAI_VLAN_TAGGING_MODE_TAGGED;
-    else if (tagging_mode == "priority_tagged")
-        sai_tagging_mode = SAI_VLAN_TAGGING_MODE_PRIORITY_TAGGED;
-    else assert(false);
     attr.value.s32 = sai_tagging_mode;
     attrs.push_back(attr);
 
@@ -7780,6 +7788,59 @@ bool PortsOrch::addVlanMember(Port &vlan, Port &port, string &tagging_mode, stri
     return true;
 }
 
+/* Update the tagging mode of a port that is already a member of the VLAN */
+bool PortsOrch::setVlanMemberTaggingMode(Port &vlan, Port &port, const string &tagging_mode)
+{
+    SWSS_LOG_ENTER();
+
+    auto vlan_member = m_portVlanMember[port.m_alias].find(vlan.m_vlan_info.vlan_id);
+    if (vlan_member == m_portVlanMember[port.m_alias].end())
+    {
+        /* Clear stale vlan.m_members state so retries take the addVlanMember path safely. */
+        SWSS_LOG_ERROR("Member %s missing from tracked VLAN members for VLAN %s vid:%hu, "
+                "treating as removed and retrying as a new add with tagging mode %s",
+                port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id, tagging_mode.c_str());
+        vlan.m_members.erase(port.m_alias);
+        m_portList[vlan.m_alias] = vlan;
+        return false;
+    }
+
+    sai_vlan_tagging_mode_t old_mode = vlan_member->second.vlan_mode;
+    sai_vlan_tagging_mode_t new_mode = taggingModeStrToSai(tagging_mode);
+
+    if (old_mode == new_mode)
+    {
+        /* True duplicate, tagging_mode did not change */
+        return true;
+    }
+
+    /* An in-place SAI_VLAN_MEMBER_ATTR_VLAN_TAGGING_MODE SET was tried first, but the
+     * Broadcom DNX vendor SAI does not support changing tagging mode on an existing
+     * VLAN member (returns SAI_STATUS_NOT_IMPLEMENTED). Remove+recreate is the fallback,
+     * which is traffic-hitful for this port (see PR description for details). */
+    if (!removeVlanMember(vlan, port))
+    {
+        SWSS_LOG_ERROR("Failed to remove member %s from VLAN %s vid:%hu while updating tagging mode to %s",
+                port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id, tagging_mode.c_str());
+        return false;
+    }
+
+    /* The bridge port carries no tagging-mode attribute and survives the member removal,
+     * so it is reused as-is (addBridgePort is a no-op when the bridge port already exists). */
+    string new_tagging_mode = tagging_mode;
+    if (!addBridgePort(port) || !addVlanMember(vlan, port, new_tagging_mode))
+    {
+        SWSS_LOG_ERROR("Failed to re-add member %s to VLAN %s vid:%hu with tagging mode %s",
+                port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id, tagging_mode.c_str());
+        return false;
+    }
+
+    SWSS_LOG_NOTICE("Updated member %s tagging mode to %s in VLAN %s vid:%hu",
+            port.m_alias.c_str(), tagging_mode.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id);
+
+    return true;
+}
+
 bool PortsOrch::getPortVlanMembers(Port &port, vlan_members_t &vlan_members)
 {
     vlan_members = m_portVlanMember[port.m_alias];

orchagent/portsorch.h

@@ -269,6 +269,7 @@ class PortsOrch : public Orch, public Subject
     bool removeBridgePort(Port &port);
     bool addVlanMember(Port &vlan, Port &port, string& tagging_mode, string end_point_ip = "");
     bool removeVlanMember(Port &vlan, Port &port, string end_point_ip = "");
+    bool setVlanMemberTaggingMode(Port &vlan, Port &port, const string& tagging_mode);
     bool isVlanMember(Port &vlan, Port &port, string end_point_ip = "");
     bool addVlanFloodGroups(Port &vlan, Port &port, string end_point_ip);
     bool removeVlanEndPointIp(Port &vlan, Port &port, string end_point_ip);

tests/dvslib/dvs_vlan.py

@@ -51,6 +51,12 @@ def remove_vlan_member(self, vlanID, interface):
         member = "Vlan{}|{}".format(vlanID, interface)
         self.config_db.delete_entry("VLAN_MEMBER", member)
 
+    def update_vlan_member_tagging_mode(self, vlanID, interface, tagging_mode):
+        member = "Vlan{}|{}".format(vlanID, interface)
+        member_entry = self.config_db.get_entry("VLAN_MEMBER", member)
+        member_entry["tagging_mode"] = tagging_mode
+        self.config_db.update_entry("VLAN_MEMBER", member, member_entry)
+
     def remove_vlan_interface(self, vlanID):
         vlan = "Vlan{}".format(vlanID)
         self.config_db.delete_entry("VLAN_INTERFACE", vlan)
@@ -82,11 +88,20 @@ def get_and_verify_vlan_ids(self,
         return [v for v in vlan_entries if v != self.asic_db.default_vlan_id]
 
     def verify_vlan_member(self, vlan_oid, iface, tagging_mode="SAI_VLAN_TAGGING_MODE_UNTAGGED"):
-        member_ids = self.asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_VLAN_MEMBER", 1)
-        member = self.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_VLAN_MEMBER", member_ids[0])
-        assert member == {"SAI_VLAN_MEMBER_ATTR_VLAN_TAGGING_MODE": tagging_mode,
-                          "SAI_VLAN_MEMBER_ATTR_VLAN_ID": vlan_oid,
-                          "SAI_VLAN_MEMBER_ATTR_BRIDGE_PORT_ID": self.get_bridge_port_id(iface)}
+        expected = {"SAI_VLAN_MEMBER_ATTR_VLAN_TAGGING_MODE": tagging_mode,
+                    "SAI_VLAN_MEMBER_ATTR_VLAN_ID": vlan_oid,
+                    "SAI_VLAN_MEMBER_ATTR_BRIDGE_PORT_ID": self.get_bridge_port_id(iface)}
+
+        # Re-resolve the VLAN member OID while polling, since tagging_mode updates replace the object.
+        def access_function():
+            member_ids = self.asic_db.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_VLAN_MEMBER")
+            if len(member_ids) != 1:
+                return (False, {})
+            member = self.asic_db.get_entry("ASIC_STATE:SAI_OBJECT_TYPE_VLAN_MEMBER", member_ids[0])
+            return (member == expected, member)
+
+        status, member = wait_for_result(access_function, PollingConfig(strict=False))
+        assert status, "expected={}, received={}".format(expected, member)
 
     def get_and_verify_vlan_member_ids(self, expected_num):
         return self.asic_db.wait_for_n_keys("ASIC_STATE:SAI_OBJECT_TYPE_VLAN_MEMBER", expected_num)

tests/mock_tests/Makefile.am

@@ -9,9 +9,9 @@ CXXFLAGS = -g -O0
 
 CFLAGS_SAI = -I /usr/include/sai
 
-TESTS = tests tests_intfmgrd tests_teammgrd tests_portsyncd tests_fpmsyncd tests_fdbsyncd tests_response_publisher tests_nbrmgrd tests_teamsyncd
+TESTS = tests tests_intfmgrd tests_teammgrd tests_portsyncd tests_fpmsyncd tests_fdbsyncd tests_response_publisher tests_nbrmgrd tests_teamsyncd tests_vlanmgrd
 
-noinst_PROGRAMS = tests tests_intfmgrd tests_teammgrd tests_portsyncd tests_fpmsyncd tests_fdbsyncd tests_response_publisher tests_nbrmgrd tests_teamsyncd
+noinst_PROGRAMS = tests tests_intfmgrd tests_teammgrd tests_portsyncd tests_fpmsyncd tests_fdbsyncd tests_response_publisher tests_nbrmgrd tests_teamsyncd tests_vlanmgrd
 
 LDADD_SAI = -lsaivs -lsairedis -lsaimeta -lsaimetadata
 
@@ -283,6 +283,26 @@ tests_intfmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTE
 tests_intfmgrd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis \
         -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread -lgmock -lgmock_main
 
+## vlanmgrd unit tests
+
+tests_vlanmgrd_SOURCES = vlanmgrd/vlanmgr_switch_ut.cpp \
+                         $(top_srcdir)/cfgmgr/vlanmgr.cpp \
+                         $(top_srcdir)/lib/recorder.cpp \
+                         $(top_srcdir)/orchagent/orch.cpp \
+                         $(top_srcdir)/orchagent/request_parser.cpp \
+                         mock_dbconnector.cpp \
+                         mock_table.cpp \
+                         mock_hiredis.cpp \
+                         fake_response_publisher.cpp \
+                         mock_redisreply.cpp \
+                         common/mock_shell_command.cpp
+
+tests_vlanmgrd_INCLUDES = -I$(top_srcdir)/cfgmgr -I$(top_srcdir)/lib -I$(top_srcdir)/orchagent -I $(FLEX_CTR_DIR) -I $(DEBUG_CTR_DIR) -I$(P4_ORCH_DIR)/tests
+tests_vlanmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI)
+tests_vlanmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) $(tests_vlanmgrd_INCLUDES)
+tests_vlanmgrd_LDADD = $(LDADD_GTEST) -lnl-genl-3 -lhiredis -lhiredis \
+        -lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread -lgmock -lgmock_main
+
 ## teammgrd unit tests
 
 tests_teammgrd_SOURCES = teammgrd/teammgr_ut.cpp \