On Mon, Dec 13, 2021 at 10:37 AM Ilya Maximets <[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]>> 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) > > 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]> > > > > 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
