[ovs-dev] controller: Don't release LRP until its claimed

Previously LRP would be released as soon as the BFD session to
another chassis is established even if that chassis is unable to
claim the LRP for whatever reason.
This patch introduces a cache to keep track of LRPs claimed by the
current chassis and release it only if an update event for LRP claim
from another chassis

Signed-off-by: Ihtisham ul Haq <ihtisham.ul_haq@mail.schwarz>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2023-September/052688.html
---


Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für die Verwertung durch den vorgesehenen Empfänger bestimmt.
Sollten Sie nicht der vorgesehene Empfänger sein, setzen Sie den Absender bitte unverzüglich in Kenntnis und löschen diese E Mail.

Hinweise zum Datenschutz finden Sie hier<https://www.datenschutz.schwarz>.


This e-mail may contain confidential content and is intended only for the specified recipient/s.
If you are not the intended recipient, please inform the sender immediately and delete this e-mail.

Information on data protection can be found here<https://www.datenschutz.schwarz>.

Hi Ihtisham ul Haq,

Thanks for the patch and sorry for the delay in reviewing it!

On 12/19/23 17:46, Ihtisham ul Haq via dev wrote:
> Previously LRP would be released as soon as the BFD session to
> another chassis is established even if that chassis is unable to
> claim the LRP for whatever reason.

But if the BFD session went down and the new ha_chassis leader can't
claim the port networking will not work anyway.  So what's the point of
keeping the current "claim"?  Or am I missing something?

> This patch introduces a cache to keep track of LRPs claimed by the
> current chassis and release it only if an update event for LRP claim
> from another chassis

IIUC there's a risk we never get that claim and then we just don't
release the LRP for ever.  That sounds wrong.

> 
> Signed-off-by: Ihtisham ul Haq <ihtisham.ul_haq@mail.schwarz>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2023-September/052688.html

From your report there:

"We observed this issue when a GTW Chassis gets rebooted without a
proper termination i.e `ovn-appctl exit()`, this results in higher
priority LRPs persisting for that GTW Chassis in the Southbound(not
rescheduled by CMS). And when the node comes back up again, the
lower priority GTW Chassis releases the LRP, but the restarted node
doesn't reclaim it, causing router outages. Below is a case we came
across in our environment, and at the bottom is a patch to remedy
this issue."

I don't really understand why not releasing the lrp fixes your issue.
Shouldn't we actually fix the fact that "the restarted node
doesn't reclaim it"?

> ---
> 
> diff --git a/controller/automake.mk b/controller/automake.mk
> --- a/controller/automake.mk
> +++ b/controller/automake.mk
> @@ -22,6 +22,7 @@ controller_ovn_controller_SOURCES = \
>         controller/lflow-conj-ids.h \
>         controller/lport.c \
>         controller/lport.h \
> +       controller/lrp-claim.h \

There's no lrp-claim.h file added by this patch.  Is this an accidental
leftover?

>         controller/ofctrl.c \
>         controller/ofctrl.h \
>         controller/ofctrl-seqno.c \
> diff --git a/controller/binding.c b/controller/binding.c
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -63,6 +63,58 @@ static struct shash _claimed_ports = SHASH_INITIALIZER(&_claimed_ports);
>  static struct sset _postponed_ports = SSET_INITIALIZER(&_postponed_ports);
>  static struct shash _qos_ports = SHASH_INITIALIZER(&_qos_ports);
> 
> +
> +void add_lrp_claim(struct binding_ctx_out *ctx, const char *lrp_id) {
> +    if (find_lrp_claim(ctx, lrp_id)) {
> +        return;
> +    }
> +
> +    struct lrp_claim *new_claim = xmalloc(sizeof *new_claim);
> +    new_claim->lrp_id = xstrdup(lrp_id);
> +    new_claim->next = ctx->lrp_claims->claim;
> +    ctx->lrp_claims->claim = new_claim;
> +    ctx->lrp_claims->size++;
> +}
> +
> +void remove_lrp_claim(struct binding_ctx_out *ctx, char *lrp_id) {
> +    struct lrp_claim *prev = NULL;
> +    struct lrp_claim *current = ctx->lrp_claims->claim;
> +
> +    while (current) {
> +        if (current->lrp_id && !strcmp(current->lrp_id, lrp_id)) {
> +            if (prev) {
> +                prev->next = current->next;
> +            } else {
> +                ctx->lrp_claims->claim = current->next;
> +            }
> +
> +            free(current->lrp_id);
> +            free(current);
> +
> +            ctx->lrp_claims->size--;
> +
> +            return;
> +        }
> +
> +        prev = current;
> +        current = current->next;
> +    }
> +}

