"Ansari, Shad" <shad.ans...@hpe.com> wrote on 03/31/2016 06:55:52 PM:
> From: "Ansari, Shad" <shad.ans...@hpe.com>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: "dev@openvswitch.org" <dev@openvswitch.org>
> Date: 03/31/2016 06:55 PM
> Subject: RE: [ovs-dev] [PATCH 1/1] ovsdb-idl: Retain column values
> of deleted rows until change-track is cleared.
>
> > > My experience with trying to clear the change track and re-establish
it
> > > leads
> > > to history being lost [this is likely from my not fully understanding
what
> > > happens with sequence numbers over turning tracking on and off] and
so it
> > > is
> > > more efficient to just leave change tracking on. Under this
scenario, I'm
> > > pretty convinced that this code would amount to a memory leak.
> > >
> >
> > Could you provide more exact description of the problem and the
> steps/code to
> > reproduce.
>
> I ran valgrind on idl test cases with this change and they all came out
clean:
> make check-valgrind TESTSUITEFLAGS='-k idl'
> find . -name "valgrind.*" | xargs cat
>
Apply this patch set:
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index bcad318..f9bcdc0 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -204,7 +204,7 @@ add_logical_flows(struct controller_ctx *ctx, const
struct lport_index *lports,
uint32_t conj_id_ofs = 1;
const struct sbrec_logical_flow *lflow;
- SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) {
+ SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
/* Determine translation of logical table IDs to physical table
IDs. */
bool ingress = !strcmp(lflow->pipeline, "ingress");
@@ -391,7 +391,7 @@ add_neighbor_flows(struct controller_ctx *ctx,
ofpbuf_init(&ofpacts, 0);
const struct sbrec_mac_binding *b;
- SBREC_MAC_BINDING_FOR_EACH (b, ctx->ovnsb_idl) {
+ SBREC_MAC_BINDING_FOR_EACH_TRACKED (b, ctx->ovnsb_idl) {
const struct sbrec_port_binding *pb
= lport_lookup_by_name(lports, b->logical_port);
if (!pb) {
diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
index 6027011..d0a5cbe 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -285,6 +285,7 @@ main(int argc, char *argv[])
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));
+ ovsdb_idl_track_add_all(ovnsb_idl_loop.idl);
ovsdb_idl_get_initial_snapshot(ovnsb_idl_loop.idl);
int probe_interval = 0;
@@ -379,6 +380,7 @@ main(int argc, char *argv[])
}
ovsdb_idl_loop_commit_and_wait(&ovnsb_idl_loop);
ovsdb_idl_loop_commit_and_wait(&ovs_idl_loop);
+ ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
poll_block();
if (should_service_stop()) {
exiting = true;
Run make check TESTSUITEFLAGS='-k ovn'
It looks like the calls to SBREC_*_EACH_TRACKED only show you the
incremental changes since tracking was turned on. Now, that may
in fact be how it was intended to work, but if so, then I question
it's usefulness. In ovn-controller, I can foresee code paths that
differ only in that one uses SBREC_*_FOR_EACH_TRACKED and the other
uses SBREC_*_FOR_EACH, and that's going to be a bit ugly.
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
