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]>> 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. 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? > > Thanks, > Han > >> >> Signed-off-by: Ilya Maximets <[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]> >> 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
