[ovs-dev] [PATCH] [ovn-controller] Add logical flow incremental processing
Russell Bryant
russell at ovn.org
Fri Feb 12 19:59:27 UTC 2016
On 02/10/2016 05:14 PM, Ryan Moats wrote:
> From: RYAN D. MOATS <rmoats at us.ibm.com>
>
> Add incremental processing of logical flows in ovn-controller by
> tracking changes to the match column of the Logical_Flow
> OVN SB table. Code is included to properly handle the order of
> checked sequence numbers and rechecking of rows skipped due to
> a logical_datapath not existing.
>
> Signed-off-by: RYAN D. MOATS <rmoats at us.ibm.com>
> ---
> ovn/controller/lflow.c | 59 +++++++++++++++++++++++++++++++++++----
> ovn/controller/lflow.h | 5 ++-
> ovn/controller/ovn-controller.c | 26 ++++++++++------
> 3 files changed, 72 insertions(+), 18 deletions(-)
This patch introduces a whitespace error:
Applying: Add logical flow incremental processing
/home/rbryant/src/ovs/.git/rebase-apply/patch:152: trailing whitespace.
warning: 1 line adds whitespace errors.
It also introduces a compiler warning:
ovn/controller/lflow.c: In function ‘lflow_run’:
ovn/controller/lflow.c:283:18: error: unused variable ‘skipped_seqno’
[-Werror=unused-variable]
unsigned int skipped_seqno;
I made some style comments throughout the code, but more importantly, I
don't think this is doing quite what you expect.
The main loop of ovn-controller is rebuilding the whole OpenFlow flow
table on every iteration. Since you've added code to skip some logical
flow rows, we end up with an incomplete flow table based on whichever
flows didn't get skipped during the last iteration.
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index d53213c..e34c8be 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -12,7 +12,6 @@
> * See the License for the specific language governing permissions and
> * limitations under the License.
> */
> -
There are some unrelated whitespace changes in this patch. Please drop
them.
> #include <config.h>
> #include "lflow.h"
> #include "dynamic-string.h"
> @@ -273,25 +272,61 @@ lflow_init(void)
>
> /* Translates logical flows in the Logical_Flow table in the OVN_SB database
> * into OpenFlow flows. See ovn-architecture(7) for more information. */
> -void
> +unsigned int
> lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
> const struct simap *ct_zones,
> - struct hmap *local_datapaths)
> + struct hmap *local_datapaths,
> + unsigned int seqno)
> {
> struct hmap flows = HMAP_INITIALIZER(&flows);
> uint32_t conj_id_ofs = 1;
> + unsigned int skipped_seqno;
> + unsigned int processed_seqno = seqno;
> + unsigned int last_seqno = 0;
>
> ldp_run(ctx);
>
> const struct sbrec_logical_flow *lflow;
> - SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) {
> - /* Find the "struct logical_datapath" associated with this
> + SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
> + unsigned int del_seqno = sbrec_logical_flow_row_get_seqno(lflow,
> + OVSDB_IDL_CHANGE_DELETE);
> + unsigned int mod_seqno = sbrec_logical_flow_row_get_seqno(lflow,
> + OVSDB_IDL_CHANGE_MODIFY);
> + unsigned int ins_seqno = sbrec_logical_flow_row_get_seqno(lflow,
> + OVSDB_IDL_CHANGE_INSERT);
> + if (del_seqno < seqno && mod_seqno < seqno && ins_seqno < seqno) {
> + continue;
> + }
> + if (del_seqno > 0) {
> + if (del_seqno > processed_seqno) {
> + processed_seqno = del_seqno;
> + }
> + /* we really don't want to re-process a deleted record so */
> + continue;
> + }
> + if (mod_seqno > processed_seqno) {
> + processed_seqno = mod_seqno;
> + }
> + if (ins_seqno > processed_seqno) {
> + processed_seqno = ins_seqno;
> + }
> +
> + /* Find the "struct logical_datapath" asssociated with this
> * Logical_Flow row. If there's no such struct, that must be because
> * no logical ports are bound to that logical datapath, so there's no
> - * point in maintaining any flows for it anyway, so skip it. */
> + * point in maintaining any flows for it anyway, so skip it.
> + * Further, we have to remember the smallest sequence number of
> + * a skipped flow to ensure that we return the correct value
> + * and don't skip things in a later pass */
> const struct logical_datapath *ldp;
> ldp = ldp_lookup(lflow->logical_datapath);
> if (!ldp) {
> + if (last_seqno == 0 || (mod_seqno > 0 && last_seqno > mod_seqno)) {
> + last_seqno = mod_seqno;
> + }
> + if (last_seqno == 0 || (ins_seqno > 0 && last_seqno > ins_seqno)) {
> + last_seqno = ins_seqno;
> + }
> continue;
> }
>
> @@ -322,6 +357,14 @@ lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
> struct hmap_node *ld;
> ld = hmap_first_with_hash(local_datapaths, ldp->tunnel_key);
> if (!ld) {
> + if (last_seqno == 0 || (mod_seqno > 0 &&
> + last_seqno > mod_seqno)) {
> + last_seqno = mod_seqno;
> + }
> + if (last_seqno == 0 || (ins_seqno > 0 &&
> + last_seqno > ins_seqno)) {
> + last_seqno = ins_seqno;
> + }
> continue;
> }
> }
> @@ -425,6 +468,10 @@ lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
> ofpbuf_uninit(&ofpacts);
> conj_id_ofs += n_conjs;
> }
> + if (last_seqno > 0) {
> + processed_seqno = last_seqno;
> + }
> + return(processed_seqno);
Please remove these parenthesis.
> }
>
> void
> diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h
> index ccbad30..91320a7 100644
> --- a/ovn/controller/lflow.h
> +++ b/ovn/controller/lflow.h
> @@ -56,9 +56,10 @@ struct uuid;
> #define LOG_PIPELINE_LEN 16
>
> void lflow_init(void);
> -void lflow_run(struct controller_ctx *, struct hmap *flow_table,
> +unsigned int lflow_run(struct controller_ctx *, struct hmap *flow_table,
> const struct simap *ct_zones,
> - struct hmap *local_datapaths);
> + struct hmap *local_datapaths,
> + unsigned int seqno);
> void lflow_destroy(void);
>
> #endif /* ovn/lflow.h */
> diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
> index 3638342..859a0f9 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -204,6 +204,7 @@ main(int argc, char *argv[])
> struct unixctl_server *unixctl;
> bool exiting;
> int retval;
> + unsigned int last_seqno;
ovn-controller works with two OVSDB databases. It would be good to
change the name of this variable to clarify that it's related to ovnsb.
>
> ovs_cmdl_proctitle_init(argc, argv);
> set_program_name(argv[0]);
> @@ -257,9 +258,16 @@ main(int argc, char *argv[])
>
> /* Connect to OVN SB database. */
> char *ovnsb_remote = get_ovnsb_remote(ovs_idl_loop.idl);
> - struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
> - ovsdb_idl_create(ovnsb_remote, &sbrec_idl_class, true, true));
It looks like you only needed to add one right here, instead of the rest
of the changes in this spot.
ovsdb_idl_track_add_column(ovnsb_idl_loop.idl,
&sbrec_logical_flow_col_match);
> - ovsdb_idl_get_initial_snapshot(ovnsb_idl_loop.idl);
> + struct ovsdb_idl *sb_idl = ovsdb_idl_create(ovnsb_remote,
> + &sbrec_idl_class, true, true);
> + /* set to track the southbound idl */
> + ovsdb_idl_track_add_column(sb_idl, &sbrec_logical_flow_col_match);
> +
> + /*ovsdb_idl_track_add_all(sb_idl);*/
> + struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(sb_idl);
> +
> +
> + ovsdb_idl_get_initial_snapshot(sb_idl);
>
> /* Initialize connection tracking zones. */
> struct simap ct_zones = SIMAP_INITIALIZER(&ct_zones);
> @@ -271,6 +279,9 @@ main(int argc, char *argv[])
> /* Main loop. */
> exiting = false;
> while (!exiting) {
> + if (last_seqno == 0) {
It looks like last_seqno is uninitialized the first time this code executes.
> + last_seqno = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl);
> + }
> struct controller_ctx ctx = {
> .ovs_idl = ovs_idl_loop.idl,
> .ovs_idl_txn = ovsdb_idl_loop_run(&ovs_idl_loop),
> @@ -294,13 +305,12 @@ main(int argc, char *argv[])
>
> if (br_int) {
> patch_run(&ctx, br_int, &local_datapaths);
> -
> enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int);
> -
> pinctrl_run(&ctx, br_int);
>
> struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
> - lflow_run(&ctx, &flow_table, &ct_zones, &local_datapaths);
> + last_seqno = lflow_run(&ctx, &flow_table, &ct_zones,
> + &local_datapaths, last_seqno);
> if (chassis_id) {
> physical_run(&ctx, mff_ovn_geneve,
> br_int, chassis_id, &ct_zones, &flow_table);
> @@ -320,17 +330,13 @@ main(int argc, char *argv[])
> free(cur_node);
> }
> hmap_destroy(&local_datapaths);
> -
> unixctl_server_run(unixctl);
> -
> unixctl_server_wait(unixctl);
> if (exiting) {
> poll_immediate_wake();
> }
> -
> ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
> ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop);
> -
> if (br_int) {
> ofctrl_wait();
> pinctrl_wait();
>
Please drop all of these formatting changes.
--
Russell Bryant
More information about the dev
mailing list