Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(SERVER-297) Consolidate JRuby environment handling #582

Merged
merged 12 commits into from Jun 5, 2015
Merged

(SERVER-297) Consolidate JRuby environment handling #582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3c5d51b
(SERVER-297) Consolidate JRuby environment handling
May 27, 2015
cbf73ef
(maint) Fix managed-load-path schema errors
May 29, 2015
5cce882
(maint) Remove unused definitions from jruby-puppet-core
May 29, 2015
d1b19da
(SERVER-297) Fix up LOAD_PATH behavior
May 29, 2015
953580e
(SERVER-297) Add jruby subcommand integration tests
Jun 1, 2015
484fe80
(SERVER-297) Remove duplicate entries at the head of $LOAD_PATH
Jun 3, 2015
e0deb85
(SERVER-297) Check if subcommand exists before loading
Jun 2, 2015
54b7bb7
(maint) Mark j_p_internal functions public / private
Jun 4, 2015
cd7a315
(maint) Add compile-mode, compat-version defs
Jun 4, 2015
fcd7aa3
(maint) use schema/maybe instead of schema/pred nil?
Jun 4, 2015
b14b711
(SERVER-297) Add acceptance tests for env behavior
Jun 5, 2015
46135d8
(maint) Add jruby-schemas/EnvMap
Jun 5, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 23 additions & 0 deletions acceptance/suites/tests/010-puppetserver-cli/subcommand/SERVER-297_common_behavior.rb
View file
Open in desktop
@@ -0,0 +1,23 @@
require 'test/unit/assertions'
require 'json'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also doesn't look like this is used?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm stupid, it's right there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used for JSON.parse


test_name "Puppetserver subcommand consolidated ENV handling tests."

step "ruby: Check that PATH, HOME, GEM_HOME JARS_REQUIRE and JARS_NO_REQUIRE are present"
on(master, "puppetserver ruby -rjson -e 'puts JSON.pretty_generate(ENV.to_hash)'") do
env = JSON.parse(stdout)
assert(env['PATH'], "PATH missing")
assert(env['HOME'], "HOME missing")
assert(env['GEM_HOME'], "GEM_HOME missing")
assert(env['JARS_REQUIRE'], "JARS_REQUIRE missing")
assert(env['JARS_NO_REQUIRE'], "JARS_NO_REQUIRE missing")
end

step "irb: Check that PATH, HOME, GEM_HOME JARS_REQUIRE and JARS_NO_REQUIRE are present"
on(master, "puppetserver irb -f -rjson -e 'puts JSON.pretty_generate(ENV.to_hash)'") do
assert_match(/\bPATH\b/, output, "PATH missing")
assert_match(/\bHOME\b/, output, "HOME missing")
assert_match(/\bGEM_HOME\b/, output, "GEM_HOME missing")
assert_match(/\bJARS_REQUIRE\b/, output, "JARS_REQUIRE missing")
assert_match(/\bJARS_NO_REQUIRE\b/, output, "JARS_NO_REQUIRE missing")
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✌️ nice looking test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

3 changes: 3 additions & 0 deletions acceptance/suites/tests/010-puppetserver-cli/subcommand/gem.rb
View file
Open in desktop
Expand Up @@ -66,3 +66,6 @@
initial_installed_gems.each do |gem_info|
assert_send([final_installed_gems, :include?, gem_info])
end

step "Verify that gem env operates"
on(master, "#{cli} gem env", :acceptable_exit_codes => [0])
14 changes: 3 additions & 11 deletions src/clj/puppetlabs/puppetserver/cli/gem.clj
View file
Open in desktop
@@ -1,18 +1,10 @@
(ns puppetlabs.puppetserver.cli.gem
(:import (org.jruby.embed ScriptingContainer))
(:require [puppetlabs.puppetserver.cli.subcommand :as cli]))
(:require [puppetlabs.puppetserver.cli.subcommand :as cli]
[puppetlabs.services.jruby.jruby-puppet-core :as jruby-core]))

