Adds CNI check for table ls_out_port_security flows

[trozet]

There is a race condition when we return that CNI ADD is complete where
a pod that immediately starts and tries to access the network may fail,
because not all OVS flows are present. Specifically we are missing the
check for flows for ls_out_port_security, table 48. Today we only check
for flows in table 9 port security, and table 48 flows may come a few
seconds later.

Signed-off-by: Tim Rozet trozet@redhat.com

[trozet]

@girishmg @vishnoianil @dceara @abhat

[coveralls]

Coverage decreased (-0.1%) to 56.665% when pulling 79d5d8b on trozet:check_table_48 into 28ac7de on ovn-org:master.

[vishnoianil]

I am not sure batching the query will work here with ovs-ofctl? But even if it works, it can give you false positive, in a scenario where only one table has a flow but second table doesn't. Stdout will still have some flow as a output, but that doesn't necessary mean that both the tables has the required flow. So seems like you need to separate the query here ?

[trozet]

If the 2nd iteration didn't have an entry or failed, then stdout would still be set to the output of that command. If either of those checks fail for any query, it will return false and the function will start over until all queries are successful.

[girishmg]

@trozet

OVS table 48 is SB table ls_out_stateful
OVS table 49 is SB table ls_out_port_sec_ip
OVS table 50 is SB table ls_out_port_sec_l2

This is in September 2020 OVN release. I think you just need to check for table 50 for the MAC address?

  table=10(ls_out_port_sec_l2 ), priority=50   , match=(outport == "kube-system_coredns-6955765f44-cpdzh" && eth.dst == {0a:58:c0:a8:01:03}), action=(output;)
    table=50 reg15=0x3,metadata=0x4,dl_dst=0a:58:c0:a8:01:03 actions=resubmit(,64)

If the OpenFlow rules are written serially starting from Table-1 to all the way up to Table-50, then wouldn't it suffice to just check for dl_dst=<mac_address> in Table 50?

[dceara]

@trozet

OVS table 48 is SB table ls_out_stateful
OVS table 49 is SB table ls_out_port_sec_ip
OVS table 50 is SB table ls_out_port_sec_l2

This is in September 2020 OVN release. I think you just need to check for table 50 for the MAC address?

@girishmg Sorry, this is my fault, I had suggested table=48 for ls_out_port_sec_ip to @trozet based on ovn20.06. I had forgotten that in ovn20.09 there's a new input/output stage being added to optimize in/out ACLs so indeed ls_out_port_sec_ip will become table=49.

Unfortunately there's no guarantee that new stages/tables will not be added or removed in the future in OVN. Until we find a better way to ensure that all latest NB changes are propagated to OVS (e.g., suggested here) it might be better to use a more flexible check. What about leaving the table id out or checking a range of tables instead of a single table?

  table=10(ls_out_port_sec_l2 ), priority=50   , match=(outport == "kube-system_coredns-6955765f44-cpdzh" && eth.dst == {0a:58:c0:a8:01:03}), action=(output;)
    table=50 reg15=0x3,metadata=0x4,dl_dst=0a:58:c0:a8:01:03 actions=resubmit(,64)

If the OpenFlow rules are written serially starting from Table-1 to all the way up to Table-50, then wouldn't it suffice to just check for dl_dst=<mac_address> in Table 50?

This might not always be the case. Flows for different tables are different records in the Logical_Flow table in the SB. In this specific case flows might be created in the same ovn-northd -> SB transaction but there's no guarantee that these records will be sent ordered by table id to ovn-controller and in turn to OVS. I need to double check but i think this can cause that flows for table=50 get installed before flows for table=49.

[trozet]

I dont think we can dump and filter on all flows. The perf impact will be too high. I'll modify the patch to try multiple tables. In the future we need a better signal to know pod is fully wired.

[abhat]

LGTM.

[trozet]

/hold

looks like something with ipv6 didnt match correctly

[trozet]

/retest

[danwinship]

lgtm

[vishnoianil]

@trozet
OVS table 48 is SB table ls_out_stateful
OVS table 49 is SB table ls_out_port_sec_ip
OVS table 50 is SB table ls_out_port_sec_l2
This is in September 2020 OVN release. I think you just need to check for table 50 for the MAC address?

@girishmg Sorry, this is my fault, I had suggested table=48 for ls_out_port_sec_ip to @trozet based on ovn20.06. I had forgotten that in ovn20.09 there's a new input/output stage being added to optimize in/out ACLs so indeed ls_out_port_sec_ip will become table=49.

Unfortunately there's no guarantee that new stages/tables will not be added or removed in the future in OVN. Until we find a better way to ensure that all latest NB changes are propagated to OVS (e.g., suggested here) it might be better to use a more flexible check. What about leaving the table id out or checking a range of tables instead of a single table?

  table=10(ls_out_port_sec_l2 ), priority=50   , match=(outport == "kube-system_coredns-6955765f44-cpdzh" && eth.dst == {0a:58:c0:a8:01:03}), action=(output;)
    table=50 reg15=0x3,metadata=0x4,dl_dst=0a:58:c0:a8:01:03 actions=resubmit(,64)

If the OpenFlow rules are written serially starting from Table-1 to all the way up to Table-50, then wouldn't it suffice to just check for dl_dst=<mac_address> in Table 50?

This might not always be the case. Flows for different tables are different records in the Logical_Flow table in the SB. In this specific case flows might be created in the same ovn-northd -> SB transaction but there's no guarantee that these records will be sent ordered by table id to ovn-controller and in turn to OVS.

@dceara will it create performance issue if northd send the records by table-id? I think having a deterministic order here would be helpful for consumers like ovn-k8s to write better logic and probably the performance impact is higher at ovn-k8s level, and they need to fire a transaction per pod. thoughts? @trozet @abhat thoughts?

I need to double check but i think this can cause that flows for table=50 get installed before flows for table=49.

[dceara]

If the OpenFlow rules are written serially starting from Table-1 to all the way up to Table-50, then wouldn't it suffice to just check for dl_dst=<mac_address> in Table 50?

This might not always be the case. Flows for different tables are different records in the Logical_Flow table in the SB. In this specific case flows might be created in the same ovn-northd -> SB transaction but there's no guarantee that these records will be sent ordered by table id to ovn-controller and in turn to OVS.

@dceara will it create performance issue if northd send the records by table-id? I think having a deterministic order here would be helpful for consumers like ovn-k8s to write better logic and probably the performance impact is higher at ovn-k8s level, and they need to fire a transaction per pod. thoughts? @trozet @abhat thoughts?

@vishnoianil northd creates records in the SB DB, it has no control about how SB sends the updates to ovn-controller. Records are hashed in the DB according to the record UUID which is generated at transaction commit time (from northd to SB). According to the monitor conditions issued by ovn-controller, ovsdb-server (SB) sends the updates to ovn-controller (in the order they're stored in memory. In my opinion the best solution is indeed to have transaction per pod in ovn-k8s and have a working mechanism in OVN to ensure that a transaction is completely processed by ovn-controller on a given node. This is what this OVN RFE is about.