On a closer look, what you should probably use here is a sset (set of
strings, i.e., set of lrp ids).  Please check 'postponed_ports' for a
similar example.

> +
> +bool find_lrp_claim(struct binding_ctx_out *ctx, const char *lrp_id) {
> +    if (!ctx->lrp_claims || !lrp_id) {
> +        return false;
> +    }
> +    struct lrp_claim *current = ctx->lrp_claims->claim;
> +    while (current) {
> +        if (current->lrp_id && strcmp(current->lrp_id, lrp_id) == 0) {
> +            return true;
> +        }
> +        current = current->next;
> +    }
> +    return false;
> +}
> +
>  static void
>  remove_additional_chassis(const struct sbrec_port_binding *pb,
>                            const struct sbrec_chassis *chassis_rec);
> @@ -1821,7 +1873,8 @@ static bool
>  consider_nonvif_lport_(const struct sbrec_port_binding *pb,
>                         bool our_chassis,
>                         struct binding_ctx_in *b_ctx_in,
> -                       struct binding_ctx_out *b_ctx_out)
> +                       struct binding_ctx_out *b_ctx_out,
> +                       bool should_release)
>  {
>      if (our_chassis) {
>          update_local_lports(pb->logical_port, b_ctx_out);
> @@ -1831,8 +1884,8 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb,
>                             pb->datapath, b_ctx_in->chassis_rec,
>                             b_ctx_out->local_datapaths,
>                             b_ctx_out->tracked_dp_bindings);
> -
>          update_related_lport(pb, b_ctx_out);
> +        add_lrp_claim(b_ctx_out, pb->logical_port);
>          return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL,
>                             !b_ctx_in->ovnsb_idl_txn, false,
>                             b_ctx_out->tracked_dp_bindings,
> @@ -1840,16 +1893,13 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb,
>                             b_ctx_out->postponed_ports);
>      }
> 
> -    if (pb->chassis == b_ctx_in->chassis_rec
> -            || is_additional_chassis(pb, b_ctx_in->chassis_rec)
> -            || if_status_is_port_claimed(b_ctx_out->if_mgr,
> -                                         pb->logical_port)) {
> +    if (should_release) {
> +        remove_lrp_claim(b_ctx_out, pb->logical_port);
>          return release_lport(pb, b_ctx_in->chassis_rec,
>                               !b_ctx_in->ovnsb_idl_txn,
>                               b_ctx_out->tracked_dp_bindings,
>                               b_ctx_out->if_mgr);
>      }
> -
>      return true;
>  }
> 
> @@ -1862,7 +1912,7 @@ consider_l2gw_lport(const struct sbrec_port_binding *pb,
>      bool our_chassis = chassis_id && !strcmp(chassis_id,
>                                               b_ctx_in->chassis_rec->name);
> 
> -    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out);
> +    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out, true);
>  }
> 
>  static bool
> @@ -1874,7 +1924,7 @@ consider_l3gw_lport(const struct sbrec_port_binding *pb,
>      bool our_chassis = chassis_id && !strcmp(chassis_id,
>                                               b_ctx_in->chassis_rec->name);
> 
> -    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out);
> +    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out, true);
>  }
> 
>  static void
> @@ -1920,6 +1970,11 @@ consider_ha_lport(const struct sbrec_port_binding *pb,
>                                               b_ctx_in->active_tunnels,
>                                               b_ctx_in->chassis_rec);
> 
> +    bool should_release = false;
> +    if (pb && pb->chassis){

Checkpatch reports "ERROR: Inappropriate bracing around statement".

> +        should_release = find_lrp_claim(b_ctx_out, pb->logical_port) && strcmp(pb->chassis->hostname, b_ctx_in->chassis_rec->hostname)  && !our_chassis;

Checkpatch reports "WARNING: Line is 152 characters long (recommended
limit is 79)"

