Implement framework for migrating system indices #78951

gwbrown · 2021-10-12T01:49:02Z

This PR adds a framework for migrating system indices as necessary prior
to Elasticsearch upgrades. This framework uses REST APIs added in
another commit:

GET _migration/system_features

This API, which gets the status of "features" (plugins which own system
indices) with regards to whether they need to be upgraded or not. As of
this PR, this API also reports errors encountered while migrating system
indices alongside the index that was being processed when this occurred.

As an example of this error reporting:

{
    "feature_name": "logstash_management",
    "minimum_index_version": "8.0.0",
    "upgrade_status": "ERROR",
    "indices": [
        {
            "index": ".logstash",
            "version": "8.0.0",
            "failure_cause": {
                "error": {
                    "root_cause": [
                        {
                            "type": "runtime_exception",
                            "reason": "whoopsie",
                            "stack_trace": "<omitted for brevity>"
                        }
                    ],
                    "type": "runtime_exception",
                    "reason": "whoopsie",
                    "stack_trace": "<omitted for brevity>"
                }
            }
        }
    ]
}

POST _migration/system_features

This API starts the migration process. The API for this has no changes,
but when called, any system indices which need to be migrated will be
migrated, with status information stored in the cluster state for later
use by the GET _migration/system_features API.

This PR adds a framework for migrating system indices as necessary prior to Elasticsearch upgrades. This framework uses REST APIs added in another commit: - GET _migration/system_features This API, which gets the status of "features" (plugins which own system indices) with regards to whether they need to be upgraded or not. As of this PR, this API also reports errors encountered while migrating system indices alongside the index that was being processed when this occurred. As an example of this error reporting: ```json { "feature_name": "logstash_management", "minimum_index_version": "8.0.0", "upgrade_status": "ERROR", "indices": [ { "index": ".logstash", "version": "8.0.0", "failure_cause": { "error": { "root_cause": [ { "type": "runtime_exception", "reason": "whoopsie", "stack_trace": "<omitted for brevity>" } ], "type": "runtime_exception", "reason": "whoopsie", "stack_trace": "<omitted for brevity>" } } } ] } ``` - POST _migration/system_features This API starts the migration process. The API for this has no changes, but when called, any system indices which need to be migrated will be migrated, with status information stored in the cluster state for later use by the GET _migration/system_features API.

elasticmachine · 2021-10-13T20:54:26Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

williamrandolph

I've read through all the code so far and I like it. I have a few comments and requests, and I will be happy to review again once we have some good tests. But overall: very impressive!

williamrandolph · 2021-10-13T20:56:07Z

...org/elasticsearch/action/admin/cluster/migration/TransportGetFeatureUpgradeStatusAction.java

