[ovs-dev] [PATCH v2 ovn] northd: add empty_lb controller_event for logical router
Lorenzo Bianconi
lorenzo.bianconi at redhat.com
Sat Sep 7 17:28:47 UTC 2019
>
> Hi Lorenzo, I only have one finding down below
>
> On 9/6/19 11:00 AM, Lorenzo Bianconi wrote:
> > Add empty load balancer controller_event support to logical router
> > pipeline. Update northd documentation even for logical switch pipeline
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
> > ---
> > Changes since v1:
> > - rebase on-top of current ovn master branch
> > ---
> > northd/ovn-northd.8.xml | 10 ++++++++
> > northd/ovn-northd.c | 26 ++++++++++++-------
> > tests/ovn.at | 56 +++++++++++++++++++++++++++++++++++------
> > 3 files changed, 76 insertions(+), 16 deletions(-)
> >
[...]
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -6148,9 +6148,15 @@ get_force_snat_ip(struct ovn_datapath *od, const char *key_type, ovs_be32 *ip)
> > static void
> > add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
> > struct ds *match, struct ds *actions, int priority,
> > - const char *lb_force_snat_ip, char *backend_ips,
> > - bool is_udp, int addr_family)
> > + const char *lb_force_snat_ip, struct smap_node *node,
>
> I don't understand the change from "backend_ips" to "node" here. You
> only ever access node->value in the function, and the name "node" is not
> very descriptive. I think this should stay how it currently is.
>
Hi Mark,
thx for the review. I did this change since I would reuse
build_empty_lb_event_flow() and it will need both node->value and
node->key.
Regards,
Lorenzo
> > + bool is_udp, int addr_family, char *ip_addr,
> > + uint16_t l4_port, struct nbrec_load_balancer *lb,
> > + struct shash *meter_groups)
> > {
> > + build_empty_lb_event_flow(od, lflows, node, ip_addr, lb,
> > + l4_port, addr_family, S_ROUTER_IN_DNAT,
> > + meter_groups);
> > +
> > /* A match and actions for new connections. */
> > char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
> > if (lb_force_snat_ip) {
> > @@ -6177,7 +6183,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
> > free(new_match);
> > free(est_match);
> >
> > - if (!od->l3dgw_port || !od->l3redirect_port || !backend_ips) {
> > + if (!od->l3dgw_port || !od->l3redirect_port || !node->value) {
> > return;
> > }
> >
> > @@ -6192,7 +6198,7 @@ add_router_lb_flow(struct hmap *lflows, struct ovn_datapath *od,
> > ds_put_cstr(&undnat_match, "ip6 && (");
> > }
> > char *start, *next, *ip_str;
> > - start = next = xstrdup(backend_ips);
> > + start = next = xstrdup(node->value);
> > ip_str = strsep(&next, ",");
> > bool backend_ips_found = false;
> > while (ip_str && ip_str[0]) {
> > @@ -6308,7 +6314,7 @@ copy_ra_to_sb(struct ovn_port *op, const char *address_mode)
> >
> > static void
> > build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> > - struct hmap *lflows)
> > + struct hmap *lflows, struct shash *meter_groups)
> > {
> > /* This flow table structure is documented in ovn-northd(8), so please
> > * update ovn-northd.8.xml if you change anything. */
> > @@ -7525,7 +7531,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> > ds_put_format(&match, "ip && ip6.dst == %s",
> > ip_address);
> > }
> > - free(ip_address);
> >
> > int prio = 110;
> > bool is_udp = lb->protocol && !strcmp(lb->protocol, "udp") ?
> > @@ -7546,8 +7551,11 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
> > od->l3redirect_port->json_key);
> > }
> > add_router_lb_flow(lflows, od, &match, &actions, prio,
> > - lb_force_snat_ip, node->value, is_udp,
> > - addr_family);
> > + lb_force_snat_ip, node, is_udp,
> > + addr_family, ip_address, port, lb,
> > + meter_groups);
> > +
> > + free(ip_address);
> > }
> > }
> > sset_destroy(&all_ips);
> > @@ -8328,7 +8336,7 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
> >
> > build_lswitch_flows(datapaths, ports, port_groups, &lflows, mcgroups,
> > igmp_groups, meter_groups);
> > - build_lrouter_flows(datapaths, ports, &lflows);
> > + build_lrouter_flows(datapaths, ports, &lflows, meter_groups);
> >
> > /* Push changes to the Logical_Flow table to database. */
> > const struct sbrec_logical_flow *sbflow, *next_sbflow;
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index de1b3b3ba..2749184eb 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -14683,9 +14683,22 @@ ovn_start
> > # Create hypervisors hv[12].
> > # Add vif1[12] to hv1, vif2[12] to hv2
> > # Add all of the vifs to a single logical switch sw0.
> > +# Create logical router lr0
> >
> > net_add n1
> > -ovn-nbctl ls-add sw0
> > +
> > +ovn-nbctl create Logical_Router name=lr0 options:chassis=hv1
> > +for i in 0 1; do
> > + idx=$((i+1))
> > + ovn-nbctl ls-add sw$i
> > + ovn-nbctl lrp-add lr0 lrp$i 00:00:00:00:ff:0$idx 192.168.$idx.254/24
> > + ovn-nbctl \
> > + -- lsp-add sw$i lrp$i-attachment \
> > + -- set Logical_Switch_Port lrp$i-attachment type=router \
> > + options:router-port=lrp$i \
> > + addresses='"00:00:00:00:ff:'0$idx'"'
> > +done
> > +
> > for i in 1 2; do
> > sim_add hv$i
> > as hv$i
> > @@ -14705,10 +14718,24 @@ for i in 1 2; do
> > done
> > done
> >
> > +as hv1
> > +ovn-nbctl lsp-add sw1 sw1-p0 \
> > + -- lsp-set-addresses sw1-p0 "00:00:00:00:00:33 192.168.2.11"
> > +ovs-vsctl -- add-port br-int vif33 -- \
> > + set interface vif33 \
> > + external-ids:iface-id=sw1-p0 \
> > + options:tx_pcap=hv$i/vif33-tx.pcap \
> > + options:rxq_pcap=hv$i/vif33-rx.pcap \
> > + ofport-request=33
> > +
> > ovn-nbctl --wait=hv set NB_Global . options:controller_event=true
> > ovn-nbctl lb-add lb0 192.168.1.100:80 ""
> > ovn-nbctl ls-lb-add sw0 lb0
> > -uuid_lb=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb0)
> > +uuid_lb0=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb0)
> > +
> > +ovn-nbctl lb-add lb1 192.168.2.100:80 ""
> > +ovn-nbctl lr-lb-add lr0 lb1
> > +uuid_lb1=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb1)
> > ovn-nbctl --wait=hv meter-add event-elb drop 100 pktps 10
> >
> > OVN_POPULATE_ARP
> > @@ -14716,10 +14743,10 @@ ovn-nbctl --timeout=3 --wait=hv sync
> > ovn-sbctl lflow-list
> > as hv1 ovs-ofctl dump-flows br-int
> >
> > -packet="inport==\"sw0-p11\" && eth.src==00:00:00:00:00:11 && eth.dst==00:00:00:00:00:21 &&
> > - ip4 && ip.ttl==64 && ip4.src==192.168.1.11 && ip4.dst==192.168.1.100 &&
> > - tcp && tcp.src==10000 && tcp.dst==80"
> > -as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet"
> > +packet0="inport==\"sw0-p11\" && eth.src==00:00:00:00:00:11 && eth.dst==00:00:00:00:00:21 &&
> > + ip4 && ip.ttl==64 && ip4.src==192.168.1.11 && ip4.dst==192.168.1.100 &&
> > + tcp && tcp.src==10000 && tcp.dst==80"
> > +as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet0"
> >
> > ovn-sbctl list controller_event
> > uuid=$(ovn-sbctl list controller_event | awk '/_uuid/{print $3}')
> > @@ -14733,12 +14760,27 @@ AT_CHECK([ovn-sbctl get controller_event $uuid event_info:protocol], [0], [dnl
> > tcp
> > ])
> > AT_CHECK_UNQUOTED([ovn-sbctl get controller_event $uuid event_info:load_balancer], [0], [dnl
> > -"$uuid_lb"
> > +"$uuid_lb0"
> > ])
> > AT_CHECK([ovn-sbctl get controller_event $uuid seq_num], [0], [dnl
> > 1
> > ])
> >
> > +ovn-sbctl destroy controller_event $uuid
> > +packet1="inport==\"sw1-p0\" && eth.src==00:00:00:00:00:33 && eth.dst==00:00:00:00:ff:02 &&
> > + ip4 && ip.ttl==64 && ip4.src==192.168.2.11 && ip4.dst==192.168.2.100 &&
> > + tcp && tcp.src==10000 && tcp.dst==80"
> > +
> > +as hv1 ovs-appctl -t ovn-controller inject-pkt "$packet1"
> > +ovn-sbctl list controller_event
> > +uuid=$(ovn-sbctl list controller_event | awk '/_uuid/{print $3}')
> > +AT_CHECK([ovn-sbctl get controller_event $uuid event_type], [0], [dnl
> > +empty_lb_backends
> > +])
> > +AT_CHECK([ovn-sbctl get controller_event $uuid event_info:vip], [0], [dnl
> > +"192.168.2.100:80"
> > +])
> > +
> > OVN_CLEANUP([hv1], [hv2])
> > AT_CLEANUP
> >
> >
>
More information about the dev
mailing list