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.

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

[WIP] Add cache columns to operating_systems table #322

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
159 changes: 159 additions & 0 deletions db/migrate/20190108031411_add_image_name_to_operating_systems.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
class AddImageNameToOperatingSystems < ActiveRecord::Migration[5.0]
include MigrationHelper

StubOperatingSystemObj = Struct.new(:operating_system) do
attr_reader :hardware # always nil
def name
""
end
end

class OperatingSystem < ActiveRecord::Base
belongs_to :host
belongs_to :vm_or_template
belongs_to :computer_system

# Using a `before_save` here since this is the mechanism that will be used
# in the app. Causes a bit of issues in the specs, but proves that this
# would work moving forward.
before_save :update_platform_and_image_name

def update_platform_and_image_name
obj = case # rubocop:disable Style/EmptyCaseCondition
when host_id then host
when vm_or_template_id then vm_or_template
when computer_system_id then computer_system
else
StubOperatingSystemObj.new(self)
end

if obj
self.image_name = self.class.image_name(obj)
self.platform = self.image_name.split("_").first # rubocop:disable Style/RedundantSelf
end
end

# rubocop:disable Style/PercentLiteralDelimiters
@@os_map = [ # rubocop:disable Style/ClassVars
["windows_generic", %w(winnetenterprise w2k3 win2k3 server2003 winnetstandard servernt)],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is winnetenterprise right? I'm guessing it's winntenterprise, but thought I'd double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied-pased this from the class I think, but let me double check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

["windows_generic", %w(winxppro winxp)],
["windows_generic", %w(vista longhorn)],
["windows_generic", %w(win2k win2000)],
["windows_generic", %w(microsoft windows winnt)],
["linux_ubuntu", %w(ubuntu)],
["linux_chrome", %w(chromeos)],
["linux_chromium", %w(chromiumos)],
["linux_suse", %w(suse sles)],
["linux_redhat", %w(redhat rhel)],
["linux_fedora", %w(fedora)],
["linux_gentoo", %w(gentoo)],
["linux_centos", %w(centos)],
["linux_debian", %w(debian)],
["linux_coreos", %w(coreos)],
["linux_esx", %w(vmnixx86 vmwareesxserver esxserver vmwareesxi)],
["linux_solaris", %w(solaris)],
["linux_generic", %w(linux)]
]
# rubocop:enable Style/PercentLiteralDelimiters

# rubocop:disable Naming/VariableName
def self.normalize_os_name(osName) # rubocop:disable Naming/UncommunicativeMethodParamName
findStr = osName.downcase.gsub(/[^a-z0-9]/, "")
@@os_map.each do |a|
a[1].each do |n|
return a[0] unless findStr.index(n).nil?
end
end
"unknown"
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to not return "unknown"?

If you want unknown, you could use a || "unknown" (or pass in a boolean for default_value or allow_unknown)

Copy link
Member Author

Choose a reason for hiding this comment

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

end
# rubocop:enable Naming/VariableName

# rubocop:disable Naming/VariableName
def self.image_name(obj)
osName = nil

# Select most accurate name field
os = obj.operating_system
if os
# check the given field names for possible matching value
osName = [:distribution, :product_type, :product_name].each do |field|
os_field = os.send(field)
break(os_field) if os_field && OperatingSystem.normalize_os_name(os_field) != "unknown"
end

# If the normalized name comes back as unknown, nil out the value so we can get it from another field
if osName.kind_of?(String)
Copy link
Member

Choose a reason for hiding this comment

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

(still selling my idea) I don't think this is necessary if "unknown" wouldn't come back :)

Copy link
Member Author

@NickLaMuro NickLaMuro May 29, 2019

Choose a reason for hiding this comment

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

Still "selling my idea" of "I copy pasted of what is in the existing OperatingSystem model" and not trying to refactor that in this PR. Just trying to replicate what would be done when saving normally and trying to calculate the value as it would be generated today.

I think the bigger question to ask is not "can this code be done better", but does the idea of "caching this value" make sense. Hence the [WIP] and [POC] labels, since I really want a this to be agreed on before moving forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, see "Considerations" section in the PR description... which I even forgot about...

osName = nil if OperatingSystem.normalize_os_name(osName) == "unknown"
else
osName = nil
end
end

# If the OS Name is still blank check the 'user_assigned_os'
if osName.nil? && obj.respond_to?(:user_assigned_os) && obj.user_assigned_os
Copy link
Member

Choose a reason for hiding this comment

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

This is probably a naive suggestion but...

Suggested change
if osName.nil? && obj.respond_to?(:user_assigned_os) && obj.user_assigned_os
osName ||= obj.try(:user_assigned_os)

Copy link
Member Author

Choose a reason for hiding this comment

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

osName = obj.user_assigned_os
end

# If the OS Name is still blank check the hardware table
if osName.nil? && obj.hardware && !obj.hardware.guest_os.nil?
osName = obj.hardware.guest_os
# if we get generic linux or unknown back see if the vm name is better
norm_os = OperatingSystem.normalize_os_name(osName)
if norm_os == "linux_generic" || norm_os == "unknown" # rubocop:disable Style/MultipleComparison
vm_name = OperatingSystem.normalize_os_name(obj.name)
return vm_name unless vm_name == "unknown"
end
end

# If the OS Name is still blank use the name field from the object given
osName = obj.name if osName.nil? && obj.respond_to?(:name)

# Normalize name to match existing icons
OperatingSystem.normalize_os_name(osName || "")
end
# rubocop:enable Naming/VariableName
end

class VmOrTemplate < ActiveRecord::Base
self.inheritance_column = :_type_disabled
self.table_name = 'vms'

has_one :operating_system
has_one :hardware
end

class Host < ActiveRecord::Base
self.inheritance_column = :_type_disabled

has_one :operating_system
has_one :hardware
end

class ComputerSystem < ActiveRecord::Base
has_one :operating_system
has_one :hardware
end

class Hardware < ActiveRecord::Base
end

def up
add_column :operating_systems, :platform, :string
add_column :operating_systems, :image_name, :string

say_with_time("Updating platform and image_name in OperatingSystems") do
base_relation = OperatingSystem.all
say_batch_started(base_relation.size)

base_relation.find_in_batches do |group|
group.each(&:save)
Copy link
Member

Choose a reason for hiding this comment

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

love the simplicity here

say_batch_processed(group.count)
end
end
end

def down
remove_column :operating_systems, :platform, :string
remove_column :operating_systems, :image_name, :string
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
require_migration

describe AddImageNameToOperatingSystems do
let(:host_stub) { migration_stub(:Host) }
let(:hardware_stub) { migration_stub(:Hardware) }
let(:vm_or_template_stub) { migration_stub(:VmOrTemplate) }
let(:computer_system_stub) { migration_stub(:ComputerSystem) }
let(:operating_system_stub) { migration_stub(:OperatingSystem) }

# rubocop:disable Layout/SpaceInsideArrayPercentLiteral
let(:test_os_values) do
[
%w[an_amazing_undiscovered_os unknown],
%w[centos-7 linux_centos],
%w[debian-8 linux_debian],
%w[opensuse-13 linux_suse],
%w[sles-12 linux_suse],
%w[rhel-7 linux_redhat],
%w[ubuntu-15-10 linux_ubuntu],
%w[windows-2012-r2 windows_generic],
%w[vmnix-x86 linux_esx],
%w[vista windows_generic],
%w[coreos-cloud linux_coreos]
]
end
# rubocop:enable Layout/SpaceInsideArrayPercentLiteral

def record_with_os(klass, os_attributes = nil, record_attributes = {:name => ""}, hardware_attributes = nil)
os_record = operating_system_stub.new(os_attributes) if os_attributes

if klass == operating_system_stub
os_record.save!
os_record
else
record = klass.new
record_attributes.each do |attr, val|
record.send("#{attr}=", val) if record.respond_to?(attr)
end

record.operating_system = os_record
record.hardware = hardware_stub.new(hardware_attributes) if hardware_attributes
record.save!
record
end
end

# Runs tests for class type to confirm they
def test_for_klass(klass)
begin
# This callback is necessary after the migration, but fails when the
# column doesn't eixst (prior to the migration). Removing it and
# re-enabling it after the migration.
operating_system_stub.skip_callback(:save, :before, :update_platform_and_image_name)

distribution_based = []
product_type_based = []
product_name_based = []
fallback_records = []

test_os_values.each do |(value, _)|
distribution_based << record_with_os(klass, :distribution => value)
product_type_based << record_with_os(klass, :product_type => value)
product_name_based << record_with_os(klass, :product_name => value)
end

# favor distribution over product_type
fallback_records << record_with_os(klass, :distribution => "rhel-7", :product_type => "centos-7")
# falls back to os.product_type if invalid os.distribution
fallback_records << record_with_os(klass, :distribution => "undiscovered-7", :product_type => "rhel-7")
# falls back to os.product_name
fallback_records << record_with_os(klass, :distribution => "undiscovered-7", :product_name => "rhel-7")
# falls back to hardware.guest_os
fallback_records << record_with_os(klass, {:distribution => "undiscovered-7"}, {}, {:guest_os => "rhel-7"})
# falls back to Host#user_assigned_os
fallback_records << record_with_os(klass, {:distribution => "undiscovered-7"}, {:user_assigned_os => "rhel-7"})
ensure
# If the any of the above fails, make sure we re-enable callbacks so
# subsequent specs don't fail trying to skip this callback when it
# doesn't exist.
operating_system_stub.set_callback(:save, :before, :update_platform_and_image_name)
end

migrate

test_os_values.each.with_index do |(_, image_name), index|
[distribution_based, product_type_based, product_name_based].each do |record_list|
os_record = record_list[index]
os_record.reload
os_record = os_record.operating_system if os_record.respond_to?(:operating_system)

expect(os_record.image_name).to eq(image_name)
expect(os_record.platform).to eq(image_name.split("_").first)
end
end

fallback_records.each(&:reload)

platform, image_name = %w[linux linux_redhat]
fallback_records.each.with_index do |record, index|
os_record = record
os_record = os_record.operating_system if os_record.respond_to?(:operating_system)

# OperatingSystem records don't have a hardware relation, so this will be
# a "unknown" OS
platform, image_name = %w[unknown unknown] if index == 3 && klass == operating_system_stub

# Both ComputerSystem and VmOrTemplate don't have :user_assigned_os, so
# these will return "unknown" instead of what we (tried to) set.
platform, image_name = %w[unknown unknown] if index == 4 && klass != host_stub

expect(os_record.image_name).to eq(image_name)
expect(os_record.platform).to eq(platform)
end
end

migration_context :up do
it "adds the columns" do
before_columns = operating_system_stub.columns.map(&:name)
expect(before_columns).to_not include("platform")
expect(before_columns).to_not include("image_name")

migrate

after_columns = operating_system_stub.columns.map(&:name)
expect(after_columns).to include("platform")
expect(after_columns).to include("image_name")
end

it "updates OperatingSystem for Host records" do
test_for_klass host_stub
end

it "updates OperatingSystem for VmOrTemplate records" do
test_for_klass vm_or_template_stub
end

it "updates OperatingSystem for ComputerSystem records" do
test_for_klass computer_system_stub
end

it "updates orphaned OperatingSystem records" do
test_for_klass operating_system_stub
end
end

migration_context :down do
it "adds the columns" do
before_columns = operating_system_stub.columns.map(&:name)
expect(before_columns).to include("platform")
expect(before_columns).to include("image_name")

migrate

after_columns = operating_system_stub.columns.map(&:name)
expect(after_columns).to_not include("platform")
expect(after_columns).to_not include("image_name")
end
end
end