Russell Bryant <russ...@ovn.org> wrote on 07/27/2016 04:13:34 PM:
> From: Russell Bryant <russ...@ovn.org>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: ovs dev <dev@openvswitch.org>
> Date: 07/27/2016 04:14 PM
> Subject: Re: [ovs-dev] [PATCH v2] ovn-controller: Restore all_lports
> for update_ct_zone
>
> On Wed, Jul 27, 2016 at 8:42 AM, Ryan Moats <rmo...@us.ibm.com> wrote:
> As [1] indicates, commit 263064a (Convert binding_run
> to incremental processing.) incorrectly localized
> all_lports to the binding module, leaving an empty
> set for update_ct_zone to work with. This patch
> restores all_lports processing to what existed prior
> to that patch.
>
> [1] http://openvswitch.org/pipermail/dev/2016-July/076171.html
>
> Signed-off-by: Ryan Moats <rmo...@us.ibm.com>
> ---
> v1->v2: Added missing reference to commit message
>
> ovn/controller/binding.c | 30 +++++++++++++++++++++++++++++-
> ovn/controller/binding.h | 3 ++-
> ovn/controller/ovn-controller.c | 3 ++-
> 3 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index e83c1d5..82ee3ba 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -230,7 +230,8 @@ consider_local_datapath(struct controller_ctx *ctx,
>
> void
> binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge
*br_int,
> - const char *chassis_id, struct hmap *local_datapaths)
> + const char *chassis_id, struct sset *all_lports,
> + struct hmap *local_datapaths)
> {
> const struct sbrec_chassis *chassis_rec;
> const struct sbrec_port_binding *binding_rec;
> @@ -251,6 +252,33 @@ binding_run(struct controller_ctx *ctx, const
> struct ovsrec_bridge *br_int,
> process_full_binding = true;
> }
>
> + struct shash_node *node;
> + SHASH_FOR_EACH (node, &lport_to_iface) {
> + sset_add(all_lports, node->name);
> + }
>
> I think you could just sset_clone() local_ids into all_lports and
> get the same result.
You are correct
>
> +
> + /* Run through all binding records to collect lists of lports
> + for later use in updating ct zones. */
> + SBREC_PORT_BINDING_FOR_EACH(binding_rec, ctx->ovnsb_idl) {
> + const struct ovsrec_interface *iface_rec
> + = shash_find_data(&lport_to_iface, binding_rec->
logical_port);
> + if (iface_rec
> + || (binding_rec->parent_port && binding_rec->parent_port[0]
&&
> + sset_contains(all_lports, binding_rec->parent_port))) {
> + if (binding_rec->parent_port && binding_rec->parent_port[0])
{
> + /* Add child logical port to the set of all local ports.
*/
> + sset_add(all_lports, binding_rec->logical_port);
> + }
> + } else if (!binding_rec->chassis
> + && !strcmp(binding_rec->type, "localnet")) {
> + /* localnet ports will never be bound to a chassis, but we
want
> + * to list them in all_lports because we want to allocate
> + * a conntrack zone ID for each one, as we'll be creating
> + * a patch port for each one. */
> + sset_add(all_lports, binding_rec->logical_port);
> + }
> + }
> +
>
> This seems to undo the benefits of the original change to do
> "incremental procesing" in binding.c.
>
> It seems like we weren't that far from a complete fix in Babu's first
patch.
We weren't/aren't - the last piece is how to handle persisting the
information
in the above code snippet, which could then be appended to the cloned
local_ids
for the final result. What I thought of during the day (and I was still
looking
for something more efficient) would be to use a hashmap that included the
binding_rec uuid and the logical_port string - then I could just check to
see if
the string already existed on the add side, and remove the value based on
the
uuid on the delete side.
Ryan
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
