From a22da80c7207695b5d77cd4d3e3ccfd7faf563bf Mon Sep 17 00:00:00 2001 From: Ambrose Bonnaire-Sergeant Date: Tue, 13 Jul 2021 17:41:42 -0400 Subject: [PATCH] Fix #294: handle larger service graphs --- src/puppetlabs/trapperkeeper/internal.clj | 35 ++++++++-- test/puppetlabs/trapperkeeper/core_test.clj | 62 +++++++++++++++++- .../trapperkeeper/internal_test.clj | 65 ++++++++++++++++++- 3 files changed, 154 insertions(+), 8 deletions(-) diff --git a/src/puppetlabs/trapperkeeper/internal.clj b/src/puppetlabs/trapperkeeper/internal.clj index afcab55e..19c829da 100644 --- a/src/puppetlabs/trapperkeeper/internal.clj +++ b/src/puppetlabs/trapperkeeper/internal.clj @@ -133,16 +133,43 @@ (i18n/trs "Services ''{0}'' not found" missing-services))))) (throw e))))) +(schema/defschema GraphCompiler + (schema/=> (schema/=> (schema/pred service-graph?) + (schema/pred map?)) + (schema/pred service-graph?))) + +(schema/defn compile-graph* + "Helper function for `compile-graph` for testing purposes." + [graph-map + eager-compile :- GraphCompiler + interpreted-eager-compile :- GraphCompiler] + {:pre [(service-graph? graph-map)] + :post [(ifn? %)]} + (try + (eager-compile graph-map) + (catch ExceptionInfo e + (handle-prismatic-exception! e)) + ;; work around https://github.com/plumatic/plumbing/issues/138 + ;; approach: switch to interpreted compilation, which scales to large + ;; service graphs. + (catch clojure.lang.Compiler$CompilerException e + (log/debug e (str "Error while compiling specialized service graph, trying interpreted mode" + " to handle larger graphs.")) + (try + (interpreted-eager-compile graph-map) + (catch ExceptionInfo e + (handle-prismatic-exception! e)))))) + (defn compile-graph "Given the merged map of services, compile it into a function suitable for instantiation. Throws an exception if there is a dependency on a service that is not found in the map." [graph-map] {:pre [(service-graph? graph-map)] :post [(ifn? %)]} - (try - (graph/eager-compile graph-map) - (catch ExceptionInfo e - (handle-prismatic-exception! e)))) + (compile-graph* + graph-map + graph/eager-compile + graph/interpreted-eager-compile)) (defn instantiate "Given the compiled graph function, instantiate the application. Throws an exception diff --git a/test/puppetlabs/trapperkeeper/core_test.clj b/test/puppetlabs/trapperkeeper/core_test.clj index 8158ab13..9bc4b2a3 100644 --- a/test/puppetlabs/trapperkeeper/core_test.clj +++ b/test/puppetlabs/trapperkeeper/core_test.clj @@ -2,13 +2,14 @@ (:require [clojure.test :refer :all] [slingshot.slingshot :refer [try+]] [puppetlabs.kitchensink.core :refer [without-ns]] - [puppetlabs.trapperkeeper.services :refer [service]] - [puppetlabs.trapperkeeper.app :refer [get-service]] + [puppetlabs.trapperkeeper.services :refer [defservice service]] + [puppetlabs.trapperkeeper.app :as app :refer [get-service]] [puppetlabs.trapperkeeper.internal :refer [parse-cli-args!]] [puppetlabs.trapperkeeper.core :as trapperkeeper] [puppetlabs.trapperkeeper.testutils.bootstrap :as testutils] [puppetlabs.trapperkeeper.config :as config] [puppetlabs.trapperkeeper.testutils.logging :as logging] + [puppetlabs.trapperkeeper.logging :refer [root-logger-name]] [schema.test :as schema-test] [puppetlabs.kitchensink.core :as ks])) @@ -130,3 +131,60 @@ {:config (str tk-config-file-with-restart) :restart-file cli-restart-file})] (is (= cli-restart-file (get-in config [:global :restart-file]))))))) + +;; related: https://github.com/plumatic/plumbing/issues/138 +(def number-of-test-services + "Should be higher than the number of defrecord fields + allowed in practice in order to hit plumatic graph specialization limits." + 200) + +;; create a chain of services of length `number-of-test-services` +(doseq [i (range number-of-test-services) + :let [->service-symbol (fn [i] (symbol (str "LargeServiceGraphTestService" i))) + ->method-symbol (fn [i] (symbol (str "large-service-graph-test-service-method" i))) + ->defservice-symbol (fn [i] (symbol (str "large-service-graph-test-service" i))) + service-symbol (->service-symbol i) + method-symbol (->method-symbol i) + defservice-symbol (->defservice-symbol i) + previous-service (when (pos? i) + (->service-symbol (dec i))) + previous-method (when (pos? i) + (->method-symbol (dec i)))]] + (eval + `(defprotocol ~service-symbol + (~method-symbol [this# example-input#]))) + (eval + `(defservice + ~defservice-symbol + ~service-symbol + ~(if previous-service + [[(keyword previous-service) previous-method]] + []) + (~method-symbol [this# example-input#] + [~i example-input# + ~(if previous-method + ;; check service graph dependencies still work + (list `first (list previous-method :dummy-arg)) + ;; makes writing tests easier + -1)])))) + +(def this-ns (ns-name *ns*)) + +(deftest large-service-graph-test + (testing "large service graphs compile correctly via interpretation" + (let [services (mapv (fn [i] + @(ns-resolve this-ns (symbol (str "large-service-graph-test-service" i)))) + (range number-of-test-services))] + (logging/with-test-logging + (testutils/with-app-with-config app services {} + (is (logged? #"Error while compiling specialized service graph.*" :debug)) + (let [sg (app/service-graph app)] + (testing "right number of services" + (is (= (+ 2 number-of-test-services) (count (app/service-graph app))))) + (doseq [i (range number-of-test-services)] + (let [g (gensym)] + (testing (str "service graph function for service " i) + (is (= [i g (dec i)] + ((get-in sg [(keyword (str "LargeServiceGraphTestService" i)) + (keyword (str "large-service-graph-test-service-method" i))]) + g)))))))))))) diff --git a/test/puppetlabs/trapperkeeper/internal_test.clj b/test/puppetlabs/trapperkeeper/internal_test.clj index abe0f6c1..e24b049a 100644 --- a/test/puppetlabs/trapperkeeper/internal_test.clj +++ b/test/puppetlabs/trapperkeeper/internal_test.clj @@ -1,10 +1,13 @@ (ns puppetlabs.trapperkeeper.internal-test (:require [clojure.test :refer :all] + [plumbing.core :refer [fnk]] + [plumbing.graph :as graph] [puppetlabs.trapperkeeper.core :as tk] [puppetlabs.trapperkeeper.app :as tk-app] [puppetlabs.trapperkeeper.internal :as internal] [puppetlabs.trapperkeeper.testutils.bootstrap :as testutils] - [puppetlabs.trapperkeeper.testutils.logging :as logging])) + [puppetlabs.trapperkeeper.testutils.logging :as logging] + [schema.core :as schema])) (deftest test-queued-restarts (testing "main lifecycle and calls to `restart-tk-apps` are not executed concurrently" @@ -108,4 +111,62 @@ (tk-app/stop app) ;; and make sure that we got one last :stop (is (= (conj expected-lifecycle-events :stop) - @lifecycle-events))))) \ No newline at end of file + @lifecycle-events))))) + +(def dummy-service-map-val + (fnk dummy-service-map-val + :- schema/Any + [] + (assert nil))) + +;; related: https://github.com/puppetlabs/trapperkeeper/issues/294 +(deftest compile-graph-test + (testing "specialized compilation" + (let [dummy-small-service-graph {:Service1 dummy-service-map-val}] + (is (ifn? (internal/compile-graph dummy-small-service-graph))))) + (testing "interpreted compilation" + (let [dummy-huge-service-graph (into {} + (map (fn [i] + {(keyword (str "Service" i)) + dummy-service-map-val})) + ;; should be larger than the number of fields + ;; allowed in a defrecord in practice. + ;; related: https://github.com/plumatic/plumbing/issues/138 + (range 1000))] + (is (ifn? (internal/compile-graph dummy-huge-service-graph))))) + (testing "internal logic" + (let [eager-compile-succeed (fn [g] + ::eager-compile-succeed) + eager-compile-too-large (fn [g] + ;; simulate a "Method too large!" + ;; or "Too many arguments in method signature in class file" exception + ;; (ie., the symptoms of hitting the limits of graph/eager-compile). + (throw (clojure.lang.Compiler$CompilerException. + "" + 0 + 0 + (Exception.)))) + interpreted-eager-compile-succeed (fn [g] + ::interpreted-eager-compile-succeed) + interpreted-eager-compile-fail (fn [g] + (throw (Exception. "Interpreted compile failed")))] + (testing "specialization succeeds" + (is (= ::eager-compile-succeed + (internal/compile-graph* + {} + eager-compile-succeed + interpreted-eager-compile-fail)))) + (testing "specialization fails, interpretation succeeds" + (is (= ::interpreted-eager-compile-succeed + (internal/compile-graph* + {} + eager-compile-too-large + interpreted-eager-compile-succeed)))) + (testing "specialization and interpretation fails, throws non-prismatic error" + (is (thrown-with-msg? + Exception + #"Interpreted compile failed" + (internal/compile-graph* + {} + eager-compile-too-large + interpreted-eager-compile-fail)))))))