Hi Mark Thanks for the review and the comments provided.
On Tue, Aug 27, 2024 at 9:50 PM Mark Michelson <[email protected]> wrote: > 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? > It is the combination of inserting a port binding, adding a key/value and deleting the pb, all within the same commit. If inserting and deleting a row (w/o adding a key/value) within the same commit, there is no issue. If, within a single commit, adding a key value and deleting the row of an existing port_binding, then no issue either. > > 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? > I agree with you. I had an initial look at idl and did not find an obvious cause for the memleak. This is why I fixed it here. Since then, I further investigated idl and think I found the route cause. I'll soon send a patch in OVS for this memory leak. If the patch gets accepted, then this ovn patch would not be necessary anymore. I'll move the state to "Not Applicable". Thanks Xavier > 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
