Russell Bryant <russ...@ovn.org> wrote on 02/12/2016 01:59:27 PM:
> From: Russell Bryant <russ...@ovn.org>
> To: Ryan Moats/Omaha/IBM@IBMUS, dev@openvswitch.org
> Date: 02/12/2016 01:59 PM
> Subject: Re: [ovs-dev] [PATCH] [ovn-controller] Add logical flow
> incremental processing
>
> On 02/10/2016 05:14 PM, Ryan Moats wrote:
> > From: RYAN D. MOATS <rmo...@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 <rmo...@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;
Yep, that's fixed now...
> 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.
Ah, thanks for the time in channel - I misread this the first time through
but now I think I see what needs to be done...
> > 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.
It looks like my editor had some fun at my expense...
>
> > #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.
Done
>
> > }
> >
> > 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.
Excellent suggestion - done...
> >
> > 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);
The commit message around change tracking did not make this clear, and
if IIRC, I tried that and it didn't work. I will try again and update
the patch accordingly
> > - 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.
I deserve a dope slap for that one.
> > + 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.
Done.
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
