Add QA module to test task cancellation #59828

dnhatn · 2020-07-18T16:41:44Z

This commit introduces a QA module that runs tests with two mixed clusters with a test plugin installed. The test plugin blocks some requests until they are canceled so we can verify BWC in the task cancellation cross clusters.

Relates #55779 (review)

elasticmachine · 2020-07-18T16:41:47Z

Pinging @elastic/es-distributed (:Distributed/Task Management)

qa/task-cancellation/build.gradle

qa/task-cancellation/src/test/java/org/elasticsearch/tasks/TaskCancellationIT.java

dnhatn · 2020-07-18T16:44:51Z

@mark-vieira Can you please take a look at the gradle things? Thank you!

dnhatn · 2020-07-20T01:52:43Z

run elasticsearch-ci/1

ywelsch

I did not check the Gradle changes, but just looked at the other bits.

qa/task-cancellation/src/main/java/org/elasticsearch/plugin/tasks/BlockingCancellableTask.java

qa/task-cancellation/src/main/java/org/elasticsearch/plugin/tasks/TransportTestAction.java

qa/task-cancellation/src/test/java/org/elasticsearch/tasks/TaskCancellationIT.java

qa/task-cancellation/build.gradle

mark-vieira · 2020-07-20T22:42:07Z

qa/task-cancellation/build.gradle

