Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On 2/23/22 19:50, Han Zhou wrote: > On Wed, Feb 23, 2022 at 12:35 AM Dumitru Ceara wrote: >> >> On 2/23/22 09:24, Dumitru Ceara wrote: >>> On 2/23/22 07:27, Han Zhou wrote: On Fri, Feb 11, 2022 at 12:52 AM Dumitru Ceara > wrote: > > On 2/11/22 00:54, Numan Siddique wrote: >> On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara wrote: >>> >>> We still need to try to ovsdb_idl_loop_commit_and_wait() on > instances >>> that are in standby mode. That's because they need to try to take > the >>> lock. But if they're in standby they won't actually build a transaction >>> json and will return early because they don't own the SB lock. >>> >>> That's reported as an error by ovsdb_idl_loop_commit_and_wait() but > we >>> shouldn't act on it. >>> >>> Also, to ensure that no DB changes are missed, > ovsdb_idl_track_clear() >>> should be called only on active instances. The standby or paused > ones >>> will get the incremental updates when they become active. > Otherwise we >>> would be forced to perform a full recompute when the instance > becomes >>> active. >> >> Hi Dumitru, >> > > Hi Numan, > > Thanks for the review! > >> I've a question on the track clear being moved out of the standby instances. >> To ensure correctness, I suppose it's better to trigger a full recompute when a >> standby instance becomes active. What do you think? >> > > I might be wrong but I don't think that's necessary. It may also be > quite costly as full recomputes can take quite long. > >> Also lets say CMS does the below operations >> - Add a logical switch S1 >> - Add a logical port p1 in S1 >> - Add a logical port p2 in S1 >> - Delete logical port p2 >> - Delete a logical switch. >> >> With this patch since we are not clearing the tracking information, >> how does ovn-northd >> process the tracked changes when it becomes active ? > > When ovn-northd becomes active, from a Northbound database > perspective, > there were no changes; that is, S1 didn't exist when it was last > active > and it doesn't exist now either. > > So, there should be no NB change to process. Accumulating tracked > changes without calling clear() on the standby has exactly this > effect. Hi Dumitru, >>> >>> Hi Han, >>> I wonder how accumulating tracked changes without calling clear() would work. Firstly, I was under the impression that ovsdb_idl_track_clear() must > be called before the next ovsdb_idl_run(), and the current change tracking implementation cannot safely carry tracking information across the iterations. This was why in ovn-controller whenever (!engine_has_run() > && engine_need_run()) we force recompute in the next iteration. If changes could be carried over we would have incrementally processed the > accumulated changes without forcing recompute. I can't recall the details, and I checked the IDL again briefly but I didn't find the exact reason why > it is not safe. But I believe it was never used this way before. I should > have added a TODO for this (but somehow forgot to, sorry). >>> >>> I've been looking at that code too and I don't see any reason why >>> accumulating changes wouldn't work. The IDL code is written such that >>> it processes multiple jsonrpc updates in a single run anyway: >>> >>> ovsdb_idl_run() -> ovsdb_cs_run() -> which can receive up to 50 >>> (hardcoded) jsonrpc messages. >>> > Maybe you are right, but I am not sure. It's true that IDL run can process > multiple messages, but there are functions called once for every IDL run, > after processing all the messages: > ovsdb_idl_reparse_refs_to_inserted, ovsdb_idl_reparse_deleted, > ovsdb_idl_row_destroy_postprocess, etc. > > Is it possible any of these could cause problems with change tracking left > with the previous run? I haven't looked deeper yet. > >>> It's possible that I missed something though, the IDL functionality is >>> complex, so maybe it's worth documenting that we recommend calling >>> ovsdb_idl_track_clear() every time ovsdb_idl_run() is called, and why. >>> What do you think? >>> > > There is a comment above the ovsdb_idl_track_clear() which suggests calling > it before ovsdb_idl_run(), but it doesn't mention if it is mandatory. I > might have just followed the comment. > Secondly, even if it is safe to accumulate changes, it is going to be a memory problem. Active-standby failover happens very rarely in a > healthy production environment. So even if the DB size is small, the change tracking can grow without any limit. I tried with this patch by doing simply a loop of adding and deleting a single logical switch 10k times > on top of an empty NB DB, and the standby northd's memory grew to 1.1GB.
Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On Wed, Feb 23, 2022 at 12:35 AM Dumitru Ceara wrote: > > On 2/23/22 09:24, Dumitru Ceara wrote: > > On 2/23/22 07:27, Han Zhou wrote: > >> On Fri, Feb 11, 2022 at 12:52 AM Dumitru Ceara wrote: > >>> > >>> On 2/11/22 00:54, Numan Siddique wrote: > On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara > >> wrote: > > > > We still need to try to ovsdb_idl_loop_commit_and_wait() on instances > > that are in standby mode. That's because they need to try to take the > > lock. But if they're in standby they won't actually build a > >> transaction > > json and will return early because they don't own the SB lock. > > > > That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we > > shouldn't act on it. > > > > Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear() > > should be called only on active instances. The standby or paused ones > > will get the incremental updates when they become active. Otherwise we > > would be forced to perform a full recompute when the instance becomes > > active. > > Hi Dumitru, > > >>> > >>> Hi Numan, > >>> > >>> Thanks for the review! > >>> > I've a question on the track clear being moved out of the standby > >> instances. > To ensure correctness, I suppose it's better to trigger a full > >> recompute when a > standby instance becomes active. What do you think? > > >>> > >>> I might be wrong but I don't think that's necessary. It may also be > >>> quite costly as full recomputes can take quite long. > >>> > Also lets say CMS does the below operations > - Add a logical switch S1 > - Add a logical port p1 in S1 > - Add a logical port p2 in S1 > - Delete logical port p2 > - Delete a logical switch. > > With this patch since we are not clearing the tracking information, > how does ovn-northd > process the tracked changes when it becomes active ? > >>> > >>> When ovn-northd becomes active, from a Northbound database perspective, > >>> there were no changes; that is, S1 didn't exist when it was last active > >>> and it doesn't exist now either. > >>> > >>> So, there should be no NB change to process. Accumulating tracked > >>> changes without calling clear() on the standby has exactly this effect. > >> > >> Hi Dumitru, > >> > > > > Hi Han, > > > >> I wonder how accumulating tracked changes without calling clear() would > >> work. > >> > >> Firstly, I was under the impression that ovsdb_idl_track_clear() must be > >> called before the next ovsdb_idl_run(), and the current change tracking > >> implementation cannot safely carry tracking information across the > >> iterations. This was why in ovn-controller whenever (!engine_has_run() && > >> engine_need_run()) we force recompute in the next iteration. If changes > >> could be carried over we would have incrementally processed the accumulated > >> changes without forcing recompute. I can't recall the details, and I > >> checked the IDL again briefly but I didn't find the exact reason why it is > >> not safe. But I believe it was never used this way before. I should have > >> added a TODO for this (but somehow forgot to, sorry). > >> > > > > I've been looking at that code too and I don't see any reason why > > accumulating changes wouldn't work. The IDL code is written such that > > it processes multiple jsonrpc updates in a single run anyway: > > > > ovsdb_idl_run() -> ovsdb_cs_run() -> which can receive up to 50 > > (hardcoded) jsonrpc messages. > > Maybe you are right, but I am not sure. It's true that IDL run can process multiple messages, but there are functions called once for every IDL run, after processing all the messages: ovsdb_idl_reparse_refs_to_inserted, ovsdb_idl_reparse_deleted, ovsdb_idl_row_destroy_postprocess, etc. Is it possible any of these could cause problems with change tracking left with the previous run? I haven't looked deeper yet. > > It's possible that I missed something though, the IDL functionality is > > complex, so maybe it's worth documenting that we recommend calling > > ovsdb_idl_track_clear() every time ovsdb_idl_run() is called, and why. > > What do you think? > > There is a comment above the ovsdb_idl_track_clear() which suggests calling it before ovsdb_idl_run(), but it doesn't mention if it is mandatory. I might have just followed the comment. > >> Secondly, even if it is safe to accumulate changes, it is going to be a > >> memory problem. Active-standby failover happens very rarely in a healthy > >> production environment. So even if the DB size is small, the change > >> tracking can grow without any limit. I tried with this patch by doing > >> simply a loop of adding and deleting a single logical switch 10k times on > >> top of an empty NB DB, and the standby northd's memory grew to 1.1GB. > >> > > > > This is surprising, sounds like an issue in the IDL, I would've expected > > the delete to
Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On 2/23/22 09:24, Dumitru Ceara wrote: > On 2/23/22 07:27, Han Zhou wrote: >> On Fri, Feb 11, 2022 at 12:52 AM Dumitru Ceara wrote: >>> >>> On 2/11/22 00:54, Numan Siddique wrote: On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara >> wrote: > > We still need to try to ovsdb_idl_loop_commit_and_wait() on instances > that are in standby mode. That's because they need to try to take the > lock. But if they're in standby they won't actually build a >> transaction > json and will return early because they don't own the SB lock. > > That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we > shouldn't act on it. > > Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear() > should be called only on active instances. The standby or paused ones > will get the incremental updates when they become active. Otherwise we > would be forced to perform a full recompute when the instance becomes > active. Hi Dumitru, >>> >>> Hi Numan, >>> >>> Thanks for the review! >>> I've a question on the track clear being moved out of the standby >> instances. To ensure correctness, I suppose it's better to trigger a full >> recompute when a standby instance becomes active. What do you think? >>> >>> I might be wrong but I don't think that's necessary. It may also be >>> quite costly as full recomputes can take quite long. >>> Also lets say CMS does the below operations - Add a logical switch S1 - Add a logical port p1 in S1 - Add a logical port p2 in S1 - Delete logical port p2 - Delete a logical switch. With this patch since we are not clearing the tracking information, how does ovn-northd process the tracked changes when it becomes active ? >>> >>> When ovn-northd becomes active, from a Northbound database perspective, >>> there were no changes; that is, S1 didn't exist when it was last active >>> and it doesn't exist now either. >>> >>> So, there should be no NB change to process. Accumulating tracked >>> changes without calling clear() on the standby has exactly this effect. >> >> Hi Dumitru, >> > > Hi Han, > >> I wonder how accumulating tracked changes without calling clear() would >> work. >> >> Firstly, I was under the impression that ovsdb_idl_track_clear() must be >> called before the next ovsdb_idl_run(), and the current change tracking >> implementation cannot safely carry tracking information across the >> iterations. This was why in ovn-controller whenever (!engine_has_run() && >> engine_need_run()) we force recompute in the next iteration. If changes >> could be carried over we would have incrementally processed the accumulated >> changes without forcing recompute. I can't recall the details, and I >> checked the IDL again briefly but I didn't find the exact reason why it is >> not safe. But I believe it was never used this way before. I should have >> added a TODO for this (but somehow forgot to, sorry). >> > > I've been looking at that code too and I don't see any reason why > accumulating changes wouldn't work. The IDL code is written such that > it processes multiple jsonrpc updates in a single run anyway: > > ovsdb_idl_run() -> ovsdb_cs_run() -> which can receive up to 50 > (hardcoded) jsonrpc messages. > > It's possible that I missed something though, the IDL functionality is > complex, so maybe it's worth documenting that we recommend calling > ovsdb_idl_track_clear() every time ovsdb_idl_run() is called, and why. > What do you think? > >> Secondly, even if it is safe to accumulate changes, it is going to be a >> memory problem. Active-standby failover happens very rarely in a healthy >> production environment. So even if the DB size is small, the change >> tracking can grow without any limit. I tried with this patch by doing >> simply a loop of adding and deleting a single logical switch 10k times on >> top of an empty NB DB, and the standby northd's memory grew to 1.1GB. >> > > This is surprising, sounds like an issue in the IDL, I would've expected > the delete to just remove the record from the track list. I'll look > into it. > I had a quick look, it's because deleting a row clears its datum, sets tracked_old_datum and adds it to the deleted track list. The subsequent "add" will not find this row, and will insert a new one. We probably need to fix this anyway because even in the current implementation the IDL can process up to 50 consecutive "add/delete/add/delete/.." updates. This can potentially cause issues due to "spurious" delete events when the client finally processes the tracked changes. >> Thirdly, processing all the tracked changes when standby becomes active is >> likely taking higher cost than recompute, if the tracked change size is >> bigger than the DB size. >> > > If changes are accumulated correctly, the total number of the tracked > record changes should never be larger than
Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On 2/23/22 07:27, Han Zhou wrote: > On Fri, Feb 11, 2022 at 12:52 AM Dumitru Ceara wrote: >> >> On 2/11/22 00:54, Numan Siddique wrote: >>> On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara > wrote: We still need to try to ovsdb_idl_loop_commit_and_wait() on instances that are in standby mode. That's because they need to try to take the lock. But if they're in standby they won't actually build a > transaction json and will return early because they don't own the SB lock. That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we shouldn't act on it. Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear() should be called only on active instances. The standby or paused ones will get the incremental updates when they become active. Otherwise we would be forced to perform a full recompute when the instance becomes active. >>> >>> Hi Dumitru, >>> >> >> Hi Numan, >> >> Thanks for the review! >> >>> I've a question on the track clear being moved out of the standby > instances. >>> To ensure correctness, I suppose it's better to trigger a full > recompute when a >>> standby instance becomes active. What do you think? >>> >> >> I might be wrong but I don't think that's necessary. It may also be >> quite costly as full recomputes can take quite long. >> >>> Also lets say CMS does the below operations >>> - Add a logical switch S1 >>> - Add a logical port p1 in S1 >>> - Add a logical port p2 in S1 >>> - Delete logical port p2 >>> - Delete a logical switch. >>> >>> With this patch since we are not clearing the tracking information, >>> how does ovn-northd >>> process the tracked changes when it becomes active ? >> >> When ovn-northd becomes active, from a Northbound database perspective, >> there were no changes; that is, S1 didn't exist when it was last active >> and it doesn't exist now either. >> >> So, there should be no NB change to process. Accumulating tracked >> changes without calling clear() on the standby has exactly this effect. > > Hi Dumitru, > Hi Han, > I wonder how accumulating tracked changes without calling clear() would > work. > > Firstly, I was under the impression that ovsdb_idl_track_clear() must be > called before the next ovsdb_idl_run(), and the current change tracking > implementation cannot safely carry tracking information across the > iterations. This was why in ovn-controller whenever (!engine_has_run() && > engine_need_run()) we force recompute in the next iteration. If changes > could be carried over we would have incrementally processed the accumulated > changes without forcing recompute. I can't recall the details, and I > checked the IDL again briefly but I didn't find the exact reason why it is > not safe. But I believe it was never used this way before. I should have > added a TODO for this (but somehow forgot to, sorry). > I've been looking at that code too and I don't see any reason why accumulating changes wouldn't work. The IDL code is written such that it processes multiple jsonrpc updates in a single run anyway: ovsdb_idl_run() -> ovsdb_cs_run() -> which can receive up to 50 (hardcoded) jsonrpc messages. It's possible that I missed something though, the IDL functionality is complex, so maybe it's worth documenting that we recommend calling ovsdb_idl_track_clear() every time ovsdb_idl_run() is called, and why. What do you think? > Secondly, even if it is safe to accumulate changes, it is going to be a > memory problem. Active-standby failover happens very rarely in a healthy > production environment. So even if the DB size is small, the change > tracking can grow without any limit. I tried with this patch by doing > simply a loop of adding and deleting a single logical switch 10k times on > top of an empty NB DB, and the standby northd's memory grew to 1.1GB. > This is surprising, sounds like an issue in the IDL, I would've expected the delete to just remove the record from the track list. I'll look into it. > Thirdly, processing all the tracked changes when standby becomes active is > likely taking higher cost than recompute, if the tracked change size is > bigger than the DB size. > If changes are accumulated correctly, the total number of the tracked record changes should never be larger than the size of the DB the last time changes were processed + the size of the DB now, resulting in a cost to process of O(size-of-DB), the same as for full recompute. Or am I missing something? > So for now I would suggest keeping the logic of clearing tracking on every > iteration and force recompute when failover. > At least until we figure out why the memory increase in the IDL, I agree to keep forcing a recompute on failover. That's also because we currently don't really incrementally process much in ovn-northd. I'll send a v2. > Thanks, > Han > Thanks, Dumitru >> >> From a Southbound database perspective there are two cases: >>
Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On Fri, Feb 11, 2022 at 12:52 AM Dumitru Ceara wrote: > > On 2/11/22 00:54, Numan Siddique wrote: > > On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara wrote: > >> > >> We still need to try to ovsdb_idl_loop_commit_and_wait() on instances > >> that are in standby mode. That's because they need to try to take the > >> lock. But if they're in standby they won't actually build a transaction > >> json and will return early because they don't own the SB lock. > >> > >> That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we > >> shouldn't act on it. > >> > >> Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear() > >> should be called only on active instances. The standby or paused ones > >> will get the incremental updates when they become active. Otherwise we > >> would be forced to perform a full recompute when the instance becomes > >> active. > > > > Hi Dumitru, > > > > Hi Numan, > > Thanks for the review! > > > I've a question on the track clear being moved out of the standby instances. > > To ensure correctness, I suppose it's better to trigger a full recompute when a > > standby instance becomes active. What do you think? > > > > I might be wrong but I don't think that's necessary. It may also be > quite costly as full recomputes can take quite long. > > > Also lets say CMS does the below operations > > - Add a logical switch S1 > > - Add a logical port p1 in S1 > > - Add a logical port p2 in S1 > > - Delete logical port p2 > > - Delete a logical switch. > > > > With this patch since we are not clearing the tracking information, > > how does ovn-northd > > process the tracked changes when it becomes active ? > > When ovn-northd becomes active, from a Northbound database perspective, > there were no changes; that is, S1 didn't exist when it was last active > and it doesn't exist now either. > > So, there should be no NB change to process. Accumulating tracked > changes without calling clear() on the standby has exactly this effect. Hi Dumitru, I wonder how accumulating tracked changes without calling clear() would work. Firstly, I was under the impression that ovsdb_idl_track_clear() must be called before the next ovsdb_idl_run(), and the current change tracking implementation cannot safely carry tracking information across the iterations. This was why in ovn-controller whenever (!engine_has_run() && engine_need_run()) we force recompute in the next iteration. If changes could be carried over we would have incrementally processed the accumulated changes without forcing recompute. I can't recall the details, and I checked the IDL again briefly but I didn't find the exact reason why it is not safe. But I believe it was never used this way before. I should have added a TODO for this (but somehow forgot to, sorry). Secondly, even if it is safe to accumulate changes, it is going to be a memory problem. Active-standby failover happens very rarely in a healthy production environment. So even if the DB size is small, the change tracking can grow without any limit. I tried with this patch by doing simply a loop of adding and deleting a single logical switch 10k times on top of an empty NB DB, and the standby northd's memory grew to 1.1GB. Thirdly, processing all the tracked changes when standby becomes active is likely taking higher cost than recompute, if the tracked change size is bigger than the DB size. So for now I would suggest keeping the logic of clearing tracking on every iteration and force recompute when failover. Thanks, Han > > From a Southbound database perspective there are two cases: > > a. The former active northd processed some (but not all) of the NB > changes and executed their corresponding SB transactions. In this case, > the standby northd also receives update messages for the SB records that > were changed. The standby northd tracks these changes. > > When the standby northd becomes active it will: > - determine that NB state didn't change > - SB state changed and needs to be reconciled (today we do this with the > help of a NULL change handler for SB_* tables which will trigger a full > recompute). > > b. The former active northd processed all of the NB changes and executed > their corresponding SB transactions. In this case, the final state of > the NB and SB databases should be equivalent to their initial states. > NB/SB changes will be accumulated by the change tracking mechanism on > the standby resulting in empty tracked changes lists. This is fine > because the new active northd doesn't need to do anything, the DB > contents are already consistent. > > c. The former active northd processed none of the NB changes yet. This > is very similar to case "b" above, the new active northd doesn't need to > change anything in the NB/SB and it won't do that either. > > > > > Thanks > > Numan > > > > Thanks, > Dumitru > > ___ > dev mailing list > d...@openvswitch.org >
Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On Fri, Feb 11, 2022 at 3:53 AM Dumitru Ceara wrote: > > On 2/11/22 00:54, Numan Siddique wrote: > > On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara wrote: > >> > >> We still need to try to ovsdb_idl_loop_commit_and_wait() on instances > >> that are in standby mode. That's because they need to try to take the > >> lock. But if they're in standby they won't actually build a transaction > >> json and will return early because they don't own the SB lock. > >> > >> That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we > >> shouldn't act on it. > >> > >> Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear() > >> should be called only on active instances. The standby or paused ones > >> will get the incremental updates when they become active. Otherwise we > >> would be forced to perform a full recompute when the instance becomes > >> active. > > > > Hi Dumitru, > > > > Hi Numan, > > Thanks for the review! > > > I've a question on the track clear being moved out of the standby instances. > > To ensure correctness, I suppose it's better to trigger a full recompute > > when a > > standby instance becomes active. What do you think? > > > > I might be wrong but I don't think that's necessary. It may also be > quite costly as full recomputes can take quite long. > > > Also lets say CMS does the below operations > > - Add a logical switch S1 > > - Add a logical port p1 in S1 > > - Add a logical port p2 in S1 > > - Delete logical port p2 > > - Delete a logical switch. > > > > With this patch since we are not clearing the tracking information, > > how does ovn-northd > > process the tracked changes when it becomes active ? > > When ovn-northd becomes active, from a Northbound database perspective, > there were no changes; that is, S1 didn't exist when it was last active > and it doesn't exist now either. > > So, there should be no NB change to process. Accumulating tracked > changes without calling clear() on the standby has exactly this effect. > > From a Southbound database perspective there are two cases: > > a. The former active northd processed some (but not all) of the NB > changes and executed their corresponding SB transactions. In this case, > the standby northd also receives update messages for the SB records that > were changed. The standby northd tracks these changes. > > When the standby northd becomes active it will: > - determine that NB state didn't change > - SB state changed and needs to be reconciled (today we do this with the > help of a NULL change handler for SB_* tables which will trigger a full > recompute). > > b. The former active northd processed all of the NB changes and executed > their corresponding SB transactions. In this case, the final state of > the NB and SB databases should be equivalent to their initial states. > NB/SB changes will be accumulated by the change tracking mechanism on > the standby resulting in empty tracked changes lists. This is fine > because the new active northd doesn't need to do anything, the DB > contents are already consistent. > > c. The former active northd processed none of the NB changes yet. This > is very similar to case "b" above, the new active northd doesn't need to > change anything in the NB/SB and it won't do that either. Thanks for the detailed explanation. Acked-by: Numan Siddique Numan > > > > > Thanks > > Numan > > > > Thanks, > Dumitru > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On 2/11/22 00:54, Numan Siddique wrote: > On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara wrote: >> >> We still need to try to ovsdb_idl_loop_commit_and_wait() on instances >> that are in standby mode. That's because they need to try to take the >> lock. But if they're in standby they won't actually build a transaction >> json and will return early because they don't own the SB lock. >> >> That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we >> shouldn't act on it. >> >> Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear() >> should be called only on active instances. The standby or paused ones >> will get the incremental updates when they become active. Otherwise we >> would be forced to perform a full recompute when the instance becomes >> active. > > Hi Dumitru, > Hi Numan, Thanks for the review! > I've a question on the track clear being moved out of the standby instances. > To ensure correctness, I suppose it's better to trigger a full recompute > when a > standby instance becomes active. What do you think? > I might be wrong but I don't think that's necessary. It may also be quite costly as full recomputes can take quite long. > Also lets say CMS does the below operations > - Add a logical switch S1 > - Add a logical port p1 in S1 > - Add a logical port p2 in S1 > - Delete logical port p2 > - Delete a logical switch. > > With this patch since we are not clearing the tracking information, > how does ovn-northd > process the tracked changes when it becomes active ? When ovn-northd becomes active, from a Northbound database perspective, there were no changes; that is, S1 didn't exist when it was last active and it doesn't exist now either. So, there should be no NB change to process. Accumulating tracked changes without calling clear() on the standby has exactly this effect. >From a Southbound database perspective there are two cases: a. The former active northd processed some (but not all) of the NB changes and executed their corresponding SB transactions. In this case, the standby northd also receives update messages for the SB records that were changed. The standby northd tracks these changes. When the standby northd becomes active it will: - determine that NB state didn't change - SB state changed and needs to be reconciled (today we do this with the help of a NULL change handler for SB_* tables which will trigger a full recompute). b. The former active northd processed all of the NB changes and executed their corresponding SB transactions. In this case, the final state of the NB and SB databases should be equivalent to their initial states. NB/SB changes will be accumulated by the change tracking mechanism on the standby resulting in empty tracked changes lists. This is fine because the new active northd doesn't need to do anything, the DB contents are already consistent. c. The former active northd processed none of the NB changes yet. This is very similar to case "b" above, the new active northd doesn't need to change anything in the NB/SB and it won't do that either. > > Thanks > Numan > Thanks, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't log transaction failures on standby instances.
On Thu, Jan 27, 2022 at 10:00 AM Dumitru Ceara wrote:
>
> We still need to try to ovsdb_idl_loop_commit_and_wait() on instances
> that are in standby mode. That's because they need to try to take the
> lock. But if they're in standby they won't actually build a transaction
> json and will return early because they don't own the SB lock.
>
> That's reported as an error by ovsdb_idl_loop_commit_and_wait() but we
> shouldn't act on it.
>
> Also, to ensure that no DB changes are missed, ovsdb_idl_track_clear()
> should be called only on active instances. The standby or paused ones
> will get the incremental updates when they become active. Otherwise we
> would be forced to perform a full recompute when the instance becomes
> active.
Hi Dumitru,
I've a question on the track clear being moved out of the standby instances.
To ensure correctness, I suppose it's better to trigger a full recompute when a
standby instance becomes active. What do you think?
Also lets say CMS does the below operations
- Add a logical switch S1
- Add a logical port p1 in S1
- Add a logical port p2 in S1
- Delete logical port p2
- Delete a logical switch.
With this patch since we are not clearing the tracking information,
how does ovn-northd
process the tracked changes when it becomes active ?
Thanks
Numan
>
> Fixes: 953832eb17f3 ("ovn-northd: Don't trigger recompute for transactions
> that are in progress.")
> Fixes: 4597317f16d1 ("northd: Introduce incremental processing for northd")
> Reported-by: Ilya Maximets
> Signed-off-by: Dumitru Ceara
> ---
> northd/ovn-northd.c | 35 ---
> 1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 793135ede..4e1e51931 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -1089,6 +1089,26 @@ main(int argc, char *argv[])
> ovnnb_txn, ovnsb_txn,
> _idl_loop);
> }
> +
> +/* If there are any errors, we force a full recompute in
> order
> + * to ensure we handle all changes. */
> +if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
> +VLOG_INFO("OVNNB commit failed, "
> + "force recompute next time.");
> +recompute = true;
> +}
> +
> +if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
> +VLOG_INFO("OVNSB commit failed, "
> + "force recompute next time.");
> +recompute = true;
> +}
> +ovsdb_idl_track_clear(ovnnb_idl_loop.idl);
> +ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
> +} else {
> +/* Make sure we send any pending requests, e.g., lock. */
> +ovsdb_idl_loop_commit_and_wait(_idl_loop);
> +ovsdb_idl_loop_commit_and_wait(_idl_loop);
> }
> } else {
> /* ovn-northd is paused
> @@ -1112,21 +1132,6 @@ main(int argc, char *argv[])
> ovsdb_idl_wait(ovnsb_idl_loop.idl);
> }
>
> -/* If there are any errors, we force a full recompute in order to
> - ensure we handle all changes. */
> -if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
> -VLOG_INFO("OVNNB commit failed, force recompute next time.");
> -recompute = true;
> -}
> -
> -if (!ovsdb_idl_loop_commit_and_wait(_idl_loop)) {
> -VLOG_INFO("OVNSB commit failed, force recompute next time.");
> -recompute = true;
> -}
> -
> -ovsdb_idl_track_clear(ovnnb_idl_loop.idl);
> -ovsdb_idl_track_clear(ovnsb_idl_loop.idl);
> -
> unixctl_server_run(unixctl);
> unixctl_server_wait(unixctl);
> memory_wait();
> --
> 2.27.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
