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

Remove obsolete typed legacy index templates #80937

Merged
merged 2 commits into from
Nov 23, 2021
Merged

Remove obsolete typed legacy index templates #80937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dace65b
Remove obsolete typed legacy index templates
andreidan Nov 23, 2021
97f6b34
Merge branch 'master' into remove-legacy-templates
elasticmachine Nov 23, 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
12 changes: 12 additions & 0 deletions x-pack/plugin/logstash/src/main/java/org/elasticsearch/xpack/logstash/Logstash.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.IndexTemplateMetadata;
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.IndexScopedSettings;
Expand Down Expand Up @@ -41,7 +42,9 @@
import java.io.UncheckedIOException;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;
import java.util.function.UnaryOperator;

import static org.elasticsearch.index.engine.EngineConfig.INDEX_CODEC_SETTING;
import static org.elasticsearch.index.mapper.MapperService.SINGLE_MAPPING_NAME;
Expand Down Expand Up @@ -191,4 +194,13 @@ public String getFeatureName() {
public String getFeatureDescription() {
return "Enables Logstash Central Management pipeline storage";
}

@Override
public UnaryOperator<Map<String, IndexTemplateMetadata>> getIndexTemplateMetadataUpgrader() {
return templates -> {
// .logstash is a system index now. deleting the legacy template
templates.remove("logstash-index-template");
return templates;
};
}
}
15 changes: 15 additions & 0 deletions x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/Monitoring.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.client.Client;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.IndexTemplateMetadata;
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
Expand Down Expand Up @@ -72,6 +73,7 @@
import java.util.Map;
import java.util.Set;
import java.util.function.Supplier;
import java.util.function.UnaryOperator;

import static org.elasticsearch.common.settings.Setting.boolSetting;

Expand Down Expand Up @@ -236,4 +238,17 @@ public void reload(Settings settings) throws Exception {
exporters.setExportersSetting(settingsForChangedExporter);
}
}

