VNET/VRF Changes by prsunny · Pull Request #632 · sonic-net/sonic-swss

prsunny · 2018-09-28T21:33:07Z

What I did
Introduce VNetOrch to support VNet peering feature.

Why I did it
Support VNet peering using Vxlan (VNI <-> VRF)

How I verified it
VS test case, Testing in-progress

Details if related

Design Document:
https://github.com/prsunny/SONiC/blob/gh-pages/doc/vxlan/Vxlan_hld.md

* VRF changes

prsunny · 2018-10-02T21:14:05Z

retest this please

nikos-github · 2018-10-02T22:22:18Z

orchagent/vnetorch.cpp

+        {
+            SWSS_LOG_ERROR("Failed to create virtual router name: %s, rv: %d",
+                           vnet_name.c_str(), status);
+            return false;


Shouldn't you throw an exception on false and use try where you call the constructor from?

nikos-github · 2018-10-02T22:22:58Z

orchagent/vnetorch.cpp

+        }
+        else if (!attrs.empty())
+        {
+            it->second->updateObj(attrs);


Check return value.

orchagent/vnetorch.cpp

orchagent/intfsorch.cpp

lguohan · 2018-10-04T22:50:55Z

orchagent/vnetorch.cpp

+    }
+
+    const std::string& vnet_name = request.getKeyString(0);
+    SWSS_LOG_INFO("VNET '%s' add request", vnet_name.c_str());


SWSS_LOG_INFO [](start = 4, length = 13)

is this debug level?

Prefer to have it as INFO for now since this is executing only during VNET scenario and DEBUG logs, if enable is considerably high

nikos-github · 2018-10-04T23:59:18Z

orchagent/intfsorch.cpp

            if (it_intfs == m_syncdIntfses.end())
            {
-                if (addRouterIntfs(port))
+                if (addRouterIntfs(vrf_name, port))


When the interface is enslaved in a vrf, the connected (subnet route) and receive (ip2me route) need to be removed from the old vrf. Is the expectation that SAI will do this automatically?

I don't see this PR taking care of that and I don't see #635 taking care of that either.

Please clarify whether it's been agreed that SAI will do this. If not, then either this PR or the one I mentioned above, need to take care of it, unless I've missed something.

lguohan · 2018-10-05T00:25:52Z

orchagent/vnetorch.h

+        {
+            return vr_ids.at(VR_TYPE::ING_VR_VALID);
+        }
+        return 0x0;


0x0 [](start = 15, length = 3)

use SAI_NULL_OBJECT_ID

orchagent/vnetorch.h

orchagent/vnetorch.cpp

orchagent/vnetorch.h

lguohan · 2018-10-05T00:46:05Z

orchagent/vnetorch.h

+    { } // no mandatory attributes
+};
+
+enum class VNET_EXEC


what does VNET_EXEC mean? VNET_TYPE?

it means the Vnet execution model. (VRF vs BRIDGE)

lguohan · 2018-10-05T00:48:45Z

orchagent/vnetorch.h

+        {
+            return vr_ids.at(VR_TYPE::EGR_VR_VALID);
+        }
+        return 0x0;


0x0 [](start = 15, length = 3)

SAI_NULL_OBJECT_ID

lguohan · 2018-10-05T00:51:02Z

orchagent/vnetorch.cpp

