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

Many backwards compatibility tests are broken in 2.x #13522

Closed
nik9000 opened this Issue Sep 11, 2015 · 12 comments

Comments

Projects
None yet
3 participants
@nik9000
Contributor

nik9000 commented Sep 11, 2015

I'm working on getting the backwards compatibility tests in 2.x but BasicBackwardsCompatibilityIT is proving to be very difficult so I'm pulling it out into a separate issue. This issue.

  • SnapshotBackwardsCompatibilityIT (#14845)
  • SignificantTermsBackwardCompatibilityIT (#14948)
  • FunctionScoreBackwardCompatibilityIT (#15332)
  • RecoveryBackwardsCompatibilityIT (#14817)

@nik9000 nik9000 changed the title from BasicBackwardsCompatibilityIT is mostly broken in 2.x to Many backwards compatibility tests are broken in 2.x Sep 14, 2015

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Sep 14, 2015

Contributor

I'm finding more and more and more backwards compatibility tests failing. Each one is a rabbit hole I can lose several hours to. I'm going to spend no more than an hour on each and if I can't fix it then I'll just mark it AwaitsFix with this bug.

Contributor

nik9000 commented Sep 14, 2015

I'm finding more and more and more backwards compatibility tests failing. Each one is a rabbit hole I can lose several hours to. I'm going to spend no more than an hour on each and if I can't fix it then I'll just mark it AwaitsFix with this bug.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Oct 23, 2015

Contributor

There are a few things @AwaitsFixed on this issue. Also we suppress any index.data_path set on the indexes during the BWC tests and we probably shouldn't have to do that. We should figure that out as part of this issue as well.

Contributor

nik9000 commented Oct 23, 2015

There are a few things @AwaitsFixed on this issue. Also we suppress any index.data_path set on the indexes during the BWC tests and we probably shouldn't have to do that. We should figure that out as part of this issue as well.

nik9000 added a commit to nik9000/elasticsearch that referenced this issue Oct 30, 2015

[TEST] Fix backwards compatibility tests for 2.x
Moves backwards compatibility tests from core to qa/backwards and uses a
service running inside of maven to fork the elasticsearch nodes so the tests
can continue to run in seccomp.

Because the tests were moved ReproduceInfoPrinter now cannot hardcode which
parameters it prints - it scans them using a new annotation.

`CompositeTestCluster#client` now remakes its client with new node addresses
after every node is upgraded. That makes it possiblie for it to survive a
rolling restart where the new nodes come up using different ports.

`CompositeTestCluster#upgradeOneNode` now brings up the upgraded node with
the same data directory as the old node which prevents the loss of data when
there are no replicas.

`ExternalNode` now uses a `unicastHosts` list to find the other running nodes
rather than relying on multicast which was moved to a plugin in 2.0.
`unicastHosts` is carefully setup by `CompositeTestCluster` to list all the
other nodes that are currently running.

`FunctionScoreBackwardCompatibilityIT` now forces a single shard so the tf/idf
parts of the score don't get juggled.

`ReproduceInfoPrinter` now appends properties required to properly reproduce
the backwards compatibility tests and properly quotes parameters like
`-Dtests.filter="@Backwards and not @awaitsfix`.

About 2/3 of the backwards compatibility tests are passing and the rest are
marked with
`@AwaitsFix(bugUrl="elastic#13522
That issue will track all work getting the backwards compatibility tests
back online. The following tests are so marked:
- BasicAnalysisBackwardCompatibilityIT
- BasicBackwardsCompatibilityIT
- RecoveryBackwardsCompatibilityIT
- SignificantTermsBackwardCompatibilityIT
- SnapshotBackwardsCompatibilityIT

Closes #13425

nik9000 added a commit that referenced this issue Nov 2, 2015

[TEST] Fix backwards compatibility tests for 2.x
Moves backwards compatibility tests from core to qa/backwards and uses a
service running inside of maven to fork the elasticsearch nodes so the tests
can continue to run in seccomp.

Because the tests were moved ReproduceInfoPrinter now cannot hardcode which
parameters it prints - it scans them using a new annotation.

`CompositeTestCluster#client` now remakes its client with new node addresses
after every node is upgraded. That makes it possiblie for it to survive a
rolling restart where the new nodes come up using different ports.

`CompositeTestCluster#upgradeOneNode` now brings up the upgraded node with
the same data directory as the old node which prevents the loss of data when
there are no replicas.

`ExternalNode` now uses a `unicastHosts` list to find the other running nodes
rather than relying on multicast which was moved to a plugin in 2.0.
`unicastHosts` is carefully setup by `CompositeTestCluster` to list all the
other nodes that are currently running.

`FunctionScoreBackwardCompatibilityIT` now forces a single shard so the tf/idf
parts of the score don't get juggled.

`ReproduceInfoPrinter` now appends properties required to properly reproduce
the backwards compatibility tests and properly quotes parameters like
`-Dtests.filter="@Backwards and not @awaitsfix`.

About 2/3 of the backwards compatibility tests are passing and the rest are
marked with
`@AwaitsFix(bugUrl="#13522
That issue will track all work getting the backwards compatibility tests
back online. The following tests are so marked:
- BasicAnalysisBackwardCompatibilityIT
- BasicBackwardsCompatibilityIT
- RecoveryBackwardsCompatibilityIT
- SignificantTermsBackwardCompatibilityIT
- SnapshotBackwardsCompatibilityIT

Closes #13425

@nik9000 nik9000 added the v2.2.0 label Nov 18, 2015

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Nov 18, 2015

Contributor

Note for those following along at home - I don't believe this should block 2.1.0. Its fine to kick this to 2.1.1, 2.1.2, etc. The really important BWC tests are passing.

Contributor

nik9000 commented Nov 18, 2015

Note for those following along at home - I don't believe this should block 2.1.0. Its fine to kick this to 2.1.1, 2.1.2, etc. The really important BWC tests are passing.

@nik9000 nik9000 self-assigned this Nov 18, 2015

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Nov 18, 2015

Contributor

I'm working on SnapshotBackwardsCompatibilityIT on and off now.

Contributor

nik9000 commented Nov 18, 2015

I'm working on SnapshotBackwardsCompatibilityIT on and off now.

@clintongormley clintongormley removed the v2.1.0 label Nov 20, 2015

nik9000 added a commit to nik9000/elasticsearch that referenced this issue Nov 20, 2015

[test] Fix RecoveryBackwardsCompatibilityIT
It had drifted from its twin integration test, RecoveryFromGatewayIT. This
factors the test into a utility class so they can share 90% of that test
method.

Related to #13522

nik9000 added a commit that referenced this issue Nov 20, 2015

[test] Fix RecoveryBackwardsCompatibilityIT
It had drifted from its twin integration test, RecoveryFromGatewayIT. This
factors the test into a utility class so they can share 90% of that test
method.

Related to #13522

@nik9000 nik9000 closed this Nov 20, 2015

@nik9000 nik9000 reopened this Nov 20, 2015

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Nov 20, 2015

Contributor

misclick

Contributor

nik9000 commented Nov 20, 2015

misclick

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Nov 20, 2015

Contributor

FunctionScoreBackwardCompatibilityIT

I got a start on this but it'll be a huge pain to do until we bundle the tar plugin in the distribution. The trouble is the security policy. Right now the bwc tests run with our default security policy which means they can't run groovy code and neither can the embedded nodes. If we bundled groovy then it'd be fairly simple to have the test run only external nodes. In fact the bwc tests should always be running only external nodes and this is a good excuse to do that across the board.

Contributor

nik9000 commented Nov 20, 2015

FunctionScoreBackwardCompatibilityIT

I got a start on this but it'll be a huge pain to do until we bundle the tar plugin in the distribution. The trouble is the security policy. Right now the bwc tests run with our default security policy which means they can't run groovy code and neither can the embedded nodes. If we bundled groovy then it'd be fairly simple to have the test run only external nodes. In fact the bwc tests should always be running only external nodes and this is a good excuse to do that across the board.

brwe added a commit to brwe/elasticsearch that referenced this issue Nov 23, 2015

aggs: fix significant terms reduce for long terms
Significant terms were not reduced correctly if they were long terms.
Also, clean up the bwc test a little. Upgrades are not needed.

related to #13522

brwe added a commit that referenced this issue Nov 24, 2015

aggs: fix significant terms reduce for long terms
Significant terms were not reduced correctly if they were long terms.
Also, clean up the bwc test a little. Upgrades are not needed.

related to #13522

brwe added a commit that referenced this issue Nov 24, 2015

aggs: fix significant terms reduce for long terms
Significant terms were not reduced correctly if they were long terms.
Also, clean up the bwc test a little. Upgrades are not needed.

related to #13522

brwe added a commit that referenced this issue Nov 24, 2015

aggs: fix significant terms reduce for long terms
Significant terms were not reduced correctly if they were long terms.
Also, clean up the bwc test a little. Upgrades are not needed.

related to #13522

nik9000 added a commit to nik9000/elasticsearch that referenced this issue Nov 25, 2015

[test] Fix SnapshotBackwardsCompatibilityIT
SnapshotBackwardsCompatibilityIT had drifted from its twin methods in
SharedClusterSnapshotRestoreIT. This creates SnapshotSharedTestCases which
makes the similar methods in both test cases use the same code, modulo the
cluster upgrade.

Related to #13522

nik9000 added a commit that referenced this issue Nov 27, 2015

[test] Fix SnapshotBackwardsCompatibilityIT
SnapshotBackwardsCompatibilityIT had drifted from its twin methods in
SharedClusterSnapshotRestoreIT. This creates SnapshotSharedTestCases which
makes the similar methods in both test cases use the same code, modulo the
cluster upgrade.

Related to #13522

@nik9000 nik9000 assigned brwe and unassigned nik9000 Dec 8, 2015

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Dec 8, 2015

Contributor

As of now all of the scary backwards compatibility tests have been fixed for 2.2. The only remaining one is that FunctionScoreBackwardCompatibilityIT which so far as I know really wants to make sure groovy works across a mixed version cluster. I don't think this is a huge, huge issue, but it needs fixing eventually.

Contributor

nik9000 commented Dec 8, 2015

As of now all of the scary backwards compatibility tests have been fixed for 2.2. The only remaining one is that FunctionScoreBackwardCompatibilityIT which so far as I know really wants to make sure groovy works across a mixed version cluster. I don't think this is a huge, huge issue, but it needs fixing eventually.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Dec 8, 2015

Contributor

This is probably the biggest remaining BWC task:
#14406

Contributor

nik9000 commented Dec 8, 2015

This is probably the biggest remaining BWC task:
#14406

brwe added a commit to brwe/elasticsearch that referenced this issue Dec 9, 2015

remove script from function score bwc test and move to BasicBWCIT
Function score test was supposed to test that old nodes can still read
whatever a new node sends. However, currently there is no way to test
scripting outside the script packages.

Also, we don't really need to upgrade nodes for this test. We only need
a mixed cluster.

related to #13522
@brwe

This comment has been minimized.

Show comment
Hide comment
@brwe

brwe Dec 9, 2015

Contributor

The only remaining one is that FunctionScoreBackwardCompatibilityIT which so far as I know really wants to make sure groovy works across a mixed version cluster.

It was supposed to test that old nodes can parse whatever new nodes send. I added the script then because I thought we might want to check that as well but for now we can simply remove that. I made a pr here: #15332

I don't think this is a huge, huge issue, but it needs fixing eventually.

I too think the problem with testing scripting in mixed clusters remains. We currently cannot test that old nodes can parse requests related to scripting that were sent by new nodes. I do wonder though if there is a more clever way to verify that parsing on old nodes still works without having to spin up a cluster and send requests...

Contributor

brwe commented Dec 9, 2015

The only remaining one is that FunctionScoreBackwardCompatibilityIT which so far as I know really wants to make sure groovy works across a mixed version cluster.

It was supposed to test that old nodes can parse whatever new nodes send. I added the script then because I thought we might want to check that as well but for now we can simply remove that. I made a pr here: #15332

I don't think this is a huge, huge issue, but it needs fixing eventually.

I too think the problem with testing scripting in mixed clusters remains. We currently cannot test that old nodes can parse requests related to scripting that were sent by new nodes. I do wonder though if there is a more clever way to verify that parsing on old nodes still works without having to spin up a cluster and send requests...

@brwe

This comment has been minimized.

Show comment
Hide comment
@brwe

brwe Dec 9, 2015

Contributor

Maybe we could make all nodes (old and new) start the same way with the external node service instead of using node builder etc. This would have the advantage that the cluster would be an actual cluster. The test would only start node clients or transport clients that then connect to the cluster. Then we could use whatever modules are available per default and also install whatever plugins we want before we start the test. Would be a larger change though.

Contributor

brwe commented Dec 9, 2015

Maybe we could make all nodes (old and new) start the same way with the external node service instead of using node builder etc. This would have the advantage that the cluster would be an actual cluster. The test would only start node clients or transport clients that then connect to the cluster. Then we could use whatever modules are available per default and also install whatever plugins we want before we start the test. Would be a larger change though.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Dec 9, 2015

Contributor

Maybe we could make all nodes (old and new) start the same way with the external node service instead of using node builder etc.

Yeah! I think it is a good idea too. Maybe another meta issue for improvements in BWC tests?

Contributor

nik9000 commented Dec 9, 2015

Maybe we could make all nodes (old and new) start the same way with the external node service instead of using node builder etc.

Yeah! I think it is a good idea too. Maybe another meta issue for improvements in BWC tests?

brwe added a commit that referenced this issue Dec 9, 2015

remove script from function score bwc test and move to BasicBWCIT
Function score test was supposed to test that old nodes can still read
whatever a new node sends. However, currently there is no way to test
scripting outside the script packages.

Also, we don't really need to upgrade nodes for this test. We only need
a mixed cluster.

related to #13522
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Dec 10, 2015

Contributor

Britta's fixed the last test! Hurray! Closing this tracking issue.

Contributor

nik9000 commented Dec 10, 2015

Britta's fixed the last test! Hurray! Closing this tracking issue.

@nik9000 nik9000 closed this Dec 10, 2015

nik9000 added a commit that referenced this issue Dec 17, 2015

[test] Fix RecoveryBackwardsCompatibilityIT
It had drifted from its twin integration test, RecoveryFromGatewayIT. This
factors the test into a utility class so they can share 90% of that test
method.

Related to #13522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment