Message ID | 1608220140-14239-1-git-send-email-dceara@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,branch-20.06] ovn-controller: Always run the I-P OVS Interface change handler. | expand |
Bleep bloop. Greetings Dumitru Ceara, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. build: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./ovn -I ./include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/pinctrl.o -MD -MP -MF $depbase.Tpo -c -o controller/pinctrl.o controller/pinctrl.c &&\ mv -f $depbase.Tpo $depbase.Po depbase=`echo controller/patch.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./ovn -I ./include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/patch.o -MD -MP -MF $depbase.Tpo -c -o controller/patch.o controller/patch.c &&\ mv -f $depbase.Tpo $depbase.Po depbase=`echo controller/ovn-controller.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./ovn -I ./include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/ovn-controller.o -MD -MP -MF $depbase.Tpo -c -o controller/ovn-controller.o controller/ovn-controller.c &&\ mv -f $depbase.Tpo $depbase.Po depbase=`echo controller/physical.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\ gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./ovn -I ./include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/include -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR/lib -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I /var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace/OVSDIR -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT controller/physical.o -MD -MP -MF $depbase.Tpo -c -o controller/physical.o controller/physical.c &&\ mv -f $depbase.Tpo $depbase.Po controller/physical.c: In function ‘put_remote_port_redirect_overlay’: controller/physical.c:379:23: error: ‘struct ofpact_bundle’ has no member named ‘n_slaves’ if (bundle->n_slaves >= BUNDLE_MAX_SLAVES) { ^ controller/physical.c:379:37: error: ‘BUNDLE_MAX_SLAVES’ undeclared (first use in this function) if (bundle->n_slaves >= BUNDLE_MAX_SLAVES) { ^ controller/physical.c:379:37: note: each undeclared identifier is reported only once for each function it appears in controller/physical.c:387:19: error: ‘struct ofpact_bundle’ has no member named ‘n_slaves’ bundle->n_slaves++; ^ make[1]: *** [controller/physical.o] Error 1 make[1]: Leaving directory `/var/lib/jenkins/jobs/0day_robot_upstream_build_ovn_from_pw/workspace' make: *** [all] Error 2 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Thu, Dec 17, 2020 at 9:19 PM Dumitru Ceara <dceara@redhat.com> wrote: > > The incremental processing engine implementation is such that if an > input node gets updated but the output node doesn't have a change > handler for it then the output node is immediately recomputed. That is, > no other input change handlers are executed. > > In case of the en_physical_flow_changes node, if a ct-zone changes we > were also skipping the OVS interface change handler. That is incorrect > as there is an implicit requirement that flows for OVS interfaces that > got deleted should be cleared before physical_run() is called. > > Instead, we now add an explicit change handler for ct-zone changes. > This change handler never processes ct-zone updates incrementally but > ensures that the OVS interface change handler is also called. > > Moreover, OVS interface changes (including deletes) are now processed > before physical_run() is called in the flow_output > physical_flow_changes_handler. This is a requirement because otherwise > physical_run() will fail to add flows for new OVS interfaces that > correspond to unchanged Port_Bindings. > > Reported-by: Daniel Alvarez <dalvarez@redhat.com> > Reported-at: https://bugzilla.redhat.com/1908391 > Fixes: a3005f0dc777 ("ovn-controller: I-P for ct zone and OVS interface changes in flow output stage.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > Reported-by: Krzysztof Klimonda <kklimonda@syntaxhighlighted.com> > Acked-by: Numan Siddique <numans@ovn.org> > (cherry picked from commit a2bf85296b2d0abe807d1cadf2ed29482318df11) Thanks. Backported to respective branches. Numan > --- > controller/ovn-controller.c | 30 ++++++++++++----- > tests/ovn.at | 78 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 100 insertions(+), 8 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index ed1a99e..0ffaf34 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1488,6 +1488,16 @@ en_physical_flow_changes_run(struct engine_node *node, void *data) > engine_set_node_state(node, EN_UPDATED); > } > > +/* ct_zone changes are not handled incrementally but a handler is required > + * to avoid skipping the ovs_iface incremental change handler. > + */ > +static bool > +physical_flow_changes_ct_zones_handler(struct engine_node *node OVS_UNUSED, > + void *data OVS_UNUSED) > +{ > + return false; > +} > + > /* There are OVS interface changes. Indicate to the flow_output engine > * to handle these OVS interface changes for physical flow computations. */ > static bool > @@ -1965,6 +1975,13 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > struct ed_type_pfc_data *pfc_data = > engine_get_input_data("physical_flow_changes", node); > > + /* If there are OVS interface changes. Try to handle them incrementally. */ > + if (pfc_data->ovs_ifaces_changed) { > + if (!physical_handle_ovs_iface_changes(&p_ctx, &fo->flow_table)) { > + return false; > + } > + } > + > if (pfc_data->recompute_physical_flows) { > /* This indicates that we need to recompute the physical flows. */ > physical_clear_unassoc_flows_with_db(&fo->flow_table); > @@ -1972,12 +1989,6 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) > return true; > } > > - if (pfc_data->ovs_ifaces_changed) { > - /* There are OVS interface changes. Try to handle them > - * incrementally. */ > - return physical_handle_ovs_iface_changes(&p_ctx, &fo->flow_table); > - } > - > return true; > } > > @@ -2184,11 +2195,14 @@ main(int argc, char *argv[]) > /* Engine node physical_flow_changes indicates whether > * we can recompute only physical flows or we can > * incrementally process the physical flows. > + * > + * Note: The order of inputs is important, all OVS interface changes must > + * be handled before any ct_zone changes. > */ > - engine_add_input(&en_physical_flow_changes, &en_ct_zones, > - NULL); > engine_add_input(&en_physical_flow_changes, &en_ovs_interface, > physical_flow_changes_ovs_iface_handler); > + engine_add_input(&en_physical_flow_changes, &en_ct_zones, > + physical_flow_changes_ct_zones_handler); > > engine_add_input(&en_flow_output, &en_addr_sets, > flow_output_addr_sets_handler); > diff --git a/tests/ovn.at b/tests/ovn.at > index 3f9d2c8..07eb952 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -20888,6 +20888,84 @@ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) = "xru > OVN_CLEANUP([hv1]) > AT_CLEANUP > > +AT_SETUP([ovn -- Multiple OVS port changes Incremental Processing]) > +ovn_start > + > +net_add n1 > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.10 > + > +ovn-nbctl ls-add sw > +ovn-nbctl lsp-add sw lsp1 > +ovn-nbctl lsp-add sw lsp2 > + > +as hv1 > +ovs-vsctl \ > + -- add-port br-int vif1 \ > + -- set Interface vif1 external_ids:iface-id=lsp1 \ > + ofport-request=1 > +ovs-vsctl \ > + -- add-port br-int vif2 \ > + -- set Interface vif2 external_ids:iface-id=lsp2 \ > + ofport-request=2 > + > +# Wait for ports to be bound. > +OVS_WAIT_UNTIL([ovn-sbctl --columns _uuid find Chassis name=hv1 | wc -l], [0], [dnl > +1 > +]) > +ch=$(ovn-sbctl --bare --columns _uuid find Chassis name=hv1) > +OVS_WAIT_UNTIL([ovn-sbctl --columns _uuid find Port_Binding logical_port=lsp1 chassis=$ch | wc -l], [0]. [dnl > +1 > +]) > +OVS_WAIT_UNTIL([ovn-sbctl --columns _uuid find Port_Binding logical_port=lsp2 chassis=$ch | wc -l], [0]. [dnl > +1 > +]) > + > +AS_BOX([check output flows for initial interfaces]) > +as hv1 ovs-ofctl dump-flows br-int table=65 > offlows_table65.txt > +AT_CAPTURE_FILE([offlows_table65.txt]) > +AT_CHECK_UNQUOTED([grep -c "output:1" offlows_table65.txt], [0], [dnl > +1 > +]) > +AT_CHECK_UNQUOTED([grep -c "output:2" offlows_table65.txt], [0], [dnl > +1 > +]) > + > +AS_BOX([delete and add OVS interfaces and force batch of updates]) > +as hv1 ovn-appctl -t ovn-controller debug/pause > + > +as hv1 > +ovs-vsctl \ > + -- del-port vif1 \ > + -- del-port vif2 > + > +as hv1 > +ovs-vsctl \ > + -- add-port br-int vif1 \ > + -- set Interface vif1 external_ids:iface-id=lsp1 \ > + ofport-request=3 \ > + -- add-port br-int vif2 \ > + -- set Interface vif2 external_ids:iface-id=lsp2 \ > + ofport-request=4 > + > +as hv1 ovn-appctl -t ovn-controller debug/resume > +ovn-nbctl --wait=hv sync > + > +AS_BOX([check output flows for new interfaces]) > +as hv1 ovs-ofctl dump-flows br-int table=65 > offlows_table65_2.txt > +AT_CAPTURE_FILE([offlows_table65_2.txt]) > +AT_CHECK_UNQUOTED([grep -c "output:3" offlows_table65_2.txt], [0], [dnl > +1 > +]) > +AT_CHECK_UNQUOTED([grep -c "output:4" offlows_table65_2.txt], [0], [dnl > +1 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > + > AT_SETUP([ovn -- ARP replies for SNAT external ips]) > ovn_start > > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index ed1a99e..0ffaf34 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1488,6 +1488,16 @@ en_physical_flow_changes_run(struct engine_node *node, void *data) engine_set_node_state(node, EN_UPDATED); } +/* ct_zone changes are not handled incrementally but a handler is required + * to avoid skipping the ovs_iface incremental change handler. + */ +static bool +physical_flow_changes_ct_zones_handler(struct engine_node *node OVS_UNUSED, + void *data OVS_UNUSED) +{ + return false; +} + /* There are OVS interface changes. Indicate to the flow_output engine * to handle these OVS interface changes for physical flow computations. */ static bool @@ -1965,6 +1975,13 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) struct ed_type_pfc_data *pfc_data = engine_get_input_data("physical_flow_changes", node); + /* If there are OVS interface changes. Try to handle them incrementally. */ + if (pfc_data->ovs_ifaces_changed) { + if (!physical_handle_ovs_iface_changes(&p_ctx, &fo->flow_table)) { + return false; + } + } + if (pfc_data->recompute_physical_flows) { /* This indicates that we need to recompute the physical flows. */ physical_clear_unassoc_flows_with_db(&fo->flow_table); @@ -1972,12 +1989,6 @@ flow_output_physical_flow_changes_handler(struct engine_node *node, void *data) return true; } - if (pfc_data->ovs_ifaces_changed) { - /* There are OVS interface changes. Try to handle them - * incrementally. */ - return physical_handle_ovs_iface_changes(&p_ctx, &fo->flow_table); - } - return true; } @@ -2184,11 +2195,14 @@ main(int argc, char *argv[]) /* Engine node physical_flow_changes indicates whether * we can recompute only physical flows or we can * incrementally process the physical flows. + * + * Note: The order of inputs is important, all OVS interface changes must + * be handled before any ct_zone changes. */ - engine_add_input(&en_physical_flow_changes, &en_ct_zones, - NULL); engine_add_input(&en_physical_flow_changes, &en_ovs_interface, physical_flow_changes_ovs_iface_handler); + engine_add_input(&en_physical_flow_changes, &en_ct_zones, + physical_flow_changes_ct_zones_handler); engine_add_input(&en_flow_output, &en_addr_sets, flow_output_addr_sets_handler); diff --git a/tests/ovn.at b/tests/ovn.at index 3f9d2c8..07eb952 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -20888,6 +20888,84 @@ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) = "xru OVN_CLEANUP([hv1]) AT_CLEANUP +AT_SETUP([ovn -- Multiple OVS port changes Incremental Processing]) +ovn_start + +net_add n1 +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.10 + +ovn-nbctl ls-add sw +ovn-nbctl lsp-add sw lsp1 +ovn-nbctl lsp-add sw lsp2 + +as hv1 +ovs-vsctl \ + -- add-port br-int vif1 \ + -- set Interface vif1 external_ids:iface-id=lsp1 \ + ofport-request=1 +ovs-vsctl \ + -- add-port br-int vif2 \ + -- set Interface vif2 external_ids:iface-id=lsp2 \ + ofport-request=2 + +# Wait for ports to be bound. +OVS_WAIT_UNTIL([ovn-sbctl --columns _uuid find Chassis name=hv1 | wc -l], [0], [dnl +1 +]) +ch=$(ovn-sbctl --bare --columns _uuid find Chassis name=hv1) +OVS_WAIT_UNTIL([ovn-sbctl --columns _uuid find Port_Binding logical_port=lsp1 chassis=$ch | wc -l], [0]. [dnl +1 +]) +OVS_WAIT_UNTIL([ovn-sbctl --columns _uuid find Port_Binding logical_port=lsp2 chassis=$ch | wc -l], [0]. [dnl +1 +]) + +AS_BOX([check output flows for initial interfaces]) +as hv1 ovs-ofctl dump-flows br-int table=65 > offlows_table65.txt +AT_CAPTURE_FILE([offlows_table65.txt]) +AT_CHECK_UNQUOTED([grep -c "output:1" offlows_table65.txt], [0], [dnl +1 +]) +AT_CHECK_UNQUOTED([grep -c "output:2" offlows_table65.txt], [0], [dnl +1 +]) + +AS_BOX([delete and add OVS interfaces and force batch of updates]) +as hv1 ovn-appctl -t ovn-controller debug/pause + +as hv1 +ovs-vsctl \ + -- del-port vif1 \ + -- del-port vif2 + +as hv1 +ovs-vsctl \ + -- add-port br-int vif1 \ + -- set Interface vif1 external_ids:iface-id=lsp1 \ + ofport-request=3 \ + -- add-port br-int vif2 \ + -- set Interface vif2 external_ids:iface-id=lsp2 \ + ofport-request=4 + +as hv1 ovn-appctl -t ovn-controller debug/resume +ovn-nbctl --wait=hv sync + +AS_BOX([check output flows for new interfaces]) +as hv1 ovs-ofctl dump-flows br-int table=65 > offlows_table65_2.txt +AT_CAPTURE_FILE([offlows_table65_2.txt]) +AT_CHECK_UNQUOTED([grep -c "output:3" offlows_table65_2.txt], [0], [dnl +1 +]) +AT_CHECK_UNQUOTED([grep -c "output:4" offlows_table65_2.txt], [0], [dnl +1 +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP + AT_SETUP([ovn -- ARP replies for SNAT external ips]) ovn_start