(defn run!
[config args]
(doto (ScriptingContainer.)
(.setArgv (into-array String args))
(.setEnvironment
(hash-map
"GEM_HOME" (get-in config [:jruby-puppet :gem-home])
"JARS_NO_REQUIRE" "true"
"JARS_REQUIRE" "false"))
(.runScriptlet "require 'jar-dependencies'")
(.runScriptlet "load 'META-INF/jruby.home/bin/gem'")))
(jruby-core/cli-run! config "gem" args))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose I could kill this function entirely and reduce this to:

(defn -main [& args] (cli/run jruby-core/cli-run! "gem" args))

Sane?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't be opposed to squashing these like you proposed. Then at least all these minimal CLI namespaces will look the same, where they just call cli/run with a jruby-core/cli-{run!,ruby!} function. No big deal though


(defn -main
[& args]
Expand Down
35 changes: 3 additions & 32 deletions src/clj/puppetlabs/puppetserver/cli/irb.clj
View file
Open in desktop
@@ -1,39 +1,10 @@
(ns puppetlabs.puppetserver.cli.irb
(:import (org.jruby Main RubyInstanceConfig CompatVersion)
(java.util HashMap))
(:require [puppetlabs.puppetserver.cli.subcommand :as cli]
[puppetlabs.services.jruby.jruby-puppet-internal :as jruby-internal]))

(defn new-jruby-main
[config]
(let [jruby-config (RubyInstanceConfig.)
gem-home (get-in config [:jruby-puppet :gem-home])
env (doto (HashMap. (.getEnvironment jruby-config))
(.put "GEM_HOME" gem-home)
(.put "JARS_NO_REQUIRE" "true")
(.put "JARS_REQUIRE" "false"))
jruby-home (.getJRubyHome jruby-config)
load-path (->> (get-in config [:jruby-puppet :ruby-load-path])
(cons jruby-internal/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")))]
(doto jruby-config
(.setEnvironment env)
(.setLoadPaths load-path)
(.setCompatVersion (CompatVersion/RUBY1_9)))
(Main. jruby-config)))
[puppetlabs.services.jruby.jruby-puppet-core :as jruby-core]))

(defn run!
[config ruby-args]
(doto (new-jruby-main config)
(.run (into-array String
(concat
["-rjar-dependencies"
"-e"
"load 'META-INF/jruby.home/bin/irb'"
"--"]
ruby-args)))))
[config args]
(jruby-core/cli-run! config "irb" args))

(defn -main
[& args]
Expand Down
32 changes: 2 additions & 30 deletions src/clj/puppetlabs/puppetserver/cli/ruby.clj
View file
Open in desktop
@@ -1,35 +1,7 @@
(ns puppetlabs.puppetserver.cli.ruby
(:import (org.jruby Main RubyInstanceConfig CompatVersion)
(java.util HashMap))
(:require [puppetlabs.puppetserver.cli.subcommand :as cli]
[puppetlabs.services.jruby.jruby-puppet-internal :as jruby-internal]))

(defn new-jruby-main
[config]
(let [jruby-config (RubyInstanceConfig.)
gem-home (get-in config [:jruby-puppet :gem-home])
env (doto (HashMap. (.getEnvironment jruby-config))
(.put "GEM_HOME" gem-home)
(.put "JARS_NO_REQUIRE" "true")
(.put "JARS_REQUIRE" "false"))
jruby-home (.getJRubyHome jruby-config)
load-path (->> (get-in config [:jruby-puppet :ruby-load-path])
(cons jruby-internal/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")))]
(doto jruby-config
(.setEnvironment env)
(.setLoadPaths load-path)
(.setCompatVersion (CompatVersion/RUBY1_9)))
(Main. jruby-config)))

(defn run!
[config ruby-args]
(doto (new-jruby-main config)
(.run (into-array String
(cons "-rjar-dependencies" ruby-args)))))
[puppetlabs.services.jruby.jruby-puppet-core :as jruby-core]))

(defn -main
[& args]
(cli/run run! args))
(cli/run jruby-core/cli-ruby! args))
35 changes: 19 additions & 16 deletions src/clj/puppetlabs/services/jruby/jruby_puppet_core.clj
View file
Open in desktop
Expand Up @@ -4,7 +4,8 @@
[puppetlabs.services.jruby.jruby-puppet-schemas :as jruby-schemas]
[puppetlabs.services.jruby.puppet-environments :as puppet-env]
[puppetlabs.services.jruby.jruby-puppet-internal :as jruby-internal]
[puppetlabs.services.jruby.jruby-puppet-agents :as jruby-agents])
[puppetlabs.services.jruby.jruby-puppet-agents :as jruby-agents]
[clojure.java.io :as io])
(:import (puppetlabs.services.jruby.jruby_puppet_schemas JRubyPuppetInstance)
(com.puppetlabs.puppetserver PuppetProfiler)))

Expand Down Expand Up @@ -41,21 +42,6 @@
(def default-master-var-dir
"/opt/puppetlabs/server/data/puppetserver")

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;; Definitions

(def jruby-puppet-env
"The environment variables that should be passed to the Puppet JRuby interpreters.
We don't want them to read any ruby environment variables, like $GEM_HOME or
$RUBY_LIB or anything like that, so pass it an empty environment map - except -
Puppet needs HOME and PATH for facter resolution, so leave those."
(select-keys (System/getenv) ["HOME" "PATH"]))

(def ruby-code-dir
"The name of the directory containing the ruby code in this project.
This directory lives under src/ruby/"
"puppet-server-lib")

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;; Private

Expand Down Expand Up @@ -190,3 +176,20 @@
"Return a borrowed pool instance to its free pool."
[instance :- jruby-schemas/JRubyPuppetInstanceOrRetry]
(jruby-internal/return-to-pool instance))

(schema/defn ^:always-validate cli-ruby! :- jruby-schemas/JRubyMainStatus
"Run JRuby as though native `ruby` were invoked with args on the CLI"
[config :- {schema/Keyword schema/Any}
args :- [schema/Str]]
(let [main (jruby-internal/new-main (initialize-config config))
argv (into-array String (concat ["-rjar-dependencies"] args))]
(.run main argv)))

(schema/defn ^:always-validate cli-run! :- (schema/maybe jruby-schemas/JRubyMainStatus)
"Run a JRuby CLI command, e.g. gem, irb, etc..."
[config :- {schema/Keyword schema/Any}
command :- schema/Str
args :- [schema/Str]]
(let [load-path (format "META-INF/jruby.home/bin/%s" command)]
(if-let [url (io/resource load-path (.getClassLoader org.jruby.Main))]
(cli-ruby! config (concat ["-e" (format "load '%s'" url) "--"] args)))))
130 changes: 93 additions & 37 deletions src/clj/puppetlabs/services/jruby/jruby_puppet_internal.clj
View file
Open in desktop
Expand Up @@ -7,61 +7,92 @@
(:import (com.puppetlabs.puppetserver PuppetProfiler JRubyPuppet)
(puppetlabs.services.jruby.jruby_puppet_schemas JRubyPuppetInstance PoisonPill)
(java.util HashMap)
(org.jruby CompatVersion RubyInstanceConfig$CompileMode)
(org.jruby CompatVersion Main RubyInstanceConfig RubyInstanceConfig$CompileMode)
(org.jruby.embed ScriptingContainer LocalContextScope)
(java.util.concurrent LinkedBlockingDeque TimeUnit)
(clojure.lang IFn)))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;; Definitions

(def jruby-puppet-env
"The environment variables that should be passed to the Puppet JRuby interpreters.
We don't want them to read any ruby environment variables, like $GEM_HOME or
$RUBY_LIB or anything like that, so pass it an empty environment map - except -
Puppet needs HOME and PATH for facter resolution, so leave those."
(select-keys (System/getenv) ["HOME" "PATH"]))

