On 3/24/23 14:11, Ales Musil wrote:
> On Fri, Mar 24, 2023 at 1:41 PM Dumitru Ceara <[email protected]> wrote:
>
>> This was working fine for IPv4 but partially by accident because IPv4
>> addresses don't contain ':'. Fix it for the general case by trying to
>> parse explicit backends if parsing templates fails.
>>
>> Also add unit and system tests for all supported cases.
>>
>> Signed-off-by: Dumitru Ceara <[email protected]>
>>
>
> Hi Dumitru,
> just one comment below.
>
Hi Ales,
Thanks for your review!
>
>> ---
>> lib/lb.c | 50 ++++++++++++++++++++++++++++++------
>> lib/lb.h | 6 +++--
>> tests/ovn-nbctl.at | 26 +++++++++++++++++++
>> tests/system-ovn.at | 60 +++++++++++++++++++++++++++++++++++++------
>> utilities/ovn-nbctl.c | 2 +-
>> 5 files changed, 125 insertions(+), 19 deletions(-)
>>
>> diff --git a/lib/lb.c b/lib/lb.c
>> index 66c8152750..1543f6279b 100644
>> --- a/lib/lb.c
>> +++ b/lib/lb.c
>> @@ -38,6 +38,7 @@ static const char *lb_neighbor_responder_mode_names[] = {
>> static struct nbrec_load_balancer_health_check *
>> ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
>> const char *vip_port_str, bool template);
>> +static void ovn_lb_backends_clear(struct ovn_lb_vip *vip);
>>
>> struct ovn_lb_ip_set *
>> ovn_lb_ip_set_create(void)
>> @@ -238,6 +239,8 @@ ovn_lb_backends_init_template(struct ovn_lb_vip
>> *lb_vip, const char *value_)
>> ds_put_format(&errors, "%s: should be a template of the form:
>> "
>>
>> "'^backendip_variable1[:^port_variable1|:port]', ",
>> atom);
>> + free(backend_port);
>> + free(backend_ip);
>> }
>> free(atom);
>> }
>> @@ -286,7 +289,25 @@ ovn_lb_vip_init_template(struct ovn_lb_vip *lb_vip,
>> const char *lb_key_,
>> }
>>
>> lb_vip->address_family = address_family;
>> - return ovn_lb_backends_init_template(lb_vip, lb_value);
>> + lb_vip->template_backends = true;
>> + char *error = ovn_lb_backends_init_template(lb_vip, lb_value);
>> + char *result = error;
>> +
>> + if (error) {
>> + char *error2;
>> +
>> + ovn_lb_backends_clear(lb_vip);
>> + error2 = ovn_lb_backends_init_explicit(lb_vip, lb_value);
>> + lb_vip->template_backends = false;
>> +
>> + if (error2) {
>> + free(error2);
>> + } else {
>> + free(error);
>> + result = NULL;
>> + }
>> + }
>> + return result;
>>
>
> This error handling part is confusing. AFAICT we could first try the
> 'ovn_lb_backends_init_explicit'
> if that fails, proceed to 'ovn_lb_backends_init_template' and share the
> error message from the second
> operation without the juggling of error2. Other than that it looks good.
>
You're right, it's not very clear. Unfortunately we can't really change
the order easily because ovn_lb_backends_init_explicit() ->
ip_address_and_port_from_lb_key() -> inet_parse_active() will log errors
on failure. So for every template load balancer that uses template
backends we'd get a confusing error log.
I changed a bit the error handling in v2 as an alternative, please let
me know what you think about it:
https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
