On 7/19/23 08:29, Ales Musil wrote:
> On Tue, Jul 18, 2023 at 11:54 AM Xavier Simonart <[email protected]>
> wrote:
>
>> When a port is added and deleted in the same loop by ovn-controller
>> this was causing the following warning
>> if_status|WARN|Trying to release unknown interface
>> We now avoid the warning in that ascenario.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2222252
>> Signed-off-by: Xavier Simonart <[email protected]>
>> ---
>> controller/binding.c | 12 ++++++++++--
>> controller/if-status.c | 12 ++++++++++++
>> controller/if-status.h | 2 ++
>> tests/ofproto-macros.at | 4 ++--
>> tests/ovn.at | 27 +++++++++++++++++++++++++++
>> 5 files changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/controller/binding.c b/controller/binding.c
>> index f619aba2e..bc7adb048 100644
>> --- a/controller/binding.c
>> +++ b/controller/binding.c
>> @@ -2682,7 +2682,13 @@ handle_deleted_lport(const struct
>> sbrec_port_binding *pb,
>> if (ld) {
>> remove_pb_from_local_datapath(pb,
>> b_ctx_out, ld);
>> - if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port);
>> + /* Only try to release the port if it was ever claimed.
>> + * If a port was added and deleted within the same ovn-controller
>> loop,
>> + * it is seen as never claimed.
>> + */
>> + if (if_status_is_port_claimed(b_ctx_out->if_mgr,
>> pb->logical_port)) {
>> + if_status_mgr_release_iface(b_ctx_out->if_mgr,
>> pb->logical_port);
>> + }
>> return;
>> }
>>
>> @@ -2706,7 +2712,9 @@ handle_deleted_lport(const struct sbrec_port_binding
>> *pb,
>> remove_pb_from_local_datapath(pb, b_ctx_out,
>> ld);
>> }
>> - if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port);
>> + if (if_status_is_port_claimed(b_ctx_out->if_mgr,
>> pb->logical_port)) {
>> + if_status_mgr_release_iface(b_ctx_out->if_mgr,
>> pb->logical_port);
>> + }
>> }
>> }
>>
>> diff --git a/controller/if-status.c b/controller/if-status.c
>> index b45208746..faf4e1f67 100644
>> --- a/controller/if-status.c
>> +++ b/controller/if-status.c
>> @@ -820,3 +820,15 @@ if_status_mgr_get_memory_usage(struct if_status_mgr
>> *mgr,
>> simap_increase(usage, "if_status_mgr_ifaces_state_usage-KB",
>> ROUND_UP(ifaces_state_usage, 1024) / 1024);
>> }
>> +
>> +bool
>> +if_status_is_port_claimed(const struct if_status_mgr *mgr,
>> + const char *iface_id)
>> +{
>> + struct ovs_iface *iface = shash_find_data(&mgr->ifaces, iface_id);
>> + if (!iface || (iface->state > OIF_INSTALLED)) {
>> + return false;
>> + } else {
>> + return true;
>> + }
>> +}
>> diff --git a/controller/if-status.h b/controller/if-status.h
>> index 15624bcfa..9714f6d8d 100644
>> --- a/controller/if-status.h
>> +++ b/controller/if-status.h
>> @@ -62,5 +62,7 @@ uint16_t if_status_mgr_iface_get_mtu(const struct
>> if_status_mgr *mgr,
>> const char *iface_id);
>> bool if_status_mgr_iface_update(const struct if_status_mgr *mgr,
>> const struct ovsrec_interface *iface_rec);
>> +bool if_status_is_port_claimed(const struct if_status_mgr *mgr,
>> + const char *iface_id);
>>
>> # endif /* controller/if-status.h */
>> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
>> index b0101330f..07ef1d092 100644
>> --- a/tests/ofproto-macros.at
>> +++ b/tests/ofproto-macros.at
>> @@ -250,11 +250,11 @@ m4_define([OVS_VSWITCHD_START],
>>
>> # check_logs scans through all *.log files (except '*.log' and
>> '*testsuite.log')
>> # and reports all WARN, ERR, EMER log entries. User can add custom sed
>> filters
>> -# in $1.
>> +# in $1 and select folder with $2.
>> m4_divert_push([PREPARE_TESTS])
>> check_logs () {
>> local logs
>> - for log in *.log; do
>> + for log in ./$2/*.log; do
Neat!
>> case ${log} in # (
>> '*.log'|*testsuite.log) ;; # (
>> *) logs="${logs} ${log}" ;;
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index f2148faac..f57e0f263 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -36568,3 +36568,30 @@ OVS_WAIT_UNTIL([test $(as hv-1 ovs-vsctl list qos
>> | grep -c linux-htb) -eq 1])
>>
>> AT_CLEANUP
>> ])
>> +
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([Add and delete port])
>> +ovn_start
>> +
>> +net_add n1
>> +
>> +sim_add hv1
>> +as hv1
>> +check ovs-vsctl add-br br-phys
>> +ovn_attach n1 br-phys 192.168.0.11
>> +
>> +check ovn-nbctl ls-add ls0
>> +check ovn-nbctl --wait=hv lsp-add ls0 lsp1
>> +as hv1 check ovs-vsctl -- add-port br-int lsp1 -- \
>> + set Interface lsp1 external-ids:iface-id=lsp1
>> +
>> +wait_for_ports_up
>> +sleep_controller hv1
>> +check ovn-nbctl --wait=sb lsp-add ls0 lsp0
>> +check ovn-nbctl --wait=sb lsp-del lsp0
>> +wake_up_controller hv1
>> +AT_CHECK([check_logs [""] hv1])
>> +OVN_CLEANUP([hv1])
>> +
>> +AT_CLEANUP
>> +])
>> --
>> 2.31.1
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> Looks good to me, thanks.
>
> Acked-by: Ales Musil <[email protected]>
>
Thanks, applied to main and backported to all branches down to 22.09.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
