Improve SystemRuleManager and refactor checking logic of maxCpuUsage strategy (#1050)

- The BBR period for maxCpuUsage strategy is deprecated. Now Sentinel will just check the CPU usage value, which could avert surge of CPU usage quickly
- Improve SystemRuleManager and SystemStatusListener
- Add some test cases

Signed-off-by: Eric Zhao <sczyh16@gmail.com>

sentinel-core/src/main/java/com/alibaba/csp/sentinel/slots/system/SystemRuleManager.java

@@ -63,7 +63,7 @@ import com.alibaba.csp.sentinel.slots.block.BlockException;
  * @author jialiang.linjl
  * @author leyou
  */
-public class SystemRuleManager {
+public final class SystemRuleManager {
 
     private static volatile double highestSystemLoad = Double.MAX_VALUE;
     /**
@@ -95,7 +95,7 @@ public class SystemRuleManager {
     static {
         checkSystemStatus.set(false);
         statusListener = new SystemStatusListener();
-        scheduler.scheduleAtFixedRate(statusListener, 5, 1, TimeUnit.SECONDS);
+        scheduler.scheduleAtFixedRate(statusListener, 0, 1, TimeUnit.SECONDS);
         currentProperty.addListener(listener);
     }
 
@@ -107,6 +107,7 @@ public class SystemRuleManager {
      */
     public static void register2Property(SentinelProperty<List<SystemRule>> property) {
         synchronized (listener) {
+            RecordLog.info("[SystemRuleManager] Registering new property to system rule manager");
             currentProperty.removeListener(listener);
             property.addListener(listener);
             currentProperty = property;
@@ -167,26 +168,22 @@ public class SystemRuleManager {
         return result;
     }
 
-    public static double getQps() {
+    public static double getInboundQpsThreshold() {
         return qps;
     }
 
-    public static void setQps(double qps) {
+    public static long getRtThreshold() {
-        SystemRuleManager.qps = qps;
-    }
-
-    public static long getMaxRt() {
         return maxRt;
     }
 
-    public static long getMaxThread() {
+    public static long getMaxThreadThreshold() {
         return maxThread;
     }
 
     static class SystemPropertyListener extends SimplePropertyListener<List<SystemRule>> {
 
         @Override
-        public void configUpdate(List<SystemRule> rules) {
+        public synchronized void configUpdate(List<SystemRule> rules) {
             restoreSetting();
             // systemRules = rules;
             if (rules != null && rules.size() >= 1) {
@@ -234,14 +231,10 @@ public class SystemRuleManager {
         return checkSystemStatus.get();
     }
 
-    public static double getHighestSystemLoad() {
+    public static double getSystemLoadThreshold() {
         return highestSystemLoad;
     }
 
-    public static void setHighestSystemLoad(double highestSystemLoad) {
-        SystemRuleManager.highestSystemLoad = highestSystemLoad;
-    }
-    
     public static double getCpuUsageThreshold() {
         return highestCpuUsage;
     }
@@ -257,9 +250,14 @@ public class SystemRuleManager {
         }
 
         if (rule.getHighestCpuUsage() >= 0) {
-            highestCpuUsage = Math.min(highestCpuUsage, rule.getHighestCpuUsage());
+            if (rule.getHighestCpuUsage() > 1) {
-            highestCpuUsageIsSet = true;
+                RecordLog.warn(String.format("[SystemRuleManager] Ignoring invalid SystemRule: "
-            checkStatus = true;
+                    + "highestCpuUsage %.3f > 1", rule.getHighestCpuUsage()));
+            } else {
+                highestCpuUsage = Math.min(highestCpuUsage, rule.getHighestCpuUsage());
+                highestCpuUsageIsSet = true;
+                checkStatus = true;
+            }
         }
 
         if (rule.getAvgRt() >= 0) {
@@ -290,6 +288,9 @@ public class SystemRuleManager {
      * @throws BlockException when any system rule's threshold is exceeded.
      */
     public static void checkSystem(ResourceWrapper resourceWrapper) throws BlockException {
+        if (resourceWrapper == null) {
+            return;
+        }
         // Ensure the checking switch is on.
         if (!checkSystemStatus.get()) {
             return;
@@ -326,9 +327,7 @@ public class SystemRuleManager {
 
         // cpu usage
         if (highestCpuUsageIsSet && getCurrentCpuUsage() > highestCpuUsage) {
-            if (!checkBbr(currentThread)) {
+            throw new SystemBlockException(resourceWrapper.getName(), "cpu");
-                throw new SystemBlockException(resourceWrapper.getName(), "cpu");
-            }
         }
     }
 
@@ -347,4 +346,4 @@ public class SystemRuleManager {
     public static double getCurrentCpuUsage() {
         return statusListener.getCpuUsage();
     }
-}
+}

sentinel-core/src/main/java/com/alibaba/csp/sentinel/slots/system/SystemStatusListener.java

@@ -46,32 +46,35 @@ public class SystemStatusListener implements Runnable {
         try {
             OperatingSystemMXBean osBean = ManagementFactory.getPlatformMXBean(OperatingSystemMXBean.class);
             currentLoad = osBean.getSystemLoadAverage();
-            /**
+            /*
              * Java Doc copied from {@link OperatingSystemMXBean#getSystemCpuLoad()}:</br>
              * Returns the "recent cpu usage" for the whole system. This value is a double in the [0.0,1.0] interval.
              * A value of 0.0 means that all CPUs were idle during the recent period of time observed, while a value
              * of 1.0 means that all CPUs were actively running 100% of the time during the recent period being
-             * observed. All values betweens 0.0 and 1.0 are possible depending of the activities going on in the
+             * observed. All values between 0.0 and 1.0 are possible depending of the activities going on in the
              * system. If the system recent cpu usage is not available, the method returns a negative value.
              */
             currentCpuUsage = osBean.getSystemCpuLoad();
 
-            StringBuilder sb = new StringBuilder();
+            if (currentLoad > SystemRuleManager.getSystemLoadThreshold()) {
-            if (currentLoad > SystemRuleManager.getHighestSystemLoad()) {
+                writeSystemStatusLog();
-                sb.append("load:").append(currentLoad).append(";");
-                sb.append("cpu:").append(currentCpuUsage).append(";");
-                sb.append("qps:").append(Constants.ENTRY_NODE.passQps()).append(";");
-                sb.append("rt:").append(Constants.ENTRY_NODE.avgRt()).append(";");
-                sb.append("thread:").append(Constants.ENTRY_NODE.curThreadNum()).append(";");
-                sb.append("success:").append(Constants.ENTRY_NODE.successQps()).append(";");
-                sb.append("minRt:").append(Constants.ENTRY_NODE.minRt()).append(";");
-                sb.append("maxSuccess:").append(Constants.ENTRY_NODE.maxSuccessQps()).append(";");
-                RecordLog.info(sb.toString());
             }
-
         } catch (Throwable e) {
-            RecordLog.info("could not get system error ", e);
+            RecordLog.warn("[SystemStatusListener] Failed to get system metrics from JMX", e);
         }
     }
 
+    private void writeSystemStatusLog() {
+        StringBuilder sb = new StringBuilder();
+        sb.append("Load exceeds the threshold: ");
+        sb.append("load:").append(String.format("%.4f", currentLoad)).append("; ");
+        sb.append("cpuUsage:").append(String.format("%.4f", currentCpuUsage)).append("; ");
+        sb.append("qps:").append(String.format("%.4f", Constants.ENTRY_NODE.passQps())).append("; ");
+        sb.append("rt:").append(String.format("%.4f", Constants.ENTRY_NODE.avgRt())).append("; ");
+        sb.append("thread:").append(Constants.ENTRY_NODE.curThreadNum()).append("; ");
+        sb.append("success:").append(String.format("%.4f", Constants.ENTRY_NODE.successQps())).append("; ");
+        sb.append("minRt:").append(String.format("%.2f", Constants.ENTRY_NODE.minRt())).append("; ");
+        sb.append("maxSuccess:").append(String.format("%.2f", Constants.ENTRY_NODE.maxSuccessQps())).append("; ");
+        RecordLog.info(sb.toString());
+    }
 }

sentinel-core/src/test/java/com/alibaba/csp/sentinel/slots/system/SystemRuleManagerTest.java

@@ -0,0 +1,111 @@
+/*
+ * Copyright 1999-2018 Alibaba Group Holding Ltd.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.alibaba.csp.sentinel.slots.system;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+import com.alibaba.csp.sentinel.EntryType;
+import com.alibaba.csp.sentinel.slotchain.StringResourceWrapper;
+import com.alibaba.csp.sentinel.slots.block.BlockException;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+/**
+ * @author Eric Zhao
+ */
+public class SystemRuleManagerTest {
+
+    @Test
+    public void testLoadInvalidRules() {
+        SystemRule rule1 = new SystemRule();
+        rule1.setHighestSystemLoad(-0.9d);
+        SystemRule rule2 = new SystemRule();
+        rule2.setHighestCpuUsage(2.7d);
+        SystemRuleManager.loadRules(Arrays.asList(rule1, rule2));
+        assertEquals(0, SystemRuleManager.getRules().size());
+    }
+
+    @Test
+    public void testLoadAndGetRules() {
+        SystemRule rule1 = new SystemRule();
+        rule1.setHighestSystemLoad(1.2d);
+        SystemRule rule2 = new SystemRule();
+        rule2.setMaxThread(17);
+        SystemRule rule3 = new SystemRule();
+        rule3.setHighestCpuUsage(0.7d);
+        SystemRule rule4 = new SystemRule();
+        rule4.setQps(1500);
+        SystemRule rule5 = new SystemRule();
+        rule5.setAvgRt(50);
+        SystemRuleManager.loadRules(Arrays.asList(rule1, rule2, rule3, rule4, rule5));
+        assertEquals(1.2d, SystemRuleManager.getSystemLoadThreshold(), 0.01);
+        assertEquals(17, SystemRuleManager.getMaxThreadThreshold());
+        assertEquals(0.7d, SystemRuleManager.getCpuUsageThreshold(), 0.01);
+        assertEquals(1500, SystemRuleManager.getInboundQpsThreshold(), 0.01);
+        assertEquals(50, SystemRuleManager.getRtThreshold());
+    }
+
+    @Test
+    public void testLoadDuplicateTypeOfRules() {
+        SystemRule rule1 = new SystemRule();
+        rule1.setHighestSystemLoad(1.2d);
+        SystemRule rule2 = new SystemRule();
+        rule2.setHighestSystemLoad(2.3d);
+        SystemRule rule3 = new SystemRule();
+        rule3.setHighestSystemLoad(3.4d);
+        SystemRuleManager.loadRules(Arrays.asList(rule1, rule2, rule3));
+
+        List<SystemRule> rules = SystemRuleManager.getRules();
+        assertEquals(1, rules.size());
+        assertEquals(1.2d, rules.get(0).getHighestSystemLoad(), 0.01);
+        assertEquals(1.2d, SystemRuleManager.getSystemLoadThreshold(), 0.01);
+    }
+
+    @Test
+    public void testCheckMaxCpuUsageNotBBR() throws Exception {
+        SystemRule rule1 = new SystemRule();
+        rule1.setHighestCpuUsage(0d);
+        SystemRuleManager.loadRules(Collections.singletonList(rule1));
+
+        // Wait until SystemStatusListener triggered the first CPU usage collecting.
+        Thread.sleep(1500);
+
+        boolean blocked = false;
+        try {
+            SystemRuleManager.checkSystem(new StringResourceWrapper("testCheckMaxCpuUsageNotBBR", EntryType.IN));
+        } catch (BlockException ex) {
+            blocked = true;
+        }
+        assertTrue("The entry should be blocked under SystemRule maxCpuUsage=0", blocked);
+    }
+
+    @Before
+    public void setUp() throws Exception {
+        SystemRuleManager.loadRules(new ArrayList<SystemRule>());
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        SystemRuleManager.loadRules(new ArrayList<SystemRule>());
+    }
+}