Skip to content

Commit

Permalink
Ensure shared cache is page-aligned (#70876)
Browse files Browse the repository at this point in the history 
This commit ensures that the shared cache can only be configured with range sizes that are aligned with the page cache
size (4KB).
  • Loading branch information
@ywelsch
ywelsch committed Mar 26, 2021
1 parent cbcc8dd commit fa2f047
Show file tree
Hide file tree
Showing 8 changed files with 101 additions and 49 deletions.
12 changes: 12 additions & 0 deletions server/src/main/java/org/elasticsearch/common/settings/Setting.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,18 @@ public Setting(String key, Function<Settings, String> defaultValue, Function<Str
this(new SimpleKey(key), defaultValue, parser, properties);
}

/**
* Creates a new Setting instance
* @param key the settings key for this setting.
* @param fallbackSetting a setting who's value to fallback on if this setting is not defined
* @param parser a parser that parses the string rep into a complex datatype.
* @param validator a {@link Validator} for validating this setting
* @param properties properties for this setting like scope, filtering...
*/
public Setting(String key, Setting<T> fallbackSetting, Function<String, T> parser, Validator<T> validator, Property... properties) {
this(new SimpleKey(key), fallbackSetting, fallbackSetting::getRaw, parser, validator, properties);
}

/**
* Creates a new Setting instance
* @param key the settings key for this setting.
Expand Down
9 changes: 5 additions & 4 deletions ...ava/org/elasticsearch/xpack/searchablesnapshots/BaseSearchableSnapshotsIntegTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@

import static org.elasticsearch.license.LicenseService.SELF_GENERATED_LICENSE_TYPE;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.xpack.searchablesnapshots.cache.shared.SharedBytes.pageAligned;
import static org.hamcrest.Matchers.equalTo;

public abstract class BaseSearchableSnapshotsIntegTestCase extends AbstractSnapshotIntegTestCase {
Expand Down Expand Up @@ -93,15 +94,15 @@ protected Settings nodeSettings(int nodeOrdinal) {
builder.put(
FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(),
rarely()
? new ByteSizeValue(randomIntBetween(4, 1024), ByteSizeUnit.KB)
: new ByteSizeValue(randomIntBetween(1, 10), ByteSizeUnit.MB)
? pageAligned(new ByteSizeValue(randomIntBetween(4, 1024), ByteSizeUnit.KB))
: pageAligned(new ByteSizeValue(randomIntBetween(1, 10), ByteSizeUnit.MB))
);
if (randomBoolean()) {
builder.put(
FrozenCacheService.SHARED_CACHE_RANGE_SIZE_SETTING.getKey(),
rarely()
? new ByteSizeValue(randomIntBetween(4, 1024), ByteSizeUnit.KB)
: new ByteSizeValue(randomIntBetween(1, 10), ByteSizeUnit.MB)
? pageAligned(new ByteSizeValue(randomIntBetween(4, 1024), ByteSizeUnit.KB))
: pageAligned(new ByteSizeValue(randomIntBetween(1, 10), ByteSizeUnit.MB))
);
}
if (randomBoolean()) {
Expand Down
3 changes: 2 additions & 1 deletion ...rch/xpack/searchablesnapshots/cache/blob/SearchableSnapshotsBlobStoreCacheIntegTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshots.DATA_TIERS_CACHE_INDEX_PREFERENCE;
import static org.elasticsearch.xpack.searchablesnapshots.SearchableSnapshotsConstants.SNAPSHOT_BLOB_CACHE_INDEX;
import static org.elasticsearch.xpack.searchablesnapshots.cache.shared.SharedBytes.pageAligned;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;

Expand All @@ -72,7 +73,7 @@ public class SearchableSnapshotsBlobStoreCacheIntegTests extends BaseSearchableS

@BeforeClass
public static void setUpCacheSettings() {
blobCacheMaxLength = new ByteSizeValue(randomLongBetween(64L, 128L), ByteSizeUnit.KB);
blobCacheMaxLength = pageAligned(new ByteSizeValue(randomLongBetween(64L, 128L), ByteSizeUnit.KB));

final Settings.Builder builder = Settings.builder();
// Cold (full copy) cache should be unlimited to not cause evictions
Expand Down
22 changes: 18 additions & 4 deletions ...ain/java/org/elasticsearch/xpack/searchablesnapshots/cache/shared/FrozenCacheService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,19 +65,33 @@ public class FrozenCacheService implements Releasable {

private static final String SHARED_CACHE_SETTINGS_PREFIX = "xpack.searchable.snapshot.shared_cache.";

public static final Setting<ByteSizeValue> SHARED_CACHE_RANGE_SIZE_SETTING = Setting.byteSizeSetting(
public static final Setting<ByteSizeValue> SHARED_CACHE_RANGE_SIZE_SETTING = new Setting<>(
SHARED_CACHE_SETTINGS_PREFIX + "range_size",
ByteSizeValue.ofMb(16), // default
ByteSizeValue.ofMb(16).getStringRep(),
s -> ByteSizeValue.parseBytesSizeValue(s, SHARED_CACHE_SETTINGS_PREFIX + "range_size"),
getPageSizeAlignedByteSizeValueValidator(SHARED_CACHE_SETTINGS_PREFIX + "range_size"),
Setting.Property.NodeScope
);

// TODO: require range size to be multiple of 4kb
public static final Setting<ByteSizeValue> SNAPSHOT_CACHE_REGION_SIZE_SETTING = Setting.byteSizeSetting(
public static final Setting<ByteSizeValue> SNAPSHOT_CACHE_REGION_SIZE_SETTING = new Setting<>(
SHARED_CACHE_SETTINGS_PREFIX + "region_size",
SHARED_CACHE_RANGE_SIZE_SETTING,
s -> ByteSizeValue.parseBytesSizeValue(s, SHARED_CACHE_SETTINGS_PREFIX + "region_size"),
getPageSizeAlignedByteSizeValueValidator(SHARED_CACHE_SETTINGS_PREFIX + "region_size"),
Setting.Property.NodeScope
);

private static Setting.Validator<ByteSizeValue> getPageSizeAlignedByteSizeValueValidator(String settingName) {
return value -> {
if (value.getBytes() == -1) {
throw new SettingsException("setting [{}] must be non-negative", settingName);
}
if (value.getBytes() % SharedBytes.PAGE_SIZE != 0L) {
throw new SettingsException("setting [{}] must be multiple of {}", settingName, SharedBytes.PAGE_SIZE);
}
};
}

public static final Setting<ByteSizeValue> SNAPSHOT_CACHE_SIZE_SETTING = new Setting<>(
SHARED_CACHE_SETTINGS_PREFIX + "size",
ByteSizeValue.ZERO.getStringRep(),
Expand Down
9 changes: 9 additions & 0 deletions ...s/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/shared/SharedBytes.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.SuppressForbidden;
import org.elasticsearch.common.io.Channels;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.util.concurrent.AbstractRefCounted;
import org.elasticsearch.common.util.concurrent.ConcurrentCollections;
import org.elasticsearch.core.internal.io.IOUtils;
Expand Down Expand Up @@ -194,4 +195,12 @@ protected void closeInternal() {
SharedBytes.this.decRef();
}
}

public static ByteSizeValue pageAligned(ByteSizeValue val) {
final long remainder = val.getBytes() % PAGE_SIZE;
if (remainder != 0L) {
return ByteSizeValue.ofBytes(val.getBytes() + PAGE_SIZE - remainder);
}
return val;
}
}
26 changes: 17 additions & 9 deletions ...java/org/elasticsearch/xpack/searchablesnapshots/AbstractSearchableSnapshotsTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@

import static com.carrotsearch.randomizedtesting.RandomizedTest.randomAsciiLettersOfLengthBetween;
import static org.elasticsearch.xpack.searchablesnapshots.cache.common.TestUtils.randomPopulateAndReads;
import static org.elasticsearch.xpack.searchablesnapshots.cache.shared.SharedBytes.pageAligned;

public abstract class AbstractSearchableSnapshotsTestCase extends ESIndexInputTestCase {

Expand Down Expand Up @@ -154,13 +155,13 @@ protected FrozenCacheService randomFrozenCacheService() {
cacheSettings.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), randomFrozenCacheSize());
}
if (randomBoolean()) {
cacheSettings.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), randomFrozenCacheSize());
cacheSettings.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), pageAligned(randomFrozenCacheSize()));
}
if (randomBoolean()) {
cacheSettings.put(FrozenCacheService.SHARED_CACHE_RANGE_SIZE_SETTING.getKey(), randomCacheRangeSize());
cacheSettings.put(FrozenCacheService.SHARED_CACHE_RANGE_SIZE_SETTING.getKey(), randomFrozenCacheRangeSize());
}
if (randomBoolean()) {
cacheSettings.put(FrozenCacheService.FROZEN_CACHE_RECOVERY_RANGE_SIZE_SETTING.getKey(), randomCacheRangeSize());
cacheSettings.put(FrozenCacheService.FROZEN_CACHE_RECOVERY_RANGE_SIZE_SETTING.getKey(), randomFrozenCacheRangeSize());
}
return new FrozenCacheService(nodeEnvironment, cacheSettings.build(), threadPool);
}
Expand Down Expand Up @@ -214,16 +215,23 @@ protected static ByteSizeValue randomFrozenCacheSize() {
* @return a random {@link ByteSizeValue} that can be used to set {@link CacheService#SNAPSHOT_CACHE_RANGE_SIZE_SETTING}
*/
protected static ByteSizeValue randomCacheRangeSize() {
return new ByteSizeValue(
randomLongBetween(CacheService.MIN_SNAPSHOT_CACHE_RANGE_SIZE.getBytes(), CacheService.MAX_SNAPSHOT_CACHE_RANGE_SIZE.getBytes())
return pageAligned(
new ByteSizeValue(
randomLongBetween(
CacheService.MIN_SNAPSHOT_CACHE_RANGE_SIZE.getBytes(),
CacheService.MAX_SNAPSHOT_CACHE_RANGE_SIZE.getBytes()
)
)
);
}

protected static ByteSizeValue randomFrozenCacheRangeSize() {
return new ByteSizeValue(
randomLongBetween(
FrozenCacheService.MIN_SNAPSHOT_CACHE_RANGE_SIZE.getBytes(),
FrozenCacheService.MAX_SNAPSHOT_CACHE_RANGE_SIZE.getBytes()
return pageAligned(
new ByteSizeValue(
randomLongBetween(
FrozenCacheService.MIN_SNAPSHOT_CACHE_RANGE_SIZE.getBytes(),
FrozenCacheService.MAX_SNAPSHOT_CACHE_RANGE_SIZE.getBytes()
)
)
);
}
Expand Down
67 changes: 37 additions & 30 deletions ...ava/org/elasticsearch/xpack/searchablesnapshots/cache/shared/FrozenCacheServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.elasticsearch.cluster.coordination.DeterministicTaskQueue;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.env.NodeEnvironment;
import org.elasticsearch.env.TestEnvironment;
import org.elasticsearch.index.shard.ShardId;
Expand All @@ -29,11 +30,15 @@

public class FrozenCacheServiceTests extends ESTestCase {

private static long size(long numPages) {
return numPages * SharedBytes.PAGE_SIZE;
}

public void testBasicEviction() throws IOException {
Settings settings = Settings.builder()
.put(NODE_NAME_SETTING.getKey(), "node")
.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), "500b")
.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), "100b")
.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), new ByteSizeValue(size(500)).getStringRep())
.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), new ByteSizeValue(size(100)).getStringRep())
.put("path.home", createTempDir())
.build();
final DeterministicTaskQueue taskQueue = new DeterministicTaskQueue(settings, random());
Expand All @@ -43,14 +48,14 @@ public void testBasicEviction() throws IOException {
) {
final CacheKey cacheKey = generateCacheKey();
assertEquals(5, cacheService.freeRegionCount());
final CacheFileRegion region0 = cacheService.get(cacheKey, 250, 0);
assertEquals(100L, region0.tracker.getLength());
final CacheFileRegion region0 = cacheService.get(cacheKey, size(250), 0);
assertEquals(size(100), region0.tracker.getLength());
assertEquals(4, cacheService.freeRegionCount());
final CacheFileRegion region1 = cacheService.get(cacheKey, 250, 1);
assertEquals(100L, region1.tracker.getLength());
final CacheFileRegion region1 = cacheService.get(cacheKey, size(250), 1);
assertEquals(size(100), region1.tracker.getLength());
assertEquals(3, cacheService.freeRegionCount());
final CacheFileRegion region2 = cacheService.get(cacheKey, 250, 2);
assertEquals(50L, region2.tracker.getLength());
final CacheFileRegion region2 = cacheService.get(cacheKey, size(250), 2);
assertEquals(size(50), region2.tracker.getLength());
assertEquals(2, cacheService.freeRegionCount());

assertTrue(region1.tryEvict());
Expand All @@ -77,8 +82,8 @@ public void testBasicEviction() throws IOException {
public void testAutoEviction() throws IOException {
Settings settings = Settings.builder()
.put(NODE_NAME_SETTING.getKey(), "node")
.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), "200b")
.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), "100b")
.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), new ByteSizeValue(size(200)).getStringRep())
.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), new ByteSizeValue(size(100)).getStringRep())
.put("path.home", createTempDir())
.build();
final DeterministicTaskQueue taskQueue = new DeterministicTaskQueue(settings, random());
Expand All @@ -88,18 +93,18 @@ public void testAutoEviction() throws IOException {
) {
final CacheKey cacheKey = generateCacheKey();
assertEquals(2, cacheService.freeRegionCount());
final CacheFileRegion region0 = cacheService.get(cacheKey, 250, 0);
assertEquals(100L, region0.tracker.getLength());
final CacheFileRegion region0 = cacheService.get(cacheKey, size(250), 0);
assertEquals(size(100), region0.tracker.getLength());
assertEquals(1, cacheService.freeRegionCount());
final CacheFileRegion region1 = cacheService.get(cacheKey, 250, 1);
assertEquals(100L, region1.tracker.getLength());
final CacheFileRegion region1 = cacheService.get(cacheKey, size(250), 1);
assertEquals(size(100), region1.tracker.getLength());
assertEquals(0, cacheService.freeRegionCount());
assertFalse(region0.isEvicted());
assertFalse(region1.isEvicted());

// acquire region 2, which should evict region 0 (oldest)
final CacheFileRegion region2 = cacheService.get(cacheKey, 250, 2);
assertEquals(50L, region2.tracker.getLength());
final CacheFileRegion region2 = cacheService.get(cacheKey, size(250), 2);
assertEquals(size(50), region2.tracker.getLength());
assertEquals(0, cacheService.freeRegionCount());
assertTrue(region0.isEvicted());
assertFalse(region1.isEvicted());
Expand All @@ -113,8 +118,8 @@ public void testAutoEviction() throws IOException {
public void testForceEviction() throws IOException {
Settings settings = Settings.builder()
.put(NODE_NAME_SETTING.getKey(), "node")
.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), "500b")
.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), "100b")
.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), new ByteSizeValue(size(500)).getStringRep())
.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), new ByteSizeValue(size(100)).getStringRep())
.put("path.home", createTempDir())
.build();
final DeterministicTaskQueue taskQueue = new DeterministicTaskQueue(settings, random());
Expand All @@ -125,9 +130,9 @@ public void testForceEviction() throws IOException {
final CacheKey cacheKey1 = generateCacheKey();
final CacheKey cacheKey2 = generateCacheKey();
assertEquals(5, cacheService.freeRegionCount());
final CacheFileRegion region0 = cacheService.get(cacheKey1, 250, 0);
final CacheFileRegion region0 = cacheService.get(cacheKey1, size(250), 0);
assertEquals(4, cacheService.freeRegionCount());
final CacheFileRegion region1 = cacheService.get(cacheKey2, 250, 1);
final CacheFileRegion region1 = cacheService.get(cacheKey2, size(250), 1);
assertEquals(3, cacheService.freeRegionCount());
assertFalse(region0.isEvicted());
assertFalse(region1.isEvicted());
Expand All @@ -141,8 +146,8 @@ public void testForceEviction() throws IOException {
public void testDecay() throws IOException {
Settings settings = Settings.builder()
.put(NODE_NAME_SETTING.getKey(), "node")
.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), "500b")
.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), "100b")
.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), new ByteSizeValue(size(500)).getStringRep())
.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), new ByteSizeValue(size(100)).getStringRep())
.put("path.home", createTempDir())
.build();
final DeterministicTaskQueue taskQueue = new DeterministicTaskQueue(settings, random());
Expand All @@ -153,9 +158,9 @@ public void testDecay() throws IOException {
final CacheKey cacheKey1 = generateCacheKey();
final CacheKey cacheKey2 = generateCacheKey();
assertEquals(5, cacheService.freeRegionCount());
final CacheFileRegion region0 = cacheService.get(cacheKey1, 250, 0);
final CacheFileRegion region0 = cacheService.get(cacheKey1, size(250), 0);
assertEquals(4, cacheService.freeRegionCount());
final CacheFileRegion region1 = cacheService.get(cacheKey2, 250, 1);
final CacheFileRegion region1 = cacheService.get(cacheKey2, size(250), 1);
assertEquals(3, cacheService.freeRegionCount());

assertEquals(0, cacheService.getFreq(region0));
Expand All @@ -164,16 +169,16 @@ public void testDecay() throws IOException {
taskQueue.advanceTime();
taskQueue.runAllRunnableTasks();

final CacheFileRegion region0Again = cacheService.get(cacheKey1, 250, 0);
final CacheFileRegion region0Again = cacheService.get(cacheKey1, size(250), 0);
assertSame(region0Again, region0);
assertEquals(1, cacheService.getFreq(region0));
assertEquals(0, cacheService.getFreq(region1));

taskQueue.advanceTime();
taskQueue.runAllRunnableTasks();
cacheService.get(cacheKey1, 250, 0);
cacheService.get(cacheKey1, size(250), 0);
assertEquals(2, cacheService.getFreq(region0));
cacheService.get(cacheKey1, 250, 0);
cacheService.get(cacheKey1, size(250), 0);
assertEquals(2, cacheService.getFreq(region0));

// advance 2 ticks (decay only starts after 2 ticks)
Expand Down Expand Up @@ -204,15 +209,17 @@ public void testCacheSizeDeprecatedOnNonFrozenNodes() {
)
);
final Settings settings = Settings.builder()
.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), "500b")
.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), "100b")
.put(FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey(), new ByteSizeValue(size(500)).getStringRep())
.put(FrozenCacheService.SNAPSHOT_CACHE_REGION_SIZE_SETTING.getKey(), new ByteSizeValue(size(100)).getStringRep())
.putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), DataTier.DATA_HOT_NODE_ROLE.roleName())
.build();
FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.get(settings);
assertWarnings(
"setting ["
+ FrozenCacheService.SNAPSHOT_CACHE_SIZE_SETTING.getKey()
+ "] to be positive [500b] on node without the data_frozen role is deprecated, roles are [data_hot]"
+ "] to be positive ["
+ new ByteSizeValue(size(500)).getStringRep()
+ "] on node without the data_frozen role is deprecated, roles are [data_hot]"
);
}

Expand Down

0 comments on commit fa2f047

Please sign in to comment.