> +    }
> +
>      if (is_ha_chassis && !our_chassis) {
>          /* If the chassis_rec is part of ha_chassis_group associated with
>           * the port_binding 'pb', we need to add to the local_datapath
> @@ -1937,7 +1992,8 @@ consider_ha_lport(const struct sbrec_port_binding *pb,
>          update_related_lport(pb, b_ctx_out);
>      }
> 
> -    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out);
> +    return consider_nonvif_lport_(pb, our_chassis, b_ctx_in, b_ctx_out,
> +                                  should_release);
>  }
> 
>  static bool
> diff --git a/controller/binding.h b/controller/binding.h
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -74,6 +74,16 @@ struct related_lports {
>  void related_lports_init(struct related_lports *);
>  void related_lports_destroy(struct related_lports *);
> 
> +struct lrp_claim {
> +    char *lrp_id;
> +    struct lrp_claim *next;

Please don't re-implement lists.  Just use struct ovs_list or other
library data structures.

> +};
> +
> +struct lrp_claim_list {
> +    struct lrp_claim *claim;
> +    size_t size;
> +};
> +
>  struct binding_ctx_out {
>      struct hmap *local_datapaths;
>      struct shash *local_active_ports_ipv6_pd;
> @@ -111,8 +121,17 @@ struct binding_ctx_out {
> 
>      bool localnet_learn_fdb;
>      bool localnet_learn_fdb_changed;
> +
> +    struct lrp_claim_list *lrp_claims;
>  };
> 
> +
> +void add_lrp_claim(struct binding_ctx_out *ctx, const char *lrp_id);
> +
> +void remove_lrp_claim(struct binding_ctx_out *ctx, char *lrp_id);
> +
> +bool find_lrp_claim(struct binding_ctx_out *ctx, const char *lrp_id);

These should be static in binding.c.  They're only used there.

> +
>  /* Local bindings. binding.c module binds the logical port (represented by
>   * Port_Binding rows) and sets the 'chassis' column when it sees the
>   * OVS interface row (of type "" or "internal") with the
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1461,6 +1461,54 @@ en_postponed_ports_run(struct engine_node *node, void *data_)
>      engine_set_node_state(node, state);
>  }
> 
> +struct ed_type_lrp_claims {
> +    struct lrp_claim_list *lrp_claims;
> +};
> +
> +static void *
> +en_lrp_claims_init(struct engine_node *node OVS_UNUSED, struct engine_arg *arg OVS_UNUSED)

Checkpatch reports "WARNING: Line is 90 characters long (recommended
limit is 79)".

> +{
> +    struct ed_type_lrp_claims *data = xzalloc(sizeof *data);
> +    data->lrp_claims = xzalloc(sizeof *(data->lrp_claims));
> +
> +    data->lrp_claims->claim = NULL;
> +    data->lrp_claims->size = 0;
> +
> +    return data;
> +}
> +
> +static void
> +en_lrp_claims_cleanup(void *data_)
> +{
> +    struct ed_type_lrp_claims *data = data_;
> +
> +    if(data->lrp_claims->size == 0){

Checkpatch reports "ERROR: Improper whitespace around control block".

> +        return;
> +    }
> +    struct lrp_claim *current = data->lrp_claims->claim;
> +    while (data->lrp_claims->size > 0) {
> +        struct lrp_claim *next = current->next;
> +        free(current->lrp_id);
> +        free(current);
> +        current = next;
> +        data->lrp_claims->size--;
> +    }
> +    data->lrp_claims = NULL;
> +}
> +
> +static void
> +en_lrp_claims_run(struct engine_node *node, void *data_)
> +{
> +    struct ed_type_lrp_claims *data = data_;
> +    enum engine_node_state state = EN_UNCHANGED;
> +
> +    if (data->lrp_claims) {
> +        state = EN_UPDATED;
> +    }
> +
> +    engine_set_node_state(node, state);
> +}
> +
>  struct ed_type_runtime_data {
>      /* Contains "struct local_datapath" nodes. */
>      struct hmap local_datapaths;
> @@ -1495,6 +1543,8 @@ struct ed_type_runtime_data {
>      struct shash local_active_ports_ras;
> 
>      struct sset *postponed_ports;
> +
> +    struct lrp_claim_list *lrp_claims;
>  };
> 
>  /* struct ed_type_runtime_data has the below members for tracking the
> @@ -1701,6 +1751,7 @@ init_binding_ctx(struct engine_node *node,
>      b_ctx_out->if_mgr = ctrl_ctx->if_mgr;
>      b_ctx_out->localnet_learn_fdb = rt_data->localnet_learn_fdb;
>      b_ctx_out->localnet_learn_fdb_changed = false;
> +    b_ctx_out->lrp_claims = rt_data->lrp_claims;
>  }
> 
>  static void
> @@ -1739,6 +1790,9 @@ en_runtime_data_run(struct engine_node *node, void *data)
>      struct ed_type_postponed_ports *pp_data =
>          engine_get_input_data("postponed_ports", node);
>      rt_data->postponed_ports = pp_data->postponed_ports;
> +    struct ed_type_lrp_claims *lrp_claims_data =
> +        engine_get_input_data("lrp_claims", node);
> +    rt_data->lrp_claims = lrp_claims_data->lrp_claims;
> 
>      struct binding_ctx_in b_ctx_in;
>      struct binding_ctx_out b_ctx_out;
> @@ -5179,6 +5233,7 @@ main(int argc, char *argv[])
>      ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
>      ENGINE_NODE_WITH_CLEAR_TRACK_DATA(activated_ports, "activated_ports");
>      ENGINE_NODE(postponed_ports, "postponed_ports");
> +    ENGINE_NODE(lrp_claims, "lrp_claims");
>      ENGINE_NODE(pflow_output, "physical_flow_output");
>      ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output, "logical_flow_output");
>      ENGINE_NODE(controller_output, "controller_output");
> @@ -5352,6 +5407,8 @@ main(int argc, char *argv[])
>      /* Reuse the same handler for any previously postponed ports. */
>      engine_add_input(&en_runtime_data, &en_postponed_ports,
>                       runtime_data_sb_port_binding_handler);
> +    engine_add_input(&en_runtime_data, &en_lrp_claims,
> +                     runtime_data_sb_port_binding_handler);
>      /* Run sb_ro_handler after port_binding_handler in case port get deleted */
>      engine_add_input(&en_runtime_data, &en_sb_ro, runtime_data_sb_ro_handler);
> 
> diff --git a/tests/ovn.at b/tests/ovn.at
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -13308,22 +13308,6 @@ AT_CHECK([tcpdump -r main/br-phys_n1-tx.pcap arp[[24:4]]=0x0a000003 | wc -l],[0]
>  0
>  ],[ignore])
> 
> -# now disown both external ports, one by moving to another (non-existing)
> -# chassis, another by removing the port from any ha groups
> -check ovn-nbctl --wait=sb ha-chassis-group-add fake_hagrp
> -fake_hagrp_uuid=`ovn-nbctl --bare --columns _uuid find ha_chassis_group name=fake_hagrp`
> -check ovn-nbctl set logical_switch_port lext ha_chassis_group=$fake_hagrp_uuid
> -check ovn-nbctl clear logical_switch_port lext2 ha_chassis_group
> -check ovn-nbctl --wait=hv sync
> -
> -# check that traffic no longer leaks into localnet
> -send_frames
> -
> -for suffix in 2 a; do
> -    AT_CHECK([tcpdump -r main/br-phys_n1-tx.pcap arp[[24:4]]=0x0a00000${suffix} | wc -l],[0],[dnl
> -1
> -],[ignore])
> -done
> 

