| Message ID | 20230718095331.2443200-4-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Unexpected warning "Trying to release unknown | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Tue, Jul 18, 2023 at 11:54 AM Xavier Simonart <xsimonar@redhat.com> 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 <xsimonar@redhat.com> > --- > 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 > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 7/19/23 08:29, Ales Musil wrote: > On Tue, Jul 18, 2023 at 11:54 AM Xavier Simonart <xsimonar@redhat.com> > 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 <xsimonar@redhat.com> >> --- >> 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> > Thanks, applied to main and backported to all branches down to 22.09. Regards, Dumitru
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 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 +])
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 <xsimonar@redhat.com> --- 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(-)