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

Rip out activesupport #2

Closed
wants to merge 1 commit into from
Closed

Rip out activesupport #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 59 additions & 33 deletions exe/kubernetes-deploy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
# Optionally, the following variables can be used to override script defaults:
# - ENV['K8S_TEMPLATE_FOLDER']: Location of Kubernetes files to deploy. Default is config/deploy/#{environment}.

require 'bundler/setup'
require 'open3'
require 'securerandom'
require 'erb'
Expand All @@ -19,10 +18,6 @@ require 'yaml'
require 'shellwords'
require 'tempfile'
require 'logger'
require 'active_support/core_ext/object/blank'
require 'active_support/descendants_tracker'
require 'active_support/core_ext/hash/slice'
require 'active_support/core_ext/numeric/time'

class KubernetesDeploy
class FatalDeploymentError < StandardError; end
Expand Down Expand Up @@ -159,7 +154,7 @@ class KubernetesDeploy

def wait_for_completion(watched_resources)
delay_sync_until = Time.now.utc
while watched_resources.present?
while !watched_resources.empty?
if Time.now.utc < delay_sync_until
sleep (delay_sync_until - Time.now.utc)
end
Expand Down Expand Up @@ -188,11 +183,11 @@ class KubernetesDeploy

def validate_configuration
errors = []
if ENV["KUBECONFIG"].blank? || !File.file?(ENV["KUBECONFIG"])
if ENV["KUBECONFIG"].nil? || !File.file?(ENV["KUBECONFIG"])
errors << "Kube config not found at #{ENV["KUBECONFIG"]}"
end

if @current_sha.blank?
if @current_sha.nil? || @current_sha.empty?
errors << "Current SHA must be specified"
end

Expand All @@ -202,11 +197,11 @@ class KubernetesDeploy
errors << "#{@template_path} doesn't contain valid templates (postfix .yml or .yml.erb)"
end

if @namespace.blank?
if @namespace.nil? || @namespace.empty?
errors << "Namespace must be specified"
end

if @context.blank?
if @context.nil? || @context.empty?
errors << "Context must be specified"
end

Expand Down Expand Up @@ -293,23 +288,48 @@ class KubernetesDeploy
end
end

module DescendantsTracker
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 believe these are the only pieces of https://github.com/rails/rails/blob/master/activesupport/lib/active_support/descendants_tracker.rb we actually need.

@@direct_descendants = {}

class << self
def direct_descendants(klass)
@@direct_descendants[klass] || []
end

def store_inherited(klass, descendant)
(@@direct_descendants[klass] ||= []) << descendant
end
end
end

class KubernetesResource
extend ActiveSupport::DescendantsTracker
extend DescendantsTracker

attr_reader :name, :namespace, :file
attr_writer :type, :deploy_started

TIMEOUT = 5.minutes
TIMEOUT = 5 * 60

def self.handled_type
name.split('::').last
end
class << self
def direct_descendants
DescendantsTracker.direct_descendants(self)
end

def self.for_type(type, name, namespace, file)
if subclass = descendants.find { |subclass| subclass.handled_type.downcase == type }
subclass.new(name, namespace, file)
else
self.new(name, namespace, file).tap { |r| r.type = type }
def inherited(base)
DescendantsTracker.store_inherited(self, base)
super
end

def handled_type
name.split('::').last
end

def for_type(type, name, namespace, file)
if subclass = direct_descendants.find { |subclass| subclass.handled_type.downcase == type }
Copy link
Contributor

Choose a reason for hiding this comment

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

If what you need is getting Service.new from "service" as a string, you can do the following:

type = "service"
Object.const_get(type.capitalize)

This way you won't need DescendantsTracker at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True (even KubernetesResource.const_get(type.capitalize), I believe?), but is that really better? It feels like a bit of a cheat (i.e. reliable because by convention constants capitalized "Randomtype" will be classes), and it would prevent us from camelizing the class names (e.g. PersistentVolumeClaim -> Persistentvolumeclaim).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is how Ruby developers usually do (iterating over DescendantsTracker sounds like an antipattern in this case).

For PVL, the right thing would be:

irb(main):025:0> "persistent_volume_claim".classify
=> "PersistentVolumeClaim"

(classify comes from AS)

Copy link
Contributor

Choose a reason for hiding this comment

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

