search

eBay / go-ovn

A Go library for OVN Northbound/Southbound DB access using native OVSDB protocol
Apache License 2.0
108 stars 59 forks source link

onvimp: enforce table ordering in cache population #120

Closed amorenoz closed 3 years ago

amorenoz commented 3 years ago

When the cache is populated, a specific order must be preserved that ensures that if table A refereces table B, B is populated before A.

This assumption seems widely made in the RowTo{XXX} functions. So, this patch enforces this ordering.

Fixes: #119 Signed-off-by: Adrian Moreno amorenoz@redhat.com

hzhou8 commented 3 years ago

Thanks for the patch. Could you help give an example where a RowToXXX function depends on the ordering? I am not aware of such kind of dependency except one that I fixed/removed in an previous commit.

amorenoz commented 3 years ago

Hi @hzhou8, sorry I did not put the "Fixes" link on the original description (fixed that now). As you can see on backtrace pasted in the linked Issue (#119), the RowToLogicalRouter depends on the on the LogicalRouterPort cache being populated.

However, I've just realized I was testing without https://github.com/eBay/go-ovn/commit/5066560cee670596aab3c3da959e01b160953431. That patch may have solved my issue. I'll retest.

OTOH, I think the intended behaviour of populateCache was to respect the order of the tablesOrder list. This behaviour was changed with https://github.com/eBay/go-ovn/commit/30abed5fb9684c24dd9b93f7a5000a85aeae6a1f. So restoring that enforcement might be a good idea even the inter-table dependency has been fixed.

amorenoz commented 3 years ago

@hzhou8 , indeed https://github.com/eBay/go-ovn/commit/5066560cee670596aab3c3da959e01b160953431 does fix my issue. Feel free to close this PR or consider the last argument stated in my previous comment.

hzhou8 commented 3 years ago

@amorenoz Thanks for the context. In fact I don't think the ordering is necessary, at least for now. In the current design it should not assume any dependency while populating cache, because the references are stored as uuids only. Thank you for pointing out that there was a behavior change introduced by https://github.com/eBay/go-ovn/commit/30abed5fb9684c24dd9b93f7a5000a85aeae6a1f, but after going deeper in the history it seems that the tablesOrder introduced by commit 30abed5 was intended only to fix this same problem of commit 82a71f5067. If commit 82a71f5067 didn't try to resolve the references, this whole tablesOrder logic wasn't necessary. I am sorry that all these slipped from the code reviews in the first place. So I would rather keep it as is, or even better, to remove the tablesOrder mechanism, to avoid introducing more confusion. What do you think?

amorenoz commented 3 years ago

@amorenoz Thanks for the context. In fact I don't think the ordering is necessary, at least for now. In the current design it should not assume any dependency while populating cache, because the references are stored as uuids only. Thank you for pointing out that there was a behavior change introduced by 30abed5, but after going deeper in the history it seems that the tablesOrder introduced by commit 30abed5 was intended only to fix this same problem of commit 82a71f5. If commit 82a71f5 didn't try to resolve the references, this whole tablesOrder logic wasn't necessary. I am sorry that all these slipped from the code reviews in the first place. So I would rather keep it as is, or even better, to remove the tablesOrder mechanism, to avoid introducing more confusion. What do you think?

Agree. It's definitely more robust not to depend on a specific ordering.

amorenoz commented 3 years ago

Closing. Sorry for the noise.