@Override
public UnaryOperator<Map<String, IndexTemplateMetadata>> getIndexTemplateMetadataUpgrader() {
return map -> {
// this template was not migrated to typeless due to the possibility of the old /_monitoring/bulk API being used
// see {@link org.elasticsearch.xpack.core.monitoring.exporter.MonitoringTemplateUtils#OLD_TEMPLATE_VERSION}
// however the bulk API is not typed (the type field is for the docs, a field inside the docs) so it's safe to remove this
// old template and rely on the updated, typeless, .monitoring-alerts-7 template
map.remove(".monitoring-alerts");
return map;
};

}
}
2 changes: 2 additions & 0 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1531,6 +1531,8 @@ public UnaryOperator<Map<String, IndexTemplateMetadata>> getIndexTemplateMetadat
return templates -> {
// .security index is not managed by using templates anymore
templates.remove("security_audit_log");
// .security is a system index now. deleting another legacy template that's not used anymore
templates.remove("security-index-template");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Security now uses a system index. FYI added this code to clean up an ancient typed index template @jkakavas

return templates;
};
}
Expand Down
5 changes: 5 additions & 0 deletions x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,11 @@ public void onIndexModule(IndexModule module) {
public UnaryOperator<Map<String, IndexTemplateMetadata>> getIndexTemplateMetadataUpgrader() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@droberts195 would it be possible to use this existing Plugin infrastructure in MachineLearning?

Copy link
Contributor

Choose a reason for hiding this comment

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

We did use that up until #51765, where we switched to

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/template/IndexTemplateRegistry.java

Line 57 in 51cee9e

public abstract class IndexTemplateRegistry implements ClusterStateListener {

Ironically IndexTemplateRegistry didn't work as well as getIndexTemplateMetadataUpgrader for templates that need to be up-to-date in the mixed version states of rolling upgrade, so we had to add our own code to ensure the most recent templates are used in certain rolling upgrade situations.

So maybe if getIndexTemplateMetadataUpgrader worked for composable templates then we could go back to it. But isn't IndexTemplateMetadata just for legacy templates? ML no longer has any legacy templates. All the ML indices are either system indices or are using composable templates now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry, I just looked at the code and saw you used it to remove old indices. Yes, I guess we could do it that way now too.

Copy link
Contributor

@droberts195 droberts195 Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, based on a recent test failure I think there's a complication and the ML approach is better for 7.16.

With the approach of this PR, in a rolling upgrade in 7.16 the template will get removed when the first node gets upgraded to 7.16. But there could be some nodes in the cluster that only understand legacy templates.

I suspect it's the reason behind https://gradle-enterprise.elastic.co/s/lz76r6wwt3eic for example, which is rolling from 7.5.2 to 7.16.0.

I used your approach for transforms in #80948 because it's cleaner for 8.0+ where we can be sure that the version being upgraded from will understand the replacement composable templates. Transforms didn't exist in 6.x so there's no chance its templates include types and it's safe to wait until 8.0 to delete its legacy templates. But it seems that for the features that existed in 6.x, 7.16 needs the ability to only remove the legacy templates if the oldest node in the cluster understands the replacement (composable templates or system indices depending on the index).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect it's the reason behind https://gradle-enterprise.elastic.co/s/lz76r6wwt3eic for example, which is rolling from 7.5.2 to 7.16.0.

Would this be a different problem though? More of a problem with how we install templates more so than how we delete templates?

In a rolling upgrade the master eligible nodes should be rolled last (as per our rolling upgrade documentation). So the 7.16 Plugin code that removes the legacy templates will be available once all the master ineligible nodes are updated. The upgrade is perform by the new 7.16 master - TemplateUpgradeService only performs the upgrade on the master node.

I'm probably missing something, but I don't understand what the "manual" way of removing templates offers vs the infrastructure in Plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

In a rolling upgrade the master eligible nodes should be rolled last (as per our rolling upgrade documentation).

I think that's only a recommendation, not compulsory.

A problem will only arise if something is done in the mixed version cluster that causes an index to need to be created using the old template. For example, if after upgrading half the nodes from 7.5.2 to 7.16.0 a user creates an ML job that causes a new ML results index to be created and that gets actioned by a 7.5.2 node that needs the legacy template. Maybe you are correct that it's impossible for a legacy template to be needed after the upgraded master node has deleted it, but the effect of ML indices getting created without mappings causes complicated support issues so for ML I would rather stick with code that doesn't remove the legacy templates until the entire cluster is upgraded.

Copy link
Member

Choose a reason for hiding this comment

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

With the approach of this PR, in a rolling upgrade in 7.16 the template will get removed when the first node gets upgraded to 7.16. But there could be some nodes in the cluster that only understand legacy templates.

IIRC the template upgrader is only active on master nodes. So data nodes can be upgraded and nothing will change to the templates. Given that new indices are created on the elected master nodes this shouldn't be an issue? Either legacy template will be applied when creating the index before upgrading elected master node or composable index / system index logic will kick after the upgrading elected master node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martijnvg ++ The TemplateUpgradeService does not perform any changes unless the local node is the master. https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/cluster/metadata/TemplateUpgradeService.java#L95

return map -> {
map.keySet().removeIf(name -> name.startsWith("watch_history_"));
// watcher migrated to using system indices so these legacy templates are not needed anymore
map.remove(".watches");
map.remove(".triggered_watches");
// post 7.x we moved to typeless watch-history-10
map.remove(".watch-history-9");
return map;
};
}
Expand Down
32 changes: 32 additions & 0 deletions x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/WatcherRestartIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,14 @@
import org.apache.http.util.EntityUtils;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseException;

import java.nio.charset.StandardCharsets;
import java.util.Map;
import java.util.concurrent.TimeUnit;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;

public class WatcherRestartIT extends AbstractUpgradeTestCase {
Expand All @@ -25,6 +29,34 @@ public void testWatcherRestart() throws Exception {
ensureWatcherStarted();
}

public void testEnsureWatcherDeletesLegacyTemplates() throws Exception {
client().performRequest(new Request("POST", "/_watcher/_start"));
ensureWatcherStarted();

if (CLUSTER_TYPE.equals(ClusterType.UPGRADED)) {
// legacy index template created in previous releases should not be present anymore
assertBusy(() -> {
Request request = new Request("GET", "/_template/*watch*");
try {
Response response = client().performRequest(request);
Map<String, Object> responseLevel = entityAsMap(response);
assertNotNull(responseLevel);

assertThat(responseLevel.containsKey(".watches"), is(false));
assertThat(responseLevel.containsKey(".triggered_watches"), is(false));
assertThat(responseLevel.containsKey(".watch-history-9"), is(false));
} catch (ResponseException e) {
// Not found is fine
assertThat(
"Unexpected failure getting templates: " + e.getResponse().getStatusLine(),
e.getResponse().getStatusLine().getStatusCode(),
is(404)
);
}
}, 30, TimeUnit.SECONDS);
}
}

private void ensureWatcherStopped() throws Exception {
assertBusy(() -> {
Response stats = client().performRequest(new Request("GET", "_watcher/stats"));
Expand Down