| Message ID | 20220912221104.2679484-1-odivlad@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: don't include disabled LSP's IP to Load Balancing | 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 | fail | github build: failed |
On Mon, Sep 12, 2022 at 6:11 PM Vladislav Odintsov <odivlad@gmail.com> wrote: > > If one has a UDP load balancer with backend IP which is located under > disabled LSP, such backend would be threated as alive and marked as > 'online' on Service_Monitor table and added to load balancing as well. > Though such LSP can't receive any traffic, this load balancer will be > broken by mentioned behaviour. > > This patch resolves this issue for Load Balancers with enabled health > check: if LSP is disabled, it wont be added to Service_Monitor and to > Load Balancing flow. > > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> Thanks. Applied to main and backported upto branch-22.03. Numan > --- > northd/northd.c | 10 ++++++---- > tests/ovn-northd.at | 19 +++++++++++++++++++ > 2 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 1eb190dc1..76007bd4d 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3713,7 +3713,8 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, struct ovn_northd_lb *lb, > backend_nb->op = op; > backend_nb->svc_mon_src_ip = svc_mon_src_ip; > > - if (!lb_vip_nb->lb_health_check || !op || !svc_mon_src_ip) { > + if (!lb_vip_nb->lb_health_check || !op || !svc_mon_src_ip || > + !lsp_is_enabled(op->nbsp)) { > continue; > } > > @@ -3777,9 +3778,10 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, > struct ovn_lb_backend *backend = &lb_vip->backends[i]; > struct ovn_northd_lb_backend *backend_nb = > &lb_vip_nb->backends_nb[i]; > - if (backend_nb->health_check && backend_nb->sbrec_monitor && > - backend_nb->sbrec_monitor->status && > - strcmp(backend_nb->sbrec_monitor->status, "online")) { > + if (!backend_nb->health_check || > + (backend_nb->health_check && backend_nb->sbrec_monitor && > + backend_nb->sbrec_monitor->status && > + strcmp(backend_nb->sbrec_monitor->status, "online"))) { > continue; > } > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index da83bce7c..7c3c84007 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -1230,6 +1230,25 @@ OVS_WAIT_FOR_OUTPUT( > (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);) > ]) > > +# disabled LSPs should not be a backend of Load Balancer > +check ovn-nbctl lsp-set-enabled sw0-p1 disabled > + > +AT_CAPTURE_FILE([sbflows]) > +OVS_WAIT_FOR_OUTPUT( > + [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | sed 's/table=..//'], 0, [dnl > + (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb(backends=20.0.0.3:80);) > +]) > +wait_row_count Service_Monitor 1 > + > +check ovn-nbctl lsp-set-enabled sw0-p1 enabled > + > +AT_CAPTURE_FILE([sbflows]) > +OVS_WAIT_FOR_OUTPUT( > + [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | sed 's/table=..//'], 0, [dnl > + (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);) > +]) > +wait_row_count Service_Monitor 2 > + > AS_BOX([Delete the Load_Balancer_Health_Check]) > ovn-nbctl --wait=sb clear load_balancer . health_check > wait_row_count Service_Monitor 0 > -- > 2.36.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/northd.c b/northd/northd.c index 1eb190dc1..76007bd4d 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3713,7 +3713,8 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn, struct ovn_northd_lb *lb, backend_nb->op = op; backend_nb->svc_mon_src_ip = svc_mon_src_ip; - if (!lb_vip_nb->lb_health_check || !op || !svc_mon_src_ip) { + if (!lb_vip_nb->lb_health_check || !op || !svc_mon_src_ip || + !lsp_is_enabled(op->nbsp)) { continue; } @@ -3777,9 +3778,10 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, struct ovn_lb_backend *backend = &lb_vip->backends[i]; struct ovn_northd_lb_backend *backend_nb = &lb_vip_nb->backends_nb[i]; - if (backend_nb->health_check && backend_nb->sbrec_monitor && - backend_nb->sbrec_monitor->status && - strcmp(backend_nb->sbrec_monitor->status, "online")) { + if (!backend_nb->health_check || + (backend_nb->health_check && backend_nb->sbrec_monitor && + backend_nb->sbrec_monitor->status && + strcmp(backend_nb->sbrec_monitor->status, "online"))) { continue; } diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index da83bce7c..7c3c84007 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1230,6 +1230,25 @@ OVS_WAIT_FOR_OUTPUT( (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);) ]) +# disabled LSPs should not be a backend of Load Balancer +check ovn-nbctl lsp-set-enabled sw0-p1 disabled + +AT_CAPTURE_FILE([sbflows]) +OVS_WAIT_FOR_OUTPUT( + [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | sed 's/table=..//'], 0, [dnl + (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb(backends=20.0.0.3:80);) +]) +wait_row_count Service_Monitor 1 + +check ovn-nbctl lsp-set-enabled sw0-p1 enabled + +AT_CAPTURE_FILE([sbflows]) +OVS_WAIT_FOR_OUTPUT( + [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | sed 's/table=..//'], 0, [dnl + (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb(backends=10.0.0.3:80,20.0.0.3:80);) +]) +wait_row_count Service_Monitor 2 + AS_BOX([Delete the Load_Balancer_Health_Check]) ovn-nbctl --wait=sb clear load_balancer . health_check wait_row_count Service_Monitor 0
If one has a UDP load balancer with backend IP which is located under disabled LSP, such backend would be threated as alive and marked as 'online' on Service_Monitor table and added to load balancing as well. Though such LSP can't receive any traffic, this load balancer will be broken by mentioned behaviour. This patch resolves this issue for Load Balancers with enabled health check: if LSP is disabled, it wont be added to Service_Monitor and to Load Balancing flow. Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- northd/northd.c | 10 ++++++---- tests/ovn-northd.at | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-)