fix the drift of the start time of backup policy

Summary: Ref T9978

Test Plan: N/A

Reviewers: qinzuoyan, sunweijie, heyuchen, wutao1, laiyingchun

Reviewed By: sunweijie

Subscribers: #pegasus

Maniphest Tasks: T9978

Differential Revision: https://phabricator.d.xiaomi.net/D79520

Conflicts:
	src/tests/dsn/fds_service_test.cpp

src/dist/replication/meta_server/meta_backup_service.cpp

@@ -644,6 +644,60 @@ void policy_context::continue_current_backup_unlocked()
     }
 }
 
+bool policy_context::should_start_backup_unlocked()
+{
+    uint64_t now = dsn_now_ms();
+    uint64_t recent_backup_start_time_ms = 0;
+    if (!_backup_history.empty()) {
+        recent_backup_start_time_ms = _backup_history.rbegin()->second.start_time_ms;
+    }
+
+    // the true start time of recent backup have drifted away with the origin start time of
+    // policy,
+    // so we should take the drift into consideration; if user change the start time of the
+    // policy,
+    // we just think the change of start time as drift
+    int32_t hour = 0, min = 0, sec = 0;
+    if (recent_backup_start_time_ms == 0) {
+        //  the first time to backup, just consider the start time
+        ::dsn::utils::time_ms_to_date_time(now, hour, min, sec);
+        return _policy.start_time.should_start_backup(hour, min);
+    } else {
+        uint64_t next_backup_time_ms =
+            recent_backup_start_time_ms + _policy.backup_interval_seconds * 1000;
+        if (_policy.start_time.hour != 24) {
+            // user have specify the time point to start backup, so we should take the the
+            // time-drift into consideration
+
+            // compute the time-drift
+            ::dsn::utils::time_ms_to_date_time(recent_backup_start_time_ms, hour, min, sec);
+            int64_t time_dirft_ms = _policy.start_time.compute_time_drift_ms(hour, min);
+
+            if (time_dirft_ms >= 0) {
+                // hour:min(the true start time) >= policy.start_time :
+                //      1, user move up the start time of policy, such as 20:00 to 2:00, we just
+                //      think this case as time drift
+                //      2, the true start time of backup is delayed, compared the origin start time
+                //      of policy, we should process this case
+                //      3, the true start time of backup is the same with the origin start time of
+                //      policy
+                next_backup_time_ms -= time_dirft_ms;
+            } else {
+                // hour:min(the true start time) < policy.start_time:
+                //      1, user delay the start time of policy, such as 2:00 to 23:00
+                //
+                // these case has already been handled, we do nothing
+            }
+        }
+        if (next_backup_time_ms <= now) {
+            ::dsn::utils::time_ms_to_date_time(now, hour, min, sec);
+            return _policy.start_time.should_start_backup(hour, min);
+        } else {
+            return false;
+        }
+    }
+}
+
 void policy_context::issue_new_backup_unlocked()
 {
     // before issue new backup, we check whether the policy is dropped
@@ -661,27 +715,8 @@ void policy_context::issue_new_backup_unlocked()
             std::chrono::milliseconds(_backup_service->backup_option().issue_backup_interval_ms));
         return;
     }
-    uint64_t now = dsn_now_ms();
-    int32_t hour = 0, min = 0, sec = 0;
-    dsn::utils::time_ms_to_date_time(now, hour, min, sec);
-    bool issue_backup_later = false;
-    if (!_backup_history.empty()) {
-        const backup_info &recent_backup = _backup_history.rbegin()->second;
-        uint64_t next_backup_moment =
-            recent_backup.start_time_ms + _policy.backup_interval_seconds * 1000;
-        if (next_backup_moment > now) {
-            issue_backup_later = true;
-        } else if (!_policy.start_time.should_start_backup(hour, min, sec)) {
-            issue_backup_later = true;
-        }
-    } else {
-        if (!_policy.start_time.should_start_backup(hour, min, sec)) {
-            issue_backup_later = true;
-        } else {
-            ddebug("%s: start first backup for this policy", _policy.policy_name.c_str());
-        }
-    }
-    if (issue_backup_later) {
+
+    if (!should_start_backup_unlocked()) {
         tasking::enqueue(
             LPC_DEFAULT_CALLBACK,
             nullptr,

src/dist/replication/meta_server/meta_backup_service.h

@@ -57,6 +57,9 @@ struct backup_info
 
 // Attention: backup_start_time == 24:00 is represent no limit for start_time, 24:00 is mainly saved
 // for testing
+//
+// current, we don't support accurating to minute, only support accurating to hour, so
+// we just set minute to 0
 struct backup_start_time
 {
     int32_t hour;   // [0 ~24)
@@ -95,20 +98,32 @@ struct backup_start_time
         }
         return true;
     }
+
+    // return the interval between new_hour:new_min and start_time,
+    // namely new_hour:new_min - start_time;
+    // unit is ms
+    int64_t compute_time_drift_ms(int32_t new_hour, int32_t new_min)
+    {
+        int64_t res = 0;
+        // unit is hour
+        res += (new_hour - hour);
+        // unit is minute
+        res *= 60;
+        res += (new_min - minute);
+        // unit is ms
+        return (res * 60 * 1000);
+    }
+
     // judge whether we should start backup base current time
-    bool should_start_backup(int32_t cur_hour, int32_t cur_min, int32_t cur_sec)
+    bool should_start_backup(int32_t cur_hour, int32_t cur_min)
     {
         if (hour == 24) {
             // erase the restrict of backup_start_time, just for testing
             return true;
         }
-        // NOTICE : if you want more precisely, you can use cur_min and cur_sec to implement
+        // NOTICE : if you want more precisely, you can use cur_min to implement
         // now, we just ignore
-        if (cur_hour == hour) {
-            return true;
-        } else {
-            return false;
-        }
+        return (cur_hour == hour);
     }
     DEFINE_JSON_SERIALIZATION(hour, minute)
 };
@@ -237,6 +252,9 @@ mock_private :
     mock_virtual void initialize_backup_progress_unlocked();
     mock_virtual void prepare_current_backup_on_new_unlocked();
     mock_virtual void issue_new_backup_unlocked();
+    // returns:
+    //  - true, should start backup right now, otherwise don't start backup
+    mock_virtual bool should_start_backup_unlocked();
     mock_virtual void continue_current_backup_unlocked();
 
     mock_virtual void on_backup_reply(dsn::error_code err,

src/dist/replication/test/meta_test/unit_test/backup_test.cpp

@@ -518,6 +518,115 @@ void meta_service_test_app::policy_context_test()
             ->wait();
         ASSERT_EQ(dsn::ERR_OK, ec);
     }
+
+    // test should_start_backup_unlock()
+    {
+        std::cout << "test should_start_backup_unlock()" << std::endl;
+        uint64_t now = dsn_now_ms();
+        int32_t hour = 0, min = 0, sec = 0;
+        ::dsn::utils::time_ms_to_date_time(now, hour, min, sec);
+        while (min == 59) {
+            std::this_thread::sleep_for(std::chrono::minutes(1));
+            now = dsn_now_ms();
+            ::dsn::utils::time_ms_to_date_time(now, hour, min, sec);
+        }
+
+        int64_t oneday_sec = 1 * 24 * 60 * 60;
+        mp._policy.start_time.hour = hour;
+        mp._policy.start_time.minute = 0;
+        mp._policy.backup_interval_seconds = oneday_sec; // oneday
+        mp._backup_history.clear();
+
+        backup_info info;
+
+        {
+            std::cout << "first backup & no limit to start_time" << std::endl;
+            mp._policy.start_time.hour = 24;
+            ASSERT_TRUE(mp.should_start_backup_unlocked());
+        }
+
+        {
+            std::cout << "first backup & cur_time.hour == start_time.hour" << std::endl;
+            ASSERT_TRUE(mp.should_start_backup_unlocked());
+        }
+
+        {
+
+            std::cout << "first backup & cur_time.hour != start_time.hour" << std::endl;
+            mp._policy.start_time.hour = hour + 100;
+            ASSERT_FALSE(mp.should_start_backup_unlocked());
+            mp._policy.start_time.hour = 25 - hour;
+            ASSERT_FALSE(mp.should_start_backup_unlocked());
+        }
+
+        {
+            std::cout << "not first backup & recent backup delay 20min to start" << std::endl;
+            info.start_time_ms = now - (oneday_sec * 1000) + 20 * 60 * 1000;
+            info.end_time_ms = info.start_time_ms + 10;
+            mp.add_backup_history(info);
+            // if we set start_time to 24:00, then will not start backup
+            mp._policy.start_time.hour = 24;
+            ASSERT_FALSE(mp.should_start_backup_unlocked());
+            // if we set start_time to hour:00, then will start backup, even if the interval <
+            // policy.backup_interval
+            mp._policy.start_time.hour = hour;
+            ASSERT_TRUE(mp.should_start_backup_unlocked());
+        }
+
+        {
+            std::cout << "not first backup & recent backup start time is equal with start_time"
+                      << std::endl;
+            mp._policy.start_time.hour = hour;
+            mp._backup_history.clear();
+            info.start_time_ms = now - (oneday_sec * 1000) - (min * 60 * 1000);
+            info.start_time_ms = (info.start_time_ms / 1000) * 1000;
+            info.end_time_ms = info.start_time_ms + 10;
+            mp.add_backup_history(info);
+            ASSERT_TRUE(mp.should_start_backup_unlocked());
+        }
+
+        {
+            // delay the start_time
+            std::cout << "not first backup & delay the start time of policy" << std::endl;
+            mp._policy.start_time.hour = hour + 1;
+            mp._backup_history.clear();
+            // make sure the start time of recent backup is litte than policy's start_time, so we
+            // minus more 3min
+            info.start_time_ms = now - (oneday_sec * 1000) - 3 * 60 * 1000;
+            info.end_time_ms = info.start_time_ms + 10;
+            mp.add_backup_history(info);
+            if (mp._policy.start_time.hour == 24) {
+                // if hour = 23, then policy.start_time.hour = 24, we should start next backup,
+                // because now - info.start_time_ms > policy.backup_interval
+                ASSERT_TRUE(mp.should_start_backup_unlocked());
+            } else {
+                // should not start, even if now - info.start_time_ms > policy.backup_interval, but
+                // not reach the time-point that policy.start_time limit
+                ASSERT_FALSE(mp.should_start_backup_unlocked());
+            }
+        }
+
+        {
+            std::cout << "not first backup & no limit to start time & should start backup"
+                      << std::endl;
+            mp._policy.start_time.hour = 24;
+            mp._backup_history.clear();
+            info.start_time_ms = now - (oneday_sec * 1000) - 3 * 60 * 60;
+            info.end_time_ms = info.start_time_ms + 10;
+            mp.add_backup_history(info);
+            ASSERT_TRUE(mp.should_start_backup_unlocked());
+        }
+
+        {
+            std::cout << "not first backup & no limit to start time & should not start backup"
+                      << std::endl;
+            mp._backup_history.clear();
+            info.start_time_ms = now - (oneday_sec * 1000) + 3 * 60 * 60;
+            info.end_time_ms = info.start_time_ms + 10;
+            mp.add_backup_history(info);
+            ASSERT_FALSE(mp.should_start_backup_unlocked());
+        }
+    }
 }
 
 void meta_service_test_app::backup_service_test()