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.

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

Conversation

andreidan
Copy link
Contributor

This removes a few legacy index templates that were superseded by
equivalent component templates or updated index templates.

Supersedes #80915

Relates to #80853

This removes a few legacy index templates that were superseeded by
equivalent component templates or updated index templates.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@@ -1531,6 +1531,8 @@ public AsyncSender interceptSender(AsyncSender sender) {
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

@@ -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

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

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.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM

@andreidan
Copy link
Contributor Author

@elasticmachine update branch

@andreidan andreidan merged commit 0ed5eab into elastic:master Nov 23, 2021
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Nov 23, 2021
This removes a few legacy index templates that were superseded by
equivalent component templates or updated index templates.

(cherry picked from commit 0ed5eab)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Nov 23, 2021
* upstream/master:
  Remove obsolete typed legacy index templates (elastic#80937)
  Remove unnecessary shuffle in unassigned shards allocation. (elastic#65172)
  TSDB: Tests for nanosecond timeprecision timestamp just beyond the limit (elastic#80932)

# Conflicts:
#	rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/10_settings.yml
andreidan added a commit that referenced this pull request Nov 23, 2021
This removes a few legacy index templates that were superseded by
equivalent component templates or updated index templates.

(cherry picked from commit 0ed5eab)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Nov 24, 2021
* upstream/master: (29 commits)
  Fix typo (elastic#80925)
  Increase docker compose timeouts for CI builds
  TSDB: fix error without feature flag (elastic#80945)
  [DOCS] Relocate `index.mapping.dimension_fields.limit` setting docs (elastic#80964)
  Explicit write methods for always-missing values (elastic#80958)
  TSDB: move TimeSeriesModeIT to yaml tests (elastic#80933)
  [ML] Removing temporary debug (elastic#80956)
  Remove unused ConnectTransportException#node (elastic#80944)
  Reinterpret dots in field names as object structure (elastic#79922)
  Remove obsolete typed legacy index templates (elastic#80937)
  Remove unnecessary shuffle in unassigned shards allocation. (elastic#65172)
  TSDB: Tests for nanosecond timeprecision timestamp just beyond the limit (elastic#80932)
  Cleanup SLM History Item .equals (elastic#80938)
  Rework breaking changes for new structure (elastic#80907)
  [DOCS] Fix elasticsearch-reset-password typo (elastic#80919)
  [ML] No need to use parent task client when internal infer delegates (elastic#80905)
  Fix shadowed vars pt6 (elastic#80899)
  add ignore info (elastic#80924)
  Fix several potential circuit breaker leaks in Aggregators (elastic#79676)
  Extract more standard metadata from binary files (elastic#78754)
  ...
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Nov 24, 2021
* upstream/master: (319 commits)
  Fix typo (elastic#80925)
  Increase docker compose timeouts for CI builds
  TSDB: fix error without feature flag (elastic#80945)
  [DOCS] Relocate `index.mapping.dimension_fields.limit` setting docs (elastic#80964)
  Explicit write methods for always-missing values (elastic#80958)
  TSDB: move TimeSeriesModeIT to yaml tests (elastic#80933)
  [ML] Removing temporary debug (elastic#80956)
  Remove unused ConnectTransportException#node (elastic#80944)
  Reinterpret dots in field names as object structure (elastic#79922)
  Remove obsolete typed legacy index templates (elastic#80937)
  Remove unnecessary shuffle in unassigned shards allocation. (elastic#65172)
  TSDB: Tests for nanosecond timeprecision timestamp just beyond the limit (elastic#80932)
  Cleanup SLM History Item .equals (elastic#80938)
  Rework breaking changes for new structure (elastic#80907)
  [DOCS] Fix elasticsearch-reset-password typo (elastic#80919)
  [ML] No need to use parent task client when internal infer delegates (elastic#80905)
  Fix shadowed vars pt6 (elastic#80899)
  add ignore info (elastic#80924)
  Fix several potential circuit breaker leaks in Aggregators (elastic#79676)
  Extract more standard metadata from binary files (elastic#78754)
  ...
@danhermann danhermann added >test Issues or PRs that are addressing/adding tests and removed >bug labels Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Monitoring :Data Management/Watcher Team:Data Management Meta label for data/management team >test Issues or PRs that are addressing/adding tests v7.16.0 v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants