| Message ID | 1608219845-6518-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,branch-20.09] 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:387:23: error: ‘struct ofpact_bundle’ has no member named ‘n_slaves’
if (bundle->n_slaves >= BUNDLE_MAX_SLAVES) {
^
controller/physical.c:387:37: error: ‘BUNDLE_MAX_SLAVES’ undeclared (first use in this function)
if (bundle->n_slaves >= BUNDLE_MAX_SLAVES) {
^
controller/physical.c:387:37: note: each undeclared identifier is reported only once for each function it appears in
controller/physical.c:395: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
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 0c818f4..3665c7b 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1543,6 +1543,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 @@ -2067,6 +2077,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); @@ -2074,12 +2091,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; } @@ -2295,11 +2306,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 bdf75c7..c59a7ff 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -21821,6 +21821,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 + # Test dropping traffic destined to router owned IPs. AT_SETUP([ovn -- gateway router drop traffic for own IPs]) ovn_start
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) --- controller/ovn-controller.c | 30 ++++++++++++----- tests/ovn.at | 78 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 8 deletions(-)