HBASE-29965: Unable to dynamically change readonly flag

hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java

@@ -18,6 +18,8 @@
 package org.apache.hadoop.hbase.master;
 
 import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_COORDINATED_BY_ZK;
+import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT;
+import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY;
 import static org.apache.hadoop.hbase.HConstants.HBASE_MASTER_LOGCLEANER_PLUGINS;
 import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_COORDINATED_BY_ZK;
 import static org.apache.hadoop.hbase.master.cleaner.HFileCleaner.CUSTOM_POOL_SIZE;
@@ -498,6 +500,8 @@ public class HMaster extends HBaseServerBase<MasterRpcServices> implements Maste
   public static final String WARMUP_BEFORE_MOVE = "hbase.master.warmup.before.move";
   private static final boolean DEFAULT_WARMUP_BEFORE_MOVE = true;
 
+  private volatile boolean isGlobalReadOnlyEnabled;
+
   /**
    * Use RSProcedureDispatcher instance to initiate master -> rs remote procedure execution. Use
    * this config to extend RSProcedureDispatcher (mainly for testing purpose).
@@ -583,6 +587,8 @@ public HMaster(final Configuration conf) throws IOException {
           getChoreService().scheduleChore(clusterStatusPublisherChore);
         }
       }
+      this.isGlobalReadOnlyEnabled =
+        conf.getBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
       this.activeMasterManager = createActiveMasterManager(zooKeeper, serverName, this);
       cachedClusterId = new CachedClusterId(this, conf);
       this.regionServerTracker = new RegionServerTracker(zooKeeper, this);
@@ -1090,8 +1096,7 @@ private void finishActiveMasterInitialization() throws IOException, InterruptedE
     if (!maintenanceMode) {
       startupTaskGroup.addTask("Initializing master coprocessors");
       setQuotasObserver(conf);
-      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(
-        ConfigurationUtil.isReadOnlyModeEnabled(conf), conf,
+      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf,
         CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
       AbstractReadOnlyController.manageActiveClusterIdFile(
         ConfigurationUtil.isReadOnlyModeEnabled(conf), this.getMasterFileSystem());
@@ -4501,21 +4506,35 @@ public void onConfigurationChange(Configuration newConf) {
     // append the quotas observer back to the master coprocessor key
     setQuotasObserver(newConf);
 
-    boolean readOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf);
-    CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, newConf,
-      CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
+    boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf);
+    boolean hasReadOnlyModeChanged = this.isGlobalReadOnlyEnabled != maybeUpdatedReadOnlyMode;
+    boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil
+      .checkConfigurationChange(this.cpHost, newConf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
+    boolean shouldUpdateCoprocessors =
+      (hasCoprocessorConfigChanged || hasReadOnlyModeChanged) && !maintenanceMode;
 
     // update region server coprocessor if the configuration has changed.
-    if (
-      CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost, newConf,
-        CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-    ) {
+    if (shouldUpdateCoprocessors) {
+      Set<String> currentlyLoadedCps = this.cpHost.getCoprocessorClassNames();
+      LOG.debug("About to update coprocessors loaded on HMaster {}. These are the current "
+        + "coprocessors before updating: {}", this, currentlyLoadedCps);
+
       LOG.info("Update the master coprocessor(s) because the configuration has changed");
-      initializeCoprocessorHost(newConf);
-      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, this.conf,
+      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf,
         CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
-      AbstractReadOnlyController.manageActiveClusterIdFile(
-        ConfigurationUtil.isReadOnlyModeEnabled(newConf), this.getMasterFileSystem());
+      initializeCoprocessorHost(newConf);
+
+      currentlyLoadedCps = this.cpHost.getCoprocessorClassNames();
+      LOG.debug("Finished updating coprocessors on HMaster {}. These are the coprocessors "
+        + "after updating: {}", this, currentlyLoadedCps);
+    }
+
+    if (hasReadOnlyModeChanged) {
+      this.isGlobalReadOnlyEnabled = maybeUpdatedReadOnlyMode;
+      LOG.info("Config {} has been dynamically changed to {} for HMaster {}",
+        HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, this.isGlobalReadOnlyEnabled, this);
+      AbstractReadOnlyController.manageActiveClusterIdFile(this.isGlobalReadOnlyEnabled,
+        this.getMasterFileSystem());
     }
   }
 

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java

@@ -17,6 +17,8 @@
  */
 package org.apache.hadoop.hbase.regionserver;
 