+  testImplementation project(':client:rest-high-level')
+}
+
+for (Version bwcVersion : BuildParams.bwcVersions.unreleasedWireCompatible) {


Why are we only testing unreleased versions here? Don't we want to test compatibility against released versions as well?

We are using a test plugin, which is not published and not available in the released versions.

So we simply don't intend to do BWC testing against released versions? How do we intend to ensure we don't break compatibility there?

distribution/bwc/build.gradle

qa/task-cancellation/build.gradle

dnhatn · 2020-07-21T03:27:18Z

@ywelsch @mark-vieira Thank you for your reviews. I've addressed your comments. Can you take another look?

ywelsch

I've left one more question, o.w. looking good

qa/task-cancellation/src/main/java/org/elasticsearch/plugin/tasks/TransportTestAction.java

ywelsch

LGTM

mark-vieira

Added a few more comments.

mark-vieira · 2020-07-21T20:16:54Z

distribution/bwc/build.gradle

@@ -292,7 +292,12 @@ BuildParams.bwcVersions.forPreviousUnreleased { BwcVersions.UnreleasedVersionInf

      createBuildBwcTask(projectName, "${baseDir}/${projectName}", projectArtifact)
    }
-
+    // Create build tasks for a task-cancellation test plugin used for compatibility testing
+    if (bwcVersion.onOrAfter(Version.fromString("8.0.0"))) {


This is temporary, yes? We shouldn't need to restrict versions here as this should only exist in branch for which the current snapshot branches also have this project.

mark-vieira · 2020-07-21T20:17:36Z

qa/task-cancellation/build.gradle

+  testImplementation project(':client:rest-high-level')
+}
+
+for (Version bwcVersion : BuildParams.bwcVersions.unreleasedWireCompatible) {


So we simply don't intend to do BWC testing against released versions? How do we intend to ensure we don't break compatibility there?

mark-vieira · 2020-07-21T20:19:22Z

qa/task-cancellation/build.gradle

+  } else {
+    UnreleasedVersionInfo unreleasedVersion = BuildParams.bwcVersions.unreleasedInfo(bwcVersion)
+    bundleOldPluginTask = project(unreleasedVersion.gradleProjectPath).tasks.findByName('buildBwcTaskCancellationTestPlugin')
+    if (bundleOldPluginTask == null) {


This is why we should do the version filtering here, and no in the bwc distribution project. That would allow us to ditch this check.

qa/task-cancellation/build.gradle

mark-vieira · 2020-07-21T20:23:27Z

qa/task-cancellation/build.gradle

+    nonInputProperties.systemProperty('tests.old_cluster', "${-> testClusters."old-${baseName}".allTransportPortURI.join(",")}")
+    nonInputProperties.systemProperty('tests.rest.old_cluster', "${-> testClusters."old-${baseName}".allHttpSocketURI.join(",")}")
+    nonInputProperties.systemProperty('tests.rest.cluster', "${-> testClusters."new-${baseName}".allHttpSocketURI.join(",")}")
+    dependsOn bundleOldPluginTask, tasks.bundlePlugin


nit: can we make this the first line in the configuration closure. Just makes it easier to grok the task graph.

mark-vieira · 2020-07-21T20:28:53Z

Since there is some plugin stuff involved here I think it might be worth @rjernst taking a look here as well.

rjernst · 2020-07-21T21:13:22Z

This pattern is quite a bit different than anything we have done before. In particular, we have yet to have a "test only plugin" that manipulates the behavior of the cluster. The version logic doesn't really make sense here, only seeming to operating on in flight versions, but not actually testing all compatible versions, as Mark pointed out.

Taking a step back, the related PR/comment asked about bwc tests, but could we not test that logic in a unit test? I'm not saying we shouldnt necessarily have high level tests like this, but I would rather start with unit tests if possible, and only expand the scope of our full integration tests if necessary. I also don't like manipulating the system like this, as I think those kinds of mocks/manipulations are better left for tightly controlled unit tests rather than a full fledged cluster running, which may be very difficult to diagnose failures.

dnhatn · 2020-07-29T15:44:14Z

@rjernst Thanks for taking a look.

I took a step back to explore unit tests, as you suggested. It's too complex to construct unit tests that cover what we want and give us the confidence like these BWC tests.

This PR is quite similar to the idea proposed in #54159 (comment). However, we avoid publishing this plugin by making it test-only. If we publish it, then we can test against released versions.

I don't find a better alternative for this PR. WDYT?

rjernst

Perhaps we can take a step back and evaluate why we need BWC tests for this specific module/feature. What about it has special bwc behavior? I view BWC tests as not testing all features, but testing things we know to have bwc behavior that we want to guarantee. Normally this means we have some conditionals based on Version in the code; am I missing that here somewhere?

dnhatn · 2020-08-11T20:29:01Z

Perhaps we can take a step back and evaluate why we need BWC tests for this specific module/feature. What about it has special bwc behavior?

Thanks @rjernst. This QA module helps us to make sure that we won't send cancellation requests to a remote cluster that is on an old version because it won't be able to handle those requests. We know how to dispatch those requests conditionally. However, we do not feel confident without having some tests with a random configuration. As I said, we can do manual testing for this, but random tests can uncover something that we miss in BWC + remote connections.

mark-vieira · 2020-08-11T22:47:03Z

This QA module helps us to make sure that we won't send cancellation requests to a remote cluster that is on an old version because it won't be able to handle those requests.

This statement seems like it could be a single assertion in a unit test to be honest, but I'm speaking from a naive perspective here. Do were really need a full on BWC integration test, w/ all the associated overhead to ensure the logic that handles cancellation works in this scenario?

dnhatn · 2020-08-24T02:55:39Z

I am closing this PR as we could not reach a consensus on the approach. I will try to tackle the issue without this QA module. Thanks everyone for the reviews!

QA test

371bad5

dnhatn added >test Issues or PRs that are addressing/adding tests :Distributed/Task Management Issues for anything around the Tasks API - both persistent and node level. v8.0.0 v6.8.12 v7.10.0 v7.9.1 labels Jul 18, 2020

dnhatn requested review from ywelsch and mark-vieira July 18, 2020 16:41

elasticmachine added the Team:Distributed Meta label for distributed team label Jul 18, 2020

dnhatn commented Jul 18, 2020

View reviewed changes

qa/task-cancellation/build.gradle Outdated Show resolved Hide resolved

dnhatn commented Jul 18, 2020

View reviewed changes

qa/task-cancellation/src/test/java/org/elasticsearch/tasks/TaskCancellationIT.java Show resolved Hide resolved

dnhatn added 4 commits July 18, 2020 15:27

spotless

72ddde9

disable test

62eab10

sub task

dd45b05

disable integ test

fc1f195

ywelsch reviewed Jul 20, 2020

View reviewed changes

dnhatn added 5 commits July 20, 2020 10:14

validation

c065a98

timeout

42b9173

dispatch to the target node on the remote cluster

1cf0074

use proxy connection

d50b9d6

Merge branch 'master' into qa-task-cancellation

7d5f9f9

mark-vieira requested changes Jul 20, 2020

View reviewed changes

Create task inside bwc project

3432138

dnhatn requested review from ywelsch and mark-vieira July 21, 2020 03:27

remove unused export

bf78066

ywelsch reviewed Jul 21, 2020

View reviewed changes

qa/task-cancellation/src/main/java/org/elasticsearch/plugin/tasks/TransportTestAction.java Outdated Show resolved Hide resolved

execute locally

5c24034

dnhatn requested a review from ywelsch July 21, 2020 14:39

ywelsch approved these changes Jul 21, 2020

View reviewed changes

mark-vieira requested changes Jul 21, 2020

View reviewed changes

rjernst reviewed Aug 11, 2020

View reviewed changes

dliappis added v6.8.13 and removed v6.8.12 labels Aug 12, 2020

dnhatn closed this Aug 24, 2020

dnhatn deleted the qa-task-cancellation branch August 24, 2020 02:55

dnhatn removed v6.8.13 v7.10.0 v7.9.1 v8.0.0 labels Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add QA module to test task cancellation #59828

Add QA module to test task cancellation #59828

dnhatn commented Jul 18, 2020

elasticmachine commented Jul 18, 2020

dnhatn commented Jul 18, 2020

dnhatn commented Jul 20, 2020

ywelsch left a comment

mark-vieira Jul 20, 2020

dnhatn Jul 21, 2020

mark-vieira Jul 21, 2020

dnhatn commented Jul 21, 2020

ywelsch left a comment

ywelsch left a comment

mark-vieira left a comment

mark-vieira Jul 21, 2020

mark-vieira Jul 21, 2020

mark-vieira Jul 21, 2020

mark-vieira Jul 21, 2020

mark-vieira commented Jul 21, 2020

rjernst commented Jul 21, 2020

dnhatn commented Jul 29, 2020 •

edited

rjernst left a comment

dnhatn commented Aug 11, 2020 •

edited

mark-vieira commented Aug 11, 2020

dnhatn commented Aug 24, 2020

Add QA module to test task cancellation #59828

Add QA module to test task cancellation #59828

Conversation

dnhatn commented Jul 18, 2020

elasticmachine commented Jul 18, 2020

dnhatn commented Jul 18, 2020

dnhatn commented Jul 20, 2020

ywelsch left a comment

Choose a reason for hiding this comment

mark-vieira Jul 20, 2020

Choose a reason for hiding this comment

dnhatn Jul 21, 2020

Choose a reason for hiding this comment

mark-vieira Jul 21, 2020

Choose a reason for hiding this comment

dnhatn commented Jul 21, 2020

ywelsch left a comment

Choose a reason for hiding this comment

ywelsch left a comment

Choose a reason for hiding this comment

mark-vieira left a comment

Choose a reason for hiding this comment

mark-vieira Jul 21, 2020

Choose a reason for hiding this comment

mark-vieira Jul 21, 2020

Choose a reason for hiding this comment

mark-vieira Jul 21, 2020

Choose a reason for hiding this comment

mark-vieira Jul 21, 2020

Choose a reason for hiding this comment

mark-vieira commented Jul 21, 2020

rjernst commented Jul 21, 2020

dnhatn commented Jul 29, 2020 • edited

rjernst left a comment

Choose a reason for hiding this comment

dnhatn commented Aug 11, 2020 • edited

mark-vieira commented Aug 11, 2020

dnhatn commented Aug 24, 2020

dnhatn commented Jul 29, 2020 •

edited

dnhatn commented Aug 11, 2020 •

edited