(def ruby-code-dir
"The name of the directory containing the ruby code in this project.
This directory lives under src/ruby/"

This directory is relative to `src/ruby` and works from source because the
`src/ruby` directory is defined as a resource in `project.clj` which places
the directory on the classpath which in turn makes the directory available on
the JRuby load path. Similarly, this works from the uberjar because this
directory is placed into the root of the jar structure which is on the
classpath.

See also: http://jruby.org/apidocs/org/jruby/runtime/load/LoadService.html"
"puppet-server-lib")

(def compile-mode
"The JRuby compile mode to use for all ruby components, e.g. the master
service and CLI tools."
RubyInstanceConfig$CompileMode/OFF)

(def compat-version
"The JRuby compatibility version to use for all ruby components, e.g. the
master service and CLI tools."
(CompatVersion/RUBY1_9))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;; Private

(schema/defn get-system-env :- jruby-schemas/EnvPersistentMap
"Same as System/getenv, but returns a clojure persistent map instead of a
Java unmodifiable map."
[]
(into {} (System/getenv)))

(defn instantiate-free-pool
"Instantiate a new queue object to use as the pool of free JRubyPuppet's."
[size]
{:post [(instance? jruby-schemas/pool-queue-type %)]}
(LinkedBlockingDeque. size))

(schema/defn ^:always-validate
create-pool-from-config :- jruby-schemas/PoolState
"Create a new PoolData based on the config input."
[{size :max-active-instances} :- jruby-schemas/JRubyPuppetConfig]
{:pool (instantiate-free-pool size)
:size size})
(schema/defn ^:always-validate managed-environment :- jruby-schemas/EnvMap
"The environment variables that should be passed to the Puppet JRuby
interpreters.

We don't want them to read any ruby environment variables, like $RUBY_LIB or
anything like that, so pass it an empty environment map - except - Puppet
needs HOME and PATH for facter resolution, so leave those, along with GEM_HOME
which is necessary for third party extensions that depend on gems.

We need to set the JARS..REQUIRE variables in order to instruct JRuby's
'jar-dependencies' to not try to load any dependent jars. This is being
done specifically to avoid JRuby trying to load its own version of Bouncy
Castle, which may not the same as the one that 'puppetlabs/ssl-utils'
uses. JARS_NO_REQUIRE was the legacy way to turn off jar loading but is
being phased out in favor of JARS_REQUIRE. As of JRuby 1.7.20, only
JARS_NO_REQUIRE is honored. Setting both of those here for forward
compatibility."
[env :- jruby-schemas/EnvMap
gem-home :- schema/Str]
(let [whitelist ["HOME" "PATH"]
clean-env (select-keys env whitelist)]
(assoc clean-env
"GEM_HOME" gem-home
"JARS_NO_REQUIRE" "true"
"JARS_REQUIRE" "false")))

(schema/defn ^:always-validate managed-load-path :- [schema/Str]
"Return a list of ruby LOAD_PATH directories built from the
user-configurable ruby-load-path setting of the jruby-puppet configuration."
[ruby-load-path :- [schema/Str]]
(cons ruby-code-dir ruby-load-path))

(defn prep-scripting-container
[scripting-container ruby-load-path gem-home]
; Note, this behavior should remain consistent with new-main
(doto scripting-container
(.setLoadPaths (cons ruby-code-dir
(map fs/absolute-path ruby-load-path)))
(.setCompatVersion (CompatVersion/RUBY1_9))
(.setCompileMode RubyInstanceConfig$CompileMode/OFF)
;; We need to set the JARS..REQUIRE variables in order to instruct
;; JRuby's 'jar-dependencies' to not try to load any dependent jars. This
;; is being done specifically to avoid JRuby trying to load its own version
;; of Bouncy Castle, which may not the same as the one that
;; 'puppetlabs/ssl-utils' uses. JARS_NO_REQUIRE was the legacy way to turn
;; off jar loading but is being phased out in favor of JARS_REQUIRE. As of
;; JRuby 1.7.20, only JARS_NO_REQUIRE is honored. Setting both of those
;; here for forward compatibility.
(.setEnvironment (merge {"GEM_HOME" gem-home
"JARS_NO_REQUIRE" "true"
"JARS_REQUIRE" "false"}
jruby-puppet-env))))
(.setLoadPaths (managed-load-path ruby-load-path))
(.setCompatVersion compat-version)
(.setCompileMode compile-mode)
(.setEnvironment (managed-environment (get-system-env) gem-home))))