+import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT;
+import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY;
 import static org.apache.hadoop.hbase.HConstants.REPLICATION_SCOPE_LOCAL;
 import static org.apache.hadoop.hbase.regionserver.HStoreFile.MAJOR_COMPACTION_KEY;
 import static org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.REGION_NAMES_KEY;
@@ -391,6 +393,8 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
   private Path regionWalDir;
   private FileSystem walFS;
 
+  private volatile boolean isGlobalReadOnlyEnabled;
+
   // set to true if the region is restored from snapshot for reading by ClientSideRegionScanner
   private boolean isRestoredRegion = false;
 
@@ -941,8 +945,9 @@ public HRegion(final HRegionFileSystem fs, final WAL wal, final Configuration co
 
     decorateRegionConfiguration(conf);
 
-    CoprocessorConfigurationUtil.syncReadOnlyConfigurations(
-      ConfigurationUtil.isReadOnlyModeEnabled(conf), this.conf,
+    this.isGlobalReadOnlyEnabled =
+      conf.getBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+    CoprocessorConfigurationUtil.syncReadOnlyConfigurations(this.conf,
       CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
 
     if (rsServices != null) {
@@ -8515,7 +8520,7 @@ public boolean registerService(Service instance) {
     ServiceDescriptor serviceDesc = instance.getDescriptorForType();
     String serviceName = CoprocessorRpcUtils.getServiceName(serviceDesc);
     if (coprocessorServiceHandlers.containsKey(serviceName)) {
-      LOG.error("Coprocessor service {} already registered, rejecting request from {} in region {}",
+      LOG.warn("Coprocessor service {} already registered, rejecting request from {} in region {}",
         serviceName, instance, this);
       return false;
     }
@@ -8986,24 +8991,37 @@ IOException throwOnInterrupt(Throwable t) {
    * {@inheritDoc}
    */
   @Override
-  public void onConfigurationChange(Configuration conf) {
-    this.storeHotnessProtector.update(conf);
+  public void onConfigurationChange(Configuration newConf) {
+    this.storeHotnessProtector.update(newConf);
 
-    boolean readOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(conf);
-    CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, conf,
-      CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
+    boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf);
+    boolean hasReadOnlyModeChanged = this.isGlobalReadOnlyEnabled != maybeUpdatedReadOnlyMode;
+    boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil.checkConfigurationChange(
+      this.coprocessorHost, newConf, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY,
+      CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY);
+    boolean shouldUpdateCoprocessors = hasCoprocessorConfigChanged || hasReadOnlyModeChanged;
 
     // update coprocessorHost if the configuration has changed.
-    if (
-      CoprocessorConfigurationUtil.checkConfigurationChange(this.coprocessorHost, conf,
-        CoprocessorHost.REGION_COPROCESSOR_CONF_KEY,
-        CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY)
-    ) {
+    if (shouldUpdateCoprocessors) {
+      Set<String> currentlyLoadedCps = this.coprocessorHost.getCoprocessorClassNames();
+      LOG.trace("About to update coprocessors loaded on HRegion {}. These are the current "
+        + "coprocessors before updating: {}", this, currentlyLoadedCps);
+
       LOG.info("Update the system coprocessors because the configuration has changed");
-      decorateRegionConfiguration(conf);
-      this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, conf);
-      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, this.conf,
+      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf,
         CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
+      decorateRegionConfiguration(newConf);
+      this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, newConf);
+
+      currentlyLoadedCps = this.coprocessorHost.getCoprocessorClassNames();
+      LOG.trace("Finished updating coprocessors on HRegion {}. These are the coprocessors "
+        + "after updating: {}", this, currentlyLoadedCps);
+    }
+
+    if (hasReadOnlyModeChanged) {
+      this.isGlobalReadOnlyEnabled = maybeUpdatedReadOnlyMode;
+      LOG.info("Config {} has been dynamically changed to {} for region {}",
+        HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, this.isGlobalReadOnlyEnabled, this);
     }
   }
 
@@ -9160,4 +9178,10 @@ public void addWriteRequestsCount(long writeRequestsCount) {
   boolean isReadsEnabled() {
     return this.writestate.readsEnabled;
   }
+
+  @RestrictedApi(explanation = "Should only be called in tests", link = "",
+      allowedOnPath = ".*/src/test/.*")
+  public ConfigurationManager getConfigurationManager() {
+    return configurationManager;
+  }
 }

hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java

@@ -20,6 +20,8 @@
 import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_COORDINATED_BY_ZK;
 import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_WAL_MAX_SPLITTER;
 import static org.apache.hadoop.hbase.HConstants.DEFAULT_SLOW_LOG_SYS_TABLE_CHORE_DURATION;
+import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT;
+import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY;
 import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_COORDINATED_BY_ZK;
 import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_MAX_SPLITTER;
 import static org.apache.hadoop.hbase.master.waleventtracker.WALEventTrackerTableCreator.WAL_EVENT_TRACKER_ENABLED_DEFAULT;
@@ -318,6 +320,7 @@ public class HRegionServer extends HBaseServerBase<RSRpcServices>
   private LeaseManager leaseManager;
 
   private volatile boolean dataFsOk;
+  private volatile boolean isGlobalReadOnlyEnabled;
 
   static final String ABORT_TIMEOUT = "hbase.regionserver.abort.timeout";
   // Default abort timeout is 1200 seconds for safe
@@ -546,6 +549,9 @@ public HRegionServer(final Configuration conf) throws IOException {
       uncaughtExceptionHandler =
         (t, e) -> abort("Uncaught exception in executorService thread " + t.getName(), e);
 
+      this.isGlobalReadOnlyEnabled =
+        conf.getBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
+
       // If no master in cluster, skip trying to track one or look for a cluster status.
       if (!this.masterless) {
         masterAddressTracker = new MasterAddressTracker(getZooKeeper(), this);
@@ -827,9 +833,9 @@ public void run() {
       if (!isStopped() && !isAborted()) {
         installShutdownHook();
 
-        CoprocessorConfigurationUtil.syncReadOnlyConfigurations(
-          ConfigurationUtil.isReadOnlyModeEnabled(conf), conf,
+        CoprocessorConfigurationUtil.syncReadOnlyConfigurations(conf,
           CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY);
+
         // Initialize the RegionServerCoprocessorHost now that our ephemeral
         // node was created, in case any coprocessors want to use ZooKeeper
         this.rsHost = new RegionServerCoprocessorHost(this, this.conf);
@@ -3488,19 +3494,32 @@ public void onConfigurationChange(Configuration newConf) {
       LOG.warn("Failed to initialize SuperUsers on reloading of the configuration");
     }
 
-    boolean readOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf);
-    CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, newConf,
-      CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY);
+    boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf);
+    boolean hasReadOnlyModeChanged = this.isGlobalReadOnlyEnabled != maybeUpdatedReadOnlyMode;
+    boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil.checkConfigurationChange(
+      this.rsHost, newConf, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY);
+    boolean shouldUpdateCoprocessors = hasCoprocessorConfigChanged || hasReadOnlyModeChanged;
 
     // update region server coprocessor if the configuration has changed.
-    if (
-      CoprocessorConfigurationUtil.checkConfigurationChange(this.rsHost, newConf,
-        CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY)
-    ) {
+    if (shouldUpdateCoprocessors) {
+      Set<String> currentlyLoadedCps = this.rsHost.getCoprocessorClassNames();
+      LOG.debug("About to update coprocessors loaded on HRegionServer {}. These are the current "
+        + "coprocessors before updating: {}", this, currentlyLoadedCps);
+
       LOG.info("Update region server coprocessors because the configuration has changed");
-      this.rsHost = new RegionServerCoprocessorHost(this, newConf);
-      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, this.conf,
+      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf,
         CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY);
+      this.rsHost = new RegionServerCoprocessorHost(this, newConf);
+
+      currentlyLoadedCps = this.rsHost.getCoprocessorClassNames();
+      LOG.debug("Finished updating coprocessors on HRegionServer {}. These are the coprocessors "
+        + "after updating: {}", this, currentlyLoadedCps);
+    }
+
+    if (hasReadOnlyModeChanged) {
+      this.isGlobalReadOnlyEnabled = maybeUpdatedReadOnlyMode;
+      LOG.info("Config {} has been dynamically changed to {} for region server {}",
+        HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, this.isGlobalReadOnlyEnabled, this);
     }
   }
 

hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java

@@ -175,16 +175,22 @@ private static List<String> getReadOnlyCoprocessors(String configurationKey) {
     };
   }
 
-  public static void syncReadOnlyConfigurations(boolean readOnlyMode, Configuration conf,
-    String configurationKey) {
-    conf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, readOnlyMode);
+  /**
+   * This method adds or removes relevant ReadOnlyController coprocessors to the provided
+   * configuration based on whether read-only mode is enabled.
+   * @param conf               The up-to-date configuration used to determine how to handle
+   *                           coprocessors
+   * @param coprocessorConfKey The configuration key name
+   */
+  public static void syncReadOnlyConfigurations(Configuration conf, String coprocessorConfKey) {
+    boolean isReadOnlyModeEnabled = conf.getBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY,
+      HConstants.HBASE_GLOBAL_READONLY_ENABLED_DEFAULT);
 
-    List<String> cpList = getReadOnlyCoprocessors(configurationKey);
-    // If readonly is true then add the coprocessor of master
-    if (readOnlyMode) {
-      CoprocessorConfigurationUtil.addCoprocessors(conf, configurationKey, cpList);
+    List<String> cpList = getReadOnlyCoprocessors(coprocessorConfKey);
+    if (isReadOnlyModeEnabled) {
+      CoprocessorConfigurationUtil.addCoprocessors(conf, coprocessorConfKey, cpList);
     } else {
-      CoprocessorConfigurationUtil.removeCoprocessors(conf, configurationKey, cpList);
+      CoprocessorConfigurationUtil.removeCoprocessors(conf, coprocessorConfKey, cpList);
     }
   }
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/security/access/TestReadOnlyControllerCoprocessorLoading.java

@@ -107,8 +107,8 @@ private void createTable() throws Exception {
     region = regions.get(0);
   }
 
-  private void setReadOnlyMode(boolean isReadOnlyEnabled) {
-    // Create a new configuration to micic client server behavior
+  private Configuration setReadOnlyMode(boolean isReadOnlyEnabled) {
+    // Create a new configuration to mimic client server behavior
     // otherwise the existing conf object is shared with the cluster
     // and can cause side effects on other tests if not reset properly.
     // This way we can ensure that only the coprocessor loading is tested
@@ -119,9 +119,10 @@ private void setReadOnlyMode(boolean isReadOnlyEnabled) {
     newConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, isReadOnlyEnabled);
     master.getConfigurationManager().notifyAllObservers(newConf);
     regionServer.getConfigurationManager().notifyAllObservers(newConf);
+    return newConf;
   }
 
-  private void verifyMasterReadOnlyControllerLoading(boolean isReadOnlyEnabled) throws Exception {
+  private void verifyMasterReadOnlyControllerLoading(boolean isReadOnlyEnabled) {
     MasterCoprocessorHost masterCPHost = master.getMasterCoprocessorHost();
     if (isReadOnlyEnabled) {
       assertNotNull(
@@ -136,8 +137,7 @@ private void verifyMasterReadOnlyControllerLoading(boolean isReadOnlyEnabled) th
     }
   }
 
-  private void verifyRegionServerReadOnlyControllerLoading(boolean isReadOnlyEnabled)
-    throws Exception {
+  private void verifyRegionServerReadOnlyControllerLoading(boolean isReadOnlyEnabled) {
     RegionServerCoprocessorHost rsCPHost = regionServer.getRegionServerCoprocessorHost();
     if (isReadOnlyEnabled) {
       assertNotNull(
@@ -152,7 +152,7 @@ private void verifyRegionServerReadOnlyControllerLoading(boolean isReadOnlyEnabl
     }
   }
 
-  private void verifyRegionReadOnlyControllerLoading(boolean isReadOnlyEnabled) throws Exception {
+  private void verifyRegionReadOnlyControllerLoading(boolean isReadOnlyEnabled) {
     RegionCoprocessorHost regionCPHost = region.getCoprocessorHost();
 
     if (isReadOnlyEnabled) {
@@ -220,8 +220,10 @@ public void testReadOnlyControllerLoadedWhenEnabledDynamically() throws Exceptio
   public void testReadOnlyControllerUnloadedWhenDisabledDynamically() throws Exception {
     setupMiniCluster(initialReadOnlyMode);
     boolean isReadOnlyEnabled = false;
-    setReadOnlyMode(isReadOnlyEnabled);
+    Configuration newConf = setReadOnlyMode(isReadOnlyEnabled);
     createTable();
+    // The newly created table's region has a stale conf that needs to be updated
+    region.onConfigurationChange(newConf);
     verifyMasterReadOnlyControllerLoading(isReadOnlyEnabled);
     verifyRegionServerReadOnlyControllerLoading(isReadOnlyEnabled);
     verifyRegionReadOnlyControllerLoading(isReadOnlyEnabled);
@@ -232,8 +234,10 @@ public void testReadOnlyControllerLoadUnloadedWhenMultipleReadOnlyToggle() throw
     setupMiniCluster(initialReadOnlyMode);
 
     // Ensure region exists before validation
-    setReadOnlyMode(false);
+    Configuration newConf = setReadOnlyMode(false);
     createTable();
+    // The newly created table's region has a stale conf that needs to be updated
+    region.onConfigurationChange(newConf);
     verifyReadOnlyState(false);
 
     // Define toggle sequence