Register Feature migration persistent task state named XContent #84192

pgomulka · 2022-02-21T13:29:44Z

This PR properly registers NamedXContentRegistry entries for SystemIndexMigrationTaskParams and SystemIndexMigrationTaskState. It also adds tests for the XContent de/serialization for those classes,
and fixes a bug revealed by these tests in SystemIndexMigrationTaskState's parser.

Finally, it adds an integration test which simulates the conditions in which #84115 occurs: A node restart
while the migration is in progress. This ensures that we have fixed that particular bug.

Closes #84115

To add context for anyone looking at this fix as it applies to #84115 which is currently occurring in a
live cluster: Because this fix only changes how these classes are read, not how they are written,
installing a version which includes this fix should be sufficient to allow the impacted nodes to start.

Registering SystemIndexMigrationTaskParams and SystemIndexMigrationTaskState parsers into named xcontent registry. closes elastic#84115

elasticmachine · 2022-02-21T13:29:48Z

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

elasticsearchmachine · 2022-02-21T13:30:25Z

Hi @pgomulka, I've created a changelog YAML for you.

…thub.com:pgomulka/elasticsearch into main/systemindexmigration_register_namedxcontent

pgomulka · 2022-02-21T14:55:21Z

I wonder if the problems from this PR together with #84142 are enough?
Should we try to have an integration test that would actually create that SystemIndexMigrationTask and attempt to read the cluster state from disk? (no idea if it is possible)

DaveCTurner · 2022-02-21T15:23:40Z

Copying the content of our Slack conversation here:

I think we should have a test that restarts a node while the upgrade task is running. Assert written metadata is readable #84142 would have caught this specific problem, but "can read the metadata without throwing an exception" is a pretty weak property and there may be other problems related to restarting nodes while this task is running. Although the task is usually short-lived, it should be possible to write an ESIntegTestCase which adds a ClusterStateListener that blocks the master thread when it sees the task, and then restarts a node when the block happens.
Apart from that comment about testing, I'd rather have a core/infra person do the detailed review of the changes.

pgomulka · 2022-02-22T08:42:31Z

server/src/test/java/org/elasticsearch/upgrades/SystemIndexMigrationTaskStateTests.java

            case 2:
                return randomLong();
            case 3:
-                return randomShort();


I have removed short and float from there because when serialised/deserialised jackson will read this as integer/double

DaveCTurner

Test looks good, I left a couple of suggestions.

...reindex/src/internalClusterTest/java/org/elasticsearch/migration/SystemIndexMigrationIT.java

DaveCTurner · 2022-02-22T09:10:44Z

...reindex/src/internalClusterTest/java/org/elasticsearch/migration/SystemIndexMigrationIT.java

+        AtomicBoolean hasBlocked = new AtomicBoolean();
+
+        final String masterName = internalCluster().startMasterOnlyNode();
+        final String dataNodeName = internalCluster().startDataOnlyNode();


I think I called this a "data node" when discussing the test but in fact it would be preferable to test it for master nodes. E.g. #84148 would drop the bad metadata on non-master nodes so this test wouldn't do anything any more.

…/migration/SystemIndexMigrationIT.java Co-authored-by: David Turner <[email protected]>

…thub.com:pgomulka/elasticsearch into main/systemindexmigration_register_namedxcontent

pgomulka · 2022-02-22T10:40:16Z

.../internalClusterTest/java/org/elasticsearch/migration/AbstractFeatureMigrationIntegTest.java

