(SERVER-297) Consolidate JRuby environment handling #582

jeffmccune · 2015-05-29T00:31:50Z

Without this patch the manner in which environment variables are handled in the
JRuby portions of puppet-server vary across the use cases of the puppetserver {gem,irb,ruby} command line utilities in addition to the service itself. This
is a problem because the behaviors have not kept in sync with one another over
time so there has been divergence at times, e.g. puppetserver gem env
currently fails because PATH is unset in that scenario but set in others, e.g.
the production service.

This patch addresses the problem by consolidating all of the environment
handling behavior into a single authoritative method. The behavior itself
remains unchanged, GEM_HOME is set based on the config file value, PATH and
HOME are the only variables allowed to flow through to JRuby unmodified, all
other variables are filtered out, and JARS_REQUIRE and JARS_NO_REQUIRE are set
appropriately.

In addition to the environment handling, this patch also consolidates the JRuby
load path handling in a similar fashion across the scenarios of command line
utilities and the production service.

jeffmccune · 2015-05-29T00:35:12Z

src/clj/puppetlabs/services/jruby/jruby_puppet_internal.clj

+                    (cons ruby-code-dir)
+                    (cons (str jruby-home "/lib/ruby/1.9"))
+                    (cons (str jruby-home "/lib/ruby/shared"))
+                    (cons (str jruby-home "/lib/ruby/1.9/site_ruby")))]


@camlow325 One question I had about this... The ticket says:

For Server 1.X only, add in the "front-loading" of the load path with embedded JRuby path libs, as is done for the irb subcommand today - see https://github.com/puppetlabs/puppet-server/blob/puppet-server-1.0.8/src/clj/puppetlabs/puppetserver/cli/irb.clj#L20-L24.

However, in the master branch both the irb and ruby commands, but not gem front-load these three directories relative to JRUBY_HOME. I left them in the consolidated implemenetation, but I was wondering if you could clarify the "for 1.X only" piece.

cprice404 · 2015-06-03T12:51:06Z

src/clj/puppetlabs/services/jruby/jruby_puppet_schemas.clj

+
+(def JRubyMainStatusOrNil
+  (schema/either JRubyMainStatus
+    (schema/pred nil? 'nil?)))


this could just be schema/maybe JRubyMainStatus, right?

maybe... I'm sure you're right, I just started writing schemas for functions so I'll check and fix it up if so.

cprice404 · 2015-06-03T13:02:48Z

I think I'm +1, except for the questions about possibly DRY'ing up the ScriptingContainer/RubyInstanceConfig stuff a bit.

jeffmccune · 2015-06-03T19:24:16Z

I think I'm +1, except for the questions about possibly DRY'ing up the ScriptingContainer/RubyInstanceConfig stuff a bit.

Cool, at this point the only thing that remains to change beyond addressing comments is to add 1 beaker test for each of the subcommands, which I hope to do today.

I also have a TODO item to build packages off this topic branch and validate we don't run into the hipchat gem problem described in SERVER-538. If we do run into the issue my plan is to just pull out d7574cc.

jeffmccune · 2015-06-05T00:25:29Z

Acceptance test added, this should be ready now. No longer "for review" but still needs the normal review.

Chris, this is largely the same as when you looked at it earlier.

nwolfe · 2015-06-05T17:35:24Z

src/clj/puppetlabs/services/jruby/jruby_puppet_internal.clj

+
+(def compat-version
+  "The JRuby compatibility version to use"
+  (CompatVersion/RUBY1_9))


Are these extra parens necessary? Just curious, since we don't have them for compile-mode above. I can't remember if there's a subtle difference

Yes, CompatVersion/RUBY1_9 is a method, so we need to call it to get the value.

Hmm, actually, it's not a method, it's an enum value ...

public enum CompatVersion { RUBY1_8, RUBY1_9, RUBY2_0, BOTH;

... which means that the parens can be removed, I think. Should be same as accessing a field.

nwolfe · 2015-06-05T18:15:59Z

acceptance/suites/tests/010-puppetserver-cli/subcommand/SERVER-297_common_behavior.rb

+# the need to fix this once Beaker has been modified to make the paths to these
+# commands available.
+#
+cli = "puppetserver"


Do we need this comment and cli variable anymore? Let's just inline it below and delete this cruft if so

Fixed up in b14b711 (SERVER-297) Add acceptance tests for env behavior

nwolfe · 2015-06-05T18:35:55Z

👍 other than minor nitpick comments

Without this patch the manner in which environment variables are handled in the JRuby portions of puppet-server vary across the use cases of the `puppetserver {gem,irb,ruby}` command line utilities in addition to the service itself. This is a problem because the behaviors have not kept in sync with one another over time so there has been divergence at times, e.g. `puppetserver gem env` currently fails because PATH is unset in that scenario but set in others, e.g. the production service. This patch addresses the problem by consolidating all of the environment handling behavior into a single authoritative method. The behavior itself remains unchanged, GEM_HOME is set based on the config file value, PATH and HOME are the only variables allowed to flow through to JRuby unmodified, all other variables are filtered out, and JARS_REQUIRE and JARS_NO_REQUIRE are set appropriately. In addition to the environment handling, this patch also consolidates the JRuby load path handling in a similar fashion across the scenarios of command line utilities and the production service.

Fix this error: Caused by: clojure.lang.ExceptionInfo: Output of managed-load-path does not match schema: [(not (instance? java.lang.String a-java.io.File)) (not (instance? java.lang.String a-java.io.File)) (not (instance? java.lang.String a-java.io.File)) (not (instance? java.lang.String a-java.io.File)) (not (instance? java.lang.String a-java.io.File)) (not (instance? java.lang.String a-java.io.File))] at puppetlabs.services.jruby.jruby_puppet_internal$eval9067$managed_load_path__9068.invoke (jruby_puppet_internal.clj:69) puppetlabs.services.jruby.jruby_puppet_internal$prep_scripting_container.invoke (jruby_puppet_internal.clj:89) puppetlabs.services.jruby.jruby_puppet_internal$empty_scripting_container.invoke (jruby_puppet_internal.clj:101) puppetlabs.services.jruby.jruby_puppet_internal$create_scripting_container.invoke (jruby_puppet_internal.clj:117) puppetlabs.services.jruby.jruby_puppet_internal$eval9122$create_pool_instance_BANG___9123$fn__9124.invoke (jruby_puppet_internal.clj:156) puppetlabs.services.jruby.jruby_puppet_internal$eval9122$create_pool_instance_BANG___9123.invoke (jruby_puppet_internal.clj:141)

These defs moved over to jruby-puppet-internal in 23154f4 and are no longer used from jruby-puppet-core.

Without this patch the load path behavior is incorrect because the use of normalized and absolute paths is incompatible with the use of loading resources from a sealed jar. This patch addresses the problem by restoring the previous behavior of implicitly relying in JRuby's LoadService implementation. For more information, particularly on the combination and interaction of load path, class path, and ruby require, please see: https://github.com/jruby/jruby/blob/1.7.20/core/src/main/java/org/jruby/runtime/load/LoadService.java

Without this patch there are not tests covering the behavior of the irb, ruby, and gem JRuby subcommands. This patch addresses the problem by adding integration tests that assert against standard output and the exit status.

Without this patch there are duplicate entries for the system ruby library paths at the head of the $LOAD_PATH. This behavior exists to prevent accidentally loading libraries from the OS MRI copy of Ruby and was necessary back when puppetserver loaded puppet code from the OS ruby library paths. This is no longer necessary as puppet code is isolated in PC1 and exists outside of any Ruby library paths. This patch addresses the problem by removing the duplicate entries from the head of the LOAD_PATH restoring the default behavior of JRuby.

Without this patch the subcommand string given to the cli-run! command may not actually exist in the assumed directory of META-INF/jruby.home/bin/ This patch checks before trying to execute the given subcommand.

Addressing PR comments, sorted by using "find usages" in cursive.

Without this patch the initialization logic for the JRuby instances in new-main and prep-scripting-container are almost identical. This is a problem because they should be identical and the compatibility version and compile mode are not referenced in a way that changing one will also change the other. This patch addresses the problem by making the behavior identical without going so far as to providing a single function that operates on both classes of objects.

Without this patch there aren't any acceptance tests that validate the behavior of environment handling. This patch addresses the problem by adding a test for the irb and ruby commands. The gem command is already tested via the `gem env` command and is difficult to inspect the environment from since it doesn't evaluate arbitrary code like irb and ruby can.

jeffmccune · 2015-06-05T19:12:51Z

@nwolfe I've addressed all of your comments. Thanks for the eye to detail.

…solidate_env_handling (SERVER-297) Consolidate JRuby environment handling

jeffmccune reviewed May 29, 2015
View reviewed changes

jeffmccune force-pushed the improve/master/SERVER-297_consolidate_env_handling branch 4 times, most recently from 646a2f8 to 6a44dd6 Compare June 3, 2015 00:11

cprice404 reviewed Jun 3, 2015
View reviewed changes

jeffmccune force-pushed the improve/master/SERVER-297_consolidate_env_handling branch from 6a44dd6 to 791ba54 Compare June 3, 2015 19:24

jeffmccune force-pushed the improve/master/SERVER-297_consolidate_env_handling branch 3 times, most recently from 8a9b0b7 to 1813197 Compare June 4, 2015 23:37

jeffmccune changed the title ~~[FOR REVIEW] (SERVER-297) Consolidate JRuby environment handling~~ (SERVER-297) Consolidate JRuby environment handling Jun 5, 2015

nwolfe reviewed Jun 5, 2015
View reviewed changes

jeffmccune force-pushed the improve/master/SERVER-297_consolidate_env_handling branch 2 times, most recently from 34ab169 to 586c8e7 Compare June 5, 2015 18:12

nwolfe reviewed Jun 5, 2015
View reviewed changes

Jeff McCune added 12 commits June 5, 2015 12:03

(maint) Remove unused definitions from jruby-puppet-core

5cce882

These defs moved over to jruby-puppet-internal in 23154f4 and are no longer used from jruby-puppet-core.

(SERVER-297) Add jruby subcommand integration tests

953580e

Without this patch there are not tests covering the behavior of the irb, ruby, and gem JRuby subcommands. This patch addresses the problem by adding integration tests that assert against standard output and the exit status.

(SERVER-297) Check if subcommand exists before loading

e0deb85

Without this patch the subcommand string given to the cli-run! command may not actually exist in the assumed directory of META-INF/jruby.home/bin/ This patch checks before trying to execute the given subcommand.

(maint) Mark j_p_internal functions public / private

54b7bb7

Addressing PR comments, sorted by using "find usages" in cursive.

(maint) use schema/maybe instead of schema/pred nil?

fcd7aa3

(maint) Add jruby-schemas/EnvMap

46135d8

jeffmccune force-pushed the improve/master/SERVER-297_consolidate_env_handling branch from 79711eb to 46135d8 Compare June 5, 2015 19:11

nwolfe added a commit that referenced this pull request Jun 5, 2015

Merge pull request #582 from jeffmccune/improve/master/SERVER-297_con…

08695d2

…solidate_env_handling (SERVER-297) Consolidate JRuby environment handling

nwolfe merged commit 08695d2 into puppetlabs:master Jun 5, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(SERVER-297) Consolidate JRuby environment handling #582

(SERVER-297) Consolidate JRuby environment handling #582

jeffmccune commented May 29, 2015

jeffmccune May 29, 2015

cprice404 Jun 3, 2015

jeffmccune Jun 4, 2015

cprice404 commented Jun 3, 2015

jeffmccune commented Jun 3, 2015

jeffmccune commented Jun 5, 2015

nwolfe Jun 5, 2015

jeffmccune Jun 5, 2015

KevinCorcoran Jun 5, 2015

nwolfe Jun 5, 2015

jeffmccune Jun 5, 2015

nwolfe commented Jun 5, 2015

jeffmccune commented Jun 5, 2015

(SERVER-297) Consolidate JRuby environment handling #582

(SERVER-297) Consolidate JRuby environment handling #582

Conversation

jeffmccune commented May 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cprice404 commented Jun 3, 2015

jeffmccune commented Jun 3, 2015

jeffmccune commented Jun 5, 2015

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

nwolfe commented Jun 5, 2015

jeffmccune commented Jun 5, 2015