Hi, Thanks for the review comments.
Please see below for some comments. Thanks Numan On Thu, Jun 18, 2020 at 3:22 AM Gabriele Cerami <gcer...@redhat.com> wrote: > Hi, > > I added some note for my better understanding, but I have some questions > at the end on the testing part. > Sorry if they seem irrelevant or too picky, still learning. > > On 18 Jun, num...@ovn.org wrote: > > + expr_symtab_add_field(symtab, "pkt.mark", MFF_PKT_MARK, NULL, > false); > > + > > Note to self: this part maps the mnemonic string with the meta flow > field in OVS as described in include/openvswitch/meta-flow.h . > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > Note to self: the pkt.mark is enabled for both allow and reroute > policies > > > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > > Note to self: even if a value is interpreted as integer in the schema, all > values > are saved as strings (HEX compatibility ?) > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 9acb48ae5..f31ab9bbf 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -965,6 +965,22 @@ ip.ttl--; > > ip.ttl > > Syntax error at end of input expecting `--'. > > Missing a section index here ? # Packet mark > Ack. > > > +pkt.mark=1; > > + formats as pkt.mark = 1; > > + encodes as set_field:0x1->pkt_mark > > + > > +pkt.mark = 1000; > > + encodes as set_field:0x3e8->pkt_mark > > + > > +pkt.mark; > > + Syntax error at `pkt.mark' expecting action. > > + > > +pkt.mark = foo; > > + Syntax error at `foo' expecting field name. > > + > > +pkt.mark = "foo"; > > + Integer field pkt.mark is not compatible with string constant. > > + > > # load balancing. > > ct_lb; > > encodes as ct(table=19,zone=NXM_NX_REG13[0..15],nat) > > @@ -19898,3 +19914,154 @@ OVS_WAIT_UNTIL([test 0 = $(ovn-sbctl show | > grep Port_Binding -c)], [0]) > > > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > + > > +AT_SETUP([ovn -- Logical router policy packet marking]) > > +ovn_start > > + > > +net_add n1 > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > + set interface hv1-vif1 external-ids:iface-id=sw0-port1 \ > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > + options:rxq_pcap=hv1/vif1-rx.pcap \ > > + ofport-request=1 > > +ovs-vsctl -- add-port br-int hv1-vif2 -- \ > > + set interface hv1-vif2 external-ids:iface-id=sw0-port2 \ > > + options:tx_pcap=hv1/vif2-tx.pcap \ > > + options:rxq_pcap=hv1/vif2-rx.pcap \ > > + ofport-request=2 > > + > > +as hv1 ovs-vsctl set Open_vSwitch . > external-ids:ovn-bridge-mappings=public:br-phys > > + > > +ovn-nbctl ls-add sw0 > > +ovn-nbctl lsp-add sw0 sw0-port1 > > +ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:03 10.0.0.3" > > +ovn-nbctl lsp-set-port-security sw0-port1 "50:54:00:00:00:03 10.0.0.3" > > really nitpicking here .... 50:54:00 is a range reserved to cisco .... > local test should use a locally administered range :P > I don't know how it was chosen. I normally copy from the existing tests. > More seriously, is there a convention for the usage of differnt OUIs ?, > Not sure. > I see a mix of cisco, dataindustrier, not sure if different ranges have > different meanings in the tests. > All the tests are run locally. Are there any serious ramifications of using these different OUIs ? I just left the MACs AS IS now. > > + > > +ovn-nbctl lsp-add sw0 sw0-port2 > > +ovn-nbctl lsp-set-addresses sw0-port2 "50:54:00:00:00:04 10.0.0.4" > > + > > +ovn-nbctl lr-add lr0 > > +ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24 > > +ovn-nbctl lsp-add sw0 sw0-lr0 > > +ovn-nbctl lsp-set-type sw0-lr0 router > > +ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01 > > You are creating a router port with a mac, then creating a switch port > ion sw0 to connect to it, with the same mac. > Is there a reason for using the same mac ? > Maybe it doesn't really matter in OVS implementation, and the router > will probably never send a packet to the switch itself, but I would > imagine problems when a host sends a packet for the router and the > switch assumes it's for itself and doesn't forward it to other ports. > > In order to connect a logical switch to a logical router, we need to create a logical switch port and logical router port and kind of attach them. Either the MAC has to be the same or we can do ovn-nbctl lsp-set-addresses sw0-lr0 router. Both works. But I think setting to router is preferable. > +ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0 > > + > > +ovn-nbctl ls-add public > > +ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24 > > +ovn-nbctl lsp-add public public-lr0 > > +ovn-nbctl lsp-set-type public-lr0 router > > +ovn-nbctl lsp-set-addresses public-lr0 router > > +ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public > > + > > +# localnet port > > +ovn-nbctl lsp-add public ln-public > > +ovn-nbctl lsp-set-type ln-public localnet > > +ovn-nbctl lsp-set-addresses ln-public unknown > > +ovn-nbctl lsp-set-options ln-public network_name=public > > + > > +ovn-nbctl lrp-set-gateway-chassis lr0-public hv1 20 > > +ovn-nbctl lr-nat-add lr0 snat 172.168.0.100 10.0.0.0/24 > > +lr0_dp_uuid=$(ovn-sbctl --bare --columns _uuid list datapath_binding > lr0) > > +ovn-sbctl create mac_binding datapath=$lr0_dp_uuid ip=172.168.0.120 \ > > +logical_port=lr0-public mac="10\:54\:00\:00\:00\:03" > > + > > +ovn-nbctl --wait=hv sync > > + > > +# Add logical router polcy and set pkt_mark on it. > > nit: policy > > Ack. Done. > > +ovn-nbctl lr-policy-add lr0 2000 "ip4.src == 10.0.0.3" allow > > +ovn-nbctl lr-policy-add lr0 1000 "ip4.src == 10.0.0.4" allow > > + > > +pol1=$(ovn-nbctl --bare --columns _uuid find logical_router_policy > priority=2000) > > +pol2=$(ovn-nbctl --bare --columns _uuid find logical_router_policy > priority=1000) > > I don't see pol2 used anywhere in the following steps. 10.0.0.4 sources > (to which pol2 would be attached to ) are used the verify the absence of > marks. And a different mark replaces the first in the same policy pol1 > in later tests. > That was intentional to not mark anything for 2nd policy. I'll just delete this storing the uuid in pol2. > > +ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=100 > > + > > +OVS_WAIT_UNTIL([ > > + test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \ > > + grep "load:0x64->NXM_NX_PKT_MARK" -c) > > +]) > > + > > +AT_DATA([flows.txt], [dnl > > +table=0, priority=0 actions=NORMAL > > +table=0, priority=100, pkt_mark=0x64 actions=drop > > +table=0, priority=100, pkt_mark=0x2 actions=drop > > +]) > > + > > +AT_CHECK([as hv1 ovs-ofctl --protocols=OpenFlow13 add-flows br-phys > flows.txt]) > > + > > +ip_to_hex() { > > + printf "%02x%02x%02x%02x" "$@" > > +} > > + > > +send_ipv4_pkt() { > > + local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 > > + local ip_src=$5 ip_dst=$6 > > + > packet=${eth_dst}${eth_src}08004500001c0000000040110000${ip_src}${ip_dst}0035111100080000 > > + as $hv ovs-appctl netdev-dummy/receive ${inport} ${packet} > > +} > > + > > +send_ipv4_pkt hv1 hv1-vif1 505400000003 00000000ff01 \ > > + $(ip_to_hex 10 0 0 3) $(ip_to_hex 172 168 0 120) > > + > > +OVS_WAIT_UNTIL([ > > + test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \ > > + grep priority=100 | grep "priority=100,pkt_mark=0x64" | \ > > The regex on the first grep is contained in the regex for the second. > Do you need both for some reason ? > I'll remove the redundant one in v2. > > + grep "n_packets=1" -c) > > +]) > > + > > +OVS_WAIT_UNTIL([ > > + test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \ > > + grep priority=0 | \ > > + grep "n_packets=0" -c) > > +]) > > + > > +# Send the pkt from sw0-port2. Packet should not be marked. > > +send_ipv4_pkt hv1 hv1-vif2 505400000004 00000000ff01 \ > > + $(ip_to_hex 10 0 0 4) $(ip_to_hex 172 168 0 120) > > + > > +OVS_WAIT_UNTIL([ > > + test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \ > > + grep priority=0 | \ > > + grep "n_packets=1" -c) > > +]) > > + > > +OVS_WAIT_UNTIL([ > > + test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \ > > + grep priority=100 | grep "priority=100,pkt_mark=0x64" | \ > > Same for the grep here. > > > + grep "n_packets=1" -c) > > +]) > > + > > + > > +ovn-nbctl set logical_router_policy $pol1 options:pkt_mark=2 > > +send_ipv4_pkt hv1 hv1-vif1 505400000003 00000000ff01 \ > > + $(ip_to_hex 10 0 0 3) $(ip_to_hex 172 168 0 120) > > + > > +OVS_WAIT_UNTIL([ > > + test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \ > > + grep "load:0x2->NXM_NX_PKT_MARK" -c) > > +]) > > + > > +OVS_WAIT_UNTIL([ > > + test 0 -eq $(as hv1 ovs-ofctl dump-flows br-int table=19 | \ > > + grep "load:0x64->NXM_NX_PKT_MARK" -c) > > +]) > > + > > +OVS_WAIT_UNTIL([ > > + test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \ > > + grep priority=100 | grep "priority=100,pkt_mark=0x2" | \ > > Same for the grep here. > > > + grep "n_packets=1" -c) > > +]) > > + > > +OVS_WAIT_UNTIL([ > > + test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \ > > + grep priority=0 | \ > > + grep "n_packets=1" -c) > > +]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > Is there any need to add a test for the reroute policy case ? > Or a test for seeing the pkt.mark option not present after removing the > policy/options:pkt_mark ? > I'll add a test for this in v2. Thanks Numan > > -- > > 2.26.2 > > > > _______________________________________________ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev