Add initial PTF tests for downlink and uplink #5

badhrinathpa · 2020-04-14T04:06:36Z

No description provided.

ccascone · 2020-04-14T04:23:31Z

Interestingly, tests failed but the status check is green. I will fix the status reporting. Here's the test error:

spgwu.GTPU_far_Test ... tcp ... ERROR

======================================================================
ERROR: spgwu.GTPU_far_Test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./tests/spgwu.py", line 84, in runTest
    pkt_add_gtp(pkt, out_ipv4_src, out_ipv4_dst, src_teid)
NameError: global name 'pkt_add_gtp' is not defined

ccascone

Test case LGTM. Comments are more about style.

ccascone · 2020-04-15T05:06:51Z

ptf/tests/spgwu.py

+UDP_SRC_PORT = None
+UDP_DST_PORT = None
+PKT_SRC_IP = None
+PKT_DST_IP = None


These variables get assigned as part of the test case, so they're not constants (as the case suggests). Let's remove them from here, and simply have (lower_case) variables as part of the test case.

ccascone · 2020-04-15T05:08:55Z

ptf/tests/spgwu.py

+        UDP_SRC_PORT = self.inner_pkt[UDP].sport
+        UDP_DST_PORT = self.inner_pkt[UDP].dport
+        PKT_SRC_IP = self.inner_pkt[IP].src
+        PKT_DST_IP = self.inner_pkt[IP].dst


Let's use lower case here, these are not constant values.

ccascone · 2020-04-15T05:09:22Z

ptf/tests/spgwu.py

+        UDP_SRC_PORT = self.inner_pkt[UDP].sport
+        UDP_DST_PORT = self.inner_pkt[UDP].dport
+        PKT_SRC_IP = self.inner_pkt[IP].src
+        PKT_DST_IP = self.inner_pkt[IP].dst
+        '''
+        for teid, src_addr, dst_addr, src_port, dst_port, proto, pdr_id in \
+           zip([(src_teid, 0xffffffff),(0xfffff,0)], \
+           [(PKT_SRC_IP,IP_MASK), (PKT_DST_IP,IP_MASK)], \
+           [(PKT_DST_IP,IP_MASK), (PKT_DST_IP,IP_MASK)], \
+           [(UDP_SRC_PORT, UDP_SRC_PORT), (UDP_DST_PORT,UDP_DST_PORT)], \
+           [(UDP_DST_PORT, UDP_DST_PORT), (UDP_SRC_PORT, UDP_SRC_PORT)], \
+#range still not sure how to write
+           [(IP_PROTO_UDP, 0xFF), (IP_PROTO_UDP, 0xFF)], \
+           [UL_PDR_ID, DL_PDR_ID]):
+                self.insert(self.helper.build_table_entry(
+                   table_name="IngressPipeImpl.pdrs",
+                   match_fields={
+                   # Exact match.
+                     "teid": teid,
+                     "ue_addr": src_addr,
+                     "inet_addr": dst_addr,
+                     "ue_l4_port":src_port,
+                     "inet_l4_port":dst_port,
+                     "ip_proto":proto
+                },
+                action_name="IngressPipeImpl.set_pdr_id",
+                action_params={"id":pdr_id},
+                priority = 1
+                ))
+        '''


Can we remove this commented block?

ccascone · 2020-04-15T05:09:46Z

ptf/tests/spgwu.py

+#self.testPacket(pkt)
+
+#pkt[IP].payload[IP]


Looks like these are leftover comments. Let's remove them.

ccascone · 2020-04-15T05:11:28Z

ptf/tests/spgwu.py

+            "ue_l4_port":(UDP_SRC_PORT, UDP_SRC_PORT),
+            "inet_l4_port":(UDP_DST_PORT, UDP_DST_PORT),


The same match value is used as mask. I think it should be all ones (16 bits for L4 ports, so 0xFFFF).

Ops! my bad, I didn't realize we use range match for ports. I tought it was ternary. Please ignore my comment...

ccascone · 2020-04-15T05:17:56Z

ptf/tests/spgwu.py

+        pkt_route(exp_pkt, next_hop_mac)
+        pkt_decrement_ttl(exp_pkt)
+#exp_pkt = pkt.copy()
+        print self.port1


This print statement seems useful for debugging, perhaps it can be removed from the commit

ccascone · 2020-04-15T05:19:02Z

ptf/tests/spgwu.py

+SPGW_DIR_UNKNOWN = 0;
+SPGW_DIR_UPLINK = 1;
+SPGW_DIR_DOWNLINK = 2;


Are these used anywhere? I think there are other constants that are not used in the test cases. We should avoid declaring unused stuff which otherwise makes the code hard to read/confusing.

not used. removed.

ccascone · 2020-04-15T05:21:46Z

ptf/tests/spgwu.py

+        for pkt_type in ["udp"]:
+              print_inline("%s ... " % pkt_type)
+              pkt = getattr(testutils, "simple_%s_packet" % pkt_type)()
+              self.inner_pkt = pkt


I think you could avoid using this class-level inner_pkt variable/attribute, as you are already passing pkt as an argument of self.testPacket()...

The expected packet is without the gtpu, outer ip and outer udp header. So, storing and using the inner_pkt as expected pkt. We were not able to handle packet access beyond the gtpu in the argument pkt.

I see. When possible I prefer the functional approach for tests (i.e., everything is a function argument, no class-level variables) to reduce the risk of ending up in spaghetti code as the tests evolve. It looks like you could be passing inner_pkt as an argument of self.testPacket() and invoke self.pkt_add_gtp() from within self.testPacket() after storing an unmmodified version of the packet. Here's a similar test that does pppoe encap for the bng case: https://github.com/opennetworkinglab/tassen/blob/master/ptf/tests/upstream.py#L115

ccascone · 2020-04-15T05:22:24Z

ptf/tests/spgwu.py

+                ))
+
+        # Add entry to pdr_rule_lookup
+        UDP_SRC_PORT = self.inner_pkt[UDP].sport


Same as before, it's not clear to me why you need to define inner_pkt. Can't you use pkt from the function arguments?

Same as above.

ccascone · 2020-04-15T05:23:10Z

ptf/tests/spgwu.py

+
+        # Expected pkt should have routed MAC addresses and decremented hop
+        # limit (TTL).
+        exp_pkt = self.inner_pkt.copy()


Same here, you can use pkt instead of inner_pkt

same as above.

ccascone

LGTM. I left some comments, but we can fix later.

ccascone · 2020-04-15T22:28:18Z

ptf/tests/spgwu.py

+UDP_HDR_SIZE = 8
+GTP_HDR_SIZE = 8
+IP_VERSION_4 = 4
+out_ipv4_src = '192.168.0.202'


It looks these are constants, so using the upper case is preferred

ccascone · 2020-04-17T00:58:11Z

ptf/lib/helper.py

+    def read_pkt_count_pre_qos_pdr(self, line_id):
+        return self.read_pkt_count("IngressPipeImpl.pre_qos_pdr_counter", line_id)
+
+    def read_byte_count_pre_qos_pdr(self, line_id):
+        return self.read_byte_count("IngressPipeImpl.pre_qos_pdr_counter", line_id)


This helper is meant to be P4 program agnostic. We don't need to fix this now, but in general, methods that are program-specific should go in a separate class, e.g. a client class with multiple implementations for the different targets (one for bmv2, one for tofino, etc.). Then we can think of ways to plug one implementation or the other when running the test cases... it would be good to fix after tonight's meeting.

ccascone · 2020-04-17T01:00:56Z

ptf/lib/helper.py

+    def read_request(self, req):
+        entities = []
+        grpc_addr = testutils.test_param_get("grpcaddr")
+        channel = grpc.insecure_channel(grpc_addr)
+        stub = p4runtime_pb2_grpc.P4RuntimeStub(channel)
+        try:
+            for resp in stub.Read(req):
+                entities.extend(resp.entities)
+        except grpc.RpcError as e:
+            if e.code() != grpc.StatusCode.UNKNOWN:
+                raise e
+            raise P4RuntimeException(e)
+        return entities
+
+    def write_request(self, req, store=True):
+        rep = self._write(req)
+        if store:
+            self.reqs.append(req)
+        return rep
+
+    def get_new_write_request(self):
+        req = p4runtime_pb2.WriteRequest()
+        req.device_id = int(testutils.test_param_get("device_id"))
+        election_id = req.election_id
+        election_id.high = 0
+        election_id.low = self.election_id
+        return req


Methods that deal with P4Runtime RPCs should go in base_test.py. As a matter of fact, there's already a grpc channel and a method for write request there. This helper is for constructing P4Runtime messages from a given P4Info (i.e. using table/counter names instead of IDs)

ccascone · 2020-04-17T01:08:23Z

ptf/tests/spgwu.py

+        for pkt_type in ["udp"]:
+              print_inline("%s ... " % pkt_type)
+              pkt = getattr(testutils, "simple_%s_packet" % pkt_type)()
+              self.inner_pkt = pkt


I see. When possible I prefer the functional approach for tests (i.e., everything is a function argument, no class-level variables) to reduce the risk of ending up in spaghetti code as the tests evolve. It looks like you could be passing inner_pkt as an argument of self.testPacket() and invoke self.pkt_add_gtp() from within self.testPacket() after storing an unmmodified version of the packet. Here's a similar test that does pppoe encap for the bng case: https://github.com/opennetworkinglab/tassen/blob/master/ptf/tests/upstream.py#L115

ccascone · 2020-04-17T01:12:38Z

ptf/tests/spgwu.py

+class GTPU_far_DOWNLINK_Test(P4RuntimeTest):
+    """Tests GTPU routing"""
+
+    outer_pkt = None


same here, no need to be class-level

ccascone · 2020-04-17T01:13:31Z

ptf/tests/spgwu.py

+    """Tests GTPU routing"""
+
+    inner_pkt = None
+    ulfseid = 1


If ulfseid is meant to be a constant, then it should go outside of this class and be upper case

ccascone · 2020-04-17T01:14:27Z

ptf/tests/spgwu.py

+    """Tests GTPU routing"""
+
+    outer_pkt = None
+    dlfseid = 2


same as before, this should go out of this class and be upper case, since it's used as a constant

ptf_test

b3cb871

ccascone mentioned this pull request Apr 14, 2020

CircleCI reports success even when tests fail #6

Closed

badhrinathpa added 2 commits April 15, 2020 00:04

ptf test run for uplink

b4a498c

ptf test downlink

e2a25e0

ccascone requested changes Apr 15, 2020

View reviewed changes

badhrinathpa added 4 commits April 15, 2020 13:01

review comment fix

1777de1

counter read update

5471a0c

merge with master

6f63cc4

ptf update for new p4

4fca9a1

ccascone approved these changes Apr 17, 2020

View reviewed changes

ccascone changed the title ~~ptf_test~~ Add initial PTF tests for downlink and uplink Apr 17, 2020

ccascone merged commit 82386da into master Apr 17, 2020

ccascone deleted the ptf_update branch April 17, 2020 01:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial PTF tests for downlink and uplink #5

Add initial PTF tests for downlink and uplink #5

badhrinathpa commented Apr 14, 2020

ccascone commented Apr 14, 2020 •

edited

ccascone left a comment

ccascone Apr 15, 2020

badhrinathpa Apr 15, 2020

ccascone Apr 15, 2020

badhrinathpa Apr 15, 2020

ccascone Apr 15, 2020

badhrinathpa Apr 15, 2020

ccascone Apr 15, 2020

badhrinathpa Apr 15, 2020

ccascone Apr 15, 2020

ccascone Apr 15, 2020 •

edited

ccascone Apr 15, 2020

badhrinathpa Apr 15, 2020

ccascone Apr 15, 2020

badhrinathpa Apr 15, 2020

ccascone Apr 15, 2020

badhrinathpa Apr 15, 2020

ccascone Apr 17, 2020

ccascone Apr 15, 2020

badhrinathpa Apr 15, 2020

ccascone Apr 15, 2020

badhrinathpa Apr 15, 2020

ccascone left a comment

ccascone Apr 15, 2020

ccascone Apr 17, 2020

ccascone Apr 17, 2020

ccascone Apr 17, 2020

ccascone Apr 17, 2020

ccascone Apr 17, 2020

ccascone Apr 17, 2020

		"ue_l4_port":(UDP_SRC_PORT, UDP_SRC_PORT),
		"inet_l4_port":(UDP_DST_PORT, UDP_DST_PORT),

		#self.testPacket(pkt)

		#pkt[IP].payload[IP]

Add initial PTF tests for downlink and uplink #5

Add initial PTF tests for downlink and uplink #5

Conversation

badhrinathpa commented Apr 14, 2020

ccascone commented Apr 14, 2020 • edited

ccascone 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

ccascone Apr 15, 2020 • edited

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

ccascone 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

ccascone commented Apr 14, 2020 •

edited

ccascone Apr 15, 2020 •

edited