HBASE-29965: Unable to dynamically change readonly flag

hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorReloadTask.java

@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 org.apache.hadoop.hbase.coprocessor;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseInterfaceAudience;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.yetus.audience.InterfaceStability;
+
+/**
+ * This interface is used to perform whatever task is necessary for reloading coprocessors on
+ * HMaster, HRegionServer, and HRegion. Since the steps required to reload coprocessors varies for
+ * each of these types, this interface helps with code flexibility by allowing a lamda function to
+ * be provided for the {@link #reload(Configuration) reload()} method. <br>
+ * <br>
+ * See {@link org.apache.hadoop.hbase.util.CoprocessorConfigurationUtil#maybeUpdateCoprocessors
+ * CoprocessorConfigurationUtil.maybeUpdateCoprocessors()} and its usage in
+ * {@link org.apache.hadoop.hbase.conf.ConfigurationObserver#onConfigurationChange
+ * onConfigurationChange()} with HMaster, HRegionServer, and HRegion for an idea of how this
+ * interface is helpful.
+ */
+@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.COPROC)
+@InterfaceStability.Evolving
+@FunctionalInterface
+public interface CoprocessorReloadTask {
+  void reload(Configuration conf);
+}

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,16 @@ 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 originalIsReadOnlyEnabled = this.isGlobalReadOnlyEnabled;
 
-    // update region server coprocessor if the configuration has changed.
-    if (
-      CoprocessorConfigurationUtil.checkConfigurationChange(this.cpHost, newConf,
-        CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY) && !maintenanceMode
-    ) {
-      LOG.info("Update the master coprocessor(s) because the configuration has changed");
-      initializeCoprocessorHost(newConf);
-      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, this.conf,
-        CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY);
-      AbstractReadOnlyController.manageActiveClusterIdFile(
-        ConfigurationUtil.isReadOnlyModeEnabled(newConf), this.getMasterFileSystem());
+    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled,
+      this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, this.maintenanceMode,
+      this.toString(), val -> this.isGlobalReadOnlyEnabled = val,
+      conf -> initializeCoprocessorHost(newConf));
+
+    if (this.isGlobalReadOnlyEnabled != originalIsReadOnlyEnabled) {
+      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;
@@ -178,7 +180,6 @@
 import org.apache.hadoop.hbase.util.CancelableProgressable;
 import org.apache.hadoop.hbase.util.ClassSize;
 import org.apache.hadoop.hbase.util.CommonFSUtils;
-import org.apache.hadoop.hbase.util.ConfigurationUtil;
 import org.apache.hadoop.hbase.util.CoprocessorConfigurationUtil;
 import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
 import org.apache.hadoop.hbase.util.FSUtils;
@@ -391,6 +392,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 +944,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 +8519,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,25 +8990,15 @@ IOException throwOnInterrupt(Throwable t) {
    * {@inheritDoc}
    */
   @Override
-  public void onConfigurationChange(Configuration conf) {
-    this.storeHotnessProtector.update(conf);
-
-    boolean readOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(conf);
-    CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, conf,
-      CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
-
-    // update coprocessorHost if the configuration has changed.
-    if (
-      CoprocessorConfigurationUtil.checkConfigurationChange(this.coprocessorHost, conf,
-        CoprocessorHost.REGION_COPROCESSOR_CONF_KEY,
-        CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY)
-    ) {
-      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,
-        CoprocessorHost.REGION_COPROCESSOR_CONF_KEY);
-    }
+  public void onConfigurationChange(Configuration newConf) {
+    this.storeHotnessProtector.update(newConf);
+
+    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled,
+      this.coprocessorHost, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, false, this.toString(),
+      val -> this.isGlobalReadOnlyEnabled = val, conf -> {
+        decorateRegionConfiguration(conf);
+        this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, newConf);
+      });
   }
 
   /**
@@ -9160,4 +9154,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;
@@ -161,7 +163,6 @@
 import org.apache.hadoop.hbase.trace.TraceUtil;
 import org.apache.hadoop.hbase.util.Bytes;
 import org.apache.hadoop.hbase.util.CompressionTest;
-import org.apache.hadoop.hbase.util.ConfigurationUtil;
 import org.apache.hadoop.hbase.util.CoprocessorConfigurationUtil;
 import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
 import org.apache.hadoop.hbase.util.FSUtils;
@@ -318,6 +319,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 +548,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 +832,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,20 +3493,10 @@ 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);
-
-    // update region server coprocessor if the configuration has changed.
-    if (
-      CoprocessorConfigurationUtil.checkConfigurationChange(this.rsHost, newConf,
-        CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY)
-    ) {
-      LOG.info("Update region server coprocessors because the configuration has changed");
-      this.rsHost = new RegionServerCoprocessorHost(this, newConf);
-      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(readOnlyMode, this.conf,
-        CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY);
-    }
+    CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, this.isGlobalReadOnlyEnabled,
+      this.rsHost, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, false, this.toString(),
+      val -> this.isGlobalReadOnlyEnabled = val,
+      conf -> this.rsHost = new RegionServerCoprocessorHost(this, newConf));
   }
 
   @Override

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

@@ -22,15 +22,19 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.function.Consumer;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.hbase.HConstants;
 import org.apache.hadoop.hbase.coprocessor.CoprocessorHost;
+import org.apache.hadoop.hbase.coprocessor.CoprocessorReloadTask;
 import org.apache.hadoop.hbase.security.access.BulkLoadReadOnlyController;
 import org.apache.hadoop.hbase.security.access.EndpointReadOnlyController;
 import org.apache.hadoop.hbase.security.access.MasterReadOnlyController;
 import org.apache.hadoop.hbase.security.access.RegionReadOnlyController;
 import org.apache.hadoop.hbase.security.access.RegionServerReadOnlyController;
 import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import org.apache.hbase.thirdparty.com.google.common.base.Preconditions;
 import org.apache.hbase.thirdparty.com.google.common.base.Strings;
@@ -40,6 +44,7 @@
  */
 @InterfaceAudience.Private
 public final class CoprocessorConfigurationUtil {
+  private static final Logger LOG = LoggerFactory.getLogger(CoprocessorConfigurationUtil.class);
 
   private CoprocessorConfigurationUtil() {
   }
@@ -175,16 +180,65 @@ 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);
+    }
+  }
+
+  /**
+   * This method updates the coprocessors on the master, region server, or region if a change has
+   * been detected. Detected changes include changes in coprocessors or changes in read-only mode
+   * configuration. If a change is detected, then new coprocessors are loaded using the provided
+   * reload method. The new value for the read-only config variable is updated as well.
+   * @param newConf                   an updated configuration
+   * @param originalIsReadOnlyEnabled the original value for
+   *                                  {@value HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY}
+   * @param coprocessorHost           the coprocessor host for HMaster, HRegionServer, or HRegion
+   * @param coprocessorConfKey        configuration key used for setting master, region server, or
+   *                                  region coprocessors
+   * @param isMaintenanceMode         whether maintenance mode is active (mainly for HMaster)
+   * @param instance                  string value of the instance calling this method (mainly helps
+   *                                  with tracking region logging)
+   * @param stateSetter               lambda function that sets the read-only instance variable with
+   *                                  an updated value from the config
+   * @param reloadTask                lambda function that reloads coprocessors on the master,
+   *                                  region server, or region
+   */
+  public static void maybeUpdateCoprocessors(Configuration newConf,
+    boolean originalIsReadOnlyEnabled, CoprocessorHost<?, ?> coprocessorHost,
+    String coprocessorConfKey, boolean isMaintenanceMode, String instance,
+    Consumer<Boolean> stateSetter, CoprocessorReloadTask reloadTask) {
+
+    boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabled(newConf);
+    boolean hasReadOnlyModeChanged = originalIsReadOnlyEnabled != maybeUpdatedReadOnlyMode;
+    boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil
+      .checkConfigurationChange(coprocessorHost, newConf, coprocessorConfKey);
+
+    // update region server coprocessor if the configuration has changed.
+    if ((hasCoprocessorConfigChanged || hasReadOnlyModeChanged) && !isMaintenanceMode) {
+      LOG.info("Updating coprocessors for {} because the configuration has changed", instance);
+      CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, coprocessorConfKey);
+      reloadTask.reload(newConf);
+    }
+
+    if (hasReadOnlyModeChanged) {
+      stateSetter.accept(maybeUpdatedReadOnlyMode);
+      LOG.info("Config {} has been dynamically changed to {} for {}",
+        HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, maybeUpdatedReadOnlyMode, instance);
     }
   }
 }