+        GetFeatureUpgradeStatusResponse.UpgradeStatus initialStatus;
+        if (featureName.equals(currentFeature)) {
+            initialStatus = IN_PROGRESS;
+        } else if (minimumVersion.before(Version.V_7_0_0)) {


We should probably pull this version number out into a constant to make it more visible and to use it in tests.

In fact, I half-heartedly introduced one while backporting to 7.x in order to get bwc tests working: 3b43555

williamrandolph · 2021-10-13T21:05:56Z

server/src/main/java/org/elasticsearch/indices/SystemIndices.java

@@ -593,6 +593,7 @@ public static void validateFeatureName(String name, String plugin) {
     * Class holding a description of a stateful feature.
     */
    public static class Feature {
+        private final String name;


Now that I see this field, it's obvious that it always should have been here.

williamrandolph · 2021-10-14T14:37:33Z

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrationTaskState.java

+    private final String currentFeature;
+    private final Map<String, Object> featureCallbackMetadata;
+
+    public SystemIndexMigrationTaskState(String currentIndex, String currentFeature, Map<String, Object> featureCallbackMetadata) {


Suggestion: A little javadoc explaining what the feature callback metadata contains and where to find more information about it

SystemIndexMigrationResult -> FeatureMigrationResults FeatureMigrationStatus -> SingleFeatureMigrationResult

williamrandolph

This is looking really, really good. I have some nitpicks and testing requests, but nothing substantial. However, I'm going to mark it as "request changes" anyway so I can get a notification when commits from end-to-end testing make it here.

One thing I want to look at is making sure that, at the end of the process, all the original aliases are pointing at the migrated index.

Great work!

...n/java/org/elasticsearch/action/admin/cluster/migration/GetFeatureUpgradeStatusResponse.java

williamrandolph · 2021-10-19T00:55:21Z

...les/reindex/src/internalClusterTest/java/org/elasticsearch/migration/FeatureMigrationIT.java

+        assertIndexHasCorrectProperties(finalMetadata, ".ext-unman-old-reindexed-for-8", EXTERNAL_UNMANAGED_FLAG_VALUE, false, false);
+    }
+
+    public void assertIndexHasCorrectProperties(


Can we add some assertions about aliases to this method? If I'm understanding things right, the original index name should now be an alias pointing to the migrated index.

Done! And yes, that should be the case. I've also updated the code to copy any aliases the old index had to the new index, which was my original intent.

williamrandolph · 2021-10-19T00:56:25Z

...les/reindex/src/internalClusterTest/java/org/elasticsearch/migration/FeatureMigrationIT.java

+    static final int EXTERNAL_UNMANAGED_FLAG_VALUE = 4;
+    static final SystemIndexDescriptor INTERNAL_MANAGED = SystemIndexDescriptor.builder()
+        .setIndexPattern(".int-man-*")
+        .setAliasName(".internal-managed-alias")


Should we create the aliases for the managed indices in these tests?

The aliases for the managed indices should be created automatically at index creation time. The tests now verify that the final, post-migration managed indices have the alias set in the descriptor.

williamrandolph · 2021-10-19T01:00:21Z

...eindex/src/internalClusterTest/java/org/elasticsearch/migration/MultiFeatureMigrationIT.java

+        SetOnce<Boolean> secondPluginPostMigrationHookCalled = new SetOnce<>();
+
+        TestPlugin.preMigrationHook.set(clusterState -> {
+            // None of the other hooks should have been called yet.


I really like this way of verifying the order in which these hooks are called.

Thanks! I can't take credit for it, I picked this style up from uh... somewhere else in Elasticsearch, not sure where exactly.

williamrandolph · 2021-10-19T01:02:16Z

...n/java/org/elasticsearch/action/admin/cluster/migration/GetFeatureUpgradeStatusResponse.java

@@ -199,34 +211,45 @@ public String toString() {
                "featureName='" + featureName + '\'' +
                ", minimumIndexVersion='" + minimumIndexVersion + '\'' +
                ", upgradeStatus='" + upgradeStatus + '\'' +
-                ", indexVersions=" + indexVersions +
+                ", indexVersions=" + indexInfos +


A very tiny nitpick: indexVersions= could be changed to indexInfos=.

williamrandolph · 2021-10-19T01:05:32Z

...org/elasticsearch/action/admin/cluster/migration/TransportGetFeatureUpgradeStatusAction.java

    }

    // visible for testing
-    static List<GetFeatureUpgradeStatusResponse.IndexVersion> getIndexVersions(ClusterState state, SystemIndices.Feature feature) {
+    static List<GetFeatureUpgradeStatusResponse.IndexInfo> getIndexVersions(ClusterState state, SystemIndices.Feature feature) {


nit: We could rename this method as well

williamrandolph · 2021-10-19T01:23:15Z

...lasticsearch/action/admin/cluster/migration/TransportGetFeatureUpgradeStatusActionTests.java


-        assertThat(status.getUpgradeStatus(), equalTo(NO_UPGRADE_NEEDED));
+        assertThat(status.getUpgradeStatus(), equalTo(UPGRADE_NEEDED));
        assertThat(status.getFeatureName(), equalTo("test-feature"));
        assertThat(status.getMinimumIndexVersion(), equalTo(Version.V_7_0_0));
        assertThat(status.getIndexVersions().size(), equalTo(2)); // additional testing below
    }

    public void testGetIndexVersion() {


tiny, tiny nit: rename to testGetIndexInfos if we rename the getIndexVersions method.

williamrandolph

The code since my last review is very straightforward, and testing a build of the branch worked like a charm.

This is looking really great. Thank you so much for all the good work here.

Also re-enables BWC following the backport of elastic#78951.

* upstream/master: (24 commits) Implement framework for migrating system indices (elastic#78951) Improve transient settings deprecation message (elastic#79504) Remove getValue and getValues from Field (elastic#79516) Store Template's mappings as bytes for disk serialization (elastic#78746) [ML] Add queue_capacity setting to start deployment API (elastic#79433) [ML] muting rest compat test issue elastic#79518 (elastic#79519) Avoid redundant available indices check (elastic#76540) Re-enable BWC tests TEST Ensure password 14 chars length on Kerberos FIPS tests (elastic#79496) [DOCS] Temporarily remove APM links (elastic#79411) Fix CCSDuelIT for skipped shards (elastic#79490) Add other time accounting in HotThreads (elastic#79392) Add deprecation info API entries for deprecated monitoring settings (elastic#78799) Add note in breaking changes for nameid_format (elastic#77785) Use 'migration' instead of 'upgrade' in GET system feature migration status responses (elastic#79302) Upgrade lucene version 8b68bf60c98 (elastic#79461) Use Strings#EMPTY_ARRAY (elastic#79452) Quicker shared cache file preallocation (elastic#79447) [ML] Removing some code that's obsolete for 8.0 (elastic#79444) Ensure indexing_data CCR requests are compressed (elastic#79413) ...

This PR adds a framework for migrating system indices as necessary prior to Elasticsearch upgrades. This framework uses REST APIs added in another commit: - GET _migration/system_features This API, which gets the status of "features" (plugins which own system indices) with regards to whether they need to be upgraded or not. As of this PR, this API also reports errors encountered while migrating system indices alongside the index that was being processed when this occurred. As an example of this error reporting: ```json { "feature_name": "logstash_management", "minimum_index_version": "8.0.0", "upgrade_status": "ERROR", "indices": [ { "index": ".logstash", "version": "8.0.0", "failure_cause": { "error": { "root_cause": [ { "type": "runtime_exception", "reason": "whoopsie", "stack_trace": "<omitted for brevity>" } ], "type": "runtime_exception", "reason": "whoopsie", "stack_trace": "<omitted for brevity>" } } } ] } ``` - POST _migration/system_features This API starts the migration process. The API for this has no changes, but when called, any system indices which need to be migrated will be migrated, with status information stored in the cluster state for later use by the GET _migration/system_features API.

Also re-enables BWC following the backport of #78951.

gwbrown added 2 commits October 11, 2021 19:37

Remove extra onResponse

1371cd2

gwbrown requested a review from williamrandolph October 12, 2021 01:49

elasticsearchmachine added the v8.0.0 label Oct 12, 2021

williamrandolph mentioned this pull request Oct 12, 2021

Use correct terms in GET feature migration status API #79017

Closed

gwbrown added :Core/Infra/Core Core issues without another label >enhancement v7.16.0 labels Oct 13, 2021

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Oct 13, 2021

gwbrown added 6 commits October 13, 2021 15:35

Address Todos/cleanup

d528882

Return the correct list when starting migration

5f8b65e

Tweak docs

dabad50

Merge branch 'master' into si/upgrade-framework

5cdff06

Merge branch 'master' into si/upgrade-framework

4c4d4b7

Fix compilation after merge

acdcd34

williamrandolph suggested changes Oct 14, 2021

View reviewed changes

gwbrown added 13 commits October 14, 2021 15:15

Merge branch 'master' into si/upgrade-framework

4fe3373

Fix test failures that expected fake responses

1961c71

Extract constant for upgrade version per review

83415e4

Javadoc + cleanup per review

932c0d0

Rename results classes to clarify their relationship

08f22f1

SystemIndexMigrationResult -> FeatureMigrationResults FeatureMigrationStatus -> SingleFeatureMigrationResult

Merge branch 'master' into si/upgrade-framework

7afd137

Tests + always run on master node

95356d9

Fix index name & doc count type

4b2bbbf

Javadoc

fae67c0

Plumb in callback metadata + test it

4552e75

Merge branch 'master' into si/upgrade-framework

9cec136

Line length

a84e781

Verify that the pre/post migration hooks are called

bad48b2

gwbrown added 2 commits October 18, 2021 14:05

Cleanup

cd11bce

Merge branch 'master' into si/upgrade-framework

6c9c9da

gwbrown requested a review from williamrandolph October 18, 2021 20:22

Fix unit tests for new logic

1ac86ad

williamrandolph suggested changes Oct 19, 2021

View reviewed changes

gwbrown added 3 commits October 19, 2021 10:53

Javadoc + missed renames per review

c753724

Copy aliases + alias assertions

cf61671

Merge branch 'master' into si/upgrade-framework

9bddfbb

gwbrown requested a review from williamrandolph October 19, 2021 21:01

gwbrown added 6 commits October 19, 2021 15:30

Add necessary system privileges

76e8aed

Merge branch 'master' into si/upgrade-framework

53da87f

Logging tweaks

427389e

Fix test for real version math

59b70a0

Adjust test for new system permissions

124c6d8

Merge branch 'master' into si/upgrade-framework

34fb47d

gwbrown mentioned this pull request Oct 20, 2021

[7.x] Implement framework for migrating system indices (#78951) #79525

Merged

williamrandolph approved these changes Oct 20, 2021

View reviewed changes

gwbrown merged commit 1a90689 into elastic:master Oct 20, 2021

gwbrown added the backport pending label Oct 20, 2021

gwbrown added a commit that referenced this pull request Oct 20, 2021

Disable BWC tests for backport of #78951

6e611ac

gwbrown added a commit to gwbrown/elasticsearch that referenced this pull request Oct 20, 2021

Adjust serialization versions for migration metadata

501c680

Also re-enables BWC following the backport of elastic#78951.

gwbrown mentioned this pull request Oct 20, 2021

Adjust serialization versions for migration metadata & re-enable BWC #79530

Merged

elasticsearchmachine pushed a commit that referenced this pull request Oct 20, 2021

Adjust serialization versions for migration metadata (#79530)

ca0d435

Also re-enables BWC following the backport of #78951.

gwbrown removed the backport pending label Oct 20, 2021

gwbrown mentioned this pull request Oct 26, 2021

Prototype for system index upgrade using persistent tasks framework #77525

Closed

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

gwbrown mentioned this pull request Nov 9, 2021

Provide an upgrade path for the .tasks index from 7.x to 8.x #38107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement framework for migrating system indices #78951

Implement framework for migrating system indices #78951

gwbrown commented Oct 12, 2021 •

edited

Loading

elasticmachine commented Oct 13, 2021

williamrandolph left a comment

williamrandolph Oct 13, 2021

williamrandolph Oct 13, 2021

williamrandolph Oct 13, 2021

williamrandolph Oct 14, 2021

williamrandolph left a comment

williamrandolph Oct 19, 2021

gwbrown Oct 19, 2021

williamrandolph Oct 19, 2021

gwbrown Oct 19, 2021

williamrandolph Oct 19, 2021

gwbrown Oct 19, 2021

williamrandolph Oct 19, 2021

williamrandolph Oct 19, 2021

williamrandolph Oct 19, 2021

williamrandolph left a comment

Implement framework for migrating system indices #78951

Implement framework for migrating system indices #78951

Conversation

gwbrown commented Oct 12, 2021 • edited Loading

elasticmachine commented Oct 13, 2021

williamrandolph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamrandolph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamrandolph left a comment

Choose a reason for hiding this comment

gwbrown commented Oct 12, 2021 •

edited

Loading