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

[Darrell Ball]

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

>
>>
>> I don't testcases for OVN NAT at all - did I miss them ?
>> How can a feature be committed without testcases ?
>>
>
> We currently do not have NAT in userspace datapath to test OVN NAT. I
> think this is the same case with OVN firewall. Both of them are currently
> dependent on OpenStack testing. We should probably do something about it.
>

Its worrisome to have features that cannot be properly tested.
I suggest to prioritize the test support.
No testability -> No feature




>
>
>>
>>
>>
>> >
>> >
>> >
>> >>
>> >>
>> >> >
>> >> >
>> >> >> ---
>> >> >>  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
>> >>
>> >
>> >
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>