+
+    if (vnet_table_.find(vnet_name) == std::end(vnet_table_))
+    {
+        SWSS_LOG_ERROR("VNET '%s' doesn't exist", vnet_name.c_str());


SWSS_LOG_ERROR [](start = 8, length = 14)

not sure this error, it should be warning since the program still can run without problem.

lguohan · 2018-10-05T00:51:38Z

orchagent/vnetorch.cpp

+
+    vnet_table_.erase(vnet_name);
+
+    SWSS_LOG_NOTICE("VNET '%s' del request", vnet_name.c_str());


SWSS_LOG_NOTICE [](start = 4, length = 15)

INFO level to match with add/update operation.

orchagent/vnetorch.cpp

jipanyang · 2018-10-05T02:21:54Z

orchagent/vnetorch.h

+
+    void setPeerList(set<string>& p_list)
+    {
+        peer_list = p_list;


How is peer_list used after getting it?

jipanyang · 2018-10-05T02:33:32Z

orchagent/vnetorch.cpp

+
+template <class T>
+std::unique_ptr<T> VNetOrch::createObject(const string& vnet_name, set<string>& plist,
+                                          vector<sai_attribute_t>& attrs)


you make it a template, are there class other than VNetVrfObject using the same parameters of (const string&, set&, vector<sai_attribute_t>&) ?

yes. There is also a bridge model for Vnet. Hence the template.

jipanyang · 2018-10-05T02:35:01Z

orchagent/vnetorch.h

+
+    VNetTable vnet_table_;
+    VNetRequest request_;
+    VNET_EXEC vnet_exec_;


What is the reason to have trailing "_" here for them? I don't see it at other implementation.

Few of the orchs, (vrf, vxlan) has member variables with trailing _. So keeping it consistent with that.

* Vnet route table handling * Addressed review comments * Fix for interface routes, add debug logs

lguohan · 2018-10-09T21:38:56Z

retest this please

prsunny · 2018-10-10T01:53:31Z

retest this please

Signed-off-by: Nazarii Hnydyn <[email protected]>

…-net#632) * builds on support for multiple switches in sonic-sairedis * new vslib switch BCM81724 implements a virtual gearbox phy. * support for launching second (BCM81724 is supported by its own syncd) * simple refactoring of tests to support switches by part number, still working with sairedis to support multiple switch in tests so BCM81724 will be a separate pull request) * changed example context_config.json to reflect renaming of phy REDIS tables (see sonic-swss-common commit 292b08a) Note that a future commit to sonic-buildimage will be required to trigger launch of physyncd (launching is based on device config files which are currently not present in sonic-buildimage). Testing done in multiple environments (broadcom fork and pure upstream). Example CLI output based on changes pushed to sonic-utilities (commit a6c4456) running in VS switch supporting BCM81724: root@sonic:/home/admin# show gearbox interfaces status PHY Id Interface MAC Lanes MAC Lane Speed PHY Lanes PHY Lane Speed Line Lanes Line Lane Speed Oper Admin -------- ----------- ----------- ---------------- ----------- ---------------- ------------ ----------------- ------ ------- 1 Ethernet0 25,26,27,28 10G 200,201 20G 206 40G up up 1 Ethernet4 29,30,31,32 10G 202,203 20G 207 40G up up 1 Ethernet8 33,34,35,36 10G 204,205 20G 208 40G up up HLD is located at https://github.com/Azure/SONiC/blob/master/doc/gearbox/gearbox_mgr_design.md Signed-off-by: [email protected]

* VNET/VRF Changes (Pterosaur#6) * VRF changes * Fixed an IPv6 address parsing issue * Updated logs, removed intfmgr changes in favour of PR sonic-net#635 * Updated VRF table name * Addressed review comment, test fixes * Remove extra semi-colon * Route handling, review comments (sonic-net#8) * Vnet route table handling * Addressed review comments * Fix for interface routes, add debug logs

…onic-net#632) In local function get_gbcounter of portcounter.lua, need to check the return value of redis HGET. We can do addition operation only if counter ID exists and returns its value number.

prsunny added 4 commits September 28, 2018 14:27

VNET/VRF Changes (#6)

ab6b57e

* VRF changes

Fixed an IPv6 address parsing issue

c636099

Updated logs, removed intfmgr changes in favour of PR sonic-net#635

7915d97

Updated VRF table name

e3d632d

nikos-github reviewed Oct 2, 2018

View reviewed changes

Addressed review comment, test fixes

d8cd6d6

jleveque suggested changes Oct 3, 2018

View reviewed changes

orchagent/vnetorch.cpp Outdated Show resolved Hide resolved

orchagent/intfsorch.cpp Outdated Show resolved Hide resolved

Remove extra semi-colon

094bafa

jleveque approved these changes Oct 4, 2018

View reviewed changes

lguohan reviewed Oct 4, 2018

View reviewed changes

nikos-github reviewed Oct 4, 2018

View reviewed changes

nikos-github mentioned this pull request Oct 5, 2018

[intfmgrd]: Merge intfsyncd into intfmgrd #635

Merged

lguohan reviewed Oct 5, 2018

View reviewed changes

orchagent/vnetorch.h Outdated Show resolved Hide resolved

lguohan reviewed Oct 5, 2018

View reviewed changes

orchagent/vnetorch.cpp Outdated Show resolved Hide resolved

lguohan reviewed Oct 5, 2018

View reviewed changes

orchagent/vnetorch.h Outdated Show resolved Hide resolved

lguohan reviewed Oct 5, 2018

View reviewed changes

jipanyang reviewed Oct 5, 2018

View reviewed changes

orchagent/vnetorch.cpp Outdated Show resolved Hide resolved

jipanyang reviewed Oct 5, 2018

View reviewed changes

Route handling, review comments (#8)

ccf7170

* Vnet route table handling * Addressed review comments * Fix for interface routes, add debug logs

lguohan approved these changes Oct 9, 2018

View reviewed changes

prsunny merged commit 8d57ae0 into sonic-net:master Oct 10, 2018

prsunny mentioned this pull request Oct 16, 2018

VNET/VXLAN changes #643

Merged

stcheng added the Enhancement ➕ label Oct 19, 2018

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022

Fixed config Asym PFC CLI. (sonic-net#632)

c1c53f5

Signed-off-by: Nazarii Hnydyn <[email protected]>


		vnet_table_.erase(vnet_name);

		SWSS_LOG_NOTICE("VNET '%s' del request", vnet_name.c_str());

Conversation

prsunny commented Sep 28, 2018

Uh oh!

prsunny commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Oct 9, 2018

Uh oh!

prsunny commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants