Fix SAI object create error in libsai adapter

[mukeshmv]

Populate SAI obj-id as P4 match key.
No change in SAI API headers.

Fixes #166

docker exec -w /tests/libsai/vnet_out simple_switch-mukesh ./vnet_out
GRPC call SetForwardingPipelineConfig 0.0.0.0:9559 => /etc/dash/dash_pipeline.json, /etc/dash/dash_pipeline_p4rt.txt
GRPC call Write::INSERT OK
GRPC call Write::INSERT OK
GRPC call Write::INSERT OK
GRPC call Write::INSERT OK
GRPC call Write::INSERT OK
GRPC call Write::INSERT OK
GRPC call Write::DELETE OK
GRPC call Write::DELETE OK
GRPC call Write::DELETE OK
GRPC call Write::DELETE OK
GRPC call Write::DELETE OK
GRPC call Write::DELETE OK
Done.

Match key:
* hdr.vxlan.vni:VNI   : EXACT     3c0000
Action entry: dash_ingress.set_outbound_direction -

[01:04:43.937] [bmv2] [D] [thread 40] Entry 0 added to table 'dash_ingress.dash_acl_group|dash_acl'
[01:04:43.937] [bmv2] [D] [thread 40] Dumping entry 0
Match key:
* meta.stage1_dash_acl_group_id:dash_acl_group_id: EXACT     0001
Action entry: dash_ingress.set_acl_group_attrs - 0,

[01:04:43.938] [bmv2] [D] [thread 16] Entry 1 added to table 'dash_ingress.dash_acl_group|dash_acl'
[01:04:43.938] [bmv2] [D] [thread 16] Dumping entry 1
Match key:
* meta.stage1_dash_acl_group_id:dash_acl_group_id: EXACT     0002
Action entry: dash_ingress.set_acl_group_attrs - 0,

[01:04:43.939] [bmv2] [D] [thread 40] Entry 0 added to table 'dash_ingress.eni|dash'
[01:04:43.939] [bmv2] [D] [thread 40] Dumping entry 0
Match key:
* meta.eni_id:eni_id  : EXACT     0003
Action entry: dash_ingress.set_eni_attrs - 27100000,86a00000,86a00000,1,10310ac,90000,1,1,1,1,1,0,0,0,0,0,2,2,2,2,2,0,0,0,0,0,

[01:04:43.939] [bmv2] [D] [thread 16] Entry 0 added to table 'dash_ingress.eni_ether_address_map|dash'
[01:04:43.939] [bmv2] [D] [thread 16] Dumping entry 0
Match key:
* meta.eni_addr:address: EXACT     aacccccccccc
Action entry: dash_ingress.set_eni - 3,

[01:04:43.940] [bmv2] [D] [thread 40] Entry 0 added to table 'dash_ingress.outbound.eni_to_vni|dash_vnet'
[01:04:43.940] [bmv2] [D] [thread 40] Dumping entry 0
Match key:
* meta.eni_id:eni_id  : EXACT     0003
Action entry: dash_ingress.outbound.set_vni - 90000,

[01:04:43.940] [bmv2] [D] [thread 16] Removed entry 0 from table 'dash_ingress.outbound.eni_to_vni|dash_vnet'
[01:04:43.941] [bmv2] [D] [thread 40] Removed entry 0 from table 'dash_ingress.eni_ether_address_map|dash'
[01:04:43.941] [bmv2] [D] [thread 16] Removed entry 0 from table 'dash_ingress.eni|dash'
[01:04:43.941] [bmv2] [D] [thread 40] Removed entry 1 from table 'dash_ingress.dash_acl_group|dash_acl'
[01:04:43.942] [bmv2] [D] [thread 16] Removed entry 0 from table 'dash_ingress.dash_acl_group|dash_acl'
[01:04:43.942] [bmv2] [D] [thread 40] Removed entry 0 from table 'dash_ingress.direction_lookup|dash'

[chrispsommers]

Hi @mukeshmv I think it'd be good to add tests in vnet_out.cpp to DELETE objects as well. I did this in the course of pending #164 and discovered an error in the libsai adaptor for DELETE: #158

Please see my submitted changes in #164 for dash-pipeline/SAI/templates/saiapi.cpp.j2, dash-pipeline/SAI/templates/utils.cpp.j2 and dash-pipeline/tests/libsai/vnet_out/vnet_out.cpp and consider applying the template fix and adding similar delete operations in reverse order. It might expose some hidden issue of the changes you're proposing (I suppose it'd be nicer if I'd already submitted these fixes in a separate PR but I was working on saithrift at the time.)

[mukeshmv]

Hi @mukeshmv I think it'd be good to add tests in vnet_out.cpp to DELETE objects as well. I did this in the course of pending #164 and discovered an error in the libsai adaptor for DELETE: #158

Thanks for the suggestion @chrispsommers. I tried reverse object deletes including the ENI object with your SAI template "==" fix and it went through fine. So this change does not introduce any new issues for Delete operation. I will add this ENI object to the delete test after your fix gets merged.

[chrispsommers]

Hi @mukeshmv I think it'd be good to add tests in vnet_out.cpp to DELETE objects as well. I did this in the course of pending #164 and discovered an error in the libsai adaptor for DELETE: #158

Thanks for the suggestion @chrispsommers. I tried reverse object deletes including the ENI object with your SAI template "==" fix and it went through fine. So this change does not introduce any new issues for Delete operation. I will add this ENI object to the delete test after your fix gets merged.
@mukeshmv thanks for trying it out, that is good validation! The fix will get merged soon.

[mukeshmv]

@chrispsommers I have added the ENI object create and delete to the ptf and pytest as well. Pls check.

[mukeshmv]

Test job fail is due to conflict with recent ACL group PR merge. Will fix the tests.

[mukeshmv]

@chrispsommers I am keeping my sai thrift test changes commented out for now.

They were working initially, but I am noticing that after rebasing to the tip where new P4 tables and parameters have been added to the dash pipeline the sai thrift test is not configuring the P4 correctly.
I suspect this is an artifact of using pre-built saithrift client and server docker container images, probably the latest SAI API changes are not reflected in the auto-generated rpc code. Because the vnet_out test is working fine.

Can you update the containers and let me know I can try again ?

[chrispsommers]

Hi @mukeshmv, two questions:

sai thrift test is not configuring the P4 correctly.

1. Can you explain further - is there an actual error thrown, or are you saying the resulting configuration isn't correct?

The pre-built containers don't include build artifacts, they are "builders" and contain only tools. The artifacts are modified in your host filesystem using volume mounts. See volume-mounts. The only exception is the saithrift client which contains python saithrift wrappers based on the P4 code -> sai header generation. That container is always rebuilt in-place, not downloaded.
2. Are you doing make all to ensure all the saithrift artifacts, including the saithrift client docker, are being rebuilt?
We can set up a 1:1 to look at it together, let me know.

[mukeshmv]

1. Can you explain further - is there an actual error thrown, or are you saying the resulting configuration isn't correct?

@chrispsommers thanks for the clarification regarding the docker containers. Strangely no error is thrown. But I dont see any output in the thrift server or the libsai / P4 simple_switch for this

            self.in_acl_group_id = sai_thrift_create_dash_acl_group(self.client,
                                                                    ip_addr_family=0)
            assert (self.in_acl_group_id != SAI_NULL_OBJECT_ID)

And then the assert fails because no acl_group_id is returned from the create.

Whereas I checked that the below call that I added is indeed hitting the thrift server, and is failing in the libasi with the error that required parameters are missing (this is known and it needs the above object id created from the dash acl group create)

            self.eni = sai_thrift_create_eni(self.client, cps=10000,
                                             pps=100000, flows=100000,
                                             admin_state=True,
                                             vm_underlay_dip=vm_underlay_dip,
                                             vm_vni=9)

All of this passes in the vnet_out test but hits the same issue in both the ptf and pytest thrift tests. The only difference is that the eni is an existing P4 table / SAI API whereas the dash ACL group is a new one. I do see thrift code for the dash ACL group is getting generated. But I am not an expert so maybe there is something missing. If it's not the thrift client then could the issue be with the server ?

2. Are you doing make all to ensure all the saithrift artifacts, including the saithrift client docker, are being rebuilt?
   We can set up a 1:1 to look at it together, let me know.

Yes I am executing below steps. Sure, lets have a 1:1.

make clean 
make all
make run-switch
make run-saithrift-server
make run-saithrift-dev-ptftests / make run-saithrift-dev-pytests

[mukeshmv]

Turns out that the sai_api_query was not returning the Dash ACL impl table. Adding that fixed the saithrift issue. Thanks @chrispsommers for your help debugging.

[chrispsommers]

LGTM, I mainly reviewed the utils.cpp.j2 and test cases, not the API generator. Nicely done.