Sense for VirtualBox and $HOME when deciding to turn on vagrant testing. #24636

jbaiera · 2017-05-11T23:12:57Z

Some of the CI boxes for Elasticsearch have inconsistently configured environments when it comes to Vagrant. Vagrant requires $HOME to be set, as well as VirtualBox to be installed. This PR checks those two things are present before enabling tests with VagrantFixtures in the HDFS Repo (only place vagrant fixtures are used right now).

rjernst · 2017-05-12T06:07:21Z

plugins/repository-hdfs/build.gradle

+
+  // Finding that HOME needs to be set when performing vagrant updates
+  String homeLocation = System.getenv("HOME")
+  if (homeLocation == null) {


why do we need this leniency? If vagrant fails because HOME is not set, that tails us there is a problem that needs fixing. With the logic here, we will simply not run the test and not know it is broken.

I can change this to throw an exception instead. If vagrant is installed but the environment can't support calling it, we should probably break earlier in the build with clear reasoning for why.

rjernst · 2017-05-12T06:11:10Z

plugins/repository-hdfs/build.gradle

+  try {
+    ByteArrayOutputStream pipe = new ByteArrayOutputStream()
+    ExecResult runResult = exec {
+      commandLine 'vboxmanage', '--version'


We really need to share this stuff...VagrantTestPlugin also does this.

Can we do this exactly once (for vagrant and vboxmanage each) and set a property on the project like vagrantSupported? See for example in BuildPlugin how we check the java version etc in globalBuildInfo. This would also allow us to remove the tasks created by VagrantTestPlugin and just check the flag.

This keeps failing the build so I am temporarily disabling it until elastic#24636 gets merged.

This keeps failing the build so I am temporarily disabling it until #24636 gets merged.

dimitris-athanasiou · 2017-05-12T11:59:57Z

@jbaiera I have merged in #24643 in order to temporarily disable the task that causes build failures. Adding it here to remind you to re-enable once this is ready to go in.

jbaiera · 2017-05-12T20:35:50Z

I've moved the environment sensing out of the hdfs plugin build script as well as the vagrant test plugin into it's own VagrantSupportPlugin. Both vagrant and virtualbox are checked to see if they are installed. After the plugin is applied, if the root project hasn't been configured by it yet, then it does the environment checks and installs the results (as Installation objects) into the root project. It then transfers the Installation objects into the project applying the plugin. Installation is just a simple object that couples together the results of the checks: If the commands are installed, if the versions are correct, what the version number is if they meet the requirements, and any errors encountered while checking if they were installed. The plugin also creates the tasks to check the version for Vagrant and VirtualBox, so that hdfs repo and the packaging tests don't have to any more. These tasks no longer perform the actual lookup, but rather call the verify() method on the Installation objects, which throws if the Installation is not valid.

jbaiera · 2017-05-12T20:37:43Z

The VagrantTestPlugin definitely depends on this VagrantSupportPlugin. I'm not aware of any good ways to orchestrate that dependency in the build script. @rjernst any thoughts?

rjernst

I left a few comments.

rjernst · 2017-05-18T00:16:52Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/VagrantSupportPlugin.groovy

+                    "and requires \$HOME to be set to function properly.")
+        }
+
+        project.ext.vagrantInstallation = project.rootProject.ext.vagrantInstallation


Do we really need to put these on all the projects? I think this plugin can just look for the flags on the root project?

rjernst · 2017-05-18T00:17:28Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/VagrantSupportPlugin.groovy

+    }
+
+    private Installation getVagrantInstallation(Project project) {
+        // Only do secure fixture support if the regular fixture is supported,


This comment seems to be specific to the old location of this check

rjernst · 2017-05-18T06:41:41Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/VagrantSupportPlugin.groovy

+    }
+
+    private Installation getVirtualBoxInstallation(Project project) {
+        // Also check to see if virtualbox is installed. We need both vagrant


This comment also refers to stuff outside this plugin

rjernst · 2017-05-18T06:55:20Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/Installation.groovy

+ * is installed, if it is the correct version, which version was found, and any
+ * errors that were encountered when checking for the installation.
+ */
+class Installation {


I don't think we really need this abstraction...can we just keep it simple and store a boolean property on the root project indicating whether vagrant is installed at the given version, and the same for virtualbox? You can even store this in a map, along with whatever error description you want about getting the version. eg.

rootProject.ext.vagrant = ['supported': false, 'info': 'Unsupported version of vagrant [${version}]. Need [Vagrant 1.8.6+]'] .... if (rootProject.vagrant.supported == false) { logger.warn(rootProject.vagrant.info) } else { // use vagrant }

rjernst · 2017-05-18T06:58:51Z

plugins/repository-hdfs/build.gradle

-    } else {
-      logger.warn("Could not read installed vagrant version:\n" + output)
+    if (project.virtualBoxInstallation.supported == false) {
+      logger.warn(project.virtualBoxInstallation.error.message)


These messages are becoming noisy, and aren't actually helpful, because they are emitted even when not trying to run this task. I wonder if instead you could check emit this at runtime if the task would have run? You could do this as a doFirst, and throw a StopExecutionException if the task should be skipped.

jbaiera · 2017-05-31T03:13:45Z

@rjernst How are we on the recent changes for this? I'd like to get this merged soon.

rjernst · 2017-05-31T03:30:39Z

@jbaiera I had not looked at it since I left my last comments. Please ping me in the future when it is ready for another review. I will look now.

rjernst

LGTM, left a couple minor comments that can be addressed in followups if desired.

rjernst · 2017-05-31T03:35:20Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/VagrantSupportPlugin.groovy

+
+        // Finding that HOME needs to be set when performing vagrant updates
+        String homeLocation = System.getenv("HOME")
+        if (project.rootProject.ext.vagrantSupported && homeLocation == null) {


Why does this check need to be done for every project using this instead of once inside the installation checks above?

rjernst · 2017-05-31T03:46:29Z

plugins/repository-hdfs/build.gradle


+if (secureFixtureSupported) {
  integTestSecure.mustRunAfter(project.integTest)


I don't think this mustRunAfter will actually do anything. The integTest task is just a dummy task that depends on setting up the integTestCluster and running integTestRunner. When you run gradle check do you see tasks form the secure and regular integTest intermixed?

…hen deciding if vagrant is supported

…on VirtualBox.

Removing Installation and replacing with Map Removing placing the installation properties on the project Got rid of spammy log messages on the repository hdfs build

…nt testing. (elastic#24636)" This reverts commit 4ed0abe.

…on vagrant testing. (#24636)"" This reverts commit b9e2a1f.

* master: Do not swallow node lock failed exception Revert "Revert "Sense for VirtualBox and $HOME when deciding to turn on vagrant testing. (elastic#24636)"" Aggregations bug: Significant_text fails on arrays of text. (elastic#25030) Speed up sorted scroll when the index sort matches the search sort (elastic#25138) TranslogTests.testWithRandomException ignored a possible simulated OOM when trimming files Adapt TranslogTests.testWithRandomException to checkpoint syncing on trim

* master: Explicitly reject duplicate data paths Do not swallow node lock failed exception Revert "Revert "Sense for VirtualBox and $HOME when deciding to turn on vagrant testing. (elastic#24636)"" Aggregations bug: Significant_text fails on arrays of text. (elastic#25030) Speed up sorted scroll when the index sort matches the search sort (elastic#25138) TranslogTests.testWithRandomException ignored a possible simulated OOM when trimming files Adapt TranslogTests.testWithRandomException to checkpoint syncing on trim Change BWC versions on get mapping 404s Fix get mappings HEAD requests TranslogTests#commit didn't allow for a concurrent closing of a view Fix handling of exceptions thrown on HEAD requests Fix comment formatting in EvilLoggerTests Remove unneeded weak reference from prefix logger Test: remove faling test that relies on merge order

…vagrant testing. (#24636)"

jbaiera added >bug :Delivery/Build Build or test infrastructure >test Issues or PRs that are addressing/adding tests labels May 11, 2017

rjernst reviewed May 12, 2017

View reviewed changes

dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this pull request May 12, 2017

[TEST] Temporarily disable the secure fixture for hdfs tests

99abcfb

This keeps failing the build so I am temporarily disabling it until elastic#24636 gets merged.

dimitris-athanasiou mentioned this pull request May 12, 2017

[TEST] Temporarily disable the secure fixture for hdfs tests #24643

Merged

dimitris-athanasiou added a commit that referenced this pull request May 12, 2017

[TEST] Temporarily disable the secure fixture for hdfs tests (#24643)

b7976bd

This keeps failing the build so I am temporarily disabling it until #24636 gets merged.

dimitris-athanasiou added a commit that referenced this pull request May 12, 2017

[TEST] Temporarily disable the secure fixture for hdfs tests (#24643)

659d40c

This keeps failing the build so I am temporarily disabling it until #24636 gets merged.

dimitris-athanasiou added a commit that referenced this pull request May 12, 2017

[TEST] Temporarily disable the secure fixture for hdfs tests (#24643)

d20834d

This keeps failing the build so I am temporarily disabling it until #24636 gets merged.

jbaiera mentioned this pull request May 12, 2017

CI Failure :plugins:repository-hdfs:krb5kdcUpdate #24641

Closed

jbaiera force-pushed the fix-virtualbox-sense branch from 6de733c to 13c9e2c Compare May 12, 2017 19:05

rjernst reviewed May 18, 2017

View reviewed changes

jbaiera force-pushed the fix-virtualbox-sense branch from de71a9b to 16e668f Compare May 18, 2017 15:33

rjernst approved these changes May 31, 2017

View reviewed changes

jbaiera added 10 commits June 2, 2017 12:14

Now sensing for virtualbox as well as the HOME environment variable w…

b3ee579

…hen deciding if vagrant is supported

Add a plugin for determining if a build environment supports Vagrant …

effcce4

…on VirtualBox.

Some fixes to a few bugs in the new files

15b9599

Actually call verify

5b52490

move previous vagrant based projects to use support plugin.

5c0a3c9

simplify repo hdfs build script.

77a1d11

re-enable secure testing in repo hdfs

9af7e6b

Addressing comments from PR:

88c4939

Removing Installation and replacing with Map Removing placing the installation properties on the project Got rid of spammy log messages on the repository hdfs build

Hoisting the home check into the single execution only

d17bff6

removing the run after stipulation as it is not needed

5d1dfc0

jbaiera force-pushed the fix-virtualbox-sense branch from 17601aa to 5d1dfc0 Compare June 2, 2017 17:31

jbaiera merged commit 4ed0abe into elastic:master Jun 2, 2017

jbaiera deleted the fix-virtualbox-sense branch June 2, 2017 20:26

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Jun 2, 2017

Revert "Sense for VirtualBox and $HOME when deciding to turn on vagra…

b9e2a1f

…nt testing. (elastic#24636)" This reverts commit 4ed0abe.

jbaiera added a commit that referenced this pull request Jun 12, 2017

Revert "Revert "Sense for VirtualBox and $HOME when deciding to turn …

2e29b69

…on vagrant testing. (#24636)"" This reverts commit b9e2a1f.

jbaiera added a commit that referenced this pull request Sep 19, 2017

Backporting "Sense for VirtualBox and $HOME when deciding to turn on …

c42eade

…vagrant testing. (#24636)"

jbaiera added a commit that referenced this pull request Sep 19, 2017

Backporting "Sense for VirtualBox and $HOME when deciding to turn on …

6f63c11

…vagrant testing. (#24636)"

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sense for VirtualBox and $HOME when deciding to turn on vagrant testing. #24636

Sense for VirtualBox and $HOME when deciding to turn on vagrant testing. #24636

jbaiera commented May 11, 2017

rjernst May 12, 2017

jbaiera May 12, 2017

rjernst May 12, 2017

dimitris-athanasiou commented May 12, 2017

jbaiera commented May 12, 2017

jbaiera commented May 12, 2017

rjernst left a comment

rjernst May 18, 2017

rjernst May 18, 2017

rjernst May 18, 2017

rjernst May 18, 2017

rjernst May 18, 2017

jbaiera commented May 31, 2017

rjernst commented May 31, 2017

rjernst left a comment

rjernst May 31, 2017

rjernst May 31, 2017


		if (secureFixtureSupported) {
		integTestSecure.mustRunAfter(project.integTest)

Sense for VirtualBox and $HOME when deciding to turn on vagrant testing. #24636

Sense for VirtualBox and $HOME when deciding to turn on vagrant testing. #24636

Conversation

jbaiera commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitris-athanasiou commented May 12, 2017

jbaiera commented May 12, 2017

jbaiera commented May 12, 2017

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

jbaiera commented May 31, 2017

rjernst commented May 31, 2017

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