This was part of the "localport doesn't suppress ARP directed to
external port" test.  Why do we need to remove it?  If it needs changes
after your patch, let's do that instead.

Also, please add a test for your specific scenario.

>  AT_CLEANUP
>  ])
> @@ -19933,7 +19917,6 @@ reset_pcap_file hv1-ext1 hv1/ext1
> 
>  # Delete the ha-chassis hv1.
>  ovn-nbctl ha-chassis-group-remove-chassis hagrp1 hv1
> -wait_row_count Port_Binding 1 logical_port=ls1-lp_ext1 chassis='[[]]'
> 
>  # Add hv2 to the ha chassis group
>  ovn-nbctl --wait=hv ha-chassis-group-add-chassis hagrp1 hv2 20
> @@ -34428,7 +34411,7 @@ check ovs-vsctl add-port br-int ls0-hv -- set Interface ls0-hv external-ids:ifac
>  check ovn-nbctl lr-add lr0
> 
>  check ovn-nbctl ls-add ls0
> -check ovn-nbctl lsp-add ls0 ls0-lr0
> +check ovn-nbctl lsp-add ls0 ls0-lr0

Was the diff manually changed?  I don't see any difference between the
two lines above.

>  check ovn-nbctl lsp-set-type ls0-lr0 router
>  check ovn-nbctl lsp-set-addresses ls0-lr0 router
>  check ovn-nbctl lrp-add lr0 lr0-ls0 00:00:00:00:00:01 10.0.0.1
> 

Regards,
Dumitru

On 1/23/24 11:55, Dumitru Ceara wrote:

[...]

>> +static void *
>> +en_lrp_claims_init(struct engine_node *node OVS_UNUSED, struct engine_arg *arg OVS_UNUSED)
> 
> Checkpatch reports "WARNING: Line is 90 characters long (recommended
> limit is 79)".
> 
>> +{
>> +    struct ed_type_lrp_claims *data = xzalloc(sizeof *data);
>> +    data->lrp_claims = xzalloc(sizeof *(data->lrp_claims));
>> +
>> +    data->lrp_claims->claim = NULL;
>> +    data->lrp_claims->size = 0;
>> +
>> +    return data;
>> +}

Running this through CI shows a memleak:

==18945==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x558463f97e88 in __interceptor_calloc (/workspace/ovn-tmp/controller/ovn-controller+0x304e88) (BuildId: b928f7b59bd18b4fec080b5a3a9f0098e471ec2a)
    #1 0x55846437e94e in xcalloc__ /workspace/ovn-tmp/ovs/lib/util.c:124:31
    #2 0x55846437e94e in xzalloc__ /workspace/ovn-tmp/ovs/lib/util.c:134:12
    #3 0x55846437e94e in xzalloc /workspace/ovn-tmp/ovs/lib/util.c:168:12
    #4 0x5584640d1ec7 in en_lrp_claims_init /workspace/ovn-tmp/controller/ovn-controller.c:1470:24
    #5 0x5584641dd636 in engine_init /workspace/ovn-tmp/lib/inc-proc-eng.c:206:17
    #6 0x5584640c73fe in main /workspace/ovn-tmp/controller/ovn-controller.c:5447:5
    #7 0x7f84fdd8fd8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e)

One way to trigger CI is to push the patch to your GitHub fork.

For example, my run:
https://github.com/dceara/ovn/actions/runs/7624514129

Hope this helps,

Regards,
Dumitru