It doesn't really make sense to use the router port MAC as service
monitor source otherwise. Worse, it may even break existing use cases
when the load balancer is only applied to logical switches and not to
the routers connected to them.
In order for the change to work properly we also had to fallback to
recompute in all cases when load balancers with health check
configuration are added/updated/deleted (through load balancer groups
too). This was currently not happening but didn't really cause issues
before ceea1d7cccad ("northd: Allow LB health checks from router IPs.").
This also uncovered a bug in pinctrl, where we don't resync the
in-memory version of the service monitor if the SB contents change
(e.g., the source mac address). The patch fixes that part too as it
wasn't especially visible until now when we change the source mac of the
monitor if it gets applied to a router and uses the router IP.
Fixes: a0a5dd8ce56f ("controller: Store src_mac, src_ip in svc_monitor struct.")
Fixes: ceea1d7cccad ("northd: Allow LB health checks from router IPs.")
Reported-at: https://redhat.atlassian.net/browse/FDP-3453
Signed-off-by: Dumitru Ceara <[email protected]>
---
V2:
- Addressed Mark's comment:
- renamed tests to show the difference between system/non-system tests
- added mac check to the system test
---
controller/pinctrl.c | 11 +++---
northd/en-lb-data.c | 20 +++++++----
northd/lb.c | 13 ++++++-
northd/lb.h | 8 +++--
northd/northd.c | 8 +++--
tests/ovn-northd.at | 78 +++++++++++++++++++++++++++++++++++++++++
tests/system-ovn.at | 82 ++++++++++++++++++++++++++++++++++++++++++++
7 files changed, 202 insertions(+), 18 deletions(-)
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 35c42942f3..18b7b0df2e 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -7197,16 +7197,13 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
if (!svc_mon) {
svc_mon = xmalloc(sizeof *svc_mon);
svc_mon->dp_key = dp_key;
- svc_mon->input_port_key = input_port_key;
svc_mon->port_key = port_key;
svc_mon->proto_port = sb_svc_mon->port;
svc_mon->ip = ip_addr;
- svc_mon->src_ip = ip_addr_src;
svc_mon->is_ip6 = !is_ipv4;
svc_mon->state = SVC_MON_S_INIT;
svc_mon->status = SVC_MON_ST_UNKNOWN;
svc_mon->protocol = protocol;
- svc_mon->type = mon_type;
smap_init(&svc_mon->options);
svc_mon->interval =
@@ -7220,15 +7217,19 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
svc_mon->n_success = 0;
svc_mon->n_failures = 0;
- eth_addr_from_string(sb_svc_mon->src_mac, &svc_mon->src_mac);
-
hmap_insert(&svc_monitors_map, &svc_mon->hmap_node, hash);
ovs_list_push_back(&svc_monitors, &svc_mon->list_node);
changed = true;
}
svc_mon->sb_svc_mon = sb_svc_mon;
+
+ svc_mon->input_port_key = input_port_key;
+ svc_mon->src_ip = ip_addr_src;
+ svc_mon->type = mon_type;
+ eth_addr_from_string(sb_svc_mon->src_mac, &svc_mon->src_mac);
svc_mon->ea = ea;
+
if (!smap_equal(&svc_mon->options, &sb_svc_mon->options)) {
smap_destroy(&svc_mon->options);
smap_clone(&svc_mon->options, &sb_svc_mon->options);
diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
index a64c06bfda..04ef20bcc6 100644
--- a/northd/en-lb-data.c
+++ b/northd/en-lb-data.c
@@ -268,6 +268,7 @@ lb_data_load_balancer_group_handler(struct engine_node
*node, void *data)
hmapx_add(&clbg->assoc_lbs, lb_group->lbs[i]);
}
+ trk_lb_data->has_health_checks |= lb_group->has_health_checks;
trk_lb_data->has_routable_lb |= lb_group->has_routable_lb;
continue;
}
@@ -283,6 +284,7 @@ lb_data_load_balancer_group_handler(struct engine_node
*node, void *data)
if (nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
hmap_remove(&lb_data->lbgrps, &lb_group->hmap_node);
add_deleted_lbgrp_to_tracked_data(lb_group, trk_lb_data);
+ trk_lb_data->has_health_checks |= lb_group->has_health_checks;
trk_lb_data->has_routable_lb |= lb_group->has_routable_lb;
} else {
/* Determine the lbs which are added or deleted for this
@@ -299,6 +301,7 @@ lb_data_load_balancer_group_handler(struct engine_node
*node, void *data)
build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
}
+ trk_lb_data->has_health_checks |= lb_group->has_health_checks;
trk_lb_data->has_routable_lb |= lb_group->has_routable_lb;
struct crupdated_lbgrp *clbg =
add_crupdated_lbgrp_to_tracked_data(lb_group, trk_lb_data);
@@ -690,10 +693,12 @@ handle_od_lb_changes(struct nbrec_load_balancer
**nbrec_lbs,
/* Add this lb to the tracked data. */
uuidset_insert(&codlb->assoc_lbs, lb_uuid);
+ struct ovn_northd_lb *lb = ovn_northd_lb_find(&lb_data->lbs,
+ lb_uuid);
+ ovs_assert(lb);
+
+ trk_lb_data->has_health_checks |= lb->health_checks;
if (!trk_lb_data->has_routable_lb) {
- struct ovn_northd_lb *lb = ovn_northd_lb_find(&lb_data->lbs,
- lb_uuid);
- ovs_assert(lb);
trk_lb_data->has_routable_lb |= lb->routable;
trk_lb_data->has_distributed_lb |= lb->is_distributed;
}
@@ -730,10 +735,12 @@ handle_od_lbgrp_changes(struct nbrec_load_balancer_group
**nbrec_lbgrps,
/* Add this lb group to the tracked data. */
uuidset_insert(&codlb->assoc_lbgrps, lbgrp_uuid);
+ struct ovn_lb_group *lbgrp =
+ ovn_lb_group_find(&lb_data->lbgrps, lbgrp_uuid);
+ ovs_assert(lbgrp);
+
+ trk_lb_data->has_health_checks |= lbgrp->has_health_checks;
if (!trk_lb_data->has_routable_lb) {
- struct ovn_lb_group *lbgrp =
- ovn_lb_group_find(&lb_data->lbgrps, lbgrp_uuid);
- ovs_assert(lbgrp);
trk_lb_data->has_routable_lb |= lbgrp->has_routable_lb;
}
}
@@ -755,6 +762,7 @@ destroy_tracked_data(struct ed_type_lb_data *lb_data)
lb_data->tracked_lb_data.has_dissassoc_lbs_from_lbgrps = false;
lb_data->tracked_lb_data.has_dissassoc_lbs_from_od = false;
lb_data->tracked_lb_data.has_dissassoc_lbgrps_from_od = false;
+ lb_data->tracked_lb_data.has_health_checks = false;
lb_data->tracked_lb_data.has_routable_lb = false;
struct hmapx_node *node;
diff --git a/northd/lb.c b/northd/lb.c
index 83c1bf97e1..2f683cef1b 100644
--- a/northd/lb.c
+++ b/northd/lb.c
@@ -464,7 +464,8 @@ ovn_northd_lb_init(struct ovn_northd_lb *lb,
* considered. If no matching LRP is found, the 'svc_mon_lrp' is set to
* NULL. */
void
-ovn_northd_lb_backend_set_mon_port(const struct ovn_port *backend_op,
+ovn_northd_lb_backend_set_mon_port(const struct ovn_lb_datapaths *lb_dps,
+ const struct ovn_port *backend_op,
struct ovn_northd_lb_backend *backend_nb)
{
if (backend_op && !backend_nb->remote_backend) {
@@ -473,6 +474,14 @@ ovn_northd_lb_backend_set_mon_port(const struct ovn_port
*backend_op,
if (!svc_mon_op->peer) {
continue;
}
+
+ /* Skip routers this load balancer is not applied onto. */
+ const struct ovn_datapath *rtr_od = svc_mon_op->peer->od;
+ if (!dynamic_bitmap_is_set(&lb_dps->nb_lr_map,
+ rtr_od->sdp->index)) {
+ continue;
+ }
+
const char *lrp_ip = lrp_find_member_ip(
svc_mon_op->peer,
backend_nb->svc_mon_src_ip);
@@ -564,6 +573,7 @@ ovn_lb_group_init(struct ovn_lb_group *lb_group,
const struct uuid *lb_uuid =
&nbrec_lb_group->load_balancer[i]->header_.uuid;
lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
+ lb_group->has_health_checks |= lb_group->lbs[i]->health_checks;
lb_group->has_routable_lb |= lb_group->lbs[i]->routable;
}
}
@@ -586,6 +596,7 @@ ovn_lb_group_cleanup(struct ovn_lb_group *lb_group)
{
ovn_lb_ip_set_destroy(lb_group->lb_ips);
lb_group->lb_ips = NULL;
+ lb_group->has_health_checks = false;
lb_group->has_routable_lb = false;
free(lb_group->lbs);
}
diff --git a/northd/lb.h b/northd/lb.h
index c1f0c95da1..97d2c4c366 100644
--- a/northd/lb.h
+++ b/northd/lb.h
@@ -108,9 +108,6 @@ struct ovn_northd_lb_backend {
struct ovn_port *svc_mon_lrp;
};
-void ovn_northd_lb_backend_set_mon_port(const struct ovn_port *,
- struct ovn_northd_lb_backend *);
-
struct ovn_northd_lb *ovn_northd_lb_create(const struct nbrec_load_balancer *);
struct ovn_northd_lb *ovn_northd_lb_find(const struct hmap *,
const struct uuid *);
@@ -136,6 +133,7 @@ struct ovn_lb_group {
size_t n_lbs;
struct ovn_northd_lb **lbs;
struct ovn_lb_ip_set *lb_ips;
+ bool has_health_checks;
bool has_routable_lb;
};
@@ -203,6 +201,10 @@ void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *,
size_t n,
struct ovn_datapath **,
size_t n_ls_datapaths);
+void ovn_northd_lb_backend_set_mon_port(const struct ovn_lb_datapaths *,
+ const struct ovn_port *,
+ struct ovn_northd_lb_backend *);
+
struct ovn_lb_group_datapaths {
struct hmap_node hmap_node;
diff --git a/northd/northd.c b/northd/northd.c
index 734efea657..c607c4c08d 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3332,13 +3332,15 @@ ovn_nf_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
static void
ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
- const struct ovn_northd_lb *lb,
+ const struct ovn_lb_datapaths *lb_dps,
const struct svc_monitor_addresses *svc_global_addresses,
struct hmap *ls_ports,
struct sset *svc_monitor_lsps,
struct hmap *local_svc_monitors_map,
struct hmap *ic_learned_svc_monitors_map)
{
+ const struct ovn_northd_lb *lb = lb_dps->lb;
+
if (lb->template) {
return;
}
@@ -3366,7 +3368,7 @@ ovn_lb_svc_create(struct ovsdb_idl_txn *ovnsb_txn,
continue;
}
- ovn_northd_lb_backend_set_mon_port(op, backend_nb);
+ ovn_northd_lb_backend_set_mon_port(lb_dps, op, backend_nb);
/* If the service monitor is backed by a real port, use its MAC
* address instead of the default service check MAC. */
@@ -3787,7 +3789,7 @@ build_svc_monitors_data(
struct ovn_lb_datapaths *lb_dps;
HMAP_FOR_EACH (lb_dps, hmap_node, lb_dps_map) {
- ovn_lb_svc_create(ovnsb_txn, lb_dps->lb,
+ ovn_lb_svc_create(ovnsb_txn, lb_dps,
svc_global_addresses,
ls_ports,
svc_monitor_lsps,
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 60258a6f25..01ae91a963 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -20170,3 +20170,81 @@ AT_CHECK([ovn-sbctl get Port_Binding $lsp_pb_uuid
external_ids:name],
OVN_CLEANUP_NORTHD
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Load balancer health checks - service monitor source MAC - SB
syncing])
+ovn_start
+
+global_svc_mon_mac="11:11:11:11:11:11"
+router_port_mac="00:00:00:00:00:01"
+check ovn-nbctl set NB_Global . options:svc_monitor_mac="$global_svc_mon_mac"
+
+check ovn-nbctl \
+ -- ls-add ls \
+ -- lsp-add ls lsp1 \
+ -- lsp-add ls lsp2 \
+ -- lr-add lr \
+ -- lrp-add lr lr-ls $router_port_mac 192.168.1.254/24 \
+ -- lsp-add-router-port ls ls-lr lr-ls
+
+check ovn-nbctl \
+ -- lb-add lb0 192.168.5.1:1 192.168.1.1:1,192.168.1.2:1 udp \
+ -- lb-add lb1 192.168.5.1:2 192.168.1.1:2,192.168.1.2:2 udp
+lb0=$(fetch_column nb:Load_Balancer _uuid name=lb0)
+lb1=$(fetch_column nb:Load_Balancer _uuid name=lb1)
+
+check ovn-nbctl \
+ -- set load_balancer $lb0 ip_port_mappings:192.168.1.1=lsp1:192.168.1.254 \
+ -- set load_balancer $lb1 ip_port_mappings:192.168.1.1=lsp1:192.168.1.254
+
+check_uuid ovn-nbctl --id=@hc create Load_Balancer_Health_Check
vip="192.168.5.1\:1" \
+ -- add Load_Balancer $lb0 health_check @hc
+check_uuid ovn-nbctl --id=@hc create Load_Balancer_Health_Check
vip="192.168.5.1\:2" \
+ -- add Load_Balancer $lb1 health_check @hc
+
+dnl Add LB0 and LB1 to the switch (LB1 via load balancer group).
+check ovn-nbctl ls-lb-add ls lb0
+lbg=$(ovn-nbctl --wait=sb create load_balancer_group name=lbg)
+check ovn-nbctl add load_balancer_group lbg load_balancer $lb1
+check ovn-nbctl add logical_switch ls load_balancer_group $lbg
+
+check ovn-nbctl --wait=sb sync
+check_row_count sb:Service_Monitor 2
+
+AS_BOX([LB0 on switch, LB1 on switch])
+check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=1
+check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=2
+
+AS_BOX([LB0 on switch + router, LB1 on switch])
+check ovn-nbctl --wait=sb lr-lb-add lr lb0
+check_column "$router_port_mac" sb:Service_Monitor src_mac port=1
+check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=2
+
+AS_BOX([LB0 on switch + router, LB1 on switch + router])
+check ovn-nbctl --wait=sb add logical_router lr load_balancer_group $lbg
+check_column "$router_port_mac" sb:Service_Monitor src_mac port=1
+check_column "$router_port_mac" sb:Service_Monitor src_mac port=2
+
+AS_BOX([LB0 on switch + router, LB1 nowhere])
+check ovn-nbctl --wait=sb remove load_balancer_group lbg load_balancer $lb1
+check_column "$router_port_mac" sb:Service_Monitor src_mac port=1
+check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=2
+
+AS_BOX([LB0 on switch + router, LB1 on switch + router, readd])
+check ovn-nbctl --wait=sb add load_balancer_group lbg load_balancer $lb1
+check_column "$router_port_mac" sb:Service_Monitor src_mac port=1
+check_column "$router_port_mac" sb:Service_Monitor src_mac port=2
+
+AS_BOX([LB0 on switch + router, LB1 on switch, remove])
+check ovn-nbctl --wait=sb remove logical_router lr load_balancer_group $lbg
+check_column "$router_port_mac" sb:Service_Monitor src_mac port=1
+check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=2
+
+AS_BOX([LB0 on switch, LB1 on switch, remove])
+check ovn-nbctl --wait=sb lr-lb-del lr lb0
+check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=1
+check_column "$global_svc_mon_mac" sb:Service_Monitor src_mac port=2
+
+OVN_CLEANUP_NORTHD
+AT_CLEANUP
+])
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 7edb8a45b0..9e58f39c8b 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -21496,3 +21496,85 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port
patch-.*/d
/connection dropped.*/d"])
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([Load balancer health checks - service monitor source MAC matching])
+AT_SKIP_IF([test $HAVE_NC = no])
+AT_SKIP_IF([test $HAVE_TCPDUMP = no])
+
+ovn_start
+OVS_TRAFFIC_VSWITCHD_START()
+ADD_BR([br-int])
+
+check ovs-vsctl \
+ -- set Open_vSwitch . external-ids:system-id=hv1 \
+ -- set Open_vSwitch .
external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
+ -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
+ -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
+ -- set bridge br-int fail-mode=secure other-config:disable-in-band=true
+
+start_daemon ovn-controller
+
+global_svc_mon_mac="11:11:11:11:11:11"
+router_port_mac="00:00:00:00:00:01"
+lsp_mac="00:00:00:01:01:01"
+check ovn-nbctl set NB_Global . options:svc_monitor_mac="$global_svc_mon_mac"
+
+check ovn-nbctl \
+ -- ls-add ls \
+ -- lsp-add ls lsp \
+ -- lsp-set-addresses lsp "$lsp_mac 192.168.1.1" \
+ -- lr-add lr \
+ -- lrp-add lr lr-ls $router_port_mac 192.168.1.254/24 \
+ -- lsp-add-router-port ls ls-lr lr-ls
+
+check ovn-nbctl lb-add lb 192.168.5.1:42 192.168.1.1:42 udp
+lb=$(fetch_column nb:Load_Balancer _uuid name=lb)
+
+lbhc=$(ovn-nbctl \
+ --id=@hc create Load_Balancer_Health_Check vip="192.168.5.1\:42" \
+ -- add Load_Balancer $lb health_check @hc)
+check ovn-nbctl set Load_Balancer_Health_Check $lbhc \
+ options:interval=1 options:timeout=1 \
+ options:success_count=2 \
+ options:failure_count=2
+check ovn-nbctl --wait=sb set load_balancer $lb \
+ ip_port_mappings:192.168.1.1=lsp:192.168.1.254
+
+check ovn-nbctl set Logical_Router lr options:chassis="hv1"
+check ovn-nbctl ls-lb-add ls lb
+
+ADD_NAMESPACES(lsp)
+ADD_VETH(lsp, lsp, br-int, "192.168.1.1/24", "$lsp_mac",
+ "192.168.1.254")
+
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+dnl Check mac address of probe requests and replies, they should use the
+dnl global svc_monitor_mac.
+NETNS_START_TCPDUMP([lsp], [-nne -i lsp -Q in 'udp'], [lsp_in])
+NETNS_START_TCPDUMP([lsp], [-nne -i lsp -Q out 'udp || icmp'], [lsp_out])
+
+dnl Start a backend server, the monitor should change status to "online".
+NETNS_DAEMONIZE([lsp], [nc -l -u 42], [nc.pid])
+wait_row_count Service_Monitor 1 status=online
+
+dnl Kill the backend server, the monitor should change status to "offline".
+check kill -9 `cat nc.pid`
+wait_row_count Service_Monitor 1 status=offline
+
+OVS_WAIT_UNTIL([grep -q "$global_svc_mon_mac > $lsp_mac" lsp_in.tcpdump])
+AT_CHECK([grep -v "$global_svc_mon_mac > $lsp_mac" lsp_in.tcpdump], [1])
+
+OVS_WAIT_UNTIL([grep -q "$lsp_mac > $global_svc_mon_mac" lsp_out.tcpdump])
+AT_CHECK([grep -v "$lsp_mac > $global_svc_mon_mac" lsp_out.tcpdump], [1])
+
+OVN_CLEANUP_CONTROLLER([hv1])
+OVN_CLEANUP_NORTHD
+
+as
+OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
+/connection dropped.*/d"])
+AT_CLEANUP
+])
--
2.53.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
