Validate if car allows for using the bundled JDK #987

hub-cap · 2020-05-06T00:12:11Z

When the bundled JDK was introduced, it was added to the car config
file. This check uses the value from the car config to ensure that the
bundled JDK is allowed to be used as specified by the config. If not, it
throws an exception.

Closes #985

When the bundled JDK was introduced, it was added to the car config file. This check uses the value from the car config to ensure that the bundled JDK is allowed to be used as specified by the config. If not, it throws an exception. Closes elastic#985

hub-cap · 2020-05-06T14:50:15Z

@elasticmachine run tests

hub-cap · 2020-05-06T16:12:59Z

@elasticmachine update branch

hub-cap · 2020-05-06T18:03:54Z

@danielmitterdorfer hey friend, this has passed CI checks and I would love a look. Im not 100% happy with where/how the check is, but I dont know if there is much we can do besides rely on a check everywhere this method is used, outside of the actual starting of the nodes. Is there possibly some other validation that is done before rally gets this deep into running?

danielmitterdorfer

Thanks! I left a few suggestions.

danielmitterdorfer · 2020-05-07T09:01:26Z

esrally/mechanic/java_resolver.py

@@ -39,6 +39,9 @@ def determine_runtime_jdks():

    runtime_jdk_versions = determine_runtime_jdks()
    if runtime_jdk_versions[0] == "bundled":
+        if not allow_bundled_jvm:
+            raise exceptions.SystemSetupError("The bundled JDK is not allowed with the selected car(s): {}"


This decision does not depend on the chosen car but rather on the Elasticsearch version. How about we change the error message to:

This Elasticsearch version does not contain a bundled JDK. Please specify a different runtime JDK.

danielmitterdorfer · 2020-05-07T09:04:36Z

esrally/mechanic/java_resolver.py

@@ -21,7 +21,7 @@
 from esrally.utils import jvm


-def java_home(car_runtime_jdks, cfg):
+def java_home(car_runtime_jdks, allow_bundled_jvm, cfg):


How about we refactor the signature a bit? Ideally we could also get rid of the cfg object here and pass the chosen runtime JDK directly. I propose the following signature:

def java_home(car_runtime_jdks, specified_runtime_jdk, provides_bundled_jdk)

where

specified_runtime_jdk is derived from cfg.opts("mechanic", "runtime.jdk")

provides_bundled_jdk determines whether this Elasticsearch version ships with a bundled JDK

danielmitterdorfer · 2020-05-07T09:06:04Z

esrally/mechanic/provisioner.py

        self.build_type = build_type
        self.car_env = car_env
        self.car_runtime_jdks = car_runtime_jdks
+        self.car_allow_bundled_jvm = convert.to_bool(car_allow_bundled_jvm)


How about we call this variable car_provides_bundled_jdk (and adapt all dependent variables and strings accordingly)?

danielmitterdorfer · 2020-05-07T09:06:42Z

tests/mechanic/java_resolver_test.py

@@ -30,7 +30,7 @@ def test_resolves_java_home_for_default_runtime_jdk(self, resolve_jvm_path):
        cfg = config.Config()
        cfg.add(config.Scope.application, "mechanic", "runtime.jdk", None)

-        major, java_home = java_resolver.java_home("12,11,10,9,8", cfg)
+        major, java_home = java_resolver.java_home("12,11,10,9,8", True, cfg)


Can we switch to named parameters here?

danielmitterdorfer

Looks much better! I left a couple of comments but I think we're almost there.

danielmitterdorfer · 2020-05-18T06:13:04Z

esrally/mechanic/provisioner.py

        self.build_type = build_type
        self.car_env = car_env
        self.car_runtime_jdks = car_runtime_jdks
+        self.car_provides_bundled_jdk = convert.to_bool(car_provides_bundled_jdk)


Can we move the conversion (i.e. convert.to_bool to the call site(s)?) IMHO we should use the correct type bool already in the constructor.

danielmitterdorfer · 2020-05-18T06:17:06Z

tests/mechanic/java_resolver_test.py

@@ -30,7 +30,9 @@ def test_resolves_java_home_for_default_runtime_jdk(self, resolve_jvm_path):
        cfg = config.Config()
        cfg.add(config.Scope.application, "mechanic", "runtime.jdk", None)

-        major, java_home = java_resolver.java_home("12,11,10,9,8", cfg)
+        major, java_home = java_resolver.java_home("12,11,10,9,8",
+                                                   specified_runtime_jdk=cfg.opts("mechanic", "runtime.jdk"),


There is no need for the Config object anymore and therefore we should simplify the test and only pass the actual value here.

danielmitterdorfer · 2020-05-18T06:17:11Z

tests/mechanic/java_resolver_test.py

@@ -42,7 +44,9 @@ def test_resolves_java_home_for_specific_runtime_jdk(self, resolve_jvm_path):
        cfg = config.Config()
        cfg.add(config.Scope.application, "mechanic", "runtime.jdk", 8)

-        major, java_home = java_resolver.java_home("12,11,10,9,8", cfg)
+        major, java_home = java_resolver.java_home("12,11,10,9,8",
+                                                   specified_runtime_jdk=cfg.opts("mechanic", "runtime.jdk"),


There is no need for the Config object anymore and therefore we should simplify the test and only pass the actual value here.

danielmitterdorfer · 2020-05-18T06:17:15Z

tests/mechanic/java_resolver_test.py

@@ -53,9 +57,22 @@ def test_resolves_java_home_for_bundled_jdk(self):
        cfg = config.Config()
        cfg.add(config.Scope.application, "mechanic", "runtime.jdk", "bundled")

-        major, java_home = java_resolver.java_home("12,11,10,9,8", cfg)
+        major, java_home = java_resolver.java_home("12,11,10,9,8",
+                                                   specified_runtime_jdk=cfg.opts("mechanic", "runtime.jdk"),


There is no need for the Config object anymore and therefore we should simplify the test and only pass the actual value here.

danielmitterdorfer · 2020-05-18T06:17:36Z

tests/mechanic/java_resolver_test.py

+        cfg.add(config.Scope.application, "mechanic", "car.names", ["default"])
+
+        with self.assertRaises(exceptions.SystemSetupError) as ctx:
+            java_resolver.java_home("12,11,10,9,8", specified_runtime_jdk=cfg.opts("mechanic", "runtime.jdk"))


There is no need for the Config object anymore and therefore we should simplify the test and only pass the actual value here.

danielmitterdorfer

I left a comment about readability of test assertions but apart from that it looks fine.

danielmitterdorfer · 2020-05-19T06:38:13Z

tests/mechanic/java_resolver_test.py

                                                   provides_bundled_jdk=True)

-        self.assertEqual(major, 12)
-        self.assertEqual(java_home, "/opt/jdk12")
+        self.assertEqual(major, resolve_jvm_path.return_value[0])


I think the previous implementation was more readable, even if we duplicate code. However, the scope of duplication is within the test method so this is ok IMHO.

im fine w/ either. I just hate reusing strings in a test method in general, cuz things can get out of sync. I can revert.

danielmitterdorfer · 2020-05-19T06:38:19Z

tests/mechanic/java_resolver_test.py

                                                   provides_bundled_jdk=True)

-        self.assertEqual(major, 8)
-        self.assertEqual(java_home, "/opt/jdk8")
+        self.assertEqual(major, resolve_jvm_path.return_value[0])


I think the previous implementation was more readable, even if we duplicate code. However, the scope of duplication is within the test method so this is ok IMHO.

hub-cap · 2020-05-20T18:44:10Z

Ive requested review again as I wasnt sure if u were meaning to approve it but just did comment, or not.

danielmitterdorfer

Thanks! LGTM

hub-cap added bug Something's wrong :misc Changes that don't affect users directly: linter fixes, test improvements, etc. labels May 6, 2020

hub-cap added this to the 2.0.1 milestone May 6, 2020

hub-cap requested a review from danielmitterdorfer May 6, 2020 00:12

hub-cap self-assigned this May 6, 2020

elasticmachine and others added 2 commits May 6, 2020 12:13

Merge branch 'master' into bundle_version_check

0c0a892

Fix docker

76d6528

danielmitterdorfer reviewed May 7, 2020

View reviewed changes

hub-cap added 4 commits May 14, 2020 10:20

Fix signature

7255c99

named params

e95be18

Rename param

c30eef8

Fix lint

20eb79f

hub-cap requested a review from danielmitterdorfer May 14, 2020 16:34

danielmitterdorfer reviewed May 18, 2020

View reviewed changes

hub-cap added 2 commits May 18, 2020 12:52

Move converter

d422648

Test cleanup

ac3e578

hub-cap requested a review from danielmitterdorfer May 18, 2020 17:58

Fix lint

667fa79

danielmitterdorfer reviewed May 19, 2020

View reviewed changes

Test un-cleanup ;)

451e2cb

hub-cap requested a review from danielmitterdorfer May 20, 2020 18:43

danielmitterdorfer approved these changes May 25, 2020

View reviewed changes

hub-cap merged commit 3300015 into elastic:master May 26, 2020

hub-cap deleted the bundle_version_check branch May 26, 2020 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate if car allows for using the bundled JDK #987

Validate if car allows for using the bundled JDK #987

hub-cap commented May 6, 2020

hub-cap commented May 6, 2020

hub-cap commented May 6, 2020

hub-cap commented May 6, 2020

danielmitterdorfer left a comment

danielmitterdorfer May 7, 2020

danielmitterdorfer May 7, 2020

danielmitterdorfer May 7, 2020

danielmitterdorfer May 7, 2020

danielmitterdorfer left a comment

danielmitterdorfer May 18, 2020

danielmitterdorfer May 18, 2020

danielmitterdorfer May 18, 2020

danielmitterdorfer May 18, 2020

danielmitterdorfer May 18, 2020

danielmitterdorfer left a comment •

edited

Loading

danielmitterdorfer May 19, 2020

hub-cap May 20, 2020

danielmitterdorfer May 19, 2020

hub-cap commented May 20, 2020

danielmitterdorfer left a comment

Validate if car allows for using the bundled JDK #987

Validate if car allows for using the bundled JDK #987

Conversation

hub-cap commented May 6, 2020

hub-cap commented May 6, 2020

hub-cap commented May 6, 2020

hub-cap commented May 6, 2020

danielmitterdorfer 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

danielmitterdorfer 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

danielmitterdorfer left a comment • edited Loading

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

hub-cap commented May 20, 2020

danielmitterdorfer left a comment

Choose a reason for hiding this comment

danielmitterdorfer left a comment •

edited

Loading