MB-29928: Reset PID when configuration changes

Update the PID so that it will recheck the configuration
against its internal state.

Whenever one of the terms changes, the PID resets.

Change-Id: I247f6d9d67fd6ef4c2484077a0fe1091dd22ac6e
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/156244
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: jimwwalker

engines/ep/configuration.json

@@ -356,7 +356,7 @@
         "defragmenter_mode" : {
             "default": "auto_pid",
             "descr": "Determines how the defragmenter controls its sleep interval. When static defragmenter_interval is used. When auto_linear, scale the sleep time using a scored defragmentation when it falls between defragmenter_auto_lower_trigger and defragmenter_auto_upper_trigger. When auto_pid use a PID controller to computer reductions in the sleep interval when scored fragmentation is above defragmenter_auto_lower_trigger.",
-            "dynamic": false,
+            "dynamic": true,
             "type": "std::string",
             "validator": {
                 "enum": [
@@ -369,49 +369,49 @@
         "defragmenter_auto_lower_threshold" : {
             "default": "0.07",
             "descr": "When mode is not static and scored fragmentation is above this value, a sleep time between defragmenter_auto_min_sleep and defragmenter_auto_max_sleep will be used",
-            "dynamic": false,
+            "dynamic": true,
             "type": "float"
         },
         "defragmenter_auto_upper_threshold" : {
             "default": "0.25",
             "descr": "When mode is auto_linear and scored fragmentation is above this value, the defragmenter will use defragmenter_auto_min_sleep",
-            "dynamic": false,
+            "dynamic": true,
             "type": "float"
         },
         "defragmenter_auto_max_sleep" : {
             "default": "10.0",
             "descr": "The maximum sleep that the auto controller can set",
-            "dynamic": false,
+            "dynamic": true,
             "type": "float"
         },
         "defragmenter_auto_min_sleep" : {
             "default": "0.0",
             "descr": "The minimum sleep that the auto controller can set",
-            "dynamic": false,
+            "dynamic": true,
             "type": "float"
         },
         "defragmenter_auto_pid_p" : {
             "default": "0.3",
             "descr": "The p term for the PID controller",
-            "dynamic": false,
+            "dynamic": true,
             "type": "float"
         },
         "defragmenter_auto_pid_i" : {
             "default": "0.0000197",
             "descr": "The i term for the PID controller",
-            "dynamic": false,
+            "dynamic": true,
             "type": "float"
         },
         "defragmenter_auto_pid_d" : {
             "default": "0.0",
             "descr": "The d term for the PID controller",
-            "dynamic": false,
+            "dynamic": true,
             "type": "float"
         },
         "defragmenter_auto_pid_dt" : {
             "default": "30000",
             "descr": "The dt (interval) term for the PID controller. Value represents milliseconds",
-            "dynamic": false,
+            "dynamic": true,
             "type": "size_t"
         },
         "durability_timeout_task_interval": {

engines/ep/management/cbepctl

@@ -234,8 +234,26 @@ Available params for "set":
                                    upgrade a NormalWrite to SyncWrite.
     defragmenter_enabled         - Enable or disable the defragmenter
                                    (true/false).
+    defragmenter_mode            - The mode the defragmenter is in static, auto_linear or auto_pid
     defragmenter_interval        - How often defragmenter task should be run
-                                   (in seconds).
+                                   when mode is static (in seconds).
+    defragmenter_auto_lower_threshold - When the mode is not static a 'scored'
+                                        threshold for when sleep time is recalculated.
+                                        (parameter is floating point)
+    defragmenter_auto_upper_threshold - When the mode is auto_linear and 'scored' fragmentation
+                                        exceeds this, defragmenter_auto_max_sleep is used.
+                                        (parameter is floating point)
+    defragmenter_auto_max_sleep  - The minimum sleep time when mode is not
+                                   static (fractional seconds, e.g. 0.2)
+    defragmenter_auto_min_sleep  - The maximum sleep time when mode is not
+                                   static (fractional seconds, e.g. 8.5)
+    defragmenter_auto_pid_p      - The P term for when mode is auto_pid
+                                   (parameter is floating point)
+    defragmenter_auto_pid_i      - The I term for when mode is auto_pid
+                                   (parameter is floating point)
+    defragmenter_auto_pid_d      - The D term for when mode is auto_pid
+                                   (parameter is floating point)
+    defragmenter_auto_pid_dt     - The interval term for when mode is auto_pid
     defragmenter_age_threshold   - How old (measured in number of defragmenter
                                    passes) must a document be to be considered
                                    for defragmentation.

engines/ep/src/defragmenter.cc

@@ -31,7 +31,22 @@ DefragmenterTask::DefragmenterTask(EventuallyPersistentEngine* e,
           engine->getConfiguration().getDefragmenterAutoPidI(),
           engine->getConfiguration().getDefragmenterAutoPidD(),
           std::chrono::milliseconds{
-                  engine->getConfiguration().getDefragmenterAutoPidDt()}) {
+                  engine->getConfiguration().getDefragmenterAutoPidDt()}),
+      pidReset([this](PIDControllerImpl& pid) {
+          const auto& conf = engine->getConfiguration();
+          // Read and compare the PID against the engine config
+          auto sp = conf.getDefragmenterAutoLowerThreshold();
+          auto p = conf.getDefragmenterAutoPidP();
+          auto i = conf.getDefragmenterAutoPidI();
+          auto d = conf.getDefragmenterAutoPidD();
+          auto dt = std::chrono::milliseconds{conf.getDefragmenterAutoPidDt()};
+          if (sp != pid.getSetPoint() || p != pid.getKp() || i != pid.getKi() ||
+              d != pid.getKd() || dt != pid.getDt()) {
+              pid.reset(sp, p, i, d, dt);
+              return true;
+          }
+          return false;
+      }) {
 }
 
 bool DefragmenterTask::run() {
@@ -284,7 +299,7 @@ DefragmenterTask::SleepTimeAndRunState DefragmenterTask::calculateSleepPID(
     // it's below the SP. In this case reset and when we go over again begin
     // the ramping
     if (score < conf.getDefragmenterAutoLowerThreshold()) {
-        // Reset the PID ready for the next time fragmentation increases
+        // Reset the PID ready for the next time fragmentation increases.
         pid.reset();
         return {std::chrono::duration<double>{maxSleep}, false};
     }
@@ -305,5 +320,5 @@ DefragmenterTask::SleepTimeAndRunState DefragmenterTask::calculateSleepPID(
 }
 
 float DefragmenterTask::stepPid(float pv) {
-    return pid.step(pv);
+    return pid.step(pv, pidReset);
 }

engines/ep/src/defragmenter.h

@@ -206,4 +206,7 @@ class DefragmenterTask : public GlobalTask {
      * PID which is used for sleep calculations when deframenter_mode==auto_pid
      */
     PIDController<> pid;
+
+    /// A function so we can reset the PID on config changes
+    std::function<bool(PIDControllerImpl&)> pidReset;
 };

engines/ep/src/ep_engine.cc

@@ -708,6 +708,26 @@ cb::engine_errc EventuallyPersistentEngine::setFlushParam(
                     std::stoull(val));
         } else if (key == "defragmenter_run") {
             runDefragmenterTask();
+        } else if (key == "defragmenter_mode") {
+            getConfiguration().setDefragmenterMode(val);
+        } else if (key == "defragmenter_auto_lower_threshold") {
+            getConfiguration().setDefragmenterAutoLowerThreshold(
+                    std::stof(val));
+        } else if (key == "defragmenter_auto_upper_threshold") {
+            getConfiguration().setDefragmenterAutoUpperThreshold(
+                    std::stof(val));
+        } else if (key == "defragmenter_auto_max_sleep") {
+            getConfiguration().setDefragmenterAutoMaxSleep(std::stof(val));
+        } else if (key == "defragmenter_auto_min_sleep") {
+            getConfiguration().setDefragmenterAutoMinSleep(std::stof(val));
+        } else if (key == "defragmenter_auto_pid_p") {
+            getConfiguration().setDefragmenterAutoPidP(std::stof(val));
+        } else if (key == "defragmenter_auto_pid_i") {
+            getConfiguration().setDefragmenterAutoPidI(std::stof(val));
+        } else if (key == "defragmenter_auto_pid_d") {
+            getConfiguration().setDefragmenterAutoPidD(std::stof(val));
+        } else if (key == "defragmenter_auto_pid_dt") {
+            getConfiguration().setDefragmenterAutoPidDt(std::stof(val));
         } else if (key == "compaction_write_queue_cap") {
             getConfiguration().setCompactionWriteQueueCap(std::stoull(val));
         } else if (key == "chk_expel_enabled") {

engines/ep/src/pid_controller.cc

@@ -23,7 +23,9 @@ PIDControllerImpl::PIDControllerImpl(
 }
 
 float PIDControllerImpl::step(
-        float current, std::chrono::time_point<std::chrono::steady_clock> now) {
+        float current,
+        std::chrono::time_point<std::chrono::steady_clock> now,
+        std::function<bool(PIDControllerImpl&)> checkAndMaybeReset) {
     auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(
             now - lastStep);
 
@@ -33,6 +35,10 @@ float PIDControllerImpl::step(
         return output;
     }
 
+    // Invoke this function now which is intended to allow the PID owner to
+    // reset the PID if configuration has changed.
+    (void)checkAndMaybeReset(*this);
+
     float error = setPoint - current;
 
     integral += (error * duration.count());
@@ -46,6 +52,19 @@ float PIDControllerImpl::step(
     return output;
 }
 
+void PIDControllerImpl::reset(float setPoint,
+                              float kp,
+                              float ki,
+                              float kd,
+                              std::chrono::milliseconds dt) {
+    this->setPoint = setPoint;
+    this->kp = kp;
+    this->ki = ki;
+    this->kd = kd;
+    this->dt = dt;
+    reset();
+}
+
 void PIDControllerImpl::reset() {
     integral = 0;
     previousError = 0;

engines/ep/src/pid_controller.h

@@ -12,6 +12,7 @@
 #pragma once
 
 #include <chrono>
+#include <functional>
 #include <iosfwd>
 
 /**
@@ -55,17 +56,57 @@ class PIDControllerImpl {
      *
      * @param current The current reading of the process variable
      * @param now The time point for 'now'
+     * @param checkAndMaybeReset a function that is called before the PID
+     *        generates a new output. The purpose of this function is to allow
+     *        the caller to examine SP, P, I, D and dt terms and maybe reset.
      * @return the PID's computed output
      */
     float step(float current,
-               std::chrono::time_point<std::chrono::steady_clock> now);
+               std::chrono::time_point<std::chrono::steady_clock> now,
+               std::function<bool(PIDControllerImpl&)> checkAndMaybeReset);
+
+    /**
+     * Reset the PID state.
+     * The setPoint, P, I, D and dt are all set to the given values
+     * The current integral, previousError and output are all set to zero.
+     */
+    void reset(float setPoint,
+               float kp,
+               float ki,
+               float kd,
+               std::chrono::milliseconds dt);
 
     /**
      * Reset the PID state.
      * The current integral, previousError and output are all set to zero.
      */
     void reset();
 
+    /// @return the current P term
+    float getKp() const {
+        return kp;
+    }
+
+    /// @return the current I term
+    float getKi() const {
+        return ki;
+    }
+
+    /// @return the current D term
+    float getKd() const {
+        return kd;
+    }
+
+    /// @return the current Dt term
+    std::chrono::milliseconds getDt() const {
+        return dt;
+    }
+
+    /// @return the current set point
+    float getSetPoint() const {
+        return setPoint;
+    }
+
 private:
     float kp{0.0};
     float ki{0.0};
@@ -101,8 +142,21 @@ class PIDController {
      * @param current The current reading of the process variable
      * @return the PID's computed output
      */
-    float step(float current) {
-        return impl.step(current, Clock::now());
+    float step(float current,
+               std::function<bool(PIDControllerImpl&)> checkAndMaybeReset) {
+        return impl.step(current, Clock::now(), checkAndMaybeReset);
+    }
+
+    /**
+     * Reset the PID state.
+     * The current integral, previousError and output are all set to zero.
+     */
+    void reset(float setPoint,
+               float kp,
+               float ki,
+               float kd,
+               std::chrono::milliseconds dt) {
+        impl.reset(setPoint, kp, ki, kd, dt);
     }
 
     /**
@@ -113,6 +167,11 @@ class PIDController {
         impl.reset();
     }
 
+    bool checkAndMaybeReset(
+            std::function<bool(PIDControllerImpl&)> checkAndMaybeResetFn) {
+        return checkAndMaybeResetFn(impl);
+    }
+
 private:
     PIDControllerImpl impl;
 

engines/ep/tests/module_tests/defragmenter_test.cc

@@ -432,7 +432,7 @@ class MockDefragmenterTask : public DefragmenterTask {
 
     // override stepPid so we can move time
     float stepPid(float pv) override {
-        return testPid.step(pv);
+        return testPid.step(pv, pidReset);
     }
 
     PIDController<MockDefragmenterTaskClock> testPid;
@@ -490,6 +490,20 @@ TEST_F(DefragmenterTaskTest, autoCalculateSleep_PID) {
     state = task->public_calculateSleepPID(cb::FragmentationStats{10, 5000});
     EXPECT_EQ(conf.getDefragmenterAutoMinSleep(), state.sleepTime.count());
     EXPECT_TRUE(state.runDefragger);
+
+    // Finally reconfigure.
+    // Test the PID spots a config change - let's set P I D to 0, so the PID
+    // returns 0 on the next step meaning it will now move back to maxSleep
+    conf.setDefragmenterAutoPidP(0.0);
+    conf.setDefragmenterAutoPidI(0.0);
+    conf.setDefragmenterAutoPidD(0.0);
+
+    // High fragmentation yet PID is 'disabled' so returns max sleep
+    state = task->public_calculateSleepPID(cb::FragmentationStats{400, 600});
+    EXPECT_EQ(conf.getDefragmenterAutoMaxSleep(), state.sleepTime.count());
+
+    // The high fragmentation so the defragger should run
+    EXPECT_TRUE(state.runDefragger);
 }
 
 TEST_F(DefragmenterTaskTest, autoCalculateSleep) {

engines/ep/tools/pid_runner.cc

@@ -134,7 +134,7 @@ int main(int argc, char* argv[]) {
         PIDController<FauxClock> pid{
                 setpoint, p, i, d, std::chrono::milliseconds{interval}};
         for (int step = 0; step < steps; step++) {
-            auto c = pid.step(pv);
+            auto c = pid.step(pv, [](PIDControllerImpl&) { return false; });
 
             // Compute the sleep like the defragger does
             auto sleep = std::max(max + c, min);