[VxlanOrch] pytest for EVPN VXLAN #1318

srj102 · 2020-06-10T10:00:34Z

Added Pytest to test changes to VxlanMgr, VxlanOrch and PortsOrch for EVPN VXLAN feature.

HLD Location : https://github.com/Azure/SONiC/pull/437/commits

Signed-off-by: Rajesh Sankaran [email protected]

What I did
Please refer to details provided above.

Why I did it
EVPN VXLAN Implementation

How I verified it
Changes as part of PR 1264 and 1266 verified with this pytest
#1264 and #1266 should be merged for this PR.

srj102 · 2020-10-23T17:28:33Z

retest vs please

Tested with PR1266 changes.

srj102 · 2020-12-14T17:54:42Z

retest vs please

srj102 · 2020-12-14T22:42:44Z

retest vs please

prsunny

lgtm.. request @daall to take a look

tests/test_evpn_tunnel.py

msreddypaluru

few minor comments, please address. approved

msreddypaluru · 2020-12-18T04:49:45Z

tests/test_evpn_tunnel.py

+def create_entry(tbl, key, pairs):
+    fvs = swsscommon.FieldValuePairs(pairs)
+    tbl.set(key, fvs)
+    time.sleep(1)


Is "sleep" mandatory for the tests?

This file was based on test_vnet.py and hence inherits some of those. However I have seen that sleeps are required.

tests/test_evpn_tunnel.py

msreddypaluru · 2020-12-18T05:04:14Z

tests/test_evpn_tunnel.py

+
+#    Test 3 - Create and Delete SIP Tunnel and Map entries
+#    @pytest.mark.skip(reason="Starting Vxlanmgr to be merged")
+    def test_p2mp_tunnel_with_dip(self, dvs, testlog):


what is the use of testlog, not used anywhere? can all print's can be moved to testlog?

test_vnet.py and test_vxlan_tunnel.py had references to testlog and hence inherited the same.
Have removed it now

testlog is a pytest fixture, it adds test <blank> start and test <blank> end to the syslogs to aid in debugging. It should probably just be auto-use so we don't have to explicitly invoke it everywhere, but that can be addressed in a different pr. 😄

tests/test_evpn_tunnel.py

srj102 · 2020-12-18T18:51:42Z

@msreddypaluru @prsunny all comments are handled. Pl look through and merge.

daall

I think the logic/flow in TestVxlanOrch makes a lot of sense and I think having all the helper methods makes it super readable, but in order to make sure the test is stable and keep the logic a little simpler we should use dvslib as much as possible in the helper methods.

I've pointed out some specific suggestions in my comments but in general (other than producerstatetable) you should use the methods in DVSDatabase instead of directly calling swsscommon. There are a lot of comments in dvs_datbase.py that should help you out and there are examples of this in action in test_acl, test_vlan, test_subport, test_route, etc.

Doing this 1) should ensure this test doesn't become flaky down the road, 2) it should improve performance because you won't have to depend on time.sleep to make the test behave correctly, and 3) it should make the code a lot less verbose. Let me know if you have any questions!

daall · 2020-12-18T18:56:13Z

tests/test_evpn_tunnel.py

+def create_entry(tbl, key, pairs):
+    fvs = swsscommon.FieldValuePairs(pairs)
+    tbl.set(key, fvs)
+    time.sleep(1)
+
+def create_entry_tbl(db, table, separator, key, pairs):
+    tbl = swsscommon.Table(db, table)
+    create_entry(tbl, key, pairs)
+
+def delete_entry_tbl(db, table, key):
+    tbl = swsscommon.Table(db, table)
+    tbl._del(key)
+    time.sleep(1)


There are helpers for these already defined in dvslib/dvs_database, I would recommend using create_entry and delete_entry from here to simplify this file a little bit. We don't have support for producerstatetable there yet so those methods will need to stay.

daall · 2020-12-18T18:57:30Z

tests/test_evpn_tunnel.py

+def how_many_entries_exist(db, table):
+    tbl =  swsscommon.Table(db, table)
+    return len(tbl.getKeys())
+
+
+def entries(db, table):
+    tbl =  swsscommon.Table(db, table)
+    return set(tbl.getKeys())


These also have analogues in dvs_database already - see get_keys.

daall · 2020-12-18T18:58:44Z

tests/test_evpn_tunnel.py

