[ovs-dev] [patch_v1] ovn: Remove unreferenced patched datapaths

Darrell Ball dlu998 at gmail.com
Thu Jul 7 22:49:36 UTC 2016


On Thu, Jul 7, 2016 at 3:45 PM, Guru Shetty <guru at ovn.org> wrote:

>
>
> On 7 July 2016 at 14:15, Darrell Ball <dlu998 at gmail.com> wrote:
>
>> On Thu, Jul 7, 2016 at 1:17 PM, Russell Bryant <russell at ovn.org> wrote:
>>
>> >
>> >
>> > On Wed, Jul 6, 2016 at 8:37 PM, Darrell Ball <dlu998 at gmail.com> wrote:
>> >
>> >> Patched datapaths that are no longer referenced should be removed from
>> >> the patched_datapaths map; otherwise incorrect state references for a
>> >> patched datapath may be used and also datapaths that are absent will be
>> >> interpreted as present.
>> >>
>> >> Signed-off-by: Darrell Ball <dlu998 at gmail.com>
>> >>
>> >
>> > What failure was observed to find this issue?
>> >
>>
>> This is caught by code reading as I would like to rely on the correctness
>> of this
>> internal datastructure in future.
>>
>>
>>
>> > Could we add a test case that would have caught it?
>> >
>>
>> I tested this with instrumented code and a contrived test.
>> Since this relates to internal datastructure management, I don't intend to
>> add a test case
>> for this.
>>
>> When testcases are added for OVN NAT code, which relies on this
>> datastructure internal fields, I might add a delta
>> on top of those if those tests are lacking. If I also add a feature that
>> depends on this datastructure, I will a
>> test for it as I normally do...
>>
>
> Fwiw, there are a couple of unit tests that currently add gateway routers
> (but don't test deleting them)
> e.g: https://github.com/openvswitch/ovs/blob/master/tests/ovn.at#L2940
>


I don't testcases for OVN NAT at all - did I miss them ?
How can a feature be committed without testcases ?



>
>
>
>>
>>
>> >
>> >
>> >> ---
>> >>  ovn/controller/ovn-controller.h |  3 ++-
>> >>  ovn/controller/patch.c          | 23 ++++++++++++++++++++---
>> >>  2 files changed, 22 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/ovn/controller/ovn-controller.h
>> >> b/ovn/controller/ovn-controller.h
>> >> index 6a021a0..8816940 100644
>> >> --- a/ovn/controller/ovn-controller.h
>> >> +++ b/ovn/controller/ovn-controller.h
>> >> @@ -51,8 +51,9 @@ struct local_datapath *get_local_datapath(const
>> struct
>> >> hmap *,
>> >>   * with at least one logical patch port binding. */
>> >>  struct patched_datapath {
>> >>      struct hmap_node hmap_node;
>> >> -    bool local; /* 'True' if the datapath is for gateway router. */
>> >>      const struct sbrec_port_binding *port_binding;
>> >> +    bool local; /* 'True' if the datapath is for gateway router. */
>> >> +    bool stale;
>> >>  };
>> >>
>> >>  struct patched_datapath *get_patched_datapath(const struct hmap *,
>> >> diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
>> >> index 589529e..a701db2 100644
>> >> --- a/ovn/controller/patch.c
>> >> +++ b/ovn/controller/patch.c
>> >> @@ -252,12 +252,14 @@ static void
>> >>  add_patched_datapath(struct hmap *patched_datapaths,
>> >>                       const struct sbrec_port_binding *binding_rec,
>> bool
>> >> local)
>> >>  {
>> >> -    if (get_patched_datapath(patched_datapaths,
>> >> -                             binding_rec->datapath->tunnel_key)) {
>> >> +    struct patched_datapath * pd;
>> >> +    if (pd = get_patched_datapath(patched_datapaths,
>> >> +                                  binding_rec->datapath->tunnel_key))
>> {
>> >>
>> >
>> > Can you move the assignment outside the if condition?
>> >
>> > CodingStyle.md says "Avoid assignments inside "if" and "while"
>> conditions."
>> >
>>
>>
>> alright
>>
>>
>>
>> >
>> >
>> >> +        pd->stale = false;
>> >>          return;
>> >>      }
>> >>
>> >> -    struct patched_datapath *pd = xzalloc(sizeof *pd);
>> >> +    pd = xzalloc(sizeof *pd);
>> >>      pd->local = local;
>> >>      pd->port_binding = binding_rec;
>> >>      hmap_insert(patched_datapaths, &pd->hmap_node,
>> >> @@ -300,6 +302,13 @@ add_logical_patch_ports(struct controller_ctx
>> *ctx,
>> >>      }
>> >>
>> >>      const struct sbrec_port_binding *binding;
>> >> +
>> >> +    /* Mark all patched datapaths as stale for later cleanup check */
>> >> +    struct patched_datapath *pd;
>> >> +    HMAP_FOR_EACH (pd, hmap_node, patched_datapaths) {
>> >> +        pd->stale = true;
>> >> +    }
>> >> +
>> >>      SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
>> >>          bool local_port = false;
>> >>          if (!strcmp(binding->type, "gateway")) {
>> >> @@ -332,6 +341,14 @@ add_logical_patch_ports(struct controller_ctx
>> *ctx,
>> >>              }
>> >>          }
>> >>      }
>> >> +    /* Clean up stale patched datapaths. */
>> >> +    struct patched_datapath *pd_cur_node, *pd_next_node;
>> >> +    HMAP_FOR_EACH_SAFE (pd_cur_node, pd_next_node, hmap_node,
>> >> patched_datapaths) {
>> >> +        if (pd_cur_node->stale == true) {
>> >> +            hmap_remove(patched_datapaths, &pd_cur_node->hmap_node);
>> >> +            free(pd_cur_node);
>> >> +        }
>> >> +    }
>> >>  }
>> >>
>> >>  void
>> >> --
>> >> 1.9.1
>> >>
>> >> _______________________________________________
>> >> dev mailing list
>> >> dev at openvswitch.org
>> >> http://openvswitch.org/mailman/listinfo/dev
>> >>
>> >
>> >
>> >
>> > --
>> > Russell Bryant
>> >
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>



More information about the dev mailing list