On Mon, Oct 25, 2021 at 8:42 PM Han Zhou <[email protected]> wrote: > > Duplicated datapaths shouldn't be created in the first place, but in > case it is created because of either bug or dirty data, it should be > properly deleted instead of causing permanent failure in northd. > > Currently, when there is a duplicated datapath record and a ip_multicast > record referencing it created, northd tries to delete the duplicated > datapath but the transaction fails due to schema constraint: > > transaction error: {"details":"Deletion of 1 weak reference(s) to deleted (or > never-existing) rows from column \"datapath\" in \"IP_Multicast\" row > 72bac803-e484-4358-9e48-11911c8aa16f caused this column to become empty, but > constraints on this column disallow an empty column.","error":"constraint > violation"} > > Northd would be blocked forever until manual deletion of the > ip_multicast record. > > The problem is that in the same transaction only datapath is deleted but > not the ip_multicast record that references the datapath. In > build_ip_mcast() there is logic to delete ip_multicast records with > non-exist datapath, but the logic of ovn_datapath_from_sbrec() can find > the datapath and regard it as valid whenever the > external-ids:logical-switch/router matches. This patch fixes that by > comparing the sbrec of the datapath afterwards, and regards it as valid > only if the sbrec matches, too. This way, both ip_multicast and datapath > records are deleted in the same transaction and won't cause any trouble. > > Reported-by: Vladislav Odintsov <[email protected]> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2021-October/388269.html > Signed-off-by: Han Zhou <[email protected]>
Acked-by: Numan Siddique <[email protected]> Numan > --- > northd/northd.c | 7 ++++++- > tests/ovn-northd.at | 20 ++++++++++++++++++++ > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/northd/northd.c b/northd/northd.c > index f71121486..da4f9cd14 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -1045,7 +1045,12 @@ ovn_datapath_from_sbrec(struct hmap *datapaths, > !smap_get_uuid(&sb->external_ids, "logical-router", &key)) { > return NULL; > } > - return ovn_datapath_find(datapaths, &key); > + struct ovn_datapath *od = ovn_datapath_find(datapaths, &key); > + if (od && (od->sb == sb)) { > + return od; > + } > + > + return NULL; > } > > static bool > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 544820764..e2b9924b6 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -5446,3 +5446,23 @@ wait_row_count Port_binding 1 logical-port=S1-vm2 > requested_chassis=$ch2_uuid > > AT_CLEANUP > ]) > + > +# Duplicated datapaths shouldn't be created, but in case it is created > because > +# of bug or dirty data, it should be properly deleted instead of causing > +# permanent failure in northd. > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([handling duplicated datapaths]) > +ovn_start > + > +check ovn-nbctl --wait=sb ls-add ls1 > +ls1_uuid=$(fetch_column nb:Logical_Switch _uuid) > + > +# create a duplicated sb datapath (and an IP_Mulicast record that references > +# it) on purpose. > +AT_CHECK([ovn-sbctl --id=@dp create Datapath_Binding > external_ids:logical-switch=$ls1_uuid external_ids:name=ls1 tunnel_key=123 -- > create IP_Multicast datapath=@dp], [0], [ignore]) > + > +# northd should delete one of the datapaths in the end > +wait_row_count Datapath_Binding 1 > + > +AT_CLEANUP > +]) > -- > 2.30.2 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
