On 4/30/20 7:05 PM, Han Zhou wrote:
>
>
> On Thu, Apr 30, 2020 at 4:07 AM Dumitru Ceara <[email protected]
> <mailto:[email protected]>> wrote:
>>
>> On 4/30/20 12:06 PM, Numan Siddique wrote:
>> > On Thu, Apr 30, 2020 at 1:41 PM Numan Siddique <[email protected]
> <mailto:[email protected]>> wrote:
>> >
>> >>
>> >>
>> >> On Thu, Apr 30, 2020 at 8:43 AM Han Zhou <[email protected]
> <mailto:[email protected]>> wrote:
>> >>
>> >>> On Wed, Apr 29, 2020 at 4:50 PM Han Zhou <[email protected]
> <mailto:[email protected]>> wrote:
>> >>>>
>> >>>>
>> >>>>
>> >>>> On Wed, Apr 29, 2020 at 2:45 PM Dumitru Ceara <[email protected]
> <mailto:[email protected]>>
>> >>> wrote:
>> >>>>>
>> >>>>> On 4/29/20 9:57 PM, Han Zhou wrote:
>> >>>>>>
>> >>>>>>
>> >>>>>> On Wed, Apr 29, 2020 at 12:17 PM Numan Siddique <[email protected]
> <mailto:[email protected]>
>> >>>>>> <mailto:[email protected] <mailto:[email protected]>>> wrote:
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> On Wed, Apr 29, 2020 at 9:57 PM Dumitru Ceara
> <[email protected] <mailto:[email protected]>
>> >>>>>> <mailto:[email protected] <mailto:[email protected]>>> wrote:
>> >>>>>>>>
>> >>>>>>>> In some cases, if the NB/SB databases ovn-northd connects to are
>> >>>>>>>> inconsistent, ovn-northd might generate transactions that fail
>> >>>>>>>> continuously due to failed integrity checks on the SB database
>> >>> server.
>> >>>>>>>>
>> >>>>>>>> The first patch of the series addresses inconsistencies due to
>> >>> stale
>> >>>>>>>> Datapath_Binding records in the SB database.
>> >>>>>>>>
>> >>>>>>>> The second patch of the series addresses inconsistencies due to
>> >>> stale
>> >>>>>>>> tunnel_key values in various SB database table records.
>> >>>>>>>>
>> >>>>>>>> Reported-by: Dan Williams <[email protected]
> <mailto:[email protected]> <mailto:
>> >>> [email protected] <mailto:[email protected]>>>
>> >>>>>>>> Reported-at: https://bugzilla.redhat.com/1828637
>> >>>>>>>> Signed-off-by: Dumitru Ceara <[email protected]
> <mailto:[email protected]>
>> >>>>>> <mailto:[email protected] <mailto:[email protected]>>>
>> >>>>>>>>
>> >>>>>>>> Dumitru Ceara (2):
>> >>>>>>>> ovn-northd: Clear SB records depending on stale datapaths.
>> >>>>>>>> ovn-northd: Fix tunnel_key allocation for SB records.
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> Hi Dumitru,
>> >>>>>>>
>> >>>>>>> I did some testing in my ovn-fake-multinode setup. These are my
>> >>>>>> observations.
>> >>>>>>>
>> >>>>>>> I created a logical switch sw0 with 4 logical ports. So the next
>> >>>>>> tunnel key should be 5.
>> >>>>>>> I stopped ovn-northd and created a couple of port_binding entries
>> >>>>>> manually using
>> >>>>>>> "ovn-sbctl create port_binding" with tunnel keys 5 and 6.
>> >>>>>>> I also created a logical port in sw0. Then I started ovn-northd.
>> >>>>>> ovn-northd deletes the port binding
>> >>>>>>> entries added by me and creates the port_binding entry for the
>> >>> logical
>> >>>>>> port with the tunnel_key=5
>> >>>>>>> in the same transaction.
>> >>>>>>>
>> >>>>>>> I think ovn-northd syncs the south db based on the contents of the
>> >>>>>> north db.
>> >>>>>>>
>> >>>>>>> There's no harm in having your patches. But I'm not really sure if
>> >>> it
>> >>>>>> resolves the issue we have observed.
>> >>>>>>>
>> >>>>>>> Just to brief everyone about the issue we are seeing, we see below
>> >>>>>> logs in ovn-northd.
>> >>>>>>>
>> >>>>>>> *******
>> >>>>>>> 2020-04-16T23:02:33Z|00127|ovsdb_idl|WARN|transaction error:
>> >>>>>> {"details":"Transaction causes multiple rows in \"Port_Binding\"
>> >>> table
>> >>>>>> to have identical values (23eb9016-45f9-4158-be35-77b2713b9a0f and
>> >>> 7)
>> >>>>>> for index on columns \"datapath\" and \"tunnel_key\". First row,
>> >>> with
>> >>>>>> UUID e4f11a7b-09b6-454f-a125-34cc4b144ef6, had the following index
>> >>>>>> values before the transaction: bdbb436e-f98c-4651-9b80-6e8b95044560
>> >>> and
>> >>>>>> 7. Second row, with UUID d37cc3f1-8633-440f-b145-8222a0d4723c,
>> >>> existed
>> >>>>>> in the database before this transaction and was not modified by the
>> >>>>>> transaction.","error":"constraint violation"}
>> >>>>>>> ******
>> >>>>>>>
>> >>>>>>> And because of this constraint violation error, ovn-northd cannot
>> >>>>>> further write to the sb db until it is restarted.
>> >>>>>>>
>> >>>>>>> In my opinion this can only happen if ovn-northd doesn't see the
>> >>> port
>> >>>>>> binding row (which is actually present in the DB) in its IDL
>> >>> in-memory db.
>> >>>>>>> I suspect this could have happened when ovn-northd reconnects to
>> >>> the
>> >>>>>> same master or connects to the new master and it doesn't get the
>> >>> proper
>> >>>>>>> updates.
>> >>>>>>>
>> >>>>>>> Maybe in this case, the IDL should request the db contents
> with txn
>> >>> id
>> >>>>>> =0, so that it receives the complete dump of the db.
>> >>>>>>>
>> >>>>>>> Is it possible that ovn-northd sees a port binding with a tunnel
>> >>> key
>> >>>>>> 'x' and still allocates the same tunnel id 'x' to a new logical
>> >>> port ?
>> >>>>>>> If so, then definitely your patches makes sense.
>> >>>>>>>
>> >>>>>>> @Han - Have you seen this issue in your deployments ? Do you have
>> >>>>>> comments here ?
>> >>>>>>>
>> >>>>>>> Thanks
>> >>>>>>> Numan
>> >>>>>>>
>> >>>>>> I never saw such issue before, but I am not sure if this is
> possible
>> >>> due
>> >>>>>> to bugs. Currently there is a bug fix under review:
>> >>>>>>
>> >>>
>> >>>
> https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
>> >>> .
>> >>>>>> However, northd doesn't conditionally monitor the rows so I am not
>> >>> sure
>> >>>>>> if this is the root cause of the northd inconsistency issue
>> >>> discussed
>> >>> here.
>> >>>>>>
>> >>>>>> I don't think we should fix in northd (or ovn-controller) to handle
>> >>> the
>> >>>>>> inconsistency of ovsdb. The consistency should be expected from
>> >>> ovsdb
>> >>>>>> and we should fix ovsdb/IDL when there is such kind of bug.
>> >>> Otherwise,
>> >>>>>> there might be too many places to fix and even re-design. My
>> >>>>>> understanding is, if the ovsdb IDL sees a temporarily stale data,
>> >>> the
>> >>>>>> current northd/ovn-controller logic should be able to correct
>> >>> themselves
>> >>>>>> once the data is up-to-date. Moreover, for northd, it is connected
>> >>> to
>> >>>>>> leader-only in clustered mode, which avoids the possibility of
>> >>> seeing
>> >>>>>> staled data in northd (unless there is a bug).
>> >>>>>>
>> >>>>>> To summarize, I think we need to find the root cause of the
>> >>>>>> inconsistency between IDL and server and fix it there, instead of
>> >>>>>> changing ovn-northd to accommodate the inconsistency. (consistency
>> >>> is
>> >>>>>> the biggest advantage of OVSDB, to ease the application
>> >>> implementation).
>> >>>>>>
>> >>>>>> Thanks,
>> >>>>>> Han
>> >>>>>
>> >>>>> Hi Han, Numan,
>> >>>>>
>> >>>>> I might have misused "inconsistency" in this context. What I
> meant was
>> >>>>> more on the note of "discrepancies between NB and SB databases".
>> >>>>>
>> >>>>> This is a very simple reproducer for the port_binding tunnel_key
>> >>> issue,
>> >>>>> no clustering of NB/SB dbs involved:
>> >>>>>
>> >>>>> # Create two logical switches with one port each.
>> >>>>> $ ovn-nbctl ls-add ls1
>> >>>>> $ ovn-nbctl lsp-add ls1 p1
>> >>>>> $ ovn-nbctl ls-add ls2
>> >>>>> $ ovn-nbctl lsp-add ls2 p2
>> >>>>> $ ovn-nbctl --wait=sb sync
>> >>>>>
>> >>>>> # At this point PB for p1 has tunnel_key=1
>> >>>>> # At this point PB for p2 has tunnel_key=2
>> >>>>>
>> >>>>> # Simulate the SB db going away (could be network
>> >>>>> # issues or crash or some other event).
>> >>>>> $ ovn-ctl stop_sb_ovsdb
>> >>>>>
>> >>>>> # CMS decides to move p2 from ls2 to ls1 and removes
>> >>>>> # ls2 completely.
>> >>>>> $ ovn-nbctl ls-del ls2
>> >>>>> $ ovn-nbctl lsp-add ls1 p2
>> >>>>>
>> >>>>> # Simulate SB DB coming back online.
>> >>>>> $ ovn-ctl start_sb_ovsdb
>> >>>>>
>> >>>>> At this point ovn-northd will try to set the datapath field in
> PB2 to
>> >>>>> point to datapath_binding corresponding to ls1 but will *not* change
>> >>>>> tunnel_key.
>> >>>>>
>> >>>>> We get:
>> >>>>> 2020-04-29T20:52:41.327Z|00016|ovsdb_idl|WARN|transaction error:
>> >>>>> {"details":"Transaction causes multiple rows in \"Port_Binding\"
> table
>> >>>>> to have identical values (1b1c4b39-c045-448d-a532-8edbe5544e13
> and 1)
>> >>>>> for index on columns \"datapath\" and \"tunnel_key\". First
> row, with
>> >>>>> UUID e20219fa-ef67-49a2-81cd-739fa80d2bd4, existed in the database
>> >>>>> before this transaction and was not modified by the transaction.
>> >>> Second
>> >>>>> row, with UUID 50b0e240-8a4d-4e98-8e2f-97c94811d1b1, had the
> following
>> >>>>> index values before the transaction:
>> >>>>> a9b5959f-2f48-44e7-b6bb-f7148c28e4b5 and 1.","error":"constraint
>> >>> violation"}
>> >>>>>
>> >>>>> And ovn-northd keeps retrying the same transaction at every
> iteration
>> >>>>> from this point on and fails continuously.
>> >>>>>
>> >>>>> For the stale datapath issue (patch #1 in the series) a similar
>> >>>>> reproducer is:
>> >>>>>
>> >>>>> # Create a logical router with on router port.
>> >>>>> $ ovn-nbctl lr-add lr
>> >>>>> $ ovn-nbctl lrp-add lr p 00:00:00:00:00:01 1.1.1.1/24
> <http://1.1.1.1/24>
>> >>>>>
>> >>>>> # Simulate that a mac binding was created for the router
>> >>>>> # port.
>> >>>>> $ dp=$(ovn-sbctl --bare --columns _uuid list datapath .)
>> >>>>> $ ovn-sbctl create mac_binding logical_port="p" ip="1.1.1.2"
>> >>> datapath="$dp"
>> >>>>> $ ovn-nbctl --wait=sb sync
>> >>>>>
>> >>>>> # Simulate the SB db going away (could be network
>> >>>>> # issues or crash or some other event).
>> >>>>> $ ovn-ctl stop_sb_ovsdb
>> >>>>>
>> >>>>> # CMS decides to delete lr.
>> >>>>> $ ovn-nbctl lr-del lr
>> >>>>>
>> >>>>> # CMS decides to readd lr and router port.
>> >>>>> $ ovn-nbctl lr-add lr
>> >>>>> $ ovn-nbctl lrp-add lr p 00:00:00:00:00:01 1.1.1.1/24
> <http://1.1.1.1/24>
>> >>>>>
>> >>>>> # Simulate SB DB coming back online.
>> >>>>> $ ovn-ctl start_sb_ovsdb
>> >>>>>
>> >>>>> At this point ovn-northd will try to clear the old datapath record
>> >>> from
>> >>>>> SB DB *without* destroying the mac binding record.
>> >>>>>
>> >>>>> We get:
>> >>>>> 2020-04-29T21:41:42.145Z|00013|ovsdb_idl|WARN|transaction error:
>> >>>>> {"details":"cannot delete Datapath_Binding row
>> >>>>> de8d19d6-d67b-499b-8825-12d34ec60946 because of 1 remaining
>> >>>>> reference(s)","error":"referential integrity violation"}
>> >>>>>
>> >>>>> I think both situations above should be addressed by ovn-northd and
>> >>>>> stale datapath/mac_binding/port_binding/etc records should be
> purged.
>> >>> I
>> >>>>> guess there might be other scenarios that would trigger constraint
>> >>>>> violations too but this is what I found so far.
>> >>>>>
>> >>>>> If you agree, I can send a v2 and add tests for the two simplified
>> >>>>> scenarios I mentioned above.
>> >>>>>
>> >>>>> What do you think?
>> >>>
>> >>
>> >> Thanks Dumitru. for the explanation. It would be great to add these
> tests
>> >> in v2.
>> >>
>> >> Thanks
>> >> Numan
>> >>
>> >>
>> >>>>>
>> >>>>> Thanks,
>> >>>>> Dumitru
>> >>>>>
>> >>>>
>> >>>> Thanks Dumitru for explaining. Now I understand the problem. So
> it has
>> >>> nothing to do with OVSDB consistency itself, but just northd'd
> logic. I
>> >>> don't even need to stop SB to reproduce. Here is how I reproduced it:
>> >>>> $ ovn-nbctl ls-add ls1
>> >>>> $ ovn-nbctl ls-add ls2
>> >>>> $ ovn-nbctl lsp-add ls1 lsp1
>> >>>> $ ovn-nbctl lsp-add ls2 lsp2
>> >>>> $ ovn-nbctl lsp-del ls2 -- lsp-add ls1 lsp2
>> >>>
>> >>> Sorry for the typo. The last command was:
>> >>> $ ovn-nbctl lsp-del lsp2 -- lsp-add ls1 lsp2
>> >>>
>> >>
>> > I applied these 2 patches locally and I ran the below commands, which is
>> > the same as the above
>> > commands shared by Han.
>> >
>> > $ovn-nbctl ls-add ls1
>> > $ovn-nbctl ls-add ls2
>> > $ovn-nbctl lsp-add ls1 lsp1
>> > $ovn-nbctl lsp-add ls2 lsp2
>> > $ovs-vsctl add-port br-int p1 -- set Interface p1
> external_ids:iface-id=lsp2
>> > $ovn-sbctl list port_binding
>> >
>> > $ovn-sbctl list port_binding
>> > _uuid : bbf2f7e4-b61b-4ce8-adb6-4d17e410b87b
>> > chassis : ff506354-ac7b-4463-b42d-d89bddf319c7
>> > datapath : ef316369-0f2c-4246-adbd-8c187bd95e41
>> > ...
>> > ...
>> > tunnel_key : 1
>> > type : ""
>> > virtual_parent : []
>> >
>> > _uuid : 7cca89fa-55f9-4326-8188-6678838467bb
>> > chassis : []
>> > datapath : 21263028-a511-457a-824b-39a1219084c8
>> > ...
>> > logical_port : lsp1
>> > ...
>> > tunnel_key : 1
>> > type : ""
>> > virtual_parent : []
>> >
>> > $ovn-nbctl lsp-del lsp2 -- lsp-add ls1 lsp2
>> >
>> > $ovn-sbctl list port_binding
>> > _uuid : bbf2f7e4-b61b-4ce8-adb6-4d17e410b87b
>> > chassis : ff506354-ac7b-4463-b42d-d89bddf319c7
>> > datapath : 21263028-a511-457a-824b-39a1219084c8
>> > ...
>> > logical_port : lsp2
>> > ...
>> > tunnel_key : 1
>> > type : ""
>> > virtual_parent : []
>> >
>> > _uuid : 7cca89fa-55f9-4326-8188-6678838467bb
>> > chassis : []
>> > datapath : 21263028-a511-457a-824b-39a1219084c8
>> > ...
>> > logical_port : lsp1
>> > ...
>> > tunnel_key : 2
>> > type : ""
>> > virtual_parent : []
>> >
>> >
>> > I notice that the same port_binding record for lsp2 is being reused.
>> > Is that intentional ?
>>
>> This happens because the order in which ovn_port entries will be
> processed:
>>
>> https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L3453
>>
>> The "both" list is populated in join_logical_ports() and depends on the
>> order of Logical_Switch/Router_Port records in
>> od->nbs->ports/od->nbr->ports arrays which is not under ovn-northd's
>> control.
>>
>> https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L2022
>> https://github.com/ovn-org/ovn/blob/master/northd/ovn-northd.c#L2103
>>
>> >
>> > Ideally the old port binding record lsp2 should get deleted and
>> > new one should get created.
>>
>> So even if we delete the old port binding and recreate it we'd still get
>> a conflict in some cases because lsp2 would be processed before lsp1.
>>
>
> I was thinking about the same thing as Numan. I think OVN is not
> designed to handle "moving a LSP from one LS to another", but the OVSDB
> schema allows user to do so. In reality, I don't think it is a real
> world use case for such support (correct me if I am wrong). So, for
> ovn-northd it should detect the change of LSP - LS mapping and treat it
> as a deletion (delete the related PB as well) and recreation (recreate a
> new PB). @Dumitru Ceara <mailto:[email protected]> wouldn't this solve
> the problem?
>
Yes, it would fix the problem, just that we'd see "delete PB" followed
by "create PB". It shouldn't be a major issue though.
I'll send a v3 with this.
Thanks,
Dumitru
>> >
>> > I found another issue with the below commands (tested in sandbox env)
>> >
>> > $ovn-nbctl ls-add ls1
>> > $ovn-nbctl ls-add ls2
>> > $ovn-nbctl lsp-add ls1 lsp1
>> > $ovn-nbctl lsp-add ls2 lsp2
>> > $ovn-nbctl lsp-set-type lsp2 external
>> > $ovn-nbctl ha-chassis-group-add chg1
>> > $ovn-nbctl ha-chassis-group-add-chassis chg1 chassis-1 30
>> > $ovn-nbctl set logical_switch_port lsp2 ha_chassis_group=<chg1_uuid>
>> > $ovn-nbctl lsp-del lsp2 -- lsp-add ls1 lsp2 -> This fails with the below
>> > logs in ovn-northd
>> >
>> > *******
>> > 2020-04-30T09:59:48.319Z|00007|ovsdb_idl|WARN|transaction error:
>> > {"details":"cannot delete HA_Chassis_Group row
>> > 6e0c88d7-20f6-473a-bd0a-9eea60b639e6 because of 1 remaining
>> > reference(s)","error":"referential integrity violation"}
>> > *******
>>
>> I'll look into this. Looks like patch #2 of the series should take care
>> of HA_Chassis_Group too.
>>
>> >
>> > I think it's better if the stale port binding entry is deleted
> instead of
>> > reusing it. What do you think ?
>>
>> As mentioned above, this wouldn't help too much and it would actually
>> create larger transactions so it seems inefficient.
>>
>> Thanks,
>> Dumitru
>>
>> >
>> > Thanks
>> > Numan
>> >
>> >
>> >
>> >
>> >>>>
>> >>>> 2020-04-29T23:46:17.675Z|00007|ovsdb_idl|WARN|transaction error:
>> >>> {"details":"Transaction causes multiple rows in \"Port_Binding\"
> table to
>> >>> have identical values (be595a3b-3904-4229-9ba2-884b27a86b75 and 1) for
>> >>> index on columns \"datapath\" and \"tunnel_key\". First row, with
> UUID
>> >>> d4cc6ec5-4817-47c9-aa83-9985d3b7b452, existed in the database
> before this
>> >>> transaction and was not modified by the transaction. Second row, with
>> >>> UUID
>> >>> b874ab93-d97a-4583-8ac3-c353a40b180d, had the following index values
>> >>> before
>> >>> the transaction: 6940ad91-83c5-4fe9-bab5-4fbec6714b0d and
>> >>> 1.","error":"constraint violation"}
>> >>>>
>> >>>> I will take a closer look at the fix.
>> >>>>
>> >>>> Thanks,
>> >>>> Han
>> >>> _______________________________________________
>> >>> dev mailing list
>> >>> [email protected] <mailto:[email protected]>
>> >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >>>
>> >>>
>> > _______________________________________________
>> > dev mailing list
>> > [email protected] <mailto:[email protected]>
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >
>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
