On 12/13/21 19:45, Han Zhou wrote: > > > On Mon, Dec 13, 2021 at 10:37 AM Ilya Maximets <[email protected] > <mailto:[email protected]>> wrote: >> >> On 12/13/21 19:09, Han Zhou wrote: >> > >> > >> > On Mon, Dec 13, 2021 at 7:43 AM Ilya Maximets <[email protected] >> > <mailto:[email protected]> <mailto:[email protected] >> > <mailto:[email protected]>>> wrote: >> >> >> >> Snapshots are scheduled for every 10-20 minutes. It's a random value >> >> in this interval for each server. Once the time is up, but the maximum >> >> time (24 hours) not reached yet, ovsdb will start checking if the log >> >> grew a lot on every iteration. Once the growth is detected, compaction >> >> is triggered. >> >> >> >> OTOH, it's very common for an OVSDB cluster to not have the log growing >> >> very fast. If the log didn't grow 2x in 20 minutes, the randomness of >> >> the initial scheduled time is gone and all the servers are checking if >> >> they need to create snapshot on every iteration. And since all of them >> >> are part of the same cluster, their logs are growing with the same >> >> speed. Once the critical mass is reached, all the servers will start >> >> creating snapshots at the same time. If the database is big enough, >> >> that might leave the cluster unresponsive for an extended period of >> >> time (e.g. 10-15 seconds for OVN_Southbound database in a larger scale >> >> OVN deployment) until the compaction completed. >> >> >> >> Fix that by re-scheduling a quick retry if the minimal time already >> >> passed. Effectively, this will work as a randomized 1-2 min delay >> >> between checks, so the servers will not synchronize. >> >> >> >> Scheduling function updated to not change the upper limit on quick >> >> reschedules to avoid delaying the snapshot creation indefinitely. >> >> Currently quick re-schedules are only used for the error cases, and >> >> there is always a 'slow' re-schedule after the successful compaction. >> >> So, the change of a scheduling function doesn't change the current >> >> behavior much. >> > >> > Thanks Ilya! >> > To understand the behavior change at the error scenario: >> > If there is not much updates to the DB, it keeps rescheduling the next >> > snapshot >> > with quick=true, which updates "min" but not "max", so in the end "max" is >> > small >> > than "min" and snapshot happens. Now if the snapshot fails, the next >> > snapshot is >> > scheduled with quick=ture (because of the error), but the "max" will not >> > be updated, >> > so it means the next snapshot will happen immediately and if it fails >> > again this >> > repeats - would this cause a busy loop? It seems a lot of scenarios can >> > return >> > errors during snapshot, so I think we'd better be careful. >> >> I agree that we need to be careful. >> >> Regarding this particular case, we will still wait for 1-2 min before >> re-trying, because 'should_snapshot' always checks the 'min' first and >> returns if we didn't surpass it. 'max' checked only if 'min' check >> passed. > > Ok, I see. Sorry that I missed this part when executing the code in my brain > :o)
No worries. :) > >> >> With the current code (without this patch applied), on error the >> 'max' will be updated, but that will only matter if the previous >> failed snapshot attempt was due to hitting the 'max' condition. >> >> Assuming that the database didn't grow a lot and 24 hours passed. >> With the current implementation: >> 1. ovsdb will try to snapshot >> 2. fail >> 3. re-schedule a quick retry updating min and max >> 4. wait 1-2 min (now < min). >> 5. should_snapshot will return 'false' since it didn't grow a lot >> and the updated max is still almost 24 hours ahead. >> 6. checking on every iteration until it grew a lot or 24 hours passed. >> 7. if it didn't grow a lot and 24 hours passed, goto 1. >> >> So, the actual next attempt after the failure in this case will be >> after 24 hours. >> If the reason for a failed snapshot was the db size, 'should_snapshot' >> will return 'true' on the step 5. So it will re-schedule and try to >> snapshot every 1-2 min until it succeds. >> >> With the patch applied we will re-schedule and re-try to create a >> snapshot every 1-2 min regardless of the reason why we wanted to >> create it in the first place. Because now < max will keep being >> false until the snapshot created successfully. >> >> That's the only difference I can think of. >> >> It should be OK to re-try every 1-2 min. What do you think? >> > > Yes, I think it is even better than before. My only concern was busy-loop but > I was wrong. So: > > Acked-by: Han Zhou <[email protected] <mailto:[email protected]>> Thanks! I want to run a couple of tests with ovn-heater before applying the change. Will reply once this is done. > >> > >> > Thanks, >> > Han >> > >> >> >> >> Signed-off-by: Ilya Maximets <[email protected] >> >> <mailto:[email protected]> <mailto:[email protected] >> >> <mailto:[email protected]>>> >> >> --- >> >> ovsdb/storage.c | 17 +++++++++++++++-- >> >> ovsdb/storage.h | 2 +- >> >> 2 files changed, 16 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/ovsdb/storage.c b/ovsdb/storage.c >> >> index 9e32efe58..d4984be25 100644 >> >> --- a/ovsdb/storage.c >> >> +++ b/ovsdb/storage.c >> >> @@ -507,7 +507,11 @@ schedule_next_snapshot(struct ovsdb_storage >> >> *storage, bool quick) >> >> >> >> long long int now = time_msec(); >> >> storage->next_snapshot_min = now + base + random_range(range); >> >> - storage->next_snapshot_max = now + 60LL * 60 * 24 * 1000; /* 1 >> >> day */ >> >> + if (!quick) { >> >> + long long int one_day = 60LL * 60 * 24 * 1000; >> >> + >> >> + storage->next_snapshot_max = now + one_day; >> >> + } >> >> } else { >> >> storage->next_snapshot_min = LLONG_MAX; >> >> storage->next_snapshot_max = LLONG_MAX; >> >> @@ -515,7 +519,7 @@ schedule_next_snapshot(struct ovsdb_storage *storage, >> >> bool quick) >> >> } >> >> >> >> bool >> >> -ovsdb_storage_should_snapshot(const struct ovsdb_storage *storage) >> >> +ovsdb_storage_should_snapshot(struct ovsdb_storage *storage) >> >> { >> >> if (storage->raft || storage->log) { >> >> /* If we haven't reached the minimum snapshot time, don't >> >> snapshot. */ >> >> @@ -544,6 +548,15 @@ ovsdb_storage_should_snapshot(const struct >> >> ovsdb_storage *storage) >> >> } >> >> >> >> if (!snapshot_recommended) { >> >> + if (storage->raft) { >> >> + /* Re-scheduling with a quick retry in order to avoid >> >> condition >> >> + * where all the raft servers passed the minimal time >> >> already, >> >> + * but the log didn't grow a lot, so they are all >> >> checking on >> >> + * every iteration. This will randomize the time of the >> >> next >> >> + * attempt, so all the servers will not start >> >> snapshotting at >> >> + * the same time when the log reaches a critical size. */ >> >> + schedule_next_snapshot(storage, true); >> >> + } >> >> return false; >> >> } >> >> >> >> diff --git a/ovsdb/storage.h b/ovsdb/storage.h >> >> index e120094d7..ff026b77f 100644 >> >> --- a/ovsdb/storage.h >> >> +++ b/ovsdb/storage.h >> >> @@ -76,7 +76,7 @@ uint64_t ovsdb_write_get_commit_index(const struct >> >> ovsdb_write *); >> >> void ovsdb_write_wait(const struct ovsdb_write *); >> >> void ovsdb_write_destroy(struct ovsdb_write *); >> >> >> >> -bool ovsdb_storage_should_snapshot(const struct ovsdb_storage *); >> >> +bool ovsdb_storage_should_snapshot(struct ovsdb_storage *); >> >> struct ovsdb_error *ovsdb_storage_store_snapshot(struct ovsdb_storage >> >> *storage, >> >> const struct json >> >> *schema, >> >> const struct json >> >> *snapshot) >> >> -- >> >> 2.31.1 >> >> >> >> _______________________________________________ >> >> dev mailing list >> >> [email protected] <mailto:[email protected]> >> >> <mailto:[email protected] <mailto:[email protected]>> >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> >> >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