+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+
+public abstract class AbstractFeatureMigrationIntegTest extends ESIntegTestCase {


all of these tests share the same setup, so I think it is better they have it in one abstract class common to them all, rather than extending from the base test (I don't think MultiFeatureMigrationIT was meant to run tests from FeatureMigrationIT, but maybe @gwbrown can confirm?)
I was considering moving this all to utility class, but I also want to make sure that cleanup of static hooks (preMigrationHook and postMigrationHook ) is performed before every test. I had a failure due to lack of this cleanup https://gradle-enterprise.elastic.co/s/z75xl73xezq36/tests/:modules:reindex:internalClusterTest/org.elasticsearch.migration.SystemIndexMigrationIT/testSystemIndexMigrationCanBeInterruptedWithShutdown?top-execution=1

Extracting this to a separate class is quite reasonable. MultiFeatureMigrationIT was to some degree intended to run the tests from FeatureMigrationIT as well as its own, but it's very possible that's not the best way to handle that, and I don't think we lose much test coverage as the test in MultiFeatureMigrationIT should cover those cases alright.

++ to adding automatic cleanup, we should have had it to start with.

elasticsearchmachine · 2022-02-22T22:53:02Z

💔 Backport failed

The backport operation could not be completed due to the following error:
An unexpected error occurred when attempting to backport this PR.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 84192

…tic#84192) This PR properly registers `NamedXContentRegistry` entries for `SystemIndexMigrationTaskParams` and `SystemIndexMigrationTaskState`. It also adds tests for the XContent de/serialization for those classes, and fixes a bug revealed by these tests in `SystemIndexMigrationTaskState`'s parser. Finally, it adds an integration test which simulates the conditions in which elastic#84115 occurs: A node restart while the migration is in progress. This ensures that we have fixed that particular bug. Co-authored-by: David Turner <[email protected]> Co-authored-by: Gordon Brown <[email protected]> (cherry picked from commit 4c3b7b1)

This PR properly registers `NamedXContentRegistry` entries for `SystemIndexMigrationTaskParams` and `SystemIndexMigrationTaskState`. It also adds tests for the XContent de/serialization for those classes, and fixes a bug revealed by these tests in `SystemIndexMigrationTaskState`'s parser. Finally, it adds an integration test which simulates the conditions in which #84115 occurs: A node restart while the migration is in progress. This ensures that we have fixed that particular bug. Co-authored-by: David Turner <[email protected]> Co-authored-by: Gordon Brown <[email protected]> (cherry picked from commit 4c3b7b1) Co-authored-by: Przemyslaw Gomulka <[email protected]>

…tic#84192) This PR properly registers `NamedXContentRegistry` entries for `SystemIndexMigrationTaskParams` and `SystemIndexMigrationTaskState`. It also adds tests for the XContent de/serialization for those classes, and fixes a bug revealed by these tests in `SystemIndexMigrationTaskState`'s parser. Finally, it adds an integration test which simulates the conditions in which elastic#84115 occurs: A node restart while the migration is in progress. This ensures that we have fixed that particular bug. Co-authored-by: David Turner <[email protected]> Co-authored-by: Gordon Brown <[email protected]> (cherry picked from commit 4c3b7b1)

gwbrown · 2022-02-23T01:32:00Z

💚 All backports created successfully

Status	Branch	Result
✅	8.1
✅	8.0
✅	7.17

Questions ?

Please refer to the Backport tool documentation

#84192) (#84258) In addition to the backport, this commit also adds SystemIndexMigrationTaskParams/State to writeableRegistry of transport client This will backport the following commits from master to 7.17: Register Feature migration persistent task state named XContent (#84192)

A follow up after elastic#84192 - refactor the static state in TestPlugin to be an instance - refactor assertions to use hamcrest - remove Simple from methods as it is not meaningful - refactor xcontent tests to support unknown fields closes elastic#84245

…tic#84192) This PR properly registers `NamedXContentRegistry` entries for `SystemIndexMigrationTaskParams` and `SystemIndexMigrationTaskState`. It also adds tests for the XContent de/serialization for those classes, and fixes a bug revealed by these tests in `SystemIndexMigrationTaskState`'s parser. Finally, it adds an integration test which simulates the conditions in which elastic#84115 occurs: A node restart while the migration is in progress. This ensures that we have fixed that particular bug. Co-authored-by: David Turner <[email protected]> Co-authored-by: Gordon Brown <[email protected]>

This PR properly registers `NamedXContentRegistry` entries for `SystemIndexMigrationTaskParams` and `SystemIndexMigrationTaskState`. It also adds tests for the XContent de/serialization for those classes, and fixes a bug revealed by these tests in `SystemIndexMigrationTaskState`'s parser. Finally, it adds an integration test which simulates the conditions in which #84115 occurs: A node restart while the migration is in progress. This ensures that we have fixed that particular bug. Co-authored-by: David Turner <[email protected]> Co-authored-by: Gordon Brown <[email protected]> Co-authored-by: Przemyslaw Gomulka <[email protected]>

A follow up after #84192 refactor the static state in TestPlugin to be an instance refactor assertions to use hamcrest remove Simple from methods as it is not meaningful refactor xcontent tests to support unknown fields closes #84245

A follow up after elastic#84192 refactor the static state in TestPlugin to be an instance refactor assertions to use hamcrest remove Simple from methods as it is not meaningful refactor xcontent tests to support unknown fields closes elastic#84245

Registration of SystemIndexMigrationTask named xcontent objects

be8326b

Registering SystemIndexMigrationTaskParams and SystemIndexMigrationTaskState parsers into named xcontent registry. closes elastic#84115

pgomulka added :Core/Infra/Core Core issues without another label auto-backport Automatically create backport pull requests when merged v8.1.1 v8.0.2 labels Feb 21, 2022

pgomulka self-assigned this Feb 21, 2022

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Feb 21, 2022

pgomulka added the >bug label Feb 21, 2022

elasticsearchmachine added the v8.2.0 label Feb 21, 2022

pgomulka added 3 commits February 21, 2022 14:30

Update docs/changelog/84192.yaml

dca8955

fix compile

7a31be8

Merge branch 'main/systemindexmigration_register_namedxcontent' of gi…

f49057f

…thub.com:pgomulka/elasticsearch into main/systemindexmigration_register_namedxcontent

pgomulka requested review from DaveCTurner and gwbrown February 21, 2022 14:38

pgomulka requested a review from rjernst February 21, 2022 15:54

integration test

c15df74

pgomulka mentioned this pull request Feb 22, 2022

Registration of SystemIndexMigrationTask named xcontent objects #84136

Closed

pgomulka added the v7.17.1 label Feb 22, 2022

pgomulka commented Feb 22, 2022

View reviewed changes

DaveCTurner reviewed Feb 22, 2022

View reviewed changes

pgomulka and others added 4 commits February 22, 2022 10:18

Update modules/reindex/src/internalClusterTest/java/org/elasticsearch…

9394447

…/migration/SystemIndexMigrationIT.java Co-authored-by: David Turner <[email protected]>

change the data node to be master node as well

94eae1d

Merge branch 'main/systemindexmigration_register_namedxcontent' of gi…

da88972

…thub.com:pgomulka/elasticsearch into main/systemindexmigration_register_namedxcontent

refactor the common setup into abstract class

d637f1a

pgomulka commented Feb 22, 2022

View reviewed changes

pgomulka requested a review from DaveCTurner February 22, 2022 11:08

gwbrown changed the title ~~Registration of SystemIndexMigrationTask named xcontent objects~~ Register Feature migration persistent task state named XContent Feb 22, 2022

gwbrown merged commit 4c3b7b1 into elastic:master Feb 22, 2022

gwbrown mentioned this pull request Feb 22, 2022

[8.1] Register Feature migration persistent task state named XContent (#84192) #84251

Merged

gwbrown mentioned this pull request Feb 22, 2022

[8.0] Register Feature migration persistent task state named XContent (#84192) #84252

Merged

gwbrown added the backport pending label Feb 23, 2022

gwbrown mentioned this pull request Feb 23, 2022

[7.17] Register Feature migration persistent task state named XContent (#84192) #84258

Merged

pugnascotia added v7.17.2 v7.17.1 and removed v7.17.1 v7.17.2 labels Feb 23, 2022

pgomulka mentioned this pull request Feb 23, 2022

Cleanup SystemIndexMigration tests #84281

Merged

gwbrown removed the backport pending label Feb 23, 2022

pgomulka deleted the main/systemindexmigration_register_namedxcontent branch July 25, 2022 13:38

ChrisHegarty unassigned pgomulka Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Register Feature migration persistent task state named XContent #84192

Register Feature migration persistent task state named XContent #84192

Uh oh!

pgomulka commented Feb 21, 2022 •

edited by gwbrown

Loading

Uh oh!

elasticmachine commented Feb 21, 2022

Uh oh!

elasticsearchmachine commented Feb 21, 2022

Uh oh!

pgomulka commented Feb 21, 2022

Uh oh!

DaveCTurner commented Feb 21, 2022

Uh oh!

pgomulka Feb 22, 2022

Uh oh!

DaveCTurner left a comment

Uh oh!

Uh oh!

DaveCTurner Feb 22, 2022

Uh oh!

pgomulka Feb 22, 2022

Uh oh!

gwbrown Feb 22, 2022

Uh oh!

elasticsearchmachine commented Feb 22, 2022

Uh oh!

gwbrown commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Register Feature migration persistent task state named XContent #84192

Register Feature migration persistent task state named XContent #84192

Uh oh!

Conversation

pgomulka commented Feb 21, 2022 • edited by gwbrown Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 21, 2022

Uh oh!

elasticsearchmachine commented Feb 21, 2022

Uh oh!

pgomulka commented Feb 21, 2022

Uh oh!

DaveCTurner commented Feb 21, 2022

Uh oh!

pgomulka Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaveCTurner Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

pgomulka Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

gwbrown Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Feb 22, 2022

💔 Backport failed

Uh oh!

gwbrown commented Feb 23, 2022

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pgomulka commented Feb 21, 2022 •

edited by gwbrown

Loading