Re: [ovs-dev] [PATCH ovn v2 1/7] Add new table Load_Balancer in Southbound database.
On Wed, Oct 28, 2020 at 3:30 AM Dumitru Ceara wrote:
>
> On 10/27/20 6:16 PM, num...@ovn.org wrote:
> > From: Numan Siddique
> >
> > This patch adds a new table 'Load_Balancer' in SB DB and syncs the
> > Load_Balancer table rows
> > from NB DB to SB DB. An upcoming patch will make use of this table for
> > handling the
> > load balancer hairpin traffic.
> >
> > Signed-off-by: Numan Siddique
> > ---
> > northd/ovn-northd.c | 141 ++
> > ovn-sb.ovsschema | 27 +++-
> > ovn-sb.xml| 45 ++
> > tests/ovn-northd.at | 81
> > utilities/ovn-sbctl.c | 3 +
> > 5 files changed, 295 insertions(+), 2 deletions(-)
> >
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index c06139d75b..d11888d203 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -11860,6 +11860,136 @@ sync_dns_entries(struct northd_context *ctx,
> > struct hmap *datapaths)
> > }
> > hmap_destroy(&dns_map);
> > }
> > +
> > +/*
> > + * struct 'sync_lb_info' is used to sync the load balancer records between
> > + * OVN Northbound db and Southbound db.
> > + */
> > +struct sync_lb_info {
> > +struct hmap_node hmap_node;
> > +const struct nbrec_load_balancer *nlb; /* LB record in the NB db. */
> > +const struct sbrec_load_balancer *slb; /* LB record in the SB db. */
> > +
> > +/* Datapaths to which the LB entry is associated with it. */
> > +const struct sbrec_datapath_binding **sbs;
> > +size_t n_sbs;
> > +};
> > +
>
> Hi Numan,
Hi Dumitru,
Thanks for the review. Please see below for the comments.
>
> Why do we need the sync_lb_info intermediate data structure? There's a 1:1
> relation between NB Load_Balancer and SB Load_Balancer records (for logical
> switches). Why don't we just store the SB record in "struct ovn_lb"? I think
> this might make the code a bit simpler.
After your suggestion, I tried this. But unfortunately, the code
doesn't get any simpler.
Since the SB load balancer schema has one extra column - datapaths, it
is not straightforward
1:1 mapping. This patch also adds a new column - load_balancer to
datapath_binding table.
Because of this we need this structure sync_lb_info.
>
> > +static inline struct sync_lb_info *
> > +get_sync_lb_info_from_hmap(struct hmap *sync_lb_map, struct uuid *uuid)
>
> Static functions in .c files should not be inline:
>
> https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst#functions
Ack.
>
> Also, to be more in sync with the naming scheme in ovn-northd (e.g.,
> ovn_datapath_find(), ovn_lb_find()) this should probably be:
>
> s/get_sync_lb_info_from_hmap/sync_lb_info_find
Ack. I'll change the function name.
Thanks
Numan
>
> Thanks,
> Dumitru
>
> > +{
> > +struct sync_lb_info *lb_info;
> > +size_t hash = uuid_hash(uuid);
> > +HMAP_FOR_EACH_WITH_HASH (lb_info, hmap_node, hash, sync_lb_map) {
> > +if (uuid_equals(&lb_info->nlb->header_.uuid, uuid)) {
> > +return lb_info;
> > +}
> > +}
> > +
> > +return NULL;
> > +}
> > +
> > +static void
> > +sync_lb_entries(struct northd_context *ctx, struct hmap *datapaths)
> > +{
> > +struct hmap lb_map = HMAP_INITIALIZER(&lb_map);
> > +struct ovn_datapath *od;
> > +
> > +HMAP_FOR_EACH (od, key_node, datapaths) {
> > +if (!od->nbs || !od->nbs->n_load_balancer) {
> > +continue;
> > +}
> > +
> > +for (size_t i = 0; i < od->nbs->n_load_balancer; i++) {
> > +struct sync_lb_info *lb_info =
> > +get_sync_lb_info_from_hmap(
> > +&lb_map, &od->nbs->load_balancer[i]->header_.uuid);
> > +
> > +if (!lb_info) {
> > +size_t hash = uuid_hash(
> > +&od->nbs->load_balancer[i]->header_.uuid);
> > +lb_info = xzalloc(sizeof *lb_info);;
> > +lb_info->nlb = od->nbs->load_balancer[i];
> > +hmap_insert(&lb_map, &lb_info->hmap_node, hash);
> > +}
> > +
> > +lb_info->n_sbs++;
> > +lb_info->sbs = xrealloc(lb_info->sbs,
> > +lb_info->n_sbs * sizeof *lb_info->sbs);
> > +lb_info->sbs[lb_info->n_sbs - 1] = od->sb;
> > +}
> > +}
> > +
> > +const struct sbrec_load_balancer *sbrec_lb, *next;
> > +SBREC_LOAD_BALANCER_FOR_EACH_SAFE (sbrec_lb, next, ctx->ovnsb_idl) {
> > +const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids,
> > "lb_id");
> > +struct uuid lb_uuid;
> > +if (!nb_lb_uuid || !uuid_from_string(&lb_uuid, nb_lb_uuid)) {
> > +sbrec_load_balancer_delete(sbrec_lb);
> > +continue;
> > +}
> > +
> > +struct sync_lb_info *lb_info =
> > +get_sync_lb_info_from_hmap(&lb_map, &lb_uuid);
> > +if (lb_info) {
> > +lb_info->slb = sbrec_lb;
>
Re: [ovs-dev] [PATCH ovn v2 1/7] Add new table Load_Balancer in Southbound database.
On 10/27/20 6:16 PM, num...@ovn.org wrote:
> From: Numan Siddique
>
> This patch adds a new table 'Load_Balancer' in SB DB and syncs the
> Load_Balancer table rows
> from NB DB to SB DB. An upcoming patch will make use of this table for
> handling the
> load balancer hairpin traffic.
>
> Signed-off-by: Numan Siddique
> ---
> northd/ovn-northd.c | 141 ++
> ovn-sb.ovsschema | 27 +++-
> ovn-sb.xml| 45 ++
> tests/ovn-northd.at | 81
> utilities/ovn-sbctl.c | 3 +
> 5 files changed, 295 insertions(+), 2 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index c06139d75b..d11888d203 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -11860,6 +11860,136 @@ sync_dns_entries(struct northd_context *ctx, struct
> hmap *datapaths)
> }
> hmap_destroy(&dns_map);
> }
> +
> +/*
> + * struct 'sync_lb_info' is used to sync the load balancer records between
> + * OVN Northbound db and Southbound db.
> + */
> +struct sync_lb_info {
> +struct hmap_node hmap_node;
> +const struct nbrec_load_balancer *nlb; /* LB record in the NB db. */
> +const struct sbrec_load_balancer *slb; /* LB record in the SB db. */
> +
> +/* Datapaths to which the LB entry is associated with it. */
> +const struct sbrec_datapath_binding **sbs;
> +size_t n_sbs;
> +};
> +
Hi Numan,
Why do we need the sync_lb_info intermediate data structure? There's a 1:1
relation between NB Load_Balancer and SB Load_Balancer records (for logical
switches). Why don't we just store the SB record in "struct ovn_lb"? I think
this might make the code a bit simpler.
> +static inline struct sync_lb_info *
> +get_sync_lb_info_from_hmap(struct hmap *sync_lb_map, struct uuid *uuid)
Static functions in .c files should not be inline:
https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst#functions
Also, to be more in sync with the naming scheme in ovn-northd (e.g.,
ovn_datapath_find(), ovn_lb_find()) this should probably be:
s/get_sync_lb_info_from_hmap/sync_lb_info_find
Thanks,
Dumitru
> +{
> +struct sync_lb_info *lb_info;
> +size_t hash = uuid_hash(uuid);
> +HMAP_FOR_EACH_WITH_HASH (lb_info, hmap_node, hash, sync_lb_map) {
> +if (uuid_equals(&lb_info->nlb->header_.uuid, uuid)) {
> +return lb_info;
> +}
> +}
> +
> +return NULL;
> +}
> +
> +static void
> +sync_lb_entries(struct northd_context *ctx, struct hmap *datapaths)
> +{
> +struct hmap lb_map = HMAP_INITIALIZER(&lb_map);
> +struct ovn_datapath *od;
> +
> +HMAP_FOR_EACH (od, key_node, datapaths) {
> +if (!od->nbs || !od->nbs->n_load_balancer) {
> +continue;
> +}
> +
> +for (size_t i = 0; i < od->nbs->n_load_balancer; i++) {
> +struct sync_lb_info *lb_info =
> +get_sync_lb_info_from_hmap(
> +&lb_map, &od->nbs->load_balancer[i]->header_.uuid);
> +
> +if (!lb_info) {
> +size_t hash = uuid_hash(
> +&od->nbs->load_balancer[i]->header_.uuid);
> +lb_info = xzalloc(sizeof *lb_info);;
> +lb_info->nlb = od->nbs->load_balancer[i];
> +hmap_insert(&lb_map, &lb_info->hmap_node, hash);
> +}
> +
> +lb_info->n_sbs++;
> +lb_info->sbs = xrealloc(lb_info->sbs,
> +lb_info->n_sbs * sizeof *lb_info->sbs);
> +lb_info->sbs[lb_info->n_sbs - 1] = od->sb;
> +}
> +}
> +
> +const struct sbrec_load_balancer *sbrec_lb, *next;
> +SBREC_LOAD_BALANCER_FOR_EACH_SAFE (sbrec_lb, next, ctx->ovnsb_idl) {
> +const char *nb_lb_uuid = smap_get(&sbrec_lb->external_ids, "lb_id");
> +struct uuid lb_uuid;
> +if (!nb_lb_uuid || !uuid_from_string(&lb_uuid, nb_lb_uuid)) {
> +sbrec_load_balancer_delete(sbrec_lb);
> +continue;
> +}
> +
> +struct sync_lb_info *lb_info =
> +get_sync_lb_info_from_hmap(&lb_map, &lb_uuid);
> +if (lb_info) {
> +lb_info->slb = sbrec_lb;
> +} else {
> +sbrec_load_balancer_delete(sbrec_lb);
> +}
> +}
> +
> +struct sync_lb_info *lb_info;
> +HMAP_FOR_EACH (lb_info, hmap_node, &lb_map) {
> +if (!lb_info->slb) {
> +sbrec_lb = sbrec_load_balancer_insert(ctx->ovnsb_txn);
> +lb_info->slb = sbrec_lb;
> +char *lb_id = xasprintf(
> +UUID_FMT, UUID_ARGS(&lb_info->nlb->header_.uuid));
> +const struct smap external_ids =
> +SMAP_CONST1(&external_ids, "lb_id", lb_id);
> +sbrec_load_balancer_set_external_ids(sbrec_lb, &external_ids);
> +free(lb_id);
> +}
> +
> +/* Set the datapaths and other columns. If nothing has changed, then
Re: [ovs-dev] [PATCH ovn v2 1/7] Add new table Load_Balancer in Southbound database.
Bleep bloop. Greetings Numan Siddique, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. git-am: error: Failed to merge in the changes. hint: Use 'git am --show-current-patch' to see the failed patch Patch failed at 0001 Add new table Load_Balancer in Southbound database. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