(defn empty-scripting-container
"Creates a clean instance of `org.jruby.embed.ScriptingContainer` with no code loaded."
Expand All @@ -71,7 +102,7 @@
(string? gem-home)]
:post [(instance? ScriptingContainer %)]}
(-> (ScriptingContainer. LocalContextScope/SINGLETHREAD)
(prep-scripting-container ruby-load-path gem-home)))
(prep-scripting-container ruby-load-path gem-home)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sorry. This snuck in with the public / private re-shuffle

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just meant that we think this formatting is a good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, heh. At least we don't have hash rockets to argue about, but I guess let bindings are effectively the same thing.


(defn create-scripting-container
"Creates an instance of `org.jruby.embed.ScriptingContainer` and loads up the
Expand Down Expand Up @@ -110,6 +141,21 @@
(.put puppet-config dir (fs/absolute-path value))))
puppet-config))

(schema/defn borrow-with-timeout-fn :- jruby-schemas/JRubyPuppetBorrowResult
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't do this, but I'm wondering why this is named as ...-fn. Does it return a function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry about spending any time tracking this down and potentially changing it unless it's worthwhile. I'm more just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK, but looking at the schema it doesn't look to be a function:

(def JRubyPuppetBorrowResult
  (schema/pred (some-fn nil?
                        poison-pill?
                        retry-poison-pill?
                        jruby-puppet-instance?)))

[timeout :- schema/Int
pool :- jruby-schemas/pool-queue-type]
(.pollFirst pool timeout TimeUnit/MILLISECONDS))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;; Public

(schema/defn ^:always-validate
create-pool-from-config :- jruby-schemas/PoolState
"Create a new PoolState based on the config input."
[{size :max-active-instances} :- jruby-schemas/JRubyPuppetConfig]
{:pool (instantiate-free-pool size)
:size size})

(schema/defn ^:always-validate
create-pool-instance! :- JRubyPuppetInstance
"Creates a new JRubyPuppet instance and adds it to the pool."
Expand Down Expand Up @@ -179,11 +225,6 @@
[pool :- jruby-schemas/pool-queue-type]
(.takeFirst pool))

(schema/defn borrow-with-timeout-fn :- jruby-schemas/JRubyPuppetBorrowResult
[timeout :- schema/Int
pool :- jruby-schemas/pool-queue-type]
(.pollFirst pool timeout TimeUnit/MILLISECONDS))

(schema/defn borrow-from-pool!* :- (schema/maybe jruby-schemas/JRubyPuppetInstanceOrRetry)
"Given a borrow function and a pool, attempts to borrow a JRuby instance from a pool.
If successful, updates the state information and returns the JRuby instance.
Expand Down Expand Up @@ -250,3 +291,18 @@
(.putFirst pool instance)))
;; if we get here, we got a Retry, so we just put it back into the pool.
(.putFirst (:pool instance) instance)))

(schema/defn ^:always-validate new-main :- jruby-schemas/JRubyMain
"Return a new JRuby Main instance which should only be used for CLI purposes,
e.g. for the ruby, gem, and irb subcommands. Internal core services should
use `create-scripting-container` instead of `new-main`."
[config :- jruby-schemas/JRubyPuppetConfig]
(let [jruby-config (RubyInstanceConfig.)
{:keys [ruby-load-path gem-home]} config]
; Note, this behavior should remain consistent with prep-scripting-container
(doto jruby-config
(.setLoadPaths (managed-load-path ruby-load-path))
(.setCompatVersion compat-version)
(.setCompileMode compile-mode)
(.setEnvironment (managed-environment (get-system-env) gem-home)))
(Main. jruby-config)))