From f31676e2767b8babedb6a927279f3afeb2fecefe Mon Sep 17 00:00:00 2001 From: Kevin Geiszler Date: Wed, 27 May 2026 15:32:08 -0700 Subject: [PATCH] HBASE-30180: Can still add data to read-only region after flipping read-only flag multiple times Code generated with Cursor: - Unit test TestHRegion.testRegionOnConfigurationChangeLoadsReadOnlyCoprocessorsAfterRepeatedToggle() Code generated with Claude Opus: - Unit test TestHRegion.testRegionCoprocessorHostUsesCompoundConfigurationAfterConfigChange() Change-Id: I8c5edf6ae4775d36dbbe2b260f77e2168a2a92da --- .../apache/hadoop/hbase/master/HMaster.java | 26 ++++--- .../hadoop/hbase/regionserver/HRegion.java | 33 +++++--- .../hbase/regionserver/HRegionServer.java | 26 ++++--- .../util/CoprocessorConfigurationUtil.java | 74 ++++++++++-------- .../hbase/regionserver/TestHRegion.java | 75 ++++++++++++++++++- 5 files changed, 170 insertions(+), 64 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java index df6c616db4ae..d2cf0cc93798 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java @@ -4491,26 +4491,32 @@ public MasterRegion getMasterRegion() { return masterRegion; } + /** + * Dynamically updates HMaster's configuration. Since HMaster inherits from + * {@link HBaseServerBase}, the {@code updatedConf} parameter references the same + * {@link Configuration} object as HMaster's {@code this.conf} instance variable in a real HBase + * deployment. This isn't necessarily the case in unit tests. + * @param updatedConf the dynamically updated configuration + */ @Override - public void onConfigurationChange(Configuration newConf) { + public void onConfigurationChange(Configuration updatedConf) { try { - Superusers.initialize(newConf); + Superusers.initialize(updatedConf); } catch (IOException e) { LOG.warn("Failed to initialize SuperUsers on reloading of the configuration"); } // append the quotas observer back to the master coprocessor key - setQuotasObserver(newConf); + setQuotasObserver(updatedConf); boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); - CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, - this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, this.maintenanceMode, - this.toString(), conf -> { - this.initializeCoprocessorHost(conf); - CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf, - CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); - }); + // updatedConf and this.conf reference the same Configuration object in an actual HBase + // deployment. However, in unit test cases they reference different Configuration objects, so + // this.conf needs to be updated. + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(updatedConf, this.conf, + originalIsReadOnlyEnabled, this.cpHost, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY, + this.maintenanceMode, this.toString(), this::initializeCoprocessorHost); boolean maybeUpdatedReadOnlyMode = CoprocessorConfigurationUtil .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.MASTER_COPROCESSOR_CONF_KEY); diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java index f2a67df842ce..6871a79324f5 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java @@ -150,6 +150,7 @@ import org.apache.hadoop.hbase.keymeta.KeyManagementService; import org.apache.hadoop.hbase.keymeta.ManagedKeyDataCache; import org.apache.hadoop.hbase.keymeta.SystemKeyCache; +import org.apache.hadoop.hbase.master.HMaster; import org.apache.hadoop.hbase.mob.MobFileCache; import org.apache.hadoop.hbase.monitoring.MonitoredTask; import org.apache.hadoop.hbase.monitoring.TaskMonitor; @@ -8982,25 +8983,33 @@ IOException throwOnInterrupt(Throwable t) { } /** - * {@inheritDoc} + * Dynamically updates HRegion's configuration. Unlike {@link HMaster} and {@link HRegionServer}, + * this {@code updatedConf} parameter does not reference the same {@link Configuration} object as + * HRegion's {@code this.conf} instance variable in a real HBase deployment. This is because + * HRegion's {@code this.conf} is a {@link CompoundConfiguration} object. Instead, + * {@code updatedConf} references the same Configuration object as HRegion's {@code this.baseConf} + * instance variable. + * @param updatedConf the dynamically updated configuration */ @Override - public void onConfigurationChange(Configuration newConf) { - this.storeHotnessProtector.update(newConf); + public void onConfigurationChange(Configuration updatedConf) { + this.storeHotnessProtector.update(updatedConf); boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); - CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, - this.coprocessorHost, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, false, this.toString(), - conf -> { + // HRegion's this.conf is a special Configuration type called CompoundConfiguration. This means + // we don't want to use the updatedConf provided in onConfigurationChange() for creating a new + // RegionCoprocessorHost. Instead, we update this.conf and use that for decorating the region + // config and updating this.coprocessorHost. + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(updatedConf, this.conf, + originalIsReadOnlyEnabled, this.coprocessorHost, CoprocessorHost.REGION_COPROCESSOR_CONF_KEY, + false, this.toString(), conf -> { decorateRegionConfiguration(conf); this.coprocessorHost = new RegionCoprocessorHost(this, rsServices, conf); - CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf, - CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); }); - boolean newReadOnlyEnabled = ConfigurationUtil.isReadOnlyModeEnabledInConf(newConf); + boolean newReadOnlyEnabled = ConfigurationUtil.isReadOnlyModeEnabledInConf(updatedConf); if (originalIsReadOnlyEnabled && !newReadOnlyEnabled) { LOG.info("Cluster Read Only mode disabled"); @@ -9157,4 +9166,10 @@ boolean isReadsEnabled() { public ConfigurationManager getConfigurationManager() { return configurationManager; } + + @RestrictedApi(explanation = "Should only be called in tests", link = "", + allowedOnPath = ".*/src/test/.*") + public Configuration getConfiguration() { + return this.conf; + } } diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java index 009e0d8a1341..a7bf72dabd74 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java @@ -3483,15 +3483,22 @@ public double getFlushPressure() { return getRegionServerAccounting().getFlushPressure(); } + /** + * Dynamically updates HRegionServer's configuration. Since HRegionServer inherits from + * {@link HBaseServerBase}, the {@code updatedConf} parameter references the same + * {@link Configuration} object as HRegionServer's {@code this.conf} instance variable in a real + * HBase deployment. This isn't necessarily the case in unit tests. + * @param updatedConf the dynamically updated configuration + */ @Override - public void onConfigurationChange(Configuration newConf) { + public void onConfigurationChange(Configuration updatedConf) { ThroughputController old = this.flushThroughputController; if (old != null) { old.stop("configuration change"); } - this.flushThroughputController = FlushThroughputControllerFactory.create(this, newConf); + this.flushThroughputController = FlushThroughputControllerFactory.create(this, updatedConf); try { - Superusers.initialize(newConf); + Superusers.initialize(updatedConf); } catch (IOException e) { LOG.warn("Failed to initialize SuperUsers on reloading of the configuration"); } @@ -3499,13 +3506,12 @@ public void onConfigurationChange(Configuration newConf) { boolean originalIsReadOnlyEnabled = CoprocessorConfigurationUtil .areReadOnlyCoprocessorsLoaded(this.conf, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); - CoprocessorConfigurationUtil.maybeUpdateCoprocessors(newConf, originalIsReadOnlyEnabled, - this.rsHost, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, false, this.toString(), - conf -> { - this.rsHost = new RegionServerCoprocessorHost(this, conf); - CoprocessorConfigurationUtil.updateCoprocessorListInConf(this.conf, conf, - CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY); - }); + // updatedConf and this.conf reference the same Configuration object in an actual HBase + // deployment. However, in unit test cases they reference different Configuration objects, so + // this.conf needs to be updated. + CoprocessorConfigurationUtil.maybeUpdateCoprocessors(updatedConf, this.conf, + originalIsReadOnlyEnabled, this.rsHost, CoprocessorHost.REGIONSERVER_COPROCESSOR_CONF_KEY, + false, this.toString(), conf -> this.rsHost = new RegionServerCoprocessorHost(this, conf)); } @Override diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java index 8c462fbbc348..6a7c5b7f7734 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/util/CoprocessorConfigurationUtil.java @@ -17,10 +17,13 @@ */ package org.apache.hadoop.hbase.util; +import static org.apache.hadoop.hbase.HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY; + import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; import java.util.List; +import java.util.Objects; import java.util.Set; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.HConstants; @@ -177,6 +180,25 @@ private static List getReadOnlyCoprocessors(String configurationKey) { return READONLY_COPROCESSORS.get(configurationKey); } + /** + * Updates the coprocessors in one Configuration object to match the coprocessors in the other + * @param srcConf the Configuration object we are getting coprocessors from + * @param dstConf the Configuration object we are updating + * @param coprocessorConfKey the type of coprocessors we are updating + */ + private static void syncCoprocessorsWithConf(Configuration srcConf, Configuration dstConf, + String coprocessorConfKey) { + String configuredCps = srcConf.get(coprocessorConfKey); + dstConf.set(coprocessorConfKey, Objects.requireNonNullElse(configuredCps, "")); + + // A configuration with region coprocessors may also have user region coprocessors + if (CoprocessorHost.REGION_COPROCESSOR_CONF_KEY.equals(coprocessorConfKey)) { + String configuredUserCps = srcConf.get(CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY); + dstConf.set(CoprocessorHost.USER_REGION_COPROCESSOR_CONF_KEY, + Objects.requireNonNullElse(configuredUserCps, "")); + } + } + /** * This method adds or removes relevant ReadOnlyController coprocessors to the provided * configuration based on whether read-only mode is enabled in the provided Configuration. @@ -211,26 +233,6 @@ public static boolean areReadOnlyCoprocessorsLoaded(Configuration conf, return allCoprocessors.containsAll(readOnlyCoprocessors); } - /** - * Takes an updated configuration and updates the coprocessors for that configuration key in the - * current configuration. - * @param currentConf the configuration currently used by the master, region server, or - * region - * @param updatedConf the updated version of the configuration whose coprocessors we want - * to copy - * @param coprocessorConfKey configuration key used for setting master, region server, or region - * coprocessors - */ - public static void updateCoprocessorListInConf(Configuration currentConf, - Configuration updatedConf, String coprocessorConfKey) { - String[] updatedCoprocessorList = updatedConf.getStrings(coprocessorConfKey); - if (updatedCoprocessorList != null) { - currentConf.setStrings(coprocessorConfKey, updatedCoprocessorList); - } else { - currentConf.unset(coprocessorConfKey); - } - } - /** * Gets the name of a component based on the provided coprocessor configuration key. * @param coprocessorConfKey configuration key used for setting master, region server, or region @@ -252,7 +254,9 @@ public static String getComponentName(String coprocessorConfKey) { * 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 updatedConf an updated configuration + * @param originalConf the actual configuration we want to update, which may or may + * not be the same Configuration object as {@code updatedConf} * @param originalIsReadOnlyEnabled the original value for * {@value HConstants#HBASE_GLOBAL_READONLY_ENABLED_KEY} * @param coprocessorHost the coprocessor host for HMaster, HRegionServer, or HRegion @@ -264,28 +268,36 @@ public static String getComponentName(String coprocessorConfKey) { * @param reloadTask lambda function that reloads coprocessors on the master, * region server, or region */ - public static void maybeUpdateCoprocessors(Configuration newConf, + public static void maybeUpdateCoprocessors(Configuration updatedConf, Configuration originalConf, boolean originalIsReadOnlyEnabled, CoprocessorHost coprocessorHost, String coprocessorConfKey, boolean isMaintenanceMode, String instance, CoprocessorReloadTask reloadTask) { - boolean maybeUpdatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabledInConf(newConf); - boolean hasReadOnlyModeChanged = originalIsReadOnlyEnabled != maybeUpdatedReadOnlyMode; + String componentName = getComponentName(coprocessorConfKey); + boolean updatedReadOnlyMode = ConfigurationUtil.isReadOnlyModeEnabledInConf(updatedConf); + boolean hasReadOnlyModeChanged = originalIsReadOnlyEnabled != updatedReadOnlyMode; boolean hasCoprocessorConfigChanged = CoprocessorConfigurationUtil - .checkConfigurationChange(coprocessorHost, newConf, coprocessorConfKey); + .checkConfigurationChange(coprocessorHost, updatedConf, coprocessorConfKey); - // update region server coprocessor if the configuration has changed. if ((hasCoprocessorConfigChanged || hasReadOnlyModeChanged) && !isMaintenanceMode) { LOG.info("Updating coprocessors for {} {} because the configuration has changed", - getComponentName(coprocessorConfKey), instance); - CoprocessorConfigurationUtil.syncReadOnlyConfigurations(newConf, coprocessorConfKey); - reloadTask.reload(newConf); + componentName, instance); + // In real HBase deployments, updatedConf and originalConf reference the same Configuration + // object for HMaster and HRegionServer respectively. However, for HRegion and for unit test + // cases, these are different objects, and the original conf needs to be updated accordingly. + if (updatedConf != originalConf) { + syncCoprocessorsWithConf(updatedConf, originalConf, coprocessorConfKey); + originalConf.setBoolean(HBASE_GLOBAL_READONLY_ENABLED_KEY, updatedReadOnlyMode); + } + // This needs to run even if read-only mode has not changed in case ReadOnly coprocessors + // were unintentionally added/removed in the previous code block + syncReadOnlyConfigurations(originalConf, coprocessorConfKey); + reloadTask.reload(originalConf); } if (hasReadOnlyModeChanged) { LOG.info("Config {} has been dynamically changed to {} for {} {}", - HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, maybeUpdatedReadOnlyMode, - getComponentName(coprocessorConfKey), instance); + HBASE_GLOBAL_READONLY_ENABLED_KEY, updatedReadOnlyMode, componentName, instance); } } } diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java index c138b0a448a7..380e657b3d15 100644 --- a/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java +++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java @@ -24,6 +24,7 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -45,6 +46,7 @@ import java.io.IOException; import java.io.InterruptedIOException; +import java.lang.reflect.Field; import java.math.BigDecimal; import java.security.PrivilegedExceptionAction; import java.util.ArrayList; @@ -82,6 +84,7 @@ import org.apache.hadoop.hbase.CellUtil; import org.apache.hadoop.hbase.CompareOperator; import org.apache.hadoop.hbase.CompatibilitySingletonFactory; +import org.apache.hadoop.hbase.CompoundConfiguration; import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.DroppedSnapshotException; import org.apache.hadoop.hbase.ExtendedCell; @@ -159,11 +162,13 @@ import org.apache.hadoop.hbase.regionserver.wal.WALUtil; import org.apache.hadoop.hbase.replication.regionserver.ReplicationObserver; import org.apache.hadoop.hbase.security.User; +import org.apache.hadoop.hbase.security.access.RegionReadOnlyController; import org.apache.hadoop.hbase.test.MetricsAssertHelper; import org.apache.hadoop.hbase.testclassification.LargeTests; import org.apache.hadoop.hbase.testclassification.VerySlowRegionServerTests; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.CommonFSUtils; +import org.apache.hadoop.hbase.util.CoprocessorConfigurationUtil; import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.hbase.util.EnvironmentEdgeManagerTestHelper; import org.apache.hadoop.hbase.util.HFileArchiveUtil; @@ -7891,9 +7896,9 @@ public void testRegionOnCoprocessorsChange() throws IOException { NoOpRegionCoprocessor.class.getName()); // trigger configuration change region.onConfigurationChange(newConf); - assertTrue(region.getCoprocessorHost() != null); + assertNotNull(region.getCoprocessorHost()); Set coprocessors = region.getCoprocessorHost().getCoprocessors(); - assertTrue(coprocessors.size() == 2); + assertEquals(2, coprocessors.size()); assertTrue(region.getCoprocessorHost().getCoprocessors() .contains(MetaTableMetrics.class.getSimpleName())); assertTrue(region.getCoprocessorHost().getCoprocessors() @@ -7902,9 +7907,9 @@ public void testRegionOnCoprocessorsChange() throws IOException { // remove region coprocessor and keep only user region coprocessor newConf.unset(CoprocessorHost.REGION_COPROCESSOR_CONF_KEY); region.onConfigurationChange(newConf); - assertTrue(region.getCoprocessorHost() != null); + assertNotNull(region.getCoprocessorHost()); coprocessors = region.getCoprocessorHost().getCoprocessors(); - assertTrue(coprocessors.size() == 1); + assertEquals(1, coprocessors.size()); assertTrue(region.getCoprocessorHost().getCoprocessors() .contains(NoOpRegionCoprocessor.class.getSimpleName())); } @@ -8006,4 +8011,66 @@ public void testHRegionInitializeFailsWithDeletedRegionDir() throws Exception { TEST_UTIL.cleanupTestDir(); } } + + /** + * A region can miss read-only coprocessors after repeated toggles if + * {@code maybeUpdateCoprocessors} syncs to the server configuration instead of the region's + * {@link org.apache.hadoop.hbase.CompoundConfiguration}. + */ + @Test + public void testRegionOnConfigurationChangeLoadsReadOnlyCoprocessorsAfterRepeatedToggle() + throws IOException { + byte[] cf1 = Bytes.toBytes("CF1"); + Configuration serverConf = new Configuration(CONF); + serverConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false); + + region = initHRegion(tableName, method, serverConf, new byte[][] { cf1 }); + region.setCoprocessorHost(new RegionCoprocessorHost(region, null, region.getConfiguration())); + + String regionCpKey = CoprocessorHost.REGION_COPROCESSOR_CONF_KEY; + boolean[] toggleSequence = new boolean[] { true, false, true, true, false, false }; + + for (boolean readOnlyEnabled : toggleSequence) { + Configuration newConf = new Configuration(serverConf); + newConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, readOnlyEnabled); + region.onConfigurationChange(newConf); + + if (readOnlyEnabled) { + assertNotNull( + region.getCoprocessorHost().findCoprocessor(RegionReadOnlyController.class.getName())); + } else { + assertNull( + region.getCoprocessorHost().findCoprocessor(RegionReadOnlyController.class.getName())); + } + assertEquals(readOnlyEnabled, CoprocessorConfigurationUtil + .areReadOnlyCoprocessorsLoaded(region.getConfiguration(), regionCpKey)); + } + } + + /** + * Verifies the new RegionCoprocessorHost object created in HRegion.onConfigurationChange() uses a + * CompoundConfiguration in its constructor rather than a Configuration object. + */ + @Test + public void testRegionCoprocessorHostUsesCompoundConfigurationAfterConfigChange() + throws Exception { + byte[] cf1 = Bytes.toBytes("CF1"); + Configuration serverConf = new Configuration(CONF); + serverConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, false); + + region = initHRegion(tableName, method, serverConf, new byte[][] { cf1 }); + region.setCoprocessorHost(new RegionCoprocessorHost(region, null, region.getConfiguration())); + + // Enable read-only mode to trigger coprocessor reload and new RegionCoprocessorHost creation + Configuration newConf = new Configuration(serverConf); + newConf.setBoolean(HConstants.HBASE_GLOBAL_READONLY_ENABLED_KEY, true); + region.onConfigurationChange(newConf); + + // Use reflection to access the protected conf field on CoprocessorHost, which holds the + // Configuration passed to the RegionCoprocessorHost constructor + Field confField = CoprocessorHost.class.getDeclaredField("conf"); + confField.setAccessible(true); + Configuration hostConf = (Configuration) confField.get(region.getCoprocessorHost()); + assertInstanceOf(CompoundConfiguration.class, hostConf); + } }