Remove generic rescues to let errors bubble up (#10)

* Refactor get_service and scale_out methods

Removed unnecessary begin-rescue blocks for service retrieval and scaling.

* Bump version from 0.7.0 to 0.8.0

* Refactor metric retrieval for New Relic API

* Remove generic rescue for network errors

Removed generic rescue block for network errors in worker.rb.

* Change exception handling from RuntimeError to ApplicationError

* Fix specs

* Classify errors as permanent or recoverable

Author: maboelnour

README.md

@@ -206,6 +206,47 @@ The configuration file (determined by `-f FILE` command line parameter) should b
 
 More details about configuration parameters can be found in [HireFire docs](https://help.hirefire.io/guides).
 
+## Error handling
+
+Scaltainer keeps going when a problem is likely to pass, and stops when it is not.
+Anything permanent, such as a mistake in your configuration or credentials that are
+refused, exits straight away so you hear about it immediately instead of finding it
+buried in the logs days later. Anything temporary is logged and tried again on the
+next tick.
+
+### Fatal errors
+
+Scaltainer exits with a non-zero status. Fix the cause and start it again.
+
+| What went wrong | Where |
+| --- | --- |
+| The configuration file is missing, or is not valid YAML | startup |
+| `--enable-newrelic-reporting` is used without `NEW_RELIC_LICENSE_KEY` or `NEW_RELIC_APP_NAME` | startup |
+| `NEW_RELIC_API_KEY` is not set, or New Relic refuses it | web services |
+| A web service has no `newrelic_app_id`, or New Relic does not recognise the one given | web services |
+| A web service is missing `min_response_time` or `max_response_time`, or the two are in the wrong order | web services |
+| A worker service is missing `ratio` | worker services |
+| `endpoint` returns JSON that is not the list of queues scaltainer expects | worker services |
+| Kubernetes refuses the credentials, or the service account may not read or patch the resource | kubernetes |
+| The Docker service is global and cannot be replicated | swarm |
+
+### Retriable errors
+
+Scaltainer logs these and carries on. Whatever could not be handled is left alone
+until the next tick.
+
+| What went wrong | What is skipped |
+| --- | --- |
+| A configured service is missing from the metrics response | that service |
+| The application is idle, so New Relic has no data for the window | that service |
+| `endpoint` reports a negative or non-integer queue size | that service |
+| The Kubernetes resource is not deployed yet, or changed while it was being scaled | that service |
+| Kubernetes returns a server error | that service |
+| No web or worker services are configured | that type |
+| `endpoint` answers with something that is not JSON | all worker services |
+| New Relic is rate limiting, erroring, or returns a payload that cannot be read | all web services |
+| The Prometheus push gateway is unreachable | nothing, that tick's metrics are dropped |
+
 ## Docker Swarm usage
 
 A service definition for scaltainer is typically something like this:

lib/scaltainer/command.rb

@@ -25,8 +25,8 @@ def self.parse(args)
         opts.on("--enable-newrelic-reporting", "Enable metrics pushing to New Relic") do
           newrelic_license_key = ENV['NEW_RELIC_LICENSE_KEY']
           newrelic_app_name = ENV['NEW_RELIC_APP_NAME']
-          raise 'Must set NEW_RELIC_LICENSE_KEY environment variable if --enable-newrelic-reporting is set' if newrelic_license_key.nil? || newrelic_license_key == ""
-          raise 'Must set NEW_RELIC_APP_NAME environment variable if --enable-newrelic-reporting is set' if newrelic_app_name.nil? || newrelic_app_name == ""
+          raise ConfigurationError.new 'Must set NEW_RELIC_LICENSE_KEY environment variable if --enable-newrelic-reporting is set' if newrelic_license_key.nil? || newrelic_license_key == ""
+          raise ConfigurationError.new 'Must set NEW_RELIC_APP_NAME environment variable if --enable-newrelic-reporting is set' if newrelic_app_name.nil? || newrelic_app_name == ""
           enable_newrelic_reporting = true
         end
         opts.on("-v", "--version", "Show version and exit") do

lib/scaltainer/exceptions.rb

@@ -1,6 +1,11 @@
 module Scaltainer
+  # Recoverable conditions. The runner logs these and skips the affected
+  # resource (or resource type) for the current tick, then carries on.
   class ApplicationError < RuntimeError; end
-  class ConfigurationError < ApplicationError; end
   class NetworkError < ApplicationError; end
   class Warning < ApplicationError; end
-end
+
+  # Permanent misconfiguration. Retrying cannot fix it, so it is left to bubble
+  # up and terminate the process instead of being logged on every tick.
+  class ConfigurationError < RuntimeError; end
+end

lib/scaltainer/newrelic/metrics.rb

@@ -1,11 +1,20 @@
 module Newrelic
   class Metrics
+    # New Relic reports failures as an error body alongside an HTTP status.
+    # These statuses mean the request can never succeed as configured (unknown
+    # application id, rejected API key), so they are permanent. Anything else
+    # (rate limiting, 5xx) is transient and worth retrying on the next tick.
+    PERMANENT_ERROR_STATUSES = [400, 401, 403, 404].freeze
+
     def initialize(license_key)
       @headers = {"X-Api-Key" => license_key}
       @base_url = "https://api.newrelic.com/v2"
     end
 
     # https://docs.newrelic.com/docs/apis/rest-api-v2/application-examples-v2/average-response-time-examples-v2
+    # Returns the average response time, or nil when New Relic holds no data for
+    # the requested window. Callers must treat nil as "no metric for this
+    # resource" rather than as a response time.
     def get_avg_response_time(app_id, from, to)
       url = "#{@base_url}/applications/#{app_id}/metrics/data.json"
       conn = Excon.new(url, persistent: true, tcp_nodelay: true)
@@ -14,31 +23,83 @@ def get_avg_response_time(app_id, from, to)
         names[]=HttpDispatcher&values[]=average_call_time&values[]=call_count
         names[]=WebFrontend/QueueTime&values[]=call_count&values[]=average_response_time
       )
-      response_array = request(conn, metric_names_array, time_range)
-      http_call_count, http_average_call_time = response_array[0]["call_count"], response_array[0]["average_call_time"]
-      webfe_call_count, webfe_average_response_time = response_array[1]["call_count"], response_array[1]["average_response_time"]
+      http_values, webfe_values = request(conn, metric_names_array, time_range)
+
+      # an application that dispatched nothing in the window has no response time
+      return nil unless http_values
+      http_call_count = numeric_value http_values, "call_count"
+      return nil if http_call_count.zero?
+      http_average_call_time = numeric_value http_values, "average_call_time"
+
+      # queue time is an additive correction and is absent for applications that
+      # do not sit behind a request queue, in which case it contributes nothing
+      webfe_call_count = webfe_values ? numeric_value(webfe_values, "call_count") : 0
+      webfe_average_response_time = webfe_values ? numeric_value(webfe_values, "average_response_time") : 0
 
-      http_average_call_time + (1.0 * webfe_call_count * webfe_average_response_time / http_call_count) rescue 0.0/0
+      http_average_call_time + (1.0 * webfe_call_count * webfe_average_response_time / http_call_count)
     end
 
   private
 
     def request(conn, metric_names_array, time_range)
       requests = metric_names_array.map {|metric_names|
         {
-          method: :get, headers: @headers, 
+          method: :get, headers: @headers,
           query: "#{metric_names}&#{time_range}&summarize=true"
         }
       }
       responses = conn.requests requests
-      responses.map {|response|
-        body = JSON.parse(response.body)
-        if body["error"] && body["error"]["title"]
-          raise body["error"]["title"]
-        else
-          body["metric_data"]["metrics"][0]["timeslices"][0]["values"] rescue {}
-        end
-      }
+      responses.map {|response| extract_values response }
+    end
+
+    # Returns the values hash of the first timeslice, or nil when New Relic
+    # reported no data for the metric over the requested window.
+    def extract_values(response)
+      body = parse_body response
+      error = body["error"]
+      raise_api_error response.status, error["title"] if error.is_a?(Hash) && error["title"]
+
+      metric_data = body["metric_data"]
+      metrics = metric_data.is_a?(Hash) ? metric_data["metrics"] : nil
+      raise_unexpected_payload response unless metrics.is_a?(Array)
+      return nil if metrics.empty?
+
+      timeslices = metrics.first.is_a?(Hash) ? metrics.first["timeslices"] : nil
+      raise_unexpected_payload response unless timeslices.is_a?(Array)
+      return nil if timeslices.empty?
+
+      values = timeslices.first.is_a?(Hash) ? timeslices.first["values"] : nil
+      raise_unexpected_payload response unless values.is_a?(Hash)
+      values
+    end
+
+    def parse_body(response)
+      body = JSON.parse response.body
+      raise_unexpected_payload response unless body.is_a?(Hash)
+      body
+    rescue JSON::ParserError
+      raise Scaltainer::NetworkError.new \
+        "New Relic API returned a non json response: #{response.body[0..128]}"
+    end
+
+    def numeric_value(values, key)
+      value = values[key]
+      unless value.is_a?(Numeric)
+        raise Scaltainer::NetworkError.new \
+          "New Relic API returned a non numeric #{key}: #{value.inspect}"
+      end
+      value
+    end
+
+    def raise_api_error(status, title)
+      message = "New Relic API error (HTTP #{status}): #{title}"
+      raise Scaltainer::ConfigurationError.new message if PERMANENT_ERROR_STATUSES.include?(status)
+      raise Scaltainer::NetworkError.new message
+    end
+
+    def raise_unexpected_payload(response)
+      raise Scaltainer::NetworkError.new \
+        "New Relic API returned an unexpected payload: #{response.body[0..128]}"
     end
   end
 end

lib/scaltainer/orchestrators/kubernetes.rb

@@ -2,6 +2,10 @@
 
 module Scaltainer
   class KubeResource < ReplicaSetBase
+    # Statuses that no later attempt can recover from with the credentials at
+    # hand, as opposed to conflicts and outages which are worth retrying.
+    PERMANENT_ERROR_STATUSES = [401, 403].freeze
+
     def initialize(name, namespace)
       @@client ||= self.class.get_client
       type = ENV['KUBERNETES_CONTROLLER_KIND'] || 'deployment'
@@ -10,6 +14,8 @@ def initialize(name, namespace)
       super(name, type, namespace)
       @resource = @@client.send("get_#{@type}", normalize_name(@name), @namespace)
       @id = @resource.metadata.uid
+    rescue Kubeclient::HttpError => e
+      raise resource_error(e, 'find')
     end
 
     def get_replicas
@@ -18,10 +24,25 @@ def get_replicas
 
     def set_replicas(replicas)
       @@client.send("patch_#{@type}", normalize_name(@name), {spec: {replicas: replicas}}, @namespace)
+    rescue Kubeclient::HttpError => e
+      raise resource_error(e, 'scale')
     end
 
     private
 
+    def resource_error(error, action)
+      message = "Could not #{action} #{@type} #{@name} in namespace #{@namespace}: #{error.message}"
+      case error.error_code
+      when *PERMANENT_ERROR_STATUSES
+        ConfigurationError.new message
+      when 404, 409
+        # the resource is absent or was changed since it was read, pick it up next tick
+        Scaltainer::Warning.new message
+      else
+        NetworkError.new message
+      end
+    end
+
     def self.get_client
       if ENV['KUBECONFIG']
         get_client_from_kubeconfig ENV['KUBECONFIG']

lib/scaltainer/runner.rb

@@ -63,12 +63,12 @@ def iterate_services(services, namespace, type, state)
             service_config = @default_service_config.merge service_config
             @logger.debug "Resource #{service_name} in namespace #{namespace} configuration: #{service_config}"
             process_service service_name, service_config, service_state, namespace, type, metrics
-          rescue RuntimeError => e
+          rescue ApplicationError => e
             # skipping service
             log_exception e
           end
         end
-      rescue RuntimeError => e
+      rescue ApplicationError => e
         # skipping service type
         log_exception e
       end
@@ -101,14 +101,10 @@ def process_service(service_name, config, state, namespace, type, metrics)
     end
 
     def get_service(service_name, namespace)
-      begin
-        service = if @orchestrator == :swarm
-          DockerService.new service_name, namespace
-        elsif @orchestrator == :kubernetes
-          KubeResource.new service_name, namespace
-        end
-      rescue => e
-        raise NetworkError.new "Could not find resource with name #{service_name} in namespace #{namespace}: #{e.message}"
+      service = if @orchestrator == :swarm
+        DockerService.new service_name, namespace
+      elsif @orchestrator == :kubernetes
+        KubeResource.new service_name, namespace
       end
       raise ConfigurationError.new "Unknown resource: #{service_name} in namespace #{namespace}" unless service
       service
@@ -118,11 +114,7 @@ def scale_out(service, current_replicas, desired_replicas)
       return if current_replicas == desired_replicas
       # send scale command to orchestrator
       @logger.info "Scaling #{service.type} #{service.name} from #{current_replicas} to #{desired_replicas}"
-      begin
-        service.set_replicas desired_replicas
-      rescue => e
-        raise NetworkError.new "Could not scale #{service.type} #{service.name} due to error: #{e.message}"
-      end
+      service.set_replicas desired_replicas
     end
 
     def register_pushgateway(pushgateway)

lib/scaltainer/service_types/web.rb

@@ -16,11 +16,7 @@ def get_metrics(services)
         app_id = service_config["newrelic_app_id"]
         raise ConfigurationError.new "Resource #{service_name} does not have a corresponding newrelic_app_id" unless app_id
 
-        begin
-          metric = nr.get_avg_response_time app_id, from, to
-        rescue => e
-          raise NetworkError.new "Could not retrieve metrics from New Relic API for #{service_name}: #{e.message}"
-        end
+        metric = nr.get_avg_response_time app_id, from, to
 
         hash.merge!(service_name => metric)
       end

lib/scaltainer/service_types/worker.rb

@@ -11,19 +11,19 @@ def get_metrics(services)
         m = JSON.parse(response.body)
         m.reduce({}){|hash, item| hash.merge!({item["name"] => (item["quantity"] || item["value"])})}
       rescue JSON::ParserError => e
-        raise ConfigurationError.new "app_endpoint returned non json response: #{response.body[0..128]}"
+        # a non json body is typically a transient upstream error page, retry next tick
+        raise NetworkError.new "app_endpoint returned non json response: #{response.body[0..128]}"
       rescue TypeError => e
         raise ConfigurationError.new "app_endpoint returned unexpected json response: #{response.body[0..128]}"
-      rescue => e
-        raise NetworkError.new "Could not retrieve metrics from application endpoint: #{@app_endpoint}.\n#{e.message}"
       end
     end
 
     def determine_desired_replicas(metric, service_config, current_replicas)
       super
       raise ConfigurationError.new "Missing ratio in worker resource configuration" unless service_config["ratio"]
       if !metric.is_a?(Integer) || metric < 0
-        raise ConfigurationError.new "#{metric} is an invalid metric value, must be a non-negative number" 
+        # the metric comes from the endpoint at runtime, not from configuration
+        raise Scaltainer::Warning.new "#{metric} is an invalid metric value, must be a non-negative number"
       end
       desired_replicas = (metric * 1.0 / service_config["ratio"]).ceil
     end

lib/scaltainer/version.rb

@@ -1,3 +1,3 @@
 module Scaltainer
-  VERSION = "0.7.0"
+  VERSION = "0.8.0"
 end

spec/scaltainer/exceptions_spec.rb

@@ -0,0 +1,29 @@
+require 'spec_helper'
+
+include Scaltainer
+
+describe 'exceptions' do
+  # The runner rescues ApplicationError to skip a resource (or resource type)
+  # and carry on to the next tick. What is and is not an ApplicationError is
+  # therefore the difference between being retried forever and being fatal.
+
+  describe 'recoverable errors' do
+    it 'includes NetworkError' do
+      expect(NetworkError.new).to be_a ApplicationError
+    end
+
+    it 'includes Warning' do
+      expect(Scaltainer::Warning.new).to be_a ApplicationError
+    end
+  end
+
+  describe 'permanent errors' do
+    it 'does not include ConfigurationError' do
+      expect(ConfigurationError.new).to_not be_a ApplicationError
+    end
+
+    it 'still makes ConfigurationError rescuable as a StandardError' do
+      expect(ConfigurationError.new).to be_a StandardError
+    end
+  end
+end