(assuming persistent_volume_claim comes from the type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(assuming persistent_volume_claim comes from the type)

Sadly no. This is coming from kubectl as persistentvolumeclaim.

This is how Ruby developers usually do (iterating over DescendantsTracker sounds like an antipattern in this case). For PVL, the right thing would be: ... (classify comes from AS)

Calling it an antipattern doesn't really help me understand why you think one option is better than the other. Both approaches are available in ActiveSupport. What I want is to find the subclass that exists to support that type, so doing exactly that doesn't sound inherently bad to me.

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 say antipattern. But it's definitely better to strive for "strategy pattern" over iterating on all candidates to find what you are looking for.

Not that it makes a huge difference performance wise (since in this kind of cases, the candidates set is generally small), but it usually makes simpler, more direct code to read.

NB: I'm just answering this specific thread cause I saw some keywords that triggered me. But I haven't kept a close watch on that PR, so feel free to disregard my opinion.

subclass.new(name, namespace, file)
else
new(name, namespace, file).tap { |r| r.type = type }
end
end
end

Expand Down Expand Up @@ -377,7 +397,7 @@ class KubernetesDeploy
end

def run_kubectl(*args)
raise FatalDeploymentError, "Namespace missing for namespaced command" if namespace.blank?
raise FatalDeploymentError, "Namespace missing for namespaced command" if namespace.nil? || namespace.empty?
args = args.unshift("kubectl").push("--namespace=#{namespace}")
KubernetesDeploy.logger.debug Shellwords.join(args)
out, err, st = Open3.capture3(*args)
Expand All @@ -389,10 +409,14 @@ class KubernetesDeploy
def log_status
STDOUT.puts "[KUBESTATUS] #{JSON.dump(status_data)}"
end

def slice_hash(original_hash, keys)
keys.each_with_object({}) { |k, new_hash| new_hash[k] = original_hash[k] if original_hash.key?(k) }
end
end

class ConfigMap < KubernetesResource
TIMEOUT = 30.seconds
TIMEOUT = 30

def initialize(name, namespace, file)
@name, @namespace, @file = name, namespace, file
Expand All @@ -419,7 +443,7 @@ class KubernetesDeploy
end

class PersistentVolumeClaim < KubernetesResource
TIMEOUT = 5.minutes
TIMEOUT = 5 * 60

def initialize(name, namespace, file)
@name, @namespace, @file = name, namespace, file
Expand All @@ -445,7 +469,7 @@ class KubernetesDeploy
end

class Ingress < KubernetesResource
TIMEOUT = 30.seconds
TIMEOUT = 30

def initialize(name, namespace, file)
@name, @namespace, @file = name, namespace, file
Expand Down Expand Up @@ -476,7 +500,7 @@ class KubernetesDeploy
end

class Service < KubernetesResource
TIMEOUT = 15.minutes
TIMEOUT = 15 * 60

def initialize(name, namespace, file)
@name, @namespace, @file = name, namespace, file
Expand Down Expand Up @@ -509,7 +533,7 @@ class KubernetesDeploy
end

class Pod < KubernetesResource
TIMEOUT = 15.minutes
TIMEOUT = 15 * 60
SUSPICIOUS_CONTAINER_STATES = %w(ImagePullBackOff RunContainerError).freeze

def initialize(name, namespace, file, parent: nil)
Expand Down Expand Up @@ -550,7 +574,7 @@ class KubernetesDeploy
@status = @phase
else
ready_condition = pod_data["status"]["conditions"].find { |condition| condition["type"] == "Ready" }
@ready = ready_condition.present? && (ready_condition["status"] == "True")
@ready = ready_condition && (ready_condition["status"] == "True")
@status = "#{@phase} (Ready: #{@ready})"
end
end
Expand Down Expand Up @@ -578,11 +602,11 @@ class KubernetesDeploy
private

def display_logs
return {} unless exists? && @containers.present? && !@already_displayed
return {} unless exists? && @containers && !@already_displayed

@containers.each do |container_name|
out, st = run_kubectl("logs", @name, "--timestamps=true", "--since-time=#{@deploy_started.to_datetime.rfc3339}")
next unless st.success? && out.present?
next unless st.success? && !out.empty?

KubernetesDeploy.logger.info "Logs from #{id} container #{container_name}:"
STDOUT.puts "#{out}"
Expand All @@ -592,7 +616,7 @@ class KubernetesDeploy
end

class Deployment < KubernetesResource
TIMEOUT = 15.minutes
TIMEOUT = 15 * 60

def initialize(name, namespace, file)
@name, @namespace, @file = name, namespace, file
Expand All @@ -606,7 +630,9 @@ class KubernetesDeploy
@pods = []

if @found
@rollout_data = JSON.parse(json_data)["status"].slice("updatedReplicas", "replicas", "availableReplicas", "unavailableReplicas")
if status_hash = JSON.parse(json_data)["status"]
@rollout_data = slice_hash(status_hash, %w(updatedReplicas replicas availableReplicas unavailableReplicas))
end
@status, _ = run_kubectl("rollout", "status", type, @name, "--watch=false") if @deploy_started

pod_list, st = run_kubectl("get", "pods", "-a", "-l", "name=#{name}", "--output=json")
Expand Down Expand Up @@ -635,12 +661,12 @@ class KubernetesDeploy

def deploy_failed?
# TODO: this should look at the current replica set's pods only or it'll never be true for rolling updates
@pods.present? && @pods.all?(&:deploy_failed?)
@pods && !@pods.empty? && @pods.all?(&:deploy_failed?)
end

def deploy_timed_out?
# TODO: this should look at the current replica set's pods only or it'll never be true for rolling updates
super || @pods.present? && @pods.all?(&:deploy_timed_out?)
super || @pods && !@pods.empty? && @pods.all?(&:deploy_timed_out?)
end

def exists?
Expand Down
1 change: 0 additions & 1 deletion kubernetes-deploy.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ Gem::Specification.new do |spec|
spec.bindir = "exe"
spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) }
spec.require_paths = ["lib"]
spec.add_dependency "activesupport", ">= 4.2"

spec.add_development_dependency "bundler", "~> 1.13"
spec.add_development_dependency "rake", "~> 10.0"
Expand Down