Make All OS tests run on GCP instances #46924

alpar-t · 2019-09-20T13:49:47Z

This PR makes the necesary adaptations to the tests and adds a power shell script to
invoke the OS tests on GCP instances connected as CI workers.

Also noticed that logs were not being produced by the tests and that theses were not using log4j so fixed that too.

One of the difficulties in working on theses tests was that the tests just stalled with no indication where the problem is.
To ease with the debugging, after process explorer suggested that the tests are running some commands, we now have multiple timeouts: one for the tests ( which will generate a thread dump ) and one for individual commands ( that bails with the command being ran and output and error so far ) to make it easier to see what went wrong.

The tests were blocking because apparently the pipes to the sub-process were not closing, thus the threads were blocking on them and we were blocking indefinitely on the join. I'm not sure why this doesn't happen in vagrant, but we now properly deal with it.

This PR makes the necesary adaptations to the tests and adds a power shell script to invoke the OS tests on GCP instances connected as CI workers.

elasticmachine · 2019-09-20T13:49:49Z

Pinging @elastic/es-core-infra

alpar-t · 2019-09-20T13:51:01Z

Here's a build scan from my testing: https://scans.gradle.com/s/lpgxftanojdv2/tests

@rjernst it seems we are running each test multiple times, but we can address that separately

pugnascotia · 2019-09-20T15:34:31Z

qa/os/src/test/java/org/elasticsearch/packaging/util/Archives.java

-                "$process.Start() | Out-Null; " +
-                "$process.Id;"
-            );
+            if (System.getenv("username").equals("vagrant")) {


Most of the PowerShell lines in the following sh.run() calls are the same. Is there any value in reducing the duplication here?

pugnascotia

I can't say anything about the PowerShell, but the rest looks good.

mark-vieira

Going to defer to @rjernst for the most part here since he's most familiar with the packaging test infrastructure.

mark-vieira · 2019-09-23T16:59:33Z

.ci/os.ps1

+Remove-Item -Recurse -Force $gradleInit -ErrorAction Ignore
+New-Item -ItemType directory -Path $gradleInit
+
+# Copy-Item .ci/init.gradle -Destination $gradleInit


Should this be commented out?

Thanks, that's left over from testing, I will un-comment it.

mark-vieira · 2019-09-23T17:03:16Z

This PR makes the necesary adaptations to the tests and adds a power shell script to
invoke the OS tests on GCP instances connected as CI workers.

Just to confirm, the build isn't actually spinning up workers. What we intend to do is launch a Windows CI work, then directly invoke the destructive test task on that worker instead of inside a Vagrant VM. Yes?

alpar-t · 2019-09-24T13:08:25Z

Just to confirm, the build isn't actually spinning up workers. What we intend to do is launch a Windows CI work, then directly invoke the destructive test task on that worker instead of inside a Vagrant VM. Yes?

That's right. We will have a matrix job so CI will spin up the workers and invoke the ps1 script from this PR on each.

alpar-t · 2019-09-30T13:24:43Z

Windows test on CI workers passed : https://gradle-enterprise.elastic.co/s/p4qyrkswxipm4

alpar-t · 2019-09-30T13:51:37Z

CentOS passed: https://gradle-enterprise.elastic.co/s/dai35e6bvhk2m

qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java

alpar-t · 2019-09-30T14:39:01Z

Debian also passing with the last fix: https://gradle-enterprise.elastic.co/s/xrc6baytgnqqg

alpar-t · 2019-09-30T14:58:50Z

@elasticmachine run elasticsearch-ci/2

rjernst

I left some comments

rjernst · 2019-09-30T18:19:06Z

.ci/os.ps1

+
+# CI configures these, uncoment if running manually 
+#
+# $env:ES_BUILD_JAVA="java12" 


Can we just set these if they are not set so manual editing is not necessary?

I was planning to read .ci/java-versions.properties like in Linux, but in a follow up.

rjernst · 2019-09-30T18:20:21Z

.ci/os.ps1

+$ErrorActionPreference="Continue"
+# TODO: remove the task exclusions once dependencies are set correctly and these don't run for Windows or buldiung the deb on windows is fixed
+& .\gradlew.bat -g "C:\Users\$env:username\.gradle" --parallel --scan --console=plain destructiveDistroTest `
+   -x :distribution:packages:buildOssDeb `


What is the task dep causing these to be built? We should never even try building these if they won't be used by the test, which was a major goal of the refactoring work here.

I'm not sure, but has to be something relating to the OS tests.
I assumed the refactoring is not yet complete.
We built these before because we check that these can be extracted but I already addressed that.

Either way I would prefer to look at this in a separate PR.

rjernst · 2019-09-30T18:24:37Z

qa/os/src/test/java/org/elasticsearch/packaging/test/PackageTests.java

-            }
+        // we don't require java be installed for the tests, but remove it if it's there
+        // since we don't require it for the tests, don't bother restoring it
+        if (Files.exists(Paths.get("/usr/bin/java"))) {


Why was the previous code causing problems? If we are going to ensure /usr/bin/java doesn't exist, we should do so outside of test execution. This seems like an environmental issue, not something a test (in the middle of other tests running) should be changing. Deleting and not restoring system files is a serious, unexpected side effect to a test running.

Vagrant VMs have a java package installed whereas CI workers do not.
Since we label these as destructive and always run them on ephemeral VMs, the restoring of this made the test unnecessarily hard to read. I think it's worth restoring when it makes it possible to re-use a VM e.x. for development, but in this case, the test works if there's no java installed as well.

The right fix here is probably to make sure java is not installed on the vagrant images ?

I added the restore back to make this less awkward.

rjernst · 2019-09-30T18:26:52Z

qa/os/src/test/java/org/elasticsearch/packaging/util/FileUtils.java

+            return;
+        }
+        logger.info("Showing contents of directory: {} ({})", logsDir, logsDir.toAbsolutePath());
+        try(Stream<Path> fileStream = Files.list(logsDir)) {


nit: space after try

rjernst · 2019-09-30T18:29:04Z

qa/os/src/test/java/org/elasticsearch/packaging/util/Shell.java

+    public void chown(Path path) throws Exception {
+        Platforms.onLinux(() -> run("chown -R elasticsearch:elasticsearch " + path));
+        Platforms.onWindows(() -> run(
+            "$account = New-Object System.Security.Principal.NTAccount '" + System.getenv("username")  + "'; " +


Don't we have the constant ARCHIVE_USER for use here and above?

There's ARCHIVE_OWNER but that's part of the ArchiveTests and has hard coded values.

rjernst · 2019-09-30T18:31:18Z

qa/os/src/test/java/org/elasticsearch/packaging/util/Shell.java

        try {
+            Path tmpDir = Paths.get(System.getProperty("java.io.tmpdir"));
+            Files.createDirectories(tmpDir);
+            stdOut = Files.createTempFile(tmpDir, getClass().getName(), ".out");


Where are these files cleaned up?

Good catch, they were not. I added cleanup.

rjernst · 2019-09-30T18:31:44Z

qa/os/src/test/java/org/elasticsearch/packaging/util/Shell.java

+            }
+            LinkedList<String> result = new LinkedList<>();
+            AtomicBoolean linesDiscarded = new AtomicBoolean(false);
+            try(Stream<String> lines = Files.lines(path, StandardCharsets.UTF_8)) {


nit: space after try

rjernst · 2019-09-30T18:36:33Z

qa/os/src/test/java/org/elasticsearch/packaging/util/Shell.java

+            try(Stream<String> lines = Files.lines(path, StandardCharsets.UTF_8)) {
+                lines.forEach(line -> {
+                    result.add(line);
+                    if (result.size() >= TAIL_WHEN_TOO_MUCH_OUTPUT) {


Why are we omitting lines at all? Once the VM is destroyed we can't get them out. If we want to debate about the merits of what is output fine, but I think it should be a separate PR from this already large PR to get the OS tests running in gcp?

I'm going to remove omitting the lines, but will keep the size check.
One of the windows installer tests is generating a 16GB stderr.
I don't know what's in it because I didn't download it and nothing on windows could open it...

rjernst

LGTM, given the followup work you noted in response to some previous comments.

This PR makes the necesary adaptations to the tests and adds a power shell script to invoke the OS tests on GCP instances connected as CI workers. Also noticed that logs were not being produced by the tests and that theses were not using log4j so fixed that too. One of the difficulties in working on theses tests was that the tests just stalled with no indication where the problem is. To ease with the debugging, after process explorer suggested that the tests are running some commands, we now have multiple timeouts: one for the tests ( which will generate a thread dump ) and one for individual commands ( that bails with the command being ran and output and error so far ) to make it easier to see what went wrong. The tests were blocking because apparently the pipes to the sub-process were not closing, thus the threads were blocking on them and we were blocking indefinitely on the join. I'm not sure why this doesn't happen in vagrant, but we now properly deal with it.

Make Windows OS tests run on GCP instances

22001a2

This PR makes the necesary adaptations to the tests and adds a power shell script to invoke the OS tests on GCP instances connected as CI workers.

alpar-t added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.0.0 v7.5.0 labels Sep 20, 2019

alpar-t requested review from rjernst and mark-vieira September 20, 2019 13:49

alpar-t added 2 commits September 20, 2019 17:23

Remove todo

acaa54d

Merge branch 'master' into windows-packaging-gcp

022b111

pugnascotia reviewed Sep 20, 2019

View reviewed changes

pugnascotia approved these changes Sep 20, 2019

View reviewed changes

alpar-t added 3 commits September 23, 2019 12:30

Merge remote-tracking branch 'origin/master' into windows-packaging-gcp

ef1d505

Show logs when ES fails to start

d32f68e

Skip non existing files when showing logs

911e0c6

mark-vieira reviewed Sep 23, 2019

View reviewed changes

alpar-t added 8 commits September 24, 2019 16:12

more debuggign

9dc9b47

Merge remote-tracking branch 'origin/master' into windows-packaging-gcp

c3f353d

Further extend debugging

f800c73

Create empty logs dir after clean

45a41dc

more logging

052cef7

Use tmp files for output redirection

a5eb27e

Add script for unix compatability jobs

5d1afba

make script executable

33f9a92

alpar-t changed the title ~~Make Windows OS tests run on GCP instances~~ Make All OS tests run on GCP instances Sep 27, 2019

alpar-t added 3 commits September 27, 2019 13:22

make the script easier to run outside ci

3251641

Improovements to the unix script

25e0850

fix init file handling

579c4ca

alpar-t added 6 commits September 30, 2019 14:17

Explicit gradle home on windows

28b7257

Protect against too much output

a4f581f

Don't read excessively large results

057363e

Fix incorrect output

042a89a

Fix failing tests

2d06337

Be explicit about the Gradle home dir on linux too

5154be9

Add explicit --scan

760376e

williamrandolph reviewed Sep 30, 2019

View reviewed changes

qa/os/src/test/java/org/elasticsearch/packaging/test/ArchiveTests.java Outdated Show resolved Hide resolved

alpar-t added 4 commits September 30, 2019 17:13

Add a workaround for Debian and ubuntu having rpm installed

60def58

Fix syntax and add sudo to removing rpm

4d300d2

Support older ( non systemd ) OSes

7715cd5

PR review: pull cleanup into base class

b9fe4e2

rjernst reviewed Sep 30, 2019

View reviewed changes

alpar-t added 5 commits October 1, 2019 10:41

PR review

ba7ee5c

PR review

e2f1382

Merge remote-tracking branch 'origin/master' into windows-packaging-gcp

30e631f

Don't remove logs, it breaks tests

b70c929

Merge remote-tracking branch 'origin/master' into windows-packaging-gcp

5ca59c7

rjernst approved these changes Oct 4, 2019

View reviewed changes

alpar-t merged commit f962d1c into elastic:master Oct 4, 2019

alpar-t deleted the windows-packaging-gcp branch October 4, 2019 05:41

jimczi added the >test Issues or PRs that are addressing/adding tests label Nov 12, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make All OS tests run on GCP instances #46924

Make All OS tests run on GCP instances #46924

alpar-t commented Sep 20, 2019

elasticmachine commented Sep 20, 2019

alpar-t commented Sep 20, 2019

pugnascotia Sep 20, 2019

pugnascotia left a comment

mark-vieira left a comment

mark-vieira Sep 23, 2019

alpar-t Sep 24, 2019

mark-vieira commented Sep 23, 2019

alpar-t commented Sep 24, 2019

alpar-t commented Sep 30, 2019

alpar-t commented Sep 30, 2019

alpar-t commented Sep 30, 2019

alpar-t commented Sep 30, 2019

rjernst left a comment

rjernst Sep 30, 2019

alpar-t Oct 1, 2019

rjernst Sep 30, 2019

alpar-t Oct 1, 2019

rjernst Sep 30, 2019

alpar-t Oct 1, 2019

alpar-t Oct 1, 2019

rjernst Sep 30, 2019

rjernst Sep 30, 2019

alpar-t Oct 1, 2019

rjernst Sep 30, 2019

alpar-t Oct 1, 2019

rjernst Sep 30, 2019

rjernst Sep 30, 2019

alpar-t Oct 1, 2019

rjernst left a comment

Make All OS tests run on GCP instances #46924

Make All OS tests run on GCP instances #46924

Conversation

alpar-t commented Sep 20, 2019

elasticmachine commented Sep 20, 2019

alpar-t commented Sep 20, 2019

Choose a reason for hiding this comment

pugnascotia left a comment

Choose a reason for hiding this comment

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira commented Sep 23, 2019

alpar-t commented Sep 24, 2019

alpar-t commented Sep 30, 2019

alpar-t commented Sep 30, 2019

alpar-t commented Sep 30, 2019

alpar-t commented Sep 30, 2019

rjernst 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

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

rjernst left a comment

Choose a reason for hiding this comment