[ovs-dev] [PATCH ovn] binding: fix localnet QoS configuration after I-P

Lorenzo Bianconi lorenzo.bianconi at redhat.com
Fri Sep 18 16:58:23 UTC 2020


[...]

> > But it is difficult to identify it right ?  I think the interface type
> would be still ""
> > when a user adds - ovs-vsctl add-port br-ex eth1.
> >
> > Maybe we check the bridge of the interface and figure it out ? What do
> you think ?
> >
> 
> Yes, since we know which bridge is the integration bridge. Would it work?

Hi Han and Numan,

Thanks for the review. I was thinking to define a struct to contain iface_ids
string and ovn-egress-iface property. Something like:

struct {
	char iface_id[32];
	bool ovn_egress_iface;
};

Doing so we can track even when ovn_egress_iface is toggled. For this approach
we will need a hashmap instead of a smap for local_iface_ids (that we will be
renamed). Is this approach too complicated?

Regards,
Lorenzo

> 
> > Thanks
> > Numan
> >
> >
> >>
> >> Thanks,
> >> Han
> >>
> >> > Lorenzo - I agree with Han for the above comment on incorrectly
> handling
> >> the case from false
> >> > to true.
> >> >
> >> > I think the ovs interface handle should return false (so that a full
> >> recompute is triggered) if
> >> >    (1) An ovs interface is added/delete/updated and it has
> >> external_ids:ovn-egress-iface value
> >> >      is set (it could be true/false or anything)
> >> >    (2) An ovs interface is modified and the
> external_ids:ovn-egress-iface
> >> key is deleted by the user.
> >> >       In this case a full recompute should be triggered so that we
> remove
> >> the configured qdiscs on this
> >> >       interface.
> >> >
> >> > I think (1) should be straightforward. But unfortunately for (2),
> >> ovn-controller should store the old state.
> >> > In my initial discussion with Lorenzo, I didn't consider this scenario.
> >> >
> >> > Thanks
> >> > Numan
> >> >
> >> >
> >> >
> >> >
> >> >> > >
> >> >> > >
> >> >> > > >          const char *iface_id =
> smap_get(&iface_rec->external_ids,
> >> >> > > "iface-id");
> >> >> > > >          int64_t ofport = iface_rec->n_ofport ?
> *iface_rec->ofport
> >> :
> >> >> 0;
> >> >> > > >          if (iface_id && ofport > 0) {
> >> >> > > > diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> >> >> > > > index 3010860d5..6cc5b2174 100644
> >> >> > > > --- a/tests/ovn-performance.at
> >> >> > > > +++ b/tests/ovn-performance.at
> >> >> > > > @@ -247,6 +247,8 @@ for i in 1 2 3; do
> >> >> > > >      ovs-vsctl set open .
> >> >> external_ids:ovn-bridge-mappings="public:br-ex"
> >> >> > > >      j=$((i + 2))
> >> >> > > >      ovn_attach n1 br-phys 192.168.0.$j
> >> >> > > > +    ip link add vgw$i type dummy
> >> >> > > > +    ovs-vsctl add-port br-ex vgw$i
> >> >> > > >  done
> >> >> > > >
> >> >> > > >  # Wait for the tunnel ports to be created and up.
> >> >> > > > @@ -477,6 +479,17 @@ OVN_CONTROLLER_EXPECT_HIT(
> >> >> > > >      [ovn-nbctl --wait=hv lrp-set-gateway-chassis lr1-public gw3
> >> 10 &&
> >> >> > > ovn-nbctl --wait=hv sync]
> >> >> > > >  )
> >> >> > > >
> >> >> > > > +# create QoS rule
> >> >> > > > +OVN_CONTROLLER_EXPECT_NO_HIT(
> >> >> > > > +    [hv1 hv2 gw1 gw2 gw3], [lflow_run],
> >> >> > > > +    [ovn-nbctl --wait=hv set Logical_Switch_Port ln-public
> >> >> > > options:qos_burst=1000]
> >> >> > > > +)
> >> >> > > > +
> >> >> > > > +OVN_CONTROLLER_EXPECT_HIT(
> >> >> > > > +    [gw1], [lflow_run],
> >> >> > > > +    [as gw1 ovs-vsctl set interface vgw1
> >> >> > > external-ids:ovn-egress-iface=true]
> >> >> > > > +)
> >> >> > > > +
> >> >> > > >  # Make gw2 master. There is remote possibility that full
> recompute
> >> >> > > >  # triggers for gw2 after it becomes master. Most of the time
> >> >> > > >  # there will be no recompute.
> >> >> > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> >> >> > > > index 4f72754c6..f779947f4 100644
> >> >> > > > --- a/tests/system-ovn.at
> >> >> > > > +++ b/tests/system-ovn.at
> >> >> > > > @@ -5397,3 +5397,63 @@ as
> >> >> > > >  OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
> >> >> > > >  /.*terminating with signal 15.*/d"])
> >> >> > > >  AT_CLEANUP
> >> >> > > > +
> >> >> > > > +AT_SETUP([ovn -- egress qos])
> >> >> > > > +AT_KEYWORDS([ovn-egress-qos])
> >> >> > > > +
> >> >> > > > +ovn_start
> >> >> > > > +OVS_TRAFFIC_VSWITCHD_START()
> >> >> > > > +
> >> >> > > > +ADD_BR([br-int])
> >> >> > > > +ADD_BR([br-ext])
> >> >> > > > +
> >> >> > > > +ovs-ofctl add-flow br-ext action=normal
> >> >> > > > +# Set external-ids in br-int needed for ovn-controller
> >> >> > > > +ovs-vsctl \
> >> >> > > > +        -- set Open_vSwitch . external-ids:system-id=hv1 \
> >> >> > > > +        -- set Open_vSwitch .
> >> >> > > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> >> >> > > > +        -- set Open_vSwitch .
> external-ids:ovn-encap-type=geneve \
> >> >> > > > +        -- set Open_vSwitch .
> external-ids:ovn-encap-ip=169.0.0.1
> >> \
> >> >> > > > +        -- set bridge br-int fail-mode=secure
> >> >> > > other-config:disable-in-band=true
> >> >> > > > +
> >> >> > > > +# Start ovn-controller
> >> >> > > > +start_daemon ovn-controller
> >> >> > > > +
> >> >> > > > +ovn-nbctl ls-add sw0
> >> >> > > > +
> >> >> > > > +ADD_NAMESPACES(sw01)
> >> >> > > > +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24",
> >> "f0:00:00:01:02:03")
> >> >> > > > +ovn-nbctl lsp-add sw0 sw01 \
> >> >> > > > +    -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2"
> >> >> > > > +
> >> >> > > > +ADD_NAMESPACES(public)
> >> >> > > > +ADD_VETH(public, public, br-ext, "192.168.2.2/24",
> >> >> "f0:00:00:01:02:05")
> >> >> > > > +
> >> >> > > > +AT_CHECK([ovs-vsctl set Open_vSwitch .
> >> >> > > external-ids:ovn-bridge-mappings=phynet:br-ext])
> >> >> > > > +ovn-nbctl lsp-add sw0 public \
> >> >> > > > +        -- lsp-set-addresses public unknown \
> >> >> > > > +        -- lsp-set-type public localnet \
> >> >> > > > +        -- lsp-set-options public network_name=phynet
> >> >> > > > +
> >> >> > > > +AT_CHECK([ovn-nbctl set Logical_Switch_Port public
> >> >> > > options:qos_burst=1000])
> >> >> > > > +AT_CHECK([ovs-vsctl set interface ovs-public
> >> >> > > external-ids:ovn-egress-iface=true])
> >> >> > > > +AT_CHECK([tc qdisc show | grep -q 'htb 1: dev ovs-public'])
> >> >> > > > +
> >> >> > > > +AT_CHECK([ovn-nbctl remove Logical_Switch_Port public options
> >> >> > > qos_burst=1000])
> >> >> > > > +AT_CHECK([tc qdisc show | grep -q 'htb 1: dev ovs-public'],[1])
> >> >> > > > +
> >> >> > > > +kill $(pidof ovn-controller)
> >> >> > > > +
> >> >> > > > +as ovn-sb
> >> >> > > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> >> >> > > > +
> >> >> > > > +as ovn-nb
> >> >> > > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> >> >> > > > +
> >> >> > > > +as northd
> >> >> > > > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> >> >> > > > +
> >> >> > > > +as
> >> >> > > > +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
> >> >> > > > +/.*terminating with signal 15.*/d"])
> >> >> > > > +AT_CLEANUP
> >> >> > > > --
> >> >> > > > 2.26.2
> >> >> > > >
> >> >> > > > _______________________________________________
> >> >> > > > dev mailing list
> >> >> > > > dev at openvswitch.org
> >> >> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> >> _______________________________________________
> >> >> dev mailing list
> >> >> dev at openvswitch.org
> >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev at openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 


More information about the dev mailing list