Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding a deprecation info API check for fractional byte value settings #77074

Merged
merged 4 commits into from
Sep 7, 2021
Merged

Adding a deprecation info API check for fractional byte value settings #77074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab28e18
Adding a deprecation info API check for fractional byte value settings
masseyke Aug 31, 2021
f3d39d3
merging 7.x
masseyke Sep 2, 2021
e098c74
merging 7.x
masseyke Sep 3, 2021
20380ca
Merge branch '7.x' into feature/deprecation-info-fractional-byte-sizes
elasticmachine Sep 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...ugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/DeprecationChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ private DeprecationChecks() {
NodeDeprecationChecks::checkImplicitlyDisabledSecurityOnBasicAndTrial,
NodeDeprecationChecks::checkSearchRemoteSettings,
NodeDeprecationChecks::checkMonitoringExporterPassword,
NodeDeprecationChecks::checkFractionalByteValueSettings,
NodeDeprecationChecks::checkFrozenCacheLeniency,
NodeDeprecationChecks::checkSslServerEnabled,
NodeDeprecationChecks::checkSslCertConfiguration,
Expand Down
32 changes: 30 additions & 2 deletions .../deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.ssl.SslConfigurationKeys;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.util.concurrent.EsExecutors;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.core.TimeValue;
Expand All @@ -36,9 +36,9 @@
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.threadpool.FixedExecutorBuilder;
import org.elasticsearch.transport.RemoteClusterService;
import org.elasticsearch.xpack.core.DataTier;
import org.elasticsearch.transport.SniffConnectionStrategy;
import org.elasticsearch.transport.TransportService;
import org.elasticsearch.xpack.core.DataTier;
import org.elasticsearch.xpack.core.XPackSettings;
import org.elasticsearch.xpack.core.security.SecurityField;
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
Expand All @@ -48,6 +48,7 @@

import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
Expand Down Expand Up @@ -671,6 +672,33 @@ static DeprecationIssue checkClusterRoutingAllocationIncludeRelocationsSetting(f
);
}

static DeprecationIssue checkFractionalByteValueSettings(final Settings settings,
final PluginsAndModules pluginsAndModules,
final ClusterState clusterState,
final XPackLicenseState licenseState) {
Map<String, String> fractionalByteSettings = new HashMap<>();
for (String key : settings.keySet()) {
try {
settings.getAsBytesSize(key, ByteSizeValue.ZERO);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it triggers deprecation logging as well for each fractional value read, do we want that?

String stringValue = settings.get(key);
if (stringValue.contains(".")) {
fractionalByteSettings.put(key, stringValue);
}
} catch (Exception ignoreThis) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not a huge fan of Exception based control flow. I know that settings are registered with ClusterSettings and IndexScopedSettings so that settings can be validated against the master set, but I don't know if we can filter on the setting's data type due to type erasure, and I'm not sure if those are comprehensive for every setting either.

// We expect anything that is not a byte setting to throw an exception, but we don't care about those
}
}
if (fractionalByteSettings.isEmpty()) {
return null;
}
String url = "https://www.elastic.co/guide/en/elasticsearch/reference/master/logging.html#deprecation-logging";
String message = "support for fractional byte size values is deprecated and will be removed in a future release";
String details = "change the following settings to non-fractional values: [" +
fractionalByteSettings.entrySet().stream().map(fractionalByteSetting -> fractionalByteSetting.getKey() + "->" +
fractionalByteSetting.getValue()).collect(Collectors.joining(", ")) + "]";
return new DeprecationIssue(DeprecationIssue.Level.WARNING, message, url, details, false, null);
}

static DeprecationIssue checkFrozenCacheLeniency(final Settings settings,
final PluginsAndModules pluginsAndModules,
final ClusterState clusterState,
Expand Down
28 changes: 28 additions & 0 deletions ...ecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -963,6 +963,34 @@ public void testImplicitlyConfiguredSecurityOnGoldPlus() {
assertThat(issues, empty());
}

public void testCheckFractionalByteValueSettings() {
String settingKey = "network.tcp.send_buffer_size";
String unit = randomFrom(new String[]{"k", "kb", "m", "mb", "g", "gb", "t", "tb", "p", "pb"});
float value = Math.abs(randomFloat());
String settingValue = value + unit;
String unaffectedSettingKey = "some.other.setting";
String unaffectedSettingValue = "54.32.43mb"; //Not an actual number, so we don't expect to see a deprecation log about it
final Settings nodeSettings =
Settings.builder().put(settingKey, settingValue).put(unaffectedSettingKey, unaffectedSettingValue).build();
final XPackLicenseState licenseState = new XPackLicenseState(Settings.EMPTY, () -> 0);
final ClusterState clusterState = ClusterState.EMPTY_STATE;
final DeprecationIssue expectedIssue = new DeprecationIssue(DeprecationIssue.Level.WARNING,
"support for fractional byte size values is deprecated and will be removed in a future release",
"https://www.elastic.co/guide/en/elasticsearch/reference/master/logging.html#deprecation-logging",
String.format(Locale.ROOT,
"change the following settings to non-fractional values: [%s->%s]",
settingKey,
settingValue),
false, null
);
assertThat(
NodeDeprecationChecks.checkFractionalByteValueSettings(nodeSettings, null, clusterState, licenseState),
equalTo(expectedIssue)
);
assertWarnings(String.format(Locale.ROOT, "Fractional bytes values are deprecated. Use non-fractional bytes values instead: [%s] " +
"found for setting [%s]", settingValue, settingKey));
}

public void testCheckFrozenCacheLeniency() {
String cacheSizeSettingValue = "10gb";
String cacheSizeSettingKey = "xpack.searchable.snapshot.shared_cache.size";
Expand Down