From a1bbfc0b45061ab2f91fdabfc755a51602f4563e Mon Sep 17 00:00:00 2001 From: Jipan Yang Date: Fri, 25 Aug 2017 10:23:11 -0700 Subject: [PATCH 1/3] VLAN trunk support: orchagent update. --- orchagent/fdborch.cpp | 2 +- orchagent/intfsorch.cpp | 5 +- orchagent/mirrororch.cpp | 2 +- orchagent/port.h | 22 ++- orchagent/portsorch.cpp | 398 ++++++++++++++++++++++++++++++++------- orchagent/portsorch.h | 15 +- orchagent/saihelper.cpp | 12 -- 7 files changed, 368 insertions(+), 88 deletions(-) diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp index 5ad21f1e4f2..69ee9b9e677 100644 --- a/orchagent/fdborch.cpp +++ b/orchagent/fdborch.cpp @@ -99,7 +99,7 @@ void FdbOrch::doTask(Consumer& consumer) FdbEntry entry; entry.mac = MacAddress(keys[1]); - entry.vlan = vlan.m_vlan_id; + entry.vlan = vlan.m_vlan_info.vlan_id; if (op == SET_COMMAND) { diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 7cf44ccd6d4..c326c7ee8ae 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -246,7 +246,7 @@ bool IntfsOrch::addRouterIntfs(Port &port) break; case Port::VLAN: attr.id = SAI_ROUTER_INTERFACE_ATTR_VLAN_ID; - attr.value.oid = port.m_vlan_oid; + attr.value.oid = port.m_vlan_info.vlan_oid; break; default: SWSS_LOG_ERROR("Unsupported port type: %d", port.m_type); @@ -287,7 +287,8 @@ bool IntfsOrch::removeRouterIntfs(Port &port) port.m_rif_id = 0; gPortsOrch->setPort(port.m_alias, port); - + /* Clean this port from default VLAN */ + gPortsOrch->removeDefaultVlanMembers(); SWSS_LOG_NOTICE("Remove router interface for port %s", port.m_alias.c_str()); return true; diff --git a/orchagent/mirrororch.cpp b/orchagent/mirrororch.cpp index 8547c4b41c7..8e8f063fbf6 100644 --- a/orchagent/mirrororch.cpp +++ b/orchagent/mirrororch.cpp @@ -327,7 +327,7 @@ bool MirrorOrch::getNeighborInfo(const string& name, MirrorEntry& session, const if (session.neighborInfo.port.m_type == Port::VLAN) { - session.neighborInfo.vlanId = session.neighborInfo.port.m_vlan_id; + session.neighborInfo.vlanId = session.neighborInfo.port.m_vlan_info.vlan_id; Port member; if (!m_fdbOrch->getPort(session.neighborInfo.mac, session.neighborInfo.vlanId, member)) diff --git a/orchagent/port.h b/orchagent/port.h index 04113654308..ca6b04c768a 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -8,11 +8,28 @@ extern "C" { #include #include #include +#include #define DEFAULT_PORT_VLAN_ID 1 namespace swss { +struct VlanMemberEntry +{ + sai_object_id_t vlan_member_id; + sai_vlan_tagging_mode_t vlan_mode; +}; + +typedef std::map port_vlan_members_t; + +struct VlanInfo +{ + sai_object_id_t vlan_oid; + sai_vlan_id_t vlan_id; + std::string autostate; + uint32_t mtu; +}; + class Port { public: @@ -54,11 +71,10 @@ class Port int m_index = 0; // PHY_PORT: index int m_ifindex = 0; sai_object_id_t m_port_id = 0; - sai_object_id_t m_vlan_oid = 0; + VlanInfo m_vlan_info; sai_object_id_t m_bridge_port_id = 0; // TODO: port could have multiple bridge port IDs - sai_vlan_id_t m_vlan_id = 0; sai_vlan_id_t m_port_vlan_id = DEFAULT_PORT_VLAN_ID; // Port VLAN ID - sai_object_id_t m_vlan_member_id = 0; + port_vlan_members_t m_vlan_members; sai_object_id_t m_rif_id = 0; sai_object_id_t m_hif_id = 0; sai_object_id_t m_lag_id = 0; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index eb96c1239ad..6fcd57ba0b2 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -24,6 +24,11 @@ extern sai_object_id_t gSwitchId; #define VLAN_PREFIX "Vlan" #define DEFAULT_VLAN_ID 1 +static char* hostif_vlan_tag[] = { + [SAI_HOSTIF_VLAN_TAG_STRIP] = "SAI_HOSTIF_VLAN_TAG_STRIP", + [SAI_HOSTIF_VLAN_TAG_KEEP] = "SAI_HOSTIF_VLAN_TAG_KEEP", + [SAI_HOSTIF_VLAN_TAG_ORIGINAL] = "SAI_HOSTIF_VLAN_TAG_ORIGINAL" +}; /* * Initialize PortsOrch * 0) By default, a switch has one CPU port, one 802.1Q bridge, and one default @@ -174,7 +179,7 @@ void PortsOrch::removeDefaultVlanMembers() } } - SWSS_LOG_NOTICE("Remove VLAN members from default VLAN"); + SWSS_LOG_NOTICE("Remove %d VLAN members from default VLAN", attr.value.objlist.count); } void PortsOrch::removeDefaultBridgePorts() @@ -345,6 +350,132 @@ bool PortsOrch::setPortMtu(sai_object_id_t id, sai_uint32_t mtu) return true; } +bool PortsOrch::setPortPvid (Port &port, sai_uint32_t pvid) +{ + SWSS_LOG_ENTER(); + + vector idv; + + if (port.m_type == Port::PHY) + { + idv.push_back(port.m_port_id); + } + else if (port.m_type == Port::LAG) + { + Port member; + auto it = port.m_members.begin(); + while (it != port.m_members.end()) + { + const string& memberName = *it; + if (!getPort(memberName, member)) + { + SWSS_LOG_ERROR("Failed to get port for %s alias\n", memberName.c_str()); + return false; + } + idv.push_back(member.m_port_id); + it++; + } + } + else + { + SWSS_LOG_ERROR("PortsOrch::setPortPvid port type %d not supported", port.m_type); + return false; + } + + for (const auto& id: idv) + { + sai_attribute_t attr; + attr.id = SAI_PORT_ATTR_PORT_VLAN_ID; + attr.value.u32 = pvid; + + sai_status_t status = sai_port_api->set_port_attribute(id, &attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to set pvid %u to port pid:%lx", attr.value.u32, id); + return false; + } + SWSS_LOG_INFO("Set pvid %u to port pid:%lx", attr.value.u32, id); + } + return true; +} + +bool PortsOrch::getPortPvid(Port &port, sai_uint32_t &pvid) +{ + + if (port.m_port_vlan_id != DEFAULT_PORT_VLAN_ID) + { + pvid = port.m_port_vlan_id; + return true; + } + + if (port.m_vlan_members.size() == 0) + { + pvid = DEFAULT_PORT_VLAN_ID; + return true; + } + + for (auto &vme: port.m_vlan_members) + { + if(vme.second.vlan_mode == SAI_VLAN_TAGGING_MODE_UNTAGGED) { + pvid = vme.first; + return true; + } + } + SWSS_LOG_ERROR("Failed to get pvid for port %s", port.m_alias.c_str()); + return false; +} +bool PortsOrch::setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip) +{ + SWSS_LOG_ENTER(); + + vector portv; + + if (port.m_type == Port::PHY) + { + portv.push_back(&port); + } + else if (port.m_type == Port::LAG) + { + Port member; + auto it = port.m_members.begin(); + while (it != port.m_members.end()) + { + const string& memberName = *it; + if (!getPort(memberName, member)) + { + SWSS_LOG_ERROR("Failed to get port for %s alias\n", memberName.c_str()); + return false; + } + portv.push_back(&member); + it++; + } + } + else + { + SWSS_LOG_ERROR("PortsOrch::setPortPvid port type %d not supported", port.m_type); + return false; + } + + for (const auto p: portv) + { + sai_attribute_t attr; + attr.id = SAI_HOSTIF_ATTR_VLAN_TAG; + attr.value.s32 = strip; + + sai_status_t status = sai_hostif_api->set_hostif_attribute(p->m_hif_id, &attr); + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_WARN("Failed to set %s to host interface %s", + hostif_vlan_tag[strip], p->m_alias.c_str()); + return false; + } + SWSS_LOG_NOTICE("Set %s to host interface %s", + hostif_vlan_tag[strip], p->m_alias.c_str()); + } + + return true; +} + bool PortsOrch::validatePortSpeed(sai_object_id_t port_id, sai_uint32_t speed) { sai_attribute_t attr; @@ -499,6 +630,7 @@ void PortsOrch::doPortTask(Consumer &consumer) set lane_set; string admin_status; uint32_t mtu = 0; + uint32_t pvid = 0; uint32_t speed = 0; for (auto i : kfvFieldsValues(t)) @@ -525,6 +657,10 @@ void PortsOrch::doPortTask(Consumer &consumer) if (fvField(i) == "mtu") mtu = (uint32_t)stoul(fvValue(i)); + /* Set port pvid */ + if (fvField(i) == "pvid") + pvid = (uint32_t)stoul(fvValue(i)); + /* Set port speed */ if (fvField(i) == "speed") speed = (uint32_t)stoul(fvValue(i)); @@ -645,6 +781,27 @@ void PortsOrch::doPortTask(Consumer &consumer) } } } + + if (pvid != 0) + { + Port p; + if (getPort(alias, p)) + { + if (setPortPvid(p, pvid)) + { + p.m_port_vlan_id = (sai_vlan_id_t)pvid; + SWSS_LOG_NOTICE("Set port %s pvid to %u", alias.c_str(), pvid); + } + else + { + SWSS_LOG_ERROR("Failed to set port %s pvid to %u", alias.c_str(), pvid); + it++; + continue; + } + } + else + SWSS_LOG_ERROR("Failed to get port id by alias:%s", alias.c_str()); + } } else SWSS_LOG_ERROR("Unknown operation type %s", op.c_str()); @@ -682,24 +839,58 @@ void PortsOrch::doVlanTask(Consumer &consumer) if (op == SET_COMMAND) { - /* Duplicate entry */ - if (m_portList.find(vlan_alias) != m_portList.end()) + if (m_portList.find(vlan_alias) == m_portList.end()) { - it = consumer.m_toSync.erase(it); - continue; + if (addVlan(vlan_alias) == false) + { + it++; + continue; + } } - if (addVlan(vlan_alias)) - it = consumer.m_toSync.erase(it); - else - it++; + string autostate; + uint32_t mtu = 0; + for (auto i : kfvFieldsValues(t)) + { + /* Set port autostate */ + if (fvField(i) == "autostate") + { + autostate = fvValue(i); + } + + /* Set port mtu */ + if (fvField(i) == "mtu") + { + mtu = (uint32_t)stoul(fvValue(i)); + } + + // TODO: unicast_miss_flood, multicast_miss_flood, broadcast_miss_flood + } + + if (autostate != "") + { + Port v; + getPort(vlan_alias, v); + v.m_vlan_info.autostate = autostate; + // TODO: update VLAN interface operational state based on autostate and member state. + } + + if (mtu != 0) + { + Port v; + getPort(vlan_alias, v); + v.m_vlan_info.mtu = mtu; + // TODO: update vlan routeif mtu + } + + it = consumer.m_toSync.erase(it); } else if (op == DEL_COMMAND) { Port vlan; getPort(vlan_alias, vlan); - if (removeVlan(vlan) && removeBridgePort(vlan)) + if (removeVlan(vlan)) it = consumer.m_toSync.erase(it); else it++; @@ -796,9 +987,6 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer) continue; } - /* Assert the port doesn't belong to any VLAN */ - assert(!port.m_vlan_id && !port.m_vlan_member_id); - if (addBridgePort(port) && addVlanMember(vlan, port, tagging_mode)) it = consumer.m_toSync.erase(it); else @@ -808,10 +996,7 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer) { if (vlan.m_members.find(port_alias) != vlan.m_members.end()) { - /* Assert the port belongs the a VLAN */ - assert(port.m_vlan_id && port.m_vlan_member_id); - - if (removeVlanMember(vlan, port)) + if (removeVlanMember(vlan, port) && removeBridgePort(port)) it = consumer.m_toSync.erase(it); else it++; @@ -970,6 +1155,8 @@ void PortsOrch::doLagMemberTask(Consumer &consumer) if (!port.m_lag_id || !port.m_lag_member_id) { + SWSS_LOG_WARN("Member %s not found in LAG %s lid:%lx lmid:%lx,", + port.m_alias.c_str(), lag.m_alias.c_str(), lag.m_lag_id, port.m_lag_member_id); it = consumer.m_toSync.erase(it); continue; } @@ -1108,6 +1295,18 @@ bool PortsOrch::initializePort(Port &p) vector.push_back(tuple); m_portTable->set(p.m_alias, vector); + /* + * Port has been removed from 1q bridge at PortsOrch constructor, + * also start stipping off VLAN tag. + */ + bool rv = setHostIntfsStripTag(p, SAI_HOSTIF_VLAN_TAG_STRIP); + if (rv != true) + { + SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", + hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], p.m_alias.c_str()); + return false; + } + return true; } @@ -1146,6 +1345,9 @@ bool PortsOrch::addBridgePort(Port &port) { SWSS_LOG_ENTER(); + if (port.m_bridge_port_id) + return true; + sai_attribute_t attr; vector attrs; @@ -1154,7 +1356,20 @@ bool PortsOrch::addBridgePort(Port &port) attrs.push_back(attr); attr.id = SAI_BRIDGE_PORT_ATTR_PORT_ID; - attr.value.oid = port.m_port_id; + if (port.m_type == Port::PHY) + { + attr.value.oid = port.m_port_id; + } + else if (port.m_type == Port::LAG) + { + attr.value.oid = port.m_lag_id; + } + else + { + SWSS_LOG_ERROR("Failed to add bridge port %s to default 1Q bridge, invalid porty type %d", + port.m_alias.c_str(), port.m_type); + return false; + } attrs.push_back(attr); /* Create a bridge port with admin status set to UP */ @@ -1170,15 +1385,32 @@ bool PortsOrch::addBridgePort(Port &port) return false; } + bool rv = setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP); + if (rv != true) + { + SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", + hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_KEEP], port.m_alias.c_str()); + return false; + } + m_portList[port.m_alias] = port; SWSS_LOG_NOTICE("Add bridge port %s to default 1Q bridge", port.m_alias.c_str()); return true; } -bool PortsOrch::removeBridgePort(Port port) +bool PortsOrch::removeBridgePort(Port &port) { SWSS_LOG_ENTER(); + if (port.m_vlan_members.size() > 0) + { + for (auto &vme: port.m_vlan_members) + { + SWSS_LOG_DEBUG("Port %s still in VLAN %d", port.m_alias.c_str(), vme.first); + } + return true; + } + /* Set bridge port admin status to DOWN */ sai_attribute_t attr; attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE; @@ -1192,6 +1424,14 @@ bool PortsOrch::removeBridgePort(Port port) return false; } + bool rv = setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_STRIP); + if (rv != true) + { + SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", + hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], port.m_alias.c_str()); + return false; + } + /* Flush FDB entries pointing to this bridge port */ // TODO: Remove all FDB entries associated with this bridge port before // removing the bridge port itself @@ -1204,9 +1444,11 @@ bool PortsOrch::removeBridgePort(Port port) port.m_alias.c_str(), status); return false; } + port.m_bridge_port_id = 0; SWSS_LOG_NOTICE("Remove bridge port %s from default 1Q bridge", port.m_alias.c_str()); + m_portList[port.m_alias] = port; return true; } @@ -1231,8 +1473,10 @@ bool PortsOrch::addVlan(string vlan_alias) SWSS_LOG_NOTICE("Create an empty VLAN %s vid:%hu", vlan_alias.c_str(), vlan_id); Port vlan(vlan_alias, Port::VLAN); - vlan.m_vlan_oid = vlan_oid; - vlan.m_vlan_id = vlan_id; + vlan.m_vlan_info.vlan_oid = vlan_oid; + vlan.m_vlan_info.vlan_id = vlan_id; + vlan.m_vlan_info.autostate = "disabled"; + vlan.m_vlan_info.mtu = 1500; vlan.m_members = set(); m_portList[vlan_alias] = vlan; @@ -1250,21 +1494,23 @@ bool PortsOrch::removeVlan(Port vlan) return false; } - sai_status_t status = sai_vlan_api->remove_vlan(vlan.m_vlan_oid); + sai_status_t status = sai_vlan_api->remove_vlan(vlan.m_vlan_info.vlan_oid); if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_ERROR("Failed to remove VLAN %s vid:%hu", vlan.m_alias.c_str(), vlan.m_vlan_id); + SWSS_LOG_ERROR("Failed to remove VLAN %s vid:%hu", + vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id); return false; } - SWSS_LOG_NOTICE("Remove VLAN %s vid:%hu", vlan.m_alias.c_str(), vlan.m_vlan_id); + SWSS_LOG_NOTICE("Remove VLAN %s vid:%hu", vlan.m_alias.c_str(), + vlan.m_vlan_info.vlan_id); m_portList.erase(vlan.m_alias); return true; } -bool PortsOrch::addVlanMember(Port vlan, Port port, string& tagging_mode) +bool PortsOrch::addVlanMember(Port &vlan, Port &port, string& tagging_mode) { SWSS_LOG_ENTER(); @@ -1272,21 +1518,23 @@ bool PortsOrch::addVlanMember(Port vlan, Port port, string& tagging_mode) vector attrs; attr.id = SAI_VLAN_MEMBER_ATTR_VLAN_ID; - attr.value.oid = vlan.m_vlan_oid; + attr.value.oid = vlan.m_vlan_info.vlan_oid; attrs.push_back(attr); attr.id = SAI_VLAN_MEMBER_ATTR_BRIDGE_PORT_ID; attr.value.oid = port.m_bridge_port_id; attrs.push_back(attr); + sai_vlan_tagging_mode_t sai_tagging_mode = SAI_VLAN_TAGGING_MODE_TAGGED; attr.id = SAI_VLAN_MEMBER_ATTR_VLAN_TAGGING_MODE; if (tagging_mode == "untagged") - attr.value.s32 = SAI_VLAN_TAGGING_MODE_UNTAGGED; + sai_tagging_mode = SAI_VLAN_TAGGING_MODE_UNTAGGED; else if (tagging_mode == "tagged") - attr.value.s32 = SAI_VLAN_TAGGING_MODE_TAGGED; + sai_tagging_mode = SAI_VLAN_TAGGING_MODE_TAGGED; else if (tagging_mode == "priority_tagged") - attr.value.s32 = SAI_VLAN_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); sai_object_id_t vlan_member_id; @@ -1294,31 +1542,24 @@ bool PortsOrch::addVlanMember(Port vlan, Port port, string& tagging_mode) if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to add member %s to VLAN %s vid:%hu pid:%lx", - port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_id, port.m_port_id); + port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id, port.m_port_id); return false; } SWSS_LOG_NOTICE("Add member %s to VLAN %s vid:%hu pid%lx", - port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_id, port.m_port_id); + port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id, port.m_port_id); - if (tagging_mode == "untagged") // set pvlan id for untagged port only + // Use untagged VLAN as pvid of the member port when no pvid has been configured explictly + if (port.m_port_vlan_id == DEFAULT_PORT_VLAN_ID && sai_tagging_mode == SAI_VLAN_TAGGING_MODE_UNTAGGED) { - attr.id = SAI_PORT_ATTR_PORT_VLAN_ID; - attr.value.u16 = vlan.m_vlan_id; - - status = sai_port_api->set_port_attribute(port.m_port_id, &attr); - if (status != SAI_STATUS_SUCCESS) + if(!setPortPvid(port, vlan.m_vlan_info.vlan_id)) { - SWSS_LOG_ERROR("Failed to set port VLAN ID vid:%hu pid:%lx", - vlan.m_vlan_id, port.m_port_id); return false; } - - SWSS_LOG_NOTICE("Set untagged port %s VLAN ID to %hu", port.m_alias.c_str(), vlan.m_vlan_id); } - port.m_vlan_id = vlan.m_vlan_id; - port.m_port_vlan_id = vlan.m_vlan_id; - port.m_vlan_member_id = vlan_member_id; + // a physical port may join multiple vlans + VlanMemberEntry vme = {vlan_member_id, sai_tagging_mode}; + port.m_vlan_members[vlan.m_vlan_info.vlan_id] = vme; m_portList[port.m_alias] = port; vlan.m_members.insert(port.m_alias); m_portList[vlan.m_alias] = vlan; @@ -1329,37 +1570,39 @@ bool PortsOrch::addVlanMember(Port vlan, Port port, string& tagging_mode) return true; } -bool PortsOrch::removeVlanMember(Port vlan, Port port) +bool PortsOrch::removeVlanMember(Port &vlan, Port &port) { SWSS_LOG_ENTER(); - sai_status_t status = sai_vlan_api->remove_vlan_member(port.m_vlan_member_id); + sai_object_id_t vlan_member_id; + sai_vlan_tagging_mode_t sai_tagging_mode; + auto vlan_member = port.m_vlan_members.find(vlan.m_vlan_info.vlan_id); + + /* Assert the port belongs to this VLAN */ + assert (vlan_member != port.m_vlan_members.end()); + sai_tagging_mode = vlan_member->second.vlan_mode; + vlan_member_id = vlan_member->second.vlan_member_id; + sai_status_t status = sai_vlan_api->remove_vlan_member(vlan_member_id); if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove member %s from VLAN %s vid:%hx vmid:%lx", - port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_id, port.m_vlan_member_id); + port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id, vlan_member_id); return false; } - + port.m_vlan_members.erase(vlan_member); SWSS_LOG_NOTICE("Remove member %s from VLAN %s lid:%hx vmid:%lx", - port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_id, port.m_vlan_member_id); - - sai_attribute_t attr; - attr.id = SAI_PORT_ATTR_PORT_VLAN_ID; - attr.value.u16 = DEFAULT_PORT_VLAN_ID; + port.m_alias.c_str(), vlan.m_alias.c_str(), vlan.m_vlan_info.vlan_id, vlan_member_id); - status = sai_port_api->set_port_attribute(port.m_port_id, &attr); - if (status != SAI_STATUS_SUCCESS) + // Restore to default pvid if no pvid has been configured explictly and this port was in untagged mode + if (port.m_port_vlan_id == DEFAULT_PORT_VLAN_ID && sai_tagging_mode == SAI_VLAN_TAGGING_MODE_UNTAGGED) { - SWSS_LOG_ERROR("Failed to reset port VLAN ID to DEFAULT_PORT_VLAN_ID pid:%lx", - port.m_port_id); - return false; + if(!setPortPvid(port, DEFAULT_PORT_VLAN_ID)) + { + return false; + } } - port.m_vlan_id = 0; - port.m_port_vlan_id = DEFAULT_PORT_VLAN_ID; - port.m_vlan_member_id = 0; m_portList[port.m_alias] = port; vlan.m_members.erase(port.m_alias); m_portList[vlan.m_alias] = vlan; @@ -1403,6 +1646,11 @@ bool PortsOrch::removeLag(Port lag) SWSS_LOG_ERROR("Failed to remove non-empty LAG %s", lag.m_alias.c_str()); return false; } + if (lag.m_vlan_members.size() > 0) + { + SWSS_LOG_ERROR("Failed to remove LAG %s, it is still in VLAN", lag.m_alias.c_str()); + return false; + } sai_status_t status = sai_lag_api->remove_lag(lag.m_lag_id); if (status != SAI_STATUS_SUCCESS) @@ -1418,7 +1666,7 @@ bool PortsOrch::removeLag(Port lag) return true; } -bool PortsOrch::addLagMember(Port lag, Port port) +bool PortsOrch::addLagMember(Port &lag, Port &port) { SWSS_LOG_ENTER(); @@ -1453,13 +1701,27 @@ bool PortsOrch::addLagMember(Port lag, Port port) m_portList[lag.m_alias] = lag; + if (lag.m_vlan_members.size() > 0) + { + bool rv = setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP); + if (rv != true) + { + SWSS_LOG_ERROR("Failed to set %s for hostif of port %s which is in LAG %s", + hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_KEEP], port.m_alias.c_str(), lag.m_alias.c_str()); + return false; + } + } + sai_uint32_t pvid = DEFAULT_PORT_VLAN_ID; + getPortPvid(lag, pvid); + setPortPvid (port, pvid); + LagMemberUpdate update = { lag, port, true }; notify(SUBJECT_TYPE_LAG_MEMBER_CHANGE, static_cast(&update)); return true; } -bool PortsOrch::removeLagMember(Port lag, Port port) +bool PortsOrch::removeLagMember(Port &lag, Port &port) { sai_status_t status = sai_lag_api->remove_lag_member(port.m_lag_member_id); @@ -1479,6 +1741,16 @@ bool PortsOrch::removeLagMember(Port lag, Port port) lag.m_members.erase(port.m_alias); m_portList[lag.m_alias] = lag; + if (lag.m_vlan_members.size() > 0) + { + bool rv = setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_STRIP); + if (rv != true) + { + SWSS_LOG_ERROR("Failed to set %s for hostif of port %s which is leaving LAG %s", + hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], port.m_alias.c_str(), lag.m_alias.c_str()); + return false; + } + } LagMemberUpdate update = { lag, port, false }; notify(SUBJECT_TYPE_LAG_MEMBER_CHANGE, static_cast(&update)); diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index b5e78297e27..e5e44837659 100644 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -55,6 +55,7 @@ class PortsOrch : public Orch, public Subject bool setHostIntfsOperStatus(sai_object_id_t id, bool up); void updateDbPortOperStatus(sai_object_id_t id, sai_port_oper_status_t status); + void removeDefaultVlanMembers(); private: unique_ptr m_counterTable; unique_ptr
m_portTable; @@ -78,7 +79,6 @@ class PortsOrch : public Orch, public Subject void doLagTask(Consumer &consumer); void doLagMemberTask(Consumer &consumer); - void removeDefaultVlanMembers(); void removeDefaultBridgePorts(); bool initializePort(Port &port); @@ -86,22 +86,25 @@ class PortsOrch : public Orch, public Subject void initializeQueues(Port &port); bool addHostIntfs(sai_object_id_t router_intfs_id, string alias, sai_object_id_t &host_intfs_id); + bool setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip); bool addBridgePort(Port &port); - bool removeBridgePort(Port port); + bool removeBridgePort(Port &port); bool addVlan(string vlan); bool removeVlan(Port vlan); - bool addVlanMember(Port vlan, Port port, string& tagging_mode); - bool removeVlanMember(Port vlan, Port port); + bool addVlanMember(Port &vlan, Port &port, string& tagging_mode); + bool removeVlanMember(Port &vlan, Port &port); bool addLag(string lag); bool removeLag(Port lag); - bool addLagMember(Port lag, Port port); - bool removeLagMember(Port lag, Port port); + bool addLagMember(Port &lag, Port &port); + bool removeLagMember(Port &lag, Port &port); bool setPortAdminStatus(sai_object_id_t id, bool up); bool setPortMtu(sai_object_id_t id, sai_uint32_t mtu); + bool setPortPvid (Port &port, sai_uint32_t pvid); + bool getPortPvid(Port &port, sai_uint32_t &pvid); bool setBridgePortAdminStatus(sai_object_id_t id, bool up); diff --git a/orchagent/saihelper.cpp b/orchagent/saihelper.cpp index 96a879364ad..96e5a6c5ec3 100644 --- a/orchagent/saihelper.cpp +++ b/orchagent/saihelper.cpp @@ -200,18 +200,6 @@ void initSaiRedis(const string &record_location) } } - /* Disable/enable SwSS recording */ - if (gSwssRecord) - { - gRecordFile = record_location + "/" + "swss.rec"; - gRecordOfs.open(gRecordFile, std::ofstream::out | std::ofstream::app); - if (!gRecordOfs.is_open()) - { - SWSS_LOG_ERROR("Failed to open SwSS recording file %s", gRecordFile.c_str()); - exit(EXIT_FAILURE); - } - } - attr.id = SAI_REDIS_SWITCH_ATTR_USE_PIPELINE; attr.value.booldata = true; From 818dbdac2d19f4528cad9a053a4ecbf365e08e88 Mon Sep 17 00:00:00 2001 From: Jipan Yang Date: Fri, 25 Aug 2017 21:25:51 -0700 Subject: [PATCH 2/3] Address various review comments such as putting mtu variable at Port class, code format and so on. --- orchagent/port.h | 4 +- orchagent/portsorch.cpp | 90 +++++++++++++++++------------------------ orchagent/portsorch.h | 2 +- 3 files changed, 41 insertions(+), 55 deletions(-) diff --git a/orchagent/port.h b/orchagent/port.h index ca6b04c768a..c331389d7b1 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -27,7 +27,6 @@ struct VlanInfo sai_object_id_t vlan_oid; sai_vlan_id_t vlan_id; std::string autostate; - uint32_t mtu; }; class Port @@ -70,16 +69,17 @@ class Port Type m_type; int m_index = 0; // PHY_PORT: index int m_ifindex = 0; + sai_uint32_t m_mtu; sai_object_id_t m_port_id = 0; VlanInfo m_vlan_info; sai_object_id_t m_bridge_port_id = 0; // TODO: port could have multiple bridge port IDs sai_vlan_id_t m_port_vlan_id = DEFAULT_PORT_VLAN_ID; // Port VLAN ID - port_vlan_members_t m_vlan_members; sai_object_id_t m_rif_id = 0; sai_object_id_t m_hif_id = 0; sai_object_id_t m_lag_id = 0; sai_object_id_t m_lag_member_id = 0; sai_object_id_t m_acl_table_group_id = 0; + port_vlan_members_t m_vlan_members; std::set m_members; std::vector m_queue_ids; std::vector m_priority_group_ids; diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 6fcd57ba0b2..41d05146542 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -353,28 +353,15 @@ bool PortsOrch::setPortMtu(sai_object_id_t id, sai_uint32_t mtu) bool PortsOrch::setPortPvid (Port &port, sai_uint32_t pvid) { SWSS_LOG_ENTER(); - - vector idv; + vector portv; if (port.m_type == Port::PHY) { - idv.push_back(port.m_port_id); + portv.push_back(port); } else if (port.m_type == Port::LAG) { - Port member; - auto it = port.m_members.begin(); - while (it != port.m_members.end()) - { - const string& memberName = *it; - if (!getPort(memberName, member)) - { - SWSS_LOG_ERROR("Failed to get port for %s alias\n", memberName.c_str()); - return false; - } - idv.push_back(member.m_port_id); - it++; - } + getLagMember(port, portv); } else { @@ -382,19 +369,19 @@ bool PortsOrch::setPortPvid (Port &port, sai_uint32_t pvid) return false; } - for (const auto& id: idv) + for (const auto p: portv) { sai_attribute_t attr; attr.id = SAI_PORT_ATTR_PORT_VLAN_ID; attr.value.u32 = pvid; - sai_status_t status = sai_port_api->set_port_attribute(id, &attr); + sai_status_t status = sai_port_api->set_port_attribute(p.m_port_id, &attr); if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_ERROR("Failed to set pvid %u to port pid:%lx", attr.value.u32, id); + SWSS_LOG_ERROR("Failed to set pvid %u to port: %s", attr.value.u32, p.m_alias.c_str()); return false; } - SWSS_LOG_INFO("Set pvid %u to port pid:%lx", attr.value.u32, id); + SWSS_LOG_NOTICE("Set pvid %u to port: %s", attr.value.u32, p.m_alias.c_str()); } return true; } @@ -427,28 +414,15 @@ bool PortsOrch::getPortPvid(Port &port, sai_uint32_t &pvid) bool PortsOrch::setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip) { SWSS_LOG_ENTER(); - - vector portv; + vector portv; if (port.m_type == Port::PHY) { - portv.push_back(&port); + portv.push_back(port); } else if (port.m_type == Port::LAG) { - Port member; - auto it = port.m_members.begin(); - while (it != port.m_members.end()) - { - const string& memberName = *it; - if (!getPort(memberName, member)) - { - SWSS_LOG_ERROR("Failed to get port for %s alias\n", memberName.c_str()); - return false; - } - portv.push_back(&member); - it++; - } + getLagMember(port, portv); } else { @@ -462,15 +436,15 @@ bool PortsOrch::setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip) attr.id = SAI_HOSTIF_ATTR_VLAN_TAG; attr.value.s32 = strip; - sai_status_t status = sai_hostif_api->set_hostif_attribute(p->m_hif_id, &attr); + sai_status_t status = sai_hostif_api->set_hostif_attribute(p.m_hif_id, &attr); if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_WARN("Failed to set %s to host interface %s", - hostif_vlan_tag[strip], p->m_alias.c_str()); + hostif_vlan_tag[strip], p.m_alias.c_str()); return false; } - SWSS_LOG_NOTICE("Set %s to host interface %s", - hostif_vlan_tag[strip], p->m_alias.c_str()); + SWSS_LOG_NOTICE("Set %s to host interface: %s", + hostif_vlan_tag[strip], p.m_alias.c_str()); } return true; @@ -772,7 +746,10 @@ void PortsOrch::doPortTask(Consumer &consumer) if (mtu != 0) { if (setPortMtu(p.m_port_id, mtu)) + { + p.m_mtu = mtu; SWSS_LOG_NOTICE("Set port %s MTU to %u", alias.c_str(), mtu); + } else { SWSS_LOG_ERROR("Failed to set port %s MTU to %u", alias.c_str(), mtu); @@ -780,12 +757,8 @@ void PortsOrch::doPortTask(Consumer &consumer) continue; } } - } - if (pvid != 0) - { - Port p; - if (getPort(alias, p)) + if (pvid != 0) { if (setPortPvid(p, pvid)) { @@ -799,8 +772,6 @@ void PortsOrch::doPortTask(Consumer &consumer) continue; } } - else - SWSS_LOG_ERROR("Failed to get port id by alias:%s", alias.c_str()); } } else @@ -841,7 +812,7 @@ void PortsOrch::doVlanTask(Consumer &consumer) { if (m_portList.find(vlan_alias) == m_portList.end()) { - if (addVlan(vlan_alias) == false) + if (!addVlan(vlan_alias)) { it++; continue; @@ -879,7 +850,7 @@ void PortsOrch::doVlanTask(Consumer &consumer) { Port v; getPort(vlan_alias, v); - v.m_vlan_info.mtu = mtu; + v.m_mtu = mtu; // TODO: update vlan routeif mtu } @@ -1476,7 +1447,7 @@ bool PortsOrch::addVlan(string vlan_alias) vlan.m_vlan_info.vlan_oid = vlan_oid; vlan.m_vlan_info.vlan_id = vlan_id; vlan.m_vlan_info.autostate = "disabled"; - vlan.m_vlan_info.mtu = 1500; + vlan.m_mtu = 1500; vlan.m_members = set(); m_portList[vlan_alias] = vlan; @@ -1510,7 +1481,7 @@ bool PortsOrch::removeVlan(Port vlan) return true; } -bool PortsOrch::addVlanMember(Port &vlan, Port &port, string& tagging_mode) +bool PortsOrch::addVlanMember(Port &vlan, Port &port, string &tagging_mode) { SWSS_LOG_ENTER(); @@ -1582,8 +1553,8 @@ bool PortsOrch::removeVlanMember(Port &vlan, Port &port) assert (vlan_member != port.m_vlan_members.end()); sai_tagging_mode = vlan_member->second.vlan_mode; vlan_member_id = vlan_member->second.vlan_member_id; - sai_status_t status = sai_vlan_api->remove_vlan_member(vlan_member_id); + sai_status_t status = sai_vlan_api->remove_vlan_member(vlan_member_id); if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_ERROR("Failed to remove member %s from VLAN %s vid:%hx vmid:%lx", @@ -1666,6 +1637,21 @@ bool PortsOrch::removeLag(Port lag) return true; } +void PortsOrch::getLagMember(Port &lag, vector &portv) +{ + Port member; + + for(auto &name: lag.m_members) + { + if (!getPort(name, member)) + { + SWSS_LOG_ERROR("Failed to get port for %s alias\n", name.c_str()); + return; + } + portv.push_back(member); + } +} + bool PortsOrch::addLagMember(Port &lag, Port &port) { SWSS_LOG_ENTER(); diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index e5e44837659..2873b01a460 100644 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -100,7 +100,7 @@ class PortsOrch : public Orch, public Subject bool removeLag(Port lag); bool addLagMember(Port &lag, Port &port); bool removeLagMember(Port &lag, Port &port); - + void getLagMember(Port &lag, vector &portv); bool setPortAdminStatus(sai_object_id_t id, bool up); bool setPortMtu(sai_object_id_t id, sai_uint32_t mtu); bool setPortPvid (Port &port, sai_uint32_t pvid); From c438b7e9d35d0e3c954d74e6642c36d6442819ed Mon Sep 17 00:00:00 2001 From: Jipan Yang Date: Tue, 12 Sep 2017 22:32:30 -0700 Subject: [PATCH 3/3] Don't set pvid for lag with router interface. Coding style, space and alignment fix. --- orchagent/portsorch.cpp | 52 ++++++++++++++++++++--------------------- orchagent/portsorch.h | 2 +- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 41d05146542..eb69dcfb91e 100644 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -353,7 +353,7 @@ bool PortsOrch::setPortMtu(sai_object_id_t id, sai_uint32_t mtu) bool PortsOrch::setPortPvid (Port &port, sai_uint32_t pvid) { SWSS_LOG_ENTER(); - vector portv; + vector portv; if (port.m_type == Port::PHY) { @@ -388,6 +388,13 @@ bool PortsOrch::setPortPvid (Port &port, sai_uint32_t pvid) bool PortsOrch::getPortPvid(Port &port, sai_uint32_t &pvid) { + /* Just return false if the router interface exists */ + if (port.m_rif_id) + { + SWSS_LOG_DEBUG("Router interface exists on %s, don't set pvid", + port.m_alias.c_str()); + return false; + } if (port.m_port_vlan_id != DEFAULT_PORT_VLAN_ID) { @@ -414,7 +421,7 @@ bool PortsOrch::getPortPvid(Port &port, sai_uint32_t &pvid) bool PortsOrch::setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip) { SWSS_LOG_ENTER(); - vector portv; + vector portv; if (port.m_type == Port::PHY) { @@ -439,12 +446,12 @@ bool PortsOrch::setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip) sai_status_t status = sai_hostif_api->set_hostif_attribute(p.m_hif_id, &attr); if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_WARN("Failed to set %s to host interface %s", - hostif_vlan_tag[strip], p.m_alias.c_str()); + SWSS_LOG_ERROR("Failed to set %s to host interface %s", + hostif_vlan_tag[strip], p.m_alias.c_str()); return false; } SWSS_LOG_NOTICE("Set %s to host interface: %s", - hostif_vlan_tag[strip], p.m_alias.c_str()); + hostif_vlan_tag[strip], p.m_alias.c_str()); } return true; @@ -1241,7 +1248,7 @@ bool PortsOrch::initializePort(Port &p) initializeQueues(p); /* Create host interface */ - addHostIntfs(p.m_port_id, p.m_alias, p.m_hif_id); + addHostIntfs(p, p.m_alias, p.m_hif_id); #if 0 // TODO: Assure if_nametoindex(p.m_alias.c_str()) != 0 @@ -1266,22 +1273,10 @@ bool PortsOrch::initializePort(Port &p) vector.push_back(tuple); m_portTable->set(p.m_alias, vector); - /* - * Port has been removed from 1q bridge at PortsOrch constructor, - * also start stipping off VLAN tag. - */ - bool rv = setHostIntfsStripTag(p, SAI_HOSTIF_VLAN_TAG_STRIP); - if (rv != true) - { - SWSS_LOG_ERROR("Failed to set %s for hostif of port %s", - hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], p.m_alias.c_str()); - return false; - } - return true; } -bool PortsOrch::addHostIntfs(sai_object_id_t id, string alias, sai_object_id_t &host_intfs_id) +bool PortsOrch::addHostIntfs(Port &port, string alias, sai_object_id_t &host_intfs_id) { SWSS_LOG_ENTER(); @@ -1293,7 +1288,7 @@ bool PortsOrch::addHostIntfs(sai_object_id_t id, string alias, sai_object_id_t & attrs.push_back(attr); attr.id = SAI_HOSTIF_ATTR_OBJ_ID; - attr.value.oid = id; + attr.value.oid = port.m_port_id; attrs.push_back(attr); attr.id = SAI_HOSTIF_ATTR_NAME; @@ -1307,6 +1302,11 @@ bool PortsOrch::addHostIntfs(sai_object_id_t id, string alias, sai_object_id_t & return false; } + /* + * Port has been removed from 1q bridge at PortsOrch constructor, + * also start stipping off VLAN tag. + */ + setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_STRIP); SWSS_LOG_NOTICE("Create host interface for port %s", alias.c_str()); return true; @@ -1545,8 +1545,8 @@ bool PortsOrch::removeVlanMember(Port &vlan, Port &port) { SWSS_LOG_ENTER(); - sai_object_id_t vlan_member_id; - sai_vlan_tagging_mode_t sai_tagging_mode; + sai_object_id_t vlan_member_id; + sai_vlan_tagging_mode_t sai_tagging_mode; auto vlan_member = port.m_vlan_members.find(vlan.m_vlan_info.vlan_id); /* Assert the port belongs to this VLAN */ @@ -1568,7 +1568,7 @@ bool PortsOrch::removeVlanMember(Port &vlan, Port &port) // Restore to default pvid if no pvid has been configured explictly and this port was in untagged mode if (port.m_port_vlan_id == DEFAULT_PORT_VLAN_ID && sai_tagging_mode == SAI_VLAN_TAGGING_MODE_UNTAGGED) { - if(!setPortPvid(port, DEFAULT_PORT_VLAN_ID)) + if (!setPortPvid(port, DEFAULT_PORT_VLAN_ID)) { return false; } @@ -1641,7 +1641,7 @@ void PortsOrch::getLagMember(Port &lag, vector &portv) { Port member; - for(auto &name: lag.m_members) + for (auto &name: lag.m_members) { if (!getPort(name, member)) { @@ -1698,8 +1698,8 @@ bool PortsOrch::addLagMember(Port &lag, Port &port) } } sai_uint32_t pvid = DEFAULT_PORT_VLAN_ID; - getPortPvid(lag, pvid); - setPortPvid (port, pvid); + if (getPortPvid(lag, pvid)) + setPortPvid (port, pvid); LagMemberUpdate update = { lag, port, true }; notify(SUBJECT_TYPE_LAG_MEMBER_CHANGE, static_cast(&update)); diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index 2873b01a460..decb7a5f128 100644 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -85,7 +85,7 @@ class PortsOrch : public Orch, public Subject void initializePriorityGroups(Port &port); void initializeQueues(Port &port); - bool addHostIntfs(sai_object_id_t router_intfs_id, string alias, sai_object_id_t &host_intfs_id); + bool addHostIntfs(Port &port, string alias, sai_object_id_t &host_intfs_id); bool setHostIntfsStripTag(Port &port, sai_hostif_vlan_tag_t strip); bool addBridgePort(Port &port);