Hi Xavier,
I understand how the patch works; it ensures that we don't try to create
the IC port binding if the binding will just end up getting deleted
during route_run().
However, the fact this is needed is unintuitive to me. It seems odd that
there is a memory leak when deleting the port binding. Does this only
happen because the transaction in which the port binding is inserted has
not been committed? Could this be considered a bug in the IDL? Is this a
problem with any key-value mapping in the database, or is it
specifically only for external-ids?
My main concerns here are
1) There are probably other places in our code where this sort of
sequence could (or does) happen. It may not be quite as easy to stop the
initial insertion of the object as it is in this case.
2) Consider what happens when the IC code is updated and we add a new
reason to delete the newly-inserted port binding either in route_run()
or some other place. Now create_isb_pb() needs to be updated to account
for this as well. This seems like something that is easy to miss by both
the coder and reviewer.
If this could be corrected at the IDL/DB level, that would make much
more sense to me. What do you think?
On 8/27/24 08:55, Xavier Simonart wrote:
Direct leak of 64 byte(s) in 1 object(s) allocated from:
0 0x56f127 in calloc (/root/master-ovn/ic/ovn-ic+0x56f127)
1 0x731de2 in xcalloc__ /root/master-ovn/ovs/lib/util.c:125:31
2 0x731e10 in xzalloc__ /root/master-ovn/ovs/lib/util.c:135:12
3 0x731ed5 in xzalloc /root/master-ovn/ovs/lib/util.c:169:12
4 0x706139 in ovsdb_idl_txn_add_map_op
/root/master-ovn/ovs/lib/ovsdb-idl.c:4178:29
5 0x705ff8 in ovsdb_idl_txn_write_partial_map
/root/master-ovn/ovs/lib/ovsdb-idl.c:4335:5
6 0x5b2f67 in update_isb_pb_external_ids /root/master-ovn/ic/ovn-ic.c:588:5
7 0x5ab08e in create_isb_pb /root/master-ovn/ic/ovn-ic.c:733:5
8 0x5ab08e in port_binding_run /root/master-ovn/ic/ovn-ic.c:821:21
9 0x5ab08e in ovn_db_run /root/master-ovn/ic/ovn-ic.c:1901:5
10 0x5a8143 in main /root/master-ovn/ic/ovn-ic.c:2337:21
11 0x7f58bd0f9b74 in __libc_start_main (/lib64/libc.so.6+0x27b74)
This happens when, in a single transaction, we run something like
- isb_pb = icsbrec_port_binding_insert(ctx->ovnisb_txn);
- icsbrec_port_binding_update_external_ids_setkey(isb_pb, "router-id",uuid_s);
- icsbrec_port_binding_delete(isb_pb)
Signed-off-by: Xavier Simonart <[email protected]>
---
ic/ovn-ic.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index 18d1df039..dfefa538e 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -121,6 +121,9 @@ Options:\n\
stream_usage("database", true, true, false);
}
+static const char *
+get_lrp_name_by_ts_port_name(struct ic_context *ctx, const char *ts_port_name);
+
static const struct icsbrec_availability_zone *
az_run(struct ic_context *ctx)
{
@@ -710,13 +713,19 @@ create_isb_pb(struct ic_context *ctx,
const char *ts_name,
uint32_t pb_tnl_key)
{
+ if (!get_lrp_name_by_ts_port_name(ctx, sb_pb->logical_port)) {
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+ VLOG_WARN_RL(&rl, "ignoring port %s on ts %s because "
+ "logical router port is not found in NB.",
+ sb_pb->logical_port, ts_name);
+ return;
+ }
const struct icsbrec_port_binding *isb_pb =
icsbrec_port_binding_insert(ctx->ovnisb_txn);
icsbrec_port_binding_set_availability_zone(isb_pb, az);
icsbrec_port_binding_set_transit_switch(isb_pb, ts_name);
icsbrec_port_binding_set_logical_port(isb_pb, sb_pb->logical_port);
icsbrec_port_binding_set_tunnel_key(isb_pb, pb_tnl_key);
-
const char *address = get_lrp_address_for_sb_pb(ctx, sb_pb);
if (address) {
icsbrec_port_binding_set_address(isb_pb, address);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
