| Message ID | 26f56183aae2e33970d6d48fb21f4605ca974218.1600349844.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] binding: fix localnet QoS configuration after I-P | expand |
On Thu, Sep 17, 2020 at 6:40 AM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > > After the I-P has been introduced in commit 354bdba51a ("ovn-controller: > I-P for SB port binding and OVS interface in runtime_data"), the QoS on > localnet ports is not properly configured if the ovs interface is marked > with "ovn-egress-iface" flag after the related record in the > logical_switch_port table is set. The issue can be triggered with the > following reproducer: > > $ovn-nbctl set Logical_Switch_Port ln-public options:qos_burst=1000 > $ovs-vsctl set interface eth0 external-ids:ovn-egress-iface=true > > Fix the issue triggering a recomputation after qos is configured for ovs > interface > > Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS > interface in runtime_data") > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > controller/binding.c | 10 ++++++- > tests/ovn-performance.at | 13 +++++++++ > tests/system-ovn.at | 60 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 82 insertions(+), 1 deletion(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 3c102dc7f..c8b099991 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -1436,7 +1436,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > break; > > case LP_LOCALNET: { > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); Why this change? > struct localnet_lport *lnet_lport = xmalloc(sizeof *lnet_lport); > lnet_lport->pb = pb; > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > @@ -1904,6 +1904,14 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, > continue; > } > > + bool egress_info = smap_get_bool(&iface_rec->external_ids, > + "ovn-egress-iface", false); > + if (egress_info) { > + handled = false; > + break; > + } > + > + With this, any such interface change will result in full recompute, if any of the changed interfaces has "ovn-egress-iface" == true, even if this key-value itself didn't change. On the other hand, if it changed from "true" to "false", the I-P will still miss the processing of unconfigure the QoS? Cc Numan. This is related to the discussion we had before, for how to handle interface changes when iface-id and ofport didn't change but some other fields changed. I am still thinking about whether we could simply release and reclaim the port and reprocessing all information related to the port, without having to handle the different combinations of the changes within the interface (and doesn't need to trigger full recompute either). But I am not as familiar as you for the implications to the later stages of the I-P. > 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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Sep 17, Han Zhou wrote: > On Thu, Sep 17, 2020 at 6:40 AM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> wrote: > > > > After the I-P has been introduced in commit 354bdba51a ("ovn-controller: > > I-P for SB port binding and OVS interface in runtime_data"), the QoS on > > localnet ports is not properly configured if the ovs interface is marked > > with "ovn-egress-iface" flag after the related record in the > > logical_switch_port table is set. The issue can be triggered with the > > following reproducer: > > > > $ovn-nbctl set Logical_Switch_Port ln-public options:qos_burst=1000 > > $ovs-vsctl set interface eth0 external-ids:ovn-egress-iface=true > > > > Fix the issue triggering a recomputation after qos is configured for ovs > > interface > > > > Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS > > interface in runtime_data") > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > controller/binding.c | 10 ++++++- > > tests/ovn-performance.at | 13 +++++++++ > > tests/system-ovn.at | 60 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 82 insertions(+), 1 deletion(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 3c102dc7f..c8b099991 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -1436,7 +1436,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct > binding_ctx_out *b_ctx_out) > > break; > > > > case LP_LOCALNET: { > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > qos_map_ptr); > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); > > Why this change? because otherwise if you do not have any local tunnel configured qos_map_ptr will be NULL and you will not update the qos_map > > > struct localnet_lport *lnet_lport = xmalloc(sizeof > *lnet_lport); > > lnet_lport->pb = pb; > > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > > @@ -1904,6 +1904,14 @@ binding_handle_ovs_interface_changes(struct > binding_ctx_in *b_ctx_in, > > continue; > > } > > > > + bool egress_info = smap_get_bool(&iface_rec->external_ids, > > + "ovn-egress-iface", false); > > + if (egress_info) { > > + handled = false; > > + break; > > + } > > + > > + > > With this, any such interface change will result in full recompute, if any > of the changed interfaces has "ovn-egress-iface" == true, even if this > key-value itself didn't change. > On the other hand, if it changed from "true" to "false", the I-P will still > miss the processing of unconfigure the QoS? I have this discussion with Numan as well and we agreed to implement a simple solution in the first attempt since this routine will not run so often with ovn-egress-iface set to true. Another possible approach would be to cache the value as done for iface-id and recompute only when the value is toggled. What do you think? Regards, Lorenzo > > Cc Numan. This is related to the discussion we had before, for how to > handle interface changes when iface-id and ofport didn't change but some > other fields changed. I am still thinking about whether we could simply > release and reclaim the port and reprocessing all information related to > the port, without having to handle the different combinations of the > changes within the interface (and doesn't need to trigger full recompute > either). But I am not as familiar as you for the implications to the later > stages of the I-P. > > > > 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@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Sep 17, 2020 at 2:41 PM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > > On Sep 17, Han Zhou wrote: > > On Thu, Sep 17, 2020 at 6:40 AM Lorenzo Bianconi < > > lorenzo.bianconi@redhat.com> wrote: > > > > > > After the I-P has been introduced in commit 354bdba51a ("ovn-controller: > > > I-P for SB port binding and OVS interface in runtime_data"), the QoS on > > > localnet ports is not properly configured if the ovs interface is marked > > > with "ovn-egress-iface" flag after the related record in the > > > logical_switch_port table is set. The issue can be triggered with the > > > following reproducer: > > > > > > $ovn-nbctl set Logical_Switch_Port ln-public options:qos_burst=1000 > > > $ovs-vsctl set interface eth0 external-ids:ovn-egress-iface=true > > > > > > Fix the issue triggering a recomputation after qos is configured for ovs > > > interface > > > > > > Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS > > > interface in runtime_data") > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > --- > > > controller/binding.c | 10 ++++++- > > > tests/ovn-performance.at | 13 +++++++++ > > > tests/system-ovn.at | 60 ++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 82 insertions(+), 1 deletion(-) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index 3c102dc7f..c8b099991 100644 > > > --- a/controller/binding.c > > > +++ b/controller/binding.c > > > @@ -1436,7 +1436,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct > > binding_ctx_out *b_ctx_out) > > > break; > > > > > > case LP_LOCALNET: { > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > > qos_map_ptr); > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); > > > > Why this change? > > because otherwise if you do not have any local tunnel configured qos_map_ptr will > be NULL and you will not update the qos_map > I am still not clear. Sounds like it is a different bug to be fixed? The title is about a problem in I-P, but here it is for full compute. If that's the case, could you send a separate fix with more detailed information such as how to reproduce and the test case to cover? > > > > > struct localnet_lport *lnet_lport = xmalloc(sizeof > > *lnet_lport); > > > lnet_lport->pb = pb; > > > ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); > > > @@ -1904,6 +1904,14 @@ binding_handle_ovs_interface_changes(struct > > binding_ctx_in *b_ctx_in, > > > continue; > > > } > > > > > > + bool egress_info = smap_get_bool(&iface_rec->external_ids, > > > + "ovn-egress-iface", false); > > > + if (egress_info) { > > > + handled = false; > > > + break; > > > + } > > > + > > > + > > > > With this, any such interface change will result in full recompute, if any > > of the changed interfaces has "ovn-egress-iface" == true, even if this > > key-value itself didn't change. > > On the other hand, if it changed from "true" to "false", the I-P will still > > miss the processing of unconfigure the QoS? > > I have this discussion with Numan as well and we agreed to implement a simple solution > in the first attempt since this routine will not run so often with ovn-egress-iface > set to true. Another possible approach would be to cache the value as done for > iface-id and recompute only when the value is toggled. What do you think? > I am ok with a simple solution but it seems incorrect to handle only the change from "false" to "true" without handling the change from "true" to "false". > Regards, > Lorenzo > > > > > Cc Numan. This is related to the discussion we had before, for how to > > handle interface changes when iface-id and ofport didn't change but some > > other fields changed. I am still thinking about whether we could simply > > release and reclaim the port and reprocessing all information related to > > the port, without having to handle the different combinations of the > > changes within the interface (and doesn't need to trigger full recompute > > either). But I am not as familiar as you for the implications to the later > > stages of the I-P. > > > > > > > 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@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, Sep 18, 2020 at 4:42 AM Han Zhou <hzhou@ovn.org> wrote: > On Thu, Sep 17, 2020 at 2:41 PM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> wrote: > > > > On Sep 17, Han Zhou wrote: > > > On Thu, Sep 17, 2020 at 6:40 AM Lorenzo Bianconi < > > > lorenzo.bianconi@redhat.com> wrote: > > > > > > > > After the I-P has been introduced in commit 354bdba51a > ("ovn-controller: > > > > I-P for SB port binding and OVS interface in runtime_data"), the QoS > on > > > > localnet ports is not properly configured if the ovs interface is > marked > > > > with "ovn-egress-iface" flag after the related record in the > > > > logical_switch_port table is set. The issue can be triggered with the > > > > following reproducer: > > > > > > > > $ovn-nbctl set Logical_Switch_Port ln-public options:qos_burst=1000 > > > > $ovs-vsctl set interface eth0 external-ids:ovn-egress-iface=true > > > > > > > > Fix the issue triggering a recomputation after qos is configured for > ovs > > > > interface > > > > > > > > Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS > > > > interface in runtime_data") > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > --- > > > > controller/binding.c | 10 ++++++- > > > > tests/ovn-performance.at | 13 +++++++++ > > > > tests/system-ovn.at | 60 > ++++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 82 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > > index 3c102dc7f..c8b099991 100644 > > > > --- a/controller/binding.c > > > > +++ b/controller/binding.c > > > > @@ -1436,7 +1436,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, > struct > > > binding_ctx_out *b_ctx_out) > > > > break; > > > > > > > > case LP_LOCALNET: { > > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > > > qos_map_ptr); > > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > &qos_map); > > > > > > Why this change? > > > > because otherwise if you do not have any local tunnel configured > qos_map_ptr will > > be NULL and you will not update the qos_map > > > I am still not clear. Sounds like it is a different bug to be fixed? The > title is about a problem in I-P, but here it is for full compute. If that's > the case, could you send a separate fix with more detailed information such > as how to reproduce and the test case to cover? > +1. I think it could be a separate patch. May be the first patch of the series uses qos_map for consider_localnet_lport() instead of qos_map_ptr and the 2nd patch can address the actual issue. > > > > > > > > struct localnet_lport *lnet_lport = xmalloc(sizeof > > > *lnet_lport); > > > > lnet_lport->pb = pb; > > > > ovs_list_push_back(&localnet_lports, > &lnet_lport->list_node); > > > > @@ -1904,6 +1904,14 @@ binding_handle_ovs_interface_changes(struct > > > binding_ctx_in *b_ctx_in, > > > > continue; > > > > } > > > > > > > > + bool egress_info = smap_get_bool(&iface_rec->external_ids, > > > > + "ovn-egress-iface", false); > > > > + if (egress_info) { > > > > + handled = false; > > > > + break; > > > > + } > > > > + > > > > + > > > > > > With this, any such interface change will result in full recompute, if > any > > > of the changed interfaces has "ovn-egress-iface" == true, even if this > > > key-value itself didn't change. > > > On the other hand, if it changed from "true" to "false", the I-P will > still > > > miss the processing of unconfigure the QoS? > > > > I have this discussion with Numan as well and we agreed to implement a > simple solution > > in the first attempt since this routine will not run so often with > ovn-egress-iface > > set to true. Another possible approach would be to cache the value as > done for > > iface-id and recompute only when the value is toggled. What do you think? > > > I am ok with a simple solution but it seems incorrect to handle only the > change from "false" to "true" without handling the change from "true" to > "false". > > > Regards, > > Lorenzo > > > > > > > > Cc Numan. This is related to the discussion we had before, for how to > > > handle interface changes when iface-id and ofport didn't change but > some > > > other fields changed. I am still thinking about whether we could simply > > > release and reclaim the port and reprocessing all information related > to > > > the port, without having to handle the different combinations of the > > > changes within the interface (and doesn't need to trigger full > recompute > > > either). But I am not as familiar as you for the implications to the > later > > > stages of the I-P. > Hi Han, I don't think releasing and then claiming going to help in this case. This patch is trying to handle changes to an ovs interface which is part of provider bridge. The interface in consideration here doesn't map to any logical port. If I understand correctly, ovn configures qdiscs (for qos) on this physical interface attached to the provider bridge to support qos (probably to support QoS for floating ips). 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@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Fri, Sep 18, 2020 at 1:39 AM Numan Siddique <numans@ovn.org> wrote: > > > > On Fri, Sep 18, 2020 at 4:42 AM Han Zhou <hzhou@ovn.org> wrote: >> >> On Thu, Sep 17, 2020 at 2:41 PM Lorenzo Bianconi < >> lorenzo.bianconi@redhat.com> wrote: >> > >> > On Sep 17, Han Zhou wrote: >> > > On Thu, Sep 17, 2020 at 6:40 AM Lorenzo Bianconi < >> > > lorenzo.bianconi@redhat.com> wrote: >> > > > >> > > > After the I-P has been introduced in commit 354bdba51a >> ("ovn-controller: >> > > > I-P for SB port binding and OVS interface in runtime_data"), the QoS >> on >> > > > localnet ports is not properly configured if the ovs interface is >> marked >> > > > with "ovn-egress-iface" flag after the related record in the >> > > > logical_switch_port table is set. The issue can be triggered with the >> > > > following reproducer: >> > > > >> > > > $ovn-nbctl set Logical_Switch_Port ln-public options:qos_burst=1000 >> > > > $ovs-vsctl set interface eth0 external-ids:ovn-egress-iface=true >> > > > >> > > > Fix the issue triggering a recomputation after qos is configured for >> ovs >> > > > interface >> > > > >> > > > Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS >> > > > interface in runtime_data") >> > > > >> > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >> > > > --- >> > > > controller/binding.c | 10 ++++++- >> > > > tests/ovn-performance.at | 13 +++++++++ >> > > > tests/system-ovn.at | 60 >> ++++++++++++++++++++++++++++++++++++++++ >> > > > 3 files changed, 82 insertions(+), 1 deletion(-) >> > > > >> > > > diff --git a/controller/binding.c b/controller/binding.c >> > > > index 3c102dc7f..c8b099991 100644 >> > > > --- a/controller/binding.c >> > > > +++ b/controller/binding.c >> > > > @@ -1436,7 +1436,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, >> struct >> > > binding_ctx_out *b_ctx_out) >> > > > break; >> > > > >> > > > case LP_LOCALNET: { >> > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, >> > > qos_map_ptr); >> > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, >> &qos_map); >> > > >> > > Why this change? >> > >> > because otherwise if you do not have any local tunnel configured >> qos_map_ptr will >> > be NULL and you will not update the qos_map >> > >> I am still not clear. Sounds like it is a different bug to be fixed? The >> title is about a problem in I-P, but here it is for full compute. If that's >> the case, could you send a separate fix with more detailed information such >> as how to reproduce and the test case to cover? > > > +1. I think it could be a separate patch. May be the first patch of the series uses qos_map for consider_localnet_lport() > instead of qos_map_ptr and the 2nd patch can address the actual issue. > > >> >> >> > > >> > > > struct localnet_lport *lnet_lport = xmalloc(sizeof >> > > *lnet_lport); >> > > > lnet_lport->pb = pb; >> > > > ovs_list_push_back(&localnet_lports, >> &lnet_lport->list_node); >> > > > @@ -1904,6 +1904,14 @@ binding_handle_ovs_interface_changes(struct >> > > binding_ctx_in *b_ctx_in, >> > > > continue; >> > > > } >> > > > >> > > > + bool egress_info = smap_get_bool(&iface_rec->external_ids, >> > > > + "ovn-egress-iface", false); >> > > > + if (egress_info) { >> > > > + handled = false; >> > > > + break; >> > > > + } >> > > > + >> > > > + >> > > >> > > With this, any such interface change will result in full recompute, if >> any >> > > of the changed interfaces has "ovn-egress-iface" == true, even if this >> > > key-value itself didn't change. >> > > On the other hand, if it changed from "true" to "false", the I-P will >> still >> > > miss the processing of unconfigure the QoS? >> > >> > I have this discussion with Numan as well and we agreed to implement a >> simple solution >> > in the first attempt since this routine will not run so often with >> ovn-egress-iface >> > set to true. Another possible approach would be to cache the value as >> done for >> > iface-id and recompute only when the value is toggled. What do you think? >> > >> I am ok with a simple solution but it seems incorrect to handle only the >> change from "false" to "true" without handling the change from "true" to >> "false". > > > >> >> > Regards, >> > Lorenzo >> > >> > > >> > > Cc Numan. This is related to the discussion we had before, for how to >> > > handle interface changes when iface-id and ofport didn't change but some >> > > other fields changed. I am still thinking about whether we could simply >> > > release and reclaim the port and reprocessing all information related to >> > > the port, without having to handle the different combinations of the >> > > changes within the interface (and doesn't need to trigger full recompute >> > > either). But I am not as familiar as you for the implications to the >> later >> > > stages of the I-P. > > > Hi Han, > > I don't think releasing and then claiming going to help in this case. This patch is trying > to handle changes to an ovs interface which is part of provider bridge. The interface > in consideration here doesn't map to any logical port. > > If I understand correctly, ovn configures qdiscs (for qos) on this physical interface attached > to the provider bridge to support qos (probably to support QoS for floating ips). > Thanks for the explanation. If this is the case, then I think we should fix the is_iface_vif() function. In this case it is not a VIF. The code already ensures triggering recompute if a non-VIF has any updates. What do you think? 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@openvswitch.org >> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On Fri, Sep 18, 2020 at 8:53 PM Han Zhou <hzhou@ovn.org> wrote: > On Fri, Sep 18, 2020 at 1:39 AM Numan Siddique <numans@ovn.org> wrote: > > > > > > > > On Fri, Sep 18, 2020 at 4:42 AM Han Zhou <hzhou@ovn.org> wrote: > >> > >> On Thu, Sep 17, 2020 at 2:41 PM Lorenzo Bianconi < > >> lorenzo.bianconi@redhat.com> wrote: > >> > > >> > On Sep 17, Han Zhou wrote: > >> > > On Thu, Sep 17, 2020 at 6:40 AM Lorenzo Bianconi < > >> > > lorenzo.bianconi@redhat.com> wrote: > >> > > > > >> > > > After the I-P has been introduced in commit 354bdba51a > >> ("ovn-controller: > >> > > > I-P for SB port binding and OVS interface in runtime_data"), the > QoS > >> on > >> > > > localnet ports is not properly configured if the ovs interface is > >> marked > >> > > > with "ovn-egress-iface" flag after the related record in the > >> > > > logical_switch_port table is set. The issue can be triggered with > the > >> > > > following reproducer: > >> > > > > >> > > > $ovn-nbctl set Logical_Switch_Port ln-public > options:qos_burst=1000 > >> > > > $ovs-vsctl set interface eth0 external-ids:ovn-egress-iface=true > >> > > > > >> > > > Fix the issue triggering a recomputation after qos is configured > for > >> ovs > >> > > > interface > >> > > > > >> > > > Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and > OVS > >> > > > interface in runtime_data") > >> > > > > >> > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > >> > > > --- > >> > > > controller/binding.c | 10 ++++++- > >> > > > tests/ovn-performance.at | 13 +++++++++ > >> > > > tests/system-ovn.at | 60 > >> ++++++++++++++++++++++++++++++++++++++++ > >> > > > 3 files changed, 82 insertions(+), 1 deletion(-) > >> > > > > >> > > > diff --git a/controller/binding.c b/controller/binding.c > >> > > > index 3c102dc7f..c8b099991 100644 > >> > > > --- a/controller/binding.c > >> > > > +++ b/controller/binding.c > >> > > > @@ -1436,7 +1436,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, > >> struct > >> > > binding_ctx_out *b_ctx_out) > >> > > > break; > >> > > > > >> > > > case LP_LOCALNET: { > >> > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > >> > > qos_map_ptr); > >> > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, > >> &qos_map); > >> > > > >> > > Why this change? > >> > > >> > because otherwise if you do not have any local tunnel configured > >> qos_map_ptr will > >> > be NULL and you will not update the qos_map > >> > > >> I am still not clear. Sounds like it is a different bug to be fixed? The > >> title is about a problem in I-P, but here it is for full compute. If > that's > >> the case, could you send a separate fix with more detailed information > such > >> as how to reproduce and the test case to cover? > > > > > > +1. I think it could be a separate patch. May be the first patch of the > series uses qos_map for consider_localnet_lport() > > instead of qos_map_ptr and the 2nd patch can address the actual issue. > > > > > >> > >> > >> > > > >> > > > struct localnet_lport *lnet_lport = xmalloc(sizeof > >> > > *lnet_lport); > >> > > > lnet_lport->pb = pb; > >> > > > ovs_list_push_back(&localnet_lports, > >> &lnet_lport->list_node); > >> > > > @@ -1904,6 +1904,14 @@ binding_handle_ovs_interface_changes(struct > >> > > binding_ctx_in *b_ctx_in, > >> > > > continue; > >> > > > } > >> > > > > >> > > > + bool egress_info = > smap_get_bool(&iface_rec->external_ids, > >> > > > + "ovn-egress-iface", > false); > >> > > > + if (egress_info) { > >> > > > + handled = false; > >> > > > + break; > >> > > > + } > >> > > > + > >> > > > + > >> > > > >> > > With this, any such interface change will result in full recompute, > if > >> any > >> > > of the changed interfaces has "ovn-egress-iface" == true, even if > this > >> > > key-value itself didn't change. > >> > > On the other hand, if it changed from "true" to "false", the I-P > will > >> still > >> > > miss the processing of unconfigure the QoS? > >> > > >> > I have this discussion with Numan as well and we agreed to implement a > >> simple solution > >> > in the first attempt since this routine will not run so often with > >> ovn-egress-iface > >> > set to true. Another possible approach would be to cache the value as > >> done for > >> > iface-id and recompute only when the value is toggled. What do you > think? > >> > > >> I am ok with a simple solution but it seems incorrect to handle only the > >> change from "false" to "true" without handling the change from "true" to > >> "false". > > > > > > > >> > >> > Regards, > >> > Lorenzo > >> > > >> > > > >> > > Cc Numan. This is related to the discussion we had before, for how > to > >> > > handle interface changes when iface-id and ofport didn't change but > some > >> > > other fields changed. I am still thinking about whether we could > simply > >> > > release and reclaim the port and reprocessing all information > related to > >> > > the port, without having to handle the different combinations of the > >> > > changes within the interface (and doesn't need to trigger full > recompute > >> > > either). But I am not as familiar as you for the implications to the > >> later > >> > > stages of the I-P. > > > > > > Hi Han, > > > > I don't think releasing and then claiming going to help in this case. > This patch is trying > > to handle changes to an ovs interface which is part of provider bridge. > The interface > > in consideration here doesn't map to any logical port. > > > > If I understand correctly, ovn configures qdiscs (for qos) on this > physical interface attached > > to the provider bridge to support qos (probably to support QoS for > floating ips). > > > Thanks for the explanation. If this is the case, then I think we should fix > the is_iface_vif() function. In this case it is not a VIF. The code already > ensures triggering recompute if a non-VIF has any updates. What do you > think? > > 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 ? 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@openvswitch.org > >> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Fri, Sep 18, 2020 at 8:34 AM Numan Siddique <nusiddiq@redhat.com> wrote: > > > > On Fri, Sep 18, 2020 at 8:53 PM Han Zhou <hzhou@ovn.org> wrote: >> >> On Fri, Sep 18, 2020 at 1:39 AM Numan Siddique <numans@ovn.org> wrote: >> > >> > >> > >> > On Fri, Sep 18, 2020 at 4:42 AM Han Zhou <hzhou@ovn.org> wrote: >> >> >> >> On Thu, Sep 17, 2020 at 2:41 PM Lorenzo Bianconi < >> >> lorenzo.bianconi@redhat.com> wrote: >> >> > >> >> > On Sep 17, Han Zhou wrote: >> >> > > On Thu, Sep 17, 2020 at 6:40 AM Lorenzo Bianconi < >> >> > > lorenzo.bianconi@redhat.com> wrote: >> >> > > > >> >> > > > After the I-P has been introduced in commit 354bdba51a >> >> ("ovn-controller: >> >> > > > I-P for SB port binding and OVS interface in runtime_data"), the >> QoS >> >> on >> >> > > > localnet ports is not properly configured if the ovs interface is >> >> marked >> >> > > > with "ovn-egress-iface" flag after the related record in the >> >> > > > logical_switch_port table is set. The issue can be triggered with >> the >> >> > > > following reproducer: >> >> > > > >> >> > > > $ovn-nbctl set Logical_Switch_Port ln-public options:qos_burst=1000 >> >> > > > $ovs-vsctl set interface eth0 external-ids:ovn-egress-iface=true >> >> > > > >> >> > > > Fix the issue triggering a recomputation after qos is configured >> for >> >> ovs >> >> > > > interface >> >> > > > >> >> > > > Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and >> OVS >> >> > > > interface in runtime_data") >> >> > > > >> >> > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >> >> > > > --- >> >> > > > controller/binding.c | 10 ++++++- >> >> > > > tests/ovn-performance.at | 13 +++++++++ >> >> > > > tests/system-ovn.at | 60 >> >> ++++++++++++++++++++++++++++++++++++++++ >> >> > > > 3 files changed, 82 insertions(+), 1 deletion(-) >> >> > > > >> >> > > > diff --git a/controller/binding.c b/controller/binding.c >> >> > > > index 3c102dc7f..c8b099991 100644 >> >> > > > --- a/controller/binding.c >> >> > > > +++ b/controller/binding.c >> >> > > > @@ -1436,7 +1436,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, >> >> struct >> >> > > binding_ctx_out *b_ctx_out) >> >> > > > break; >> >> > > > >> >> > > > case LP_LOCALNET: { >> >> > > > - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, >> >> > > qos_map_ptr); >> >> > > > + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, >> >> &qos_map); >> >> > > >> >> > > Why this change? >> >> > >> >> > because otherwise if you do not have any local tunnel configured >> >> qos_map_ptr will >> >> > be NULL and you will not update the qos_map >> >> > >> >> I am still not clear. Sounds like it is a different bug to be fixed? The >> >> title is about a problem in I-P, but here it is for full compute. If >> that's >> >> the case, could you send a separate fix with more detailed information >> such >> >> as how to reproduce and the test case to cover? >> > >> > >> > +1. I think it could be a separate patch. May be the first patch of the >> series uses qos_map for consider_localnet_lport() >> > instead of qos_map_ptr and the 2nd patch can address the actual issue. >> > >> > >> >> >> >> >> >> > > >> >> > > > struct localnet_lport *lnet_lport = xmalloc(sizeof >> >> > > *lnet_lport); >> >> > > > lnet_lport->pb = pb; >> >> > > > ovs_list_push_back(&localnet_lports, >> >> &lnet_lport->list_node); >> >> > > > @@ -1904,6 +1904,14 @@ binding_handle_ovs_interface_changes(struct >> >> > > binding_ctx_in *b_ctx_in, >> >> > > > continue; >> >> > > > } >> >> > > > >> >> > > > + bool egress_info = smap_get_bool(&iface_rec->external_ids, >> >> > > > + "ovn-egress-iface", >> false); >> >> > > > + if (egress_info) { >> >> > > > + handled = false; >> >> > > > + break; >> >> > > > + } >> >> > > > + >> >> > > > + >> >> > > >> >> > > With this, any such interface change will result in full recompute, >> if >> >> any >> >> > > of the changed interfaces has "ovn-egress-iface" == true, even if >> this >> >> > > key-value itself didn't change. >> >> > > On the other hand, if it changed from "true" to "false", the I-P will >> >> still >> >> > > miss the processing of unconfigure the QoS? >> >> > >> >> > I have this discussion with Numan as well and we agreed to implement a >> >> simple solution >> >> > in the first attempt since this routine will not run so often with >> >> ovn-egress-iface >> >> > set to true. Another possible approach would be to cache the value as >> >> done for >> >> > iface-id and recompute only when the value is toggled. What do you >> think? >> >> > >> >> I am ok with a simple solution but it seems incorrect to handle only the >> >> change from "false" to "true" without handling the change from "true" to >> >> "false". >> > >> > >> > >> >> >> >> > Regards, >> >> > Lorenzo >> >> > >> >> > > >> >> > > Cc Numan. This is related to the discussion we had before, for how to >> >> > > handle interface changes when iface-id and ofport didn't change but >> some >> >> > > other fields changed. I am still thinking about whether we could >> simply >> >> > > release and reclaim the port and reprocessing all information >> related to >> >> > > the port, without having to handle the different combinations of the >> >> > > changes within the interface (and doesn't need to trigger full >> recompute >> >> > > either). But I am not as familiar as you for the implications to the >> >> later >> >> > > stages of the I-P. >> > >> > >> > Hi Han, >> > >> > I don't think releasing and then claiming going to help in this case. >> This patch is trying >> > to handle changes to an ovs interface which is part of provider bridge. >> The interface >> > in consideration here doesn't map to any logical port. >> > >> > If I understand correctly, ovn configures qdiscs (for qos) on this >> physical interface attached >> > to the provider bridge to support qos (probably to support QoS for >> floating ips). >> > >> Thanks for the explanation. If this is the case, then I think we should fix >> the is_iface_vif() function. In this case it is not a VIF. The code already >> ensures triggering recompute if a non-VIF has any updates. What do you >> think? >> > > 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? > 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@openvswitch.org >> >> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> _______________________________________________ >> >> dev mailing list >> >> dev@openvswitch.org >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
[...] > > 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@openvswitch.org > >> >> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> >> _______________________________________________ > >> >> dev mailing list > >> >> dev@openvswitch.org > >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/binding.c b/controller/binding.c index 3c102dc7f..c8b099991 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -1436,7 +1436,7 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) break; case LP_LOCALNET: { - consider_localnet_lport(pb, b_ctx_in, b_ctx_out, qos_map_ptr); + consider_localnet_lport(pb, b_ctx_in, b_ctx_out, &qos_map); struct localnet_lport *lnet_lport = xmalloc(sizeof *lnet_lport); lnet_lport->pb = pb; ovs_list_push_back(&localnet_lports, &lnet_lport->list_node); @@ -1904,6 +1904,14 @@ binding_handle_ovs_interface_changes(struct binding_ctx_in *b_ctx_in, continue; } + bool egress_info = smap_get_bool(&iface_rec->external_ids, + "ovn-egress-iface", false); + if (egress_info) { + handled = false; + break; + } + + 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
After the I-P has been introduced in commit 354bdba51a ("ovn-controller: I-P for SB port binding and OVS interface in runtime_data"), the QoS on localnet ports is not properly configured if the ovs interface is marked with "ovn-egress-iface" flag after the related record in the logical_switch_port table is set. The issue can be triggered with the following reproducer: $ovn-nbctl set Logical_Switch_Port ln-public options:qos_burst=1000 $ovs-vsctl set interface eth0 external-ids:ovn-egress-iface=true Fix the issue triggering a recomputation after qos is configured for ovs interface Fixes: 354bdba51abf ("ovn-controller: I-P for SB port binding and OVS interface in runtime_data") Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- controller/binding.c | 10 ++++++- tests/ovn-performance.at | 13 +++++++++ tests/system-ovn.at | 60 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 1 deletion(-)