+    tbl =  swsscommon.Table(db, table)
+    entries = set(tbl.getKeys())
+    new_entries = list(entries - existed_entries)
+    assert len(new_entries) == 1, "Wrong number of created entries."
+    return new_entries[0]


You should be able to accomplish the same behavior with wait_for_n_keys from dvs_database.

daall · 2020-12-18T18:59:23Z

tests/test_evpn_tunnel.py

+    tbl =  swsscommon.Table(db, table)
+    entries = set(tbl.getKeys())
+    new_entries = list(entries - existed_entries)
+    assert len(new_entries) == count, "Wrong number of created entries."
+    new_entries.sort()
+    return new_entries


Same as above - recommend using wait_for_n_keys to simplify the code + make the test more stable.

daall · 2020-12-18T19:00:46Z

tests/test_evpn_tunnel.py

+    db = swsscommon.DBConnector(swsscommon.ASIC_DB, dvs.redis_sock, 0)
+    table = 'ASIC_STATE:SAI_OBJECT_TYPE_VIRTUAL_ROUTER'
+    tbl =  swsscommon.Table(db, table)
+    keys = tbl.getKeys()
+    assert len(keys) == 1, "Wrong number of virtual routers found"
+
+    return keys[0]


wait_for_n_keys

daall · 2020-12-18T19:01:39Z

tests/test_evpn_tunnel.py

+    tbl =  swsscommon.Table(db, table)
+    keys = tbl.getKeys()
+    assert key in keys, "The desired key is not presented"
+
+    status, fvs = tbl.get(key)
+    assert status, "Got an error when get a key"
+
+    assert len(fvs) >= len(expected_attributes), "Incorrect attributes"
+
+    attr_keys = {entry[0] for entry in fvs}
+
+    for name, value in fvs:
+        if name in expected_attributes:
+            assert expected_attributes[name] == value, "Wrong value %s for the attribute %s = %s" % \
+                                               (value, name, expected_attributes[name])


I think this can be simplified down to wait_for_field_match from dvs_database.

daall · 2020-12-18T19:03:53Z

tests/test_evpn_tunnel.py

+
+
+def create_evpn_nvo(dvs, nvoname, tnl_name):
+    conf_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0)


You can use get_config_db from dvs here (and other places below where you construct the DBConnector).

daall · 2020-12-18T19:06:30Z

tests/test_evpn_tunnel.py

+            status, fvs = tbl.get(self.map_entry_map[tunnel_name + vidlist[x]])
+            assert status == False, "SIP Tunnel Map entry not deleted"


I would wait_for_deleted_entry here.

daall · 2020-12-18T19:12:49Z

tests/test_evpn_tunnel.py

+        tbl = swsscommon.Table(app_db, "VXLAN_TUNNEL_TABLE")
+        status, fvs = tbl.get(self.tunnel_appdb[tunnel_name])
+        assert status == False, "SIP Tunnel entry not deleted from APP_DB"
+
+        tbl = swsscommon.Table(asic_db, self.ASIC_TUNNEL_TERM_ENTRY)
+        status, fvs = tbl.get(self.tunnel_term[tunnel_name])
+        assert status == False, "SIP Tunnel Term entry not deleted from ASIC_DB"
+
+        tbl = swsscommon.Table(asic_db, self.ASIC_TUNNEL_TABLE)
+        status, fvs = tbl.get(self.tunnel[tunnel_name])
+        assert status == False, "SIP Tunnel entry not deleted from ASIC_DB"
+
+        tbl = swsscommon.Table(asic_db, self.ASIC_TUNNEL_MAP)
+        status, fvs = tbl.get(self.tunnel_map_map[tunnel_name][0])
+        assert status == False, "SIP Tunnel mapper0 not deleted from ASIC_DB"
+        status, fvs = tbl.get(self.tunnel_map_map[tunnel_name][1])
+        assert status == False, "SIP Tunnel mapper1 not deleted from ASIC_DB"
+        status, fvs = tbl.get(self.tunnel_map_map[tunnel_name][2])
+        assert status == False, "SIP Tunnel mapper2 not deleted from ASIC_DB"
+        status, fvs = tbl.get(self.tunnel_map_map[tunnel_name][3])
+        assert status == False, "SIP Tunnel mapper3 not deleted from ASIC_DB"


I think these can also be reduced to wait_for_deleted_entry.

daall · 2020-12-18T19:53:19Z

tests/test_evpn_tunnel.py

+        assert how_many_entries_exist(asic_db, self.ASIC_TUNNEL_MAP) == (len(self.tunnel_map_ids) + 4), "The TUNNEL_MAP wasn't created"
+        assert how_many_entries_exist(asic_db, self.ASIC_TUNNEL_MAP_ENTRY) == (len(self.tunnel_map_entry_ids) + 3), "The TUNNEL_MAP_ENTRY is created"
+        assert how_many_entries_exist(asic_db, self.ASIC_TUNNEL_TABLE) == (len(self.tunnel_ids) + 1), "The TUNNEL wasn't created"
+        assert how_many_entries_exist(asic_db, self.ASIC_TUNNEL_TERM_ENTRY) == (len(self.tunnel_term_ids) + 1), "The TUNNEL_TERM_TABLE_ENTRY wasm't created"


I think this can be reduced to wait_for_n_keys.

daall · 2020-12-18T20:04:07Z

tests/test_evpn_tunnel.py

+    )
+
+def remove_evpn_remote_vni(dvs, vlan_id, remote_vtep ):
+    app_db = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0)


There are also connectors you can use in dvs like get_app_db and get_asic_db.

daall

comments to be addressed in #1564

…f genetlink type hostif object id attribute (sonic-net#1318) Fix fast reboot failure due to invalid parsing of genetlink type hostif object id attribute. Genetlink type hostif does NOT have object ID attribute. SAI_HOSTIF_ATTR_OBJ_ID (SAI_HOSTIF_ATTR_TYPE == SAI_HOSTIF_TYPE_GENETLINK)

[Orchagent] pytest for EVPN VXLAN

96a3ecd

lguohan added the evpn label Nov 4, 2020

Changes to support Python 3.

eaab4cb

Tested with PR1266 changes.

prsunny previously approved these changes Dec 18, 2020

View reviewed changes

prsunny reviewed Dec 18, 2020

View reviewed changes

tests/test_evpn_tunnel.py Outdated Show resolved Hide resolved

msreddypaluru previously approved these changes Dec 18, 2020

View reviewed changes

Review comments incorporated

34cd49b

srj102 dismissed stale reviews from msreddypaluru and prsunny via 34cd49b December 18, 2020 18:43

daall suggested changes Dec 18, 2020

View reviewed changes

daall reviewed Dec 18, 2020

View reviewed changes

prsunny mentioned this pull request Dec 21, 2020

Use DVSDatabase instead of directly calling swsscommon #1564

Open

daall approved these changes Dec 21, 2020

View reviewed changes

prsunny merged commit 5859499 into sonic-net:master Dec 21, 2020

srj102 deleted the evpn_vxlan_pytest branch December 12, 2024 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VxlanOrch] pytest for EVPN VXLAN #1318

[VxlanOrch] pytest for EVPN VXLAN #1318

srj102 commented Jun 10, 2020 •

edited

Loading

srj102 commented Oct 23, 2020

srj102 commented Dec 14, 2020

srj102 commented Dec 14, 2020

prsunny left a comment

msreddypaluru left a comment

msreddypaluru Dec 18, 2020

srj102 Dec 18, 2020

msreddypaluru Dec 18, 2020

srj102 Dec 18, 2020

daall Dec 18, 2020

srj102 commented Dec 18, 2020

daall left a comment

daall Dec 18, 2020

daall Dec 18, 2020

daall Dec 18, 2020

daall Dec 18, 2020

daall Dec 18, 2020

daall Dec 18, 2020

daall Dec 18, 2020

daall Dec 18, 2020

daall Dec 18, 2020

daall Dec 18, 2020

daall Dec 18, 2020

daall left a comment



		def create_evpn_nvo(dvs, nvoname, tnl_name):
		conf_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0)

		status, fvs = tbl.get(self.map_entry_map[tunnel_name + vidlist[x]])
		assert status == False, "SIP Tunnel Map entry not deleted"

[VxlanOrch] pytest for EVPN VXLAN #1318

[VxlanOrch] pytest for EVPN VXLAN #1318

Conversation

srj102 commented Jun 10, 2020 • edited Loading

srj102 commented Oct 23, 2020

srj102 commented Dec 14, 2020

srj102 commented Dec 14, 2020

prsunny left a comment

Choose a reason for hiding this comment

msreddypaluru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srj102 commented Dec 18, 2020

daall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daall left a comment

Choose a reason for hiding this comment

srj102 commented Jun 10, 2020 •

edited

Loading