On 11/15/23 06:30, Han Zhou wrote:
> On Thu, Oct 26, 2023 at 11:14 AM <[email protected]> wrote:
>>
>> From: Numan Siddique <[email protected]>
>>
>> northd engine tracked data now also stores the logical switches
>> and logical routers that got updated due to the changed load balancers.
>>
>> Eg 1. For this command 'ovn-nbctl ls-lb-add sw0 lb1 -- lr-lb-add lr0
>> lb1', northd engine tracking data will store 'sw0' and 'lr0'.
>>
>> Eg 2. If load balancer lb1 is already associated with 'sw0' and 'lr0'
>> then for this command 'ovn-nbctl set load_balancer <lb1>
>> vips:10.0.0.10=20.0.0.20', northd engine tracking data will store
>> 'sw0' and 'lr0'.
>>
>> An upcoming commit will make use of this tracked data.
>>
>> Signed-off-by: Numan Siddique <[email protected]>
>> ---
>> northd/northd.c | 34 +++++++++++++++++++++++++++++++++-
>> northd/northd.h | 12 ++++++++++++
>> 2 files changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index df22a9c658..9ce1b2cb5a 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -5146,6 +5146,8 @@ destroy_northd_data_tracked_changes(struct
> northd_data *nd)
>> struct northd_tracked_data *trk_changes = &nd->trk_northd_changes;
>> destroy_tracked_ovn_ports(&trk_changes->trk_ovn_ports);
>> destroy_tracked_lbs(&trk_changes->trk_lbs);
>> + hmapx_clear(&trk_changes->ls_with_changed_lbs.crupdated);
>> + hmapx_clear(&trk_changes->lr_with_changed_lbs.crupdated);
>> nd->change_tracked = false;
>> }
>>
>> @@ -5158,6 +5160,8 @@ init_northd_tracked_data(struct northd_data *nd)
>> hmapx_init(&trk_changes->trk_ovn_ports.deleted);
>> hmapx_init(&trk_changes->trk_lbs.crupdated);
>> hmapx_init(&trk_changes->trk_lbs.deleted);
>> + hmapx_init(&trk_changes->ls_with_changed_lbs.crupdated);
>> + hmapx_init(&trk_changes->lr_with_changed_lbs.crupdated);
>> }
>>
>> static void
>> @@ -5169,6 +5173,8 @@ destroy_northd_tracked_data(struct northd_data *nd)
>> hmapx_destroy(&trk_changes->trk_ovn_ports.deleted);
>> hmapx_destroy(&trk_changes->trk_lbs.crupdated);
>> hmapx_destroy(&trk_changes->trk_lbs.deleted);
>> + hmapx_destroy(&trk_changes->ls_with_changed_lbs.crupdated);
>> + hmapx_destroy(&trk_changes->lr_with_changed_lbs.crupdated);
>> }
>>
>>
>> @@ -5179,7 +5185,10 @@ northd_has_tracked_data(struct northd_tracked_data
> *trk_nd_changes)
>> || !hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.updated)
>> || !hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.deleted)
>> || !hmapx_is_empty(&trk_nd_changes->trk_lbs.crupdated)
>> - || !hmapx_is_empty(&trk_nd_changes->trk_lbs.deleted));
>> + || !hmapx_is_empty(&trk_nd_changes->trk_lbs.deleted)
>> + ||
> !hmapx_is_empty(&trk_nd_changes->ls_with_changed_lbs.crupdated)
>> + ||
> !hmapx_is_empty(&trk_nd_changes->lr_with_changed_lbs.crupdated)
>> + );
>> }
>>
>> bool
>> @@ -5188,6 +5197,8 @@ northd_has_only_ports_in_tracked_data(
>> {
>> return (hmapx_is_empty(&trk_nd_changes->trk_lbs.crupdated)
>> && hmapx_is_empty(&trk_nd_changes->trk_lbs.deleted)
>> + &&
> hmapx_is_empty(&trk_nd_changes->ls_with_changed_lbs.crupdated)
>> + &&
> hmapx_is_empty(&trk_nd_changes->lr_with_changed_lbs.crupdated)
>> && (!hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.created)
>> || !hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.updated)
>> || !hmapx_is_empty(&trk_nd_changes->trk_ovn_ports.deleted)));
>> @@ -5828,6 +5839,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
>> lb_dps->nb_ls_map) {
>> od = ls_datapaths->array[index];
>> init_lb_for_datapath(od);
>> +
>> + /* Add the ls datapath to the northd tracked data. */
>> + hmapx_add(&nd_changes->ls_with_changed_lbs.crupdated, od);
>> }
>>
>> hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
>> @@ -5909,6 +5923,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
>>
>> /* Re-evaluate 'od->has_lb_vip' */
>> init_lb_for_datapath(od);
>> +
>> + /* Add the ls datapath to the northd tracked data. */
>> + hmapx_add(&nd_changes->ls_with_changed_lbs.crupdated, od);
>> }
>>
>> LIST_FOR_EACH (codlb, list_node, &trk_lb_data->crupdated_lr_lbs) {
>> @@ -5954,6 +5971,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
>>
>> /* Re-evaluate 'od->has_lb_vip' */
>> init_lb_for_datapath(od);
>> +
>> + /* Add the lr datapath to the northd tracked data. */
>> + hmapx_add(&nd_changes->lr_with_changed_lbs.crupdated, od);
>> }
>>
>> HMAP_FOR_EACH (clb, hmap_node, &trk_lb_data->crupdated_lbs) {
>> @@ -5968,6 +5988,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
>> od = ls_datapaths->array[index];
>> /* Re-evaluate 'od->has_lb_vip' */
>> init_lb_for_datapath(od);
>> +
>> + /* Add the ls datapath to the northd tracked data. */
>> + hmapx_add(&nd_changes->ls_with_changed_lbs.crupdated, od);
>> }
>>
>> BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths),
>> @@ -5991,6 +6014,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
>> add_neigh_ips_to_lrouter(od, lb->neigh_mode,
>> &clb->inserted_vips_v4,
>> &clb->inserted_vips_v6);
>> +
>> + /* Add the lr datapath to the northd tracked data. */
>> + hmapx_add(&nd_changes->lr_with_changed_lbs.crupdated, od);
>> }
>> }
>>
>> @@ -6018,6 +6044,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
>>
>> /* Add the lb_ips of lb_dps to the od. */
>> build_lrouter_lb_ips(od->lb_ips, lb_dps->lb);
>> +
>> + /* Add the lr datapath to the northd tracked data. */
>> + hmapx_add(&nd_changes->lr_with_changed_lbs.crupdated,
> od);
>> }
>>
>> for (size_t i = 0; i < lbgrp_dps->n_ls; i++) {
>> @@ -6026,6 +6055,9 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
>>
>> /* Re-evaluate 'od->has_lb_vip' */
>> init_lb_for_datapath(od);
>> +
>> + /* Add the ls datapath to the northd tracked data. */
>> + hmapx_add(&nd_changes->ls_with_changed_lbs.crupdated,
> od);
>> }
>>
>> /* Add the lb to the northd tracked data. */
>> diff --git a/northd/northd.h b/northd/northd.h
>> index 2e4f125902..0478826f0a 100644
>> --- a/northd/northd.h
>> +++ b/northd/northd.h
>> @@ -107,12 +107,24 @@ struct tracked_lbs {
>> struct hmapx deleted;
>> };
>>
>> +/* Tracked logical switches whose load balancers have changed. */
>> +struct tracked_lswitches_with_changed_lbs {
>> + struct hmapx crupdated;
>> +};
>> +
>> +/* Tracked logical routers whose load balancers have changed. */
>> +struct tracked_lrouters_with_changed_lbs {
>> + struct hmapx crupdated;
>> +};
>> +
>> /* Track what's changed in the northd engine node.
>> * Now only tracks ovn_ports (of vif type) - created, updated
>> * and deleted. */
>> struct northd_tracked_data {
>> struct tracked_ovn_ports trk_ovn_ports;
>> struct tracked_lbs trk_lbs;
>> + struct tracked_lswitches_with_changed_lbs ls_with_changed_lbs;
>> + struct tracked_lrouters_with_changed_lbs lr_with_changed_lbs;
>> };
>
> The data structure looks a little strange. struct
> tracked_lswitches_with_changed_lbs and tracked_lrouters_with_changed_lbs
> are the same and both have only a single member. IMHO it would be
> unnecessary to define such structures. The vars ls_with_changed_lbs and
> lr_with_changed_lbs can be defined as struct hmapx, and the var names are
> sufficient to tell their different use. In addition, the name "crupdated"
> is also a little misleading. It seems to indicate the tracked objects are
> either created or updated but never deleted, but it is not the case. But
> anyway, I think it is better not defining the struct at all.
>
I agree. Moreoever, I think I'd squash this patch together with the one
that actually uses the tracked data.
Regards,
Dumitru
> Thanks,
> Han
>
>>
>> struct northd_data {
>> --
>> 2.41.0
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
