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

Subclass Kubevirt InfraManager to separate Openshift Virtualization provider #264

Merged
merged 1 commit into from
Sep 10, 2024
Merged
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
15 changes: 15 additions & 0 deletions app/assets/images/svg/vendor-openshift_infra.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
21 changes: 21 additions & 0 deletions app/models/manageiq/providers/openshift/container_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@
class ManageIQ::Providers::Openshift::ContainerManager < ManageIQ::Providers::Kubernetes::ContainerManager
DEFAULT_EXTERNAL_LOGGING_ROUTE_NAME = "logging-kibana-ops".freeze

has_one :infra_manager,
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 makes sense for this to just live in the base class, or is this intentionally overriding the class name?

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 here because we have to override the class_name to be Openshift::InfraManager so the correct class gets created

:foreign_key => :parent_ems_id,
:class_name => "ManageIQ::Providers::Openshift::InfraManager",
:autosave => true,
:inverse_of => :parent_manager,
:dependent => :destroy

include ManageIQ::Providers::Openshift::ContainerManager::Options

supports :catalog
Expand All @@ -21,6 +28,20 @@ def self.event_monitor_class
ManageIQ::Providers::Openshift::ContainerManager::EventCatcher
end

def self.virtualization_options
[
{
:label => _('Disabled'),
:value => 'none',
},
{
:label => _('OpenShift Virtualization'),
:value => 'kubevirt',
:pivot => 'endpoints.openshift.hostname',
},
]
end

def self.raw_connect(hostname, port, options)
options[:service] ||= "openshift"
send("#{options[:service]}_connect", hostname, port, options)
Expand Down
46 changes: 46 additions & 0 deletions app/models/manageiq/providers/openshift/infra_manager.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
ManageIQ::Providers::Kubevirt::InfraManager.include(ActsAsStiLeafClass)

class ManageIQ::Providers::Openshift::InfraManager < ManageIQ::Providers::Kubevirt::InfraManager
belongs_to :parent_manager,
:foreign_key => :parent_ems_id,
:inverse_of => :infra_manager,
:class_name => "ManageIQ::Providers::Openshift::ContainerManager"

#
# This is the list of features that this provider supports:
#
supports :catalog
supports :provisioning

def self.ems_type
@ems_type ||= "openshift_infra".freeze
end

def self.description
@description ||= "OpenShift Virtualization".freeze
end

def self.catalog_types
{"openshift" => N_("OpenShift Virtualization")}
end

def self.vendor
"openshift_infra".freeze
end

def self.product_name
"OpenShift Virtualization".freeze
end

def self.version
"0.1.0".freeze
end

def default_authentication_type
:openshift
end

def self.display_name(number = 1)
n_('Infrastructure Provider (OpenShift Virtualization)', 'Infrastructure Providers (OpenShift Virtualization)', number)
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
ManageIQ::Providers::Kubevirt::InfraManager::Cluster.include(ActsAsStiLeafClass)

class ManageIQ::Providers::Openshift::InfraManager::Cluster < ManageIQ::Providers::Kubevirt::InfraManager::Cluster
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
ManageIQ::Providers::Kubevirt::InfraManager::Connection.include(ActsAsStiLeafClass)

class ManageIQ::Providers::Openshift::InfraManager::Connection < ManageIQ::Providers::Kubevirt::InfraManager::Connection
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
ManageIQ::Providers::Kubevirt::InfraManager::Host.include(ActsAsStiLeafClass)

class ManageIQ::Providers::Openshift::InfraManager::Host < ManageIQ::Providers::Kubevirt::InfraManager::Host
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
ManageIQ::Providers::Kubevirt::InfraManager::Provision.include(ActsAsStiLeafClass)

class ManageIQ::Providers::Openshift::InfraManager::Provision < ManageIQ::Providers::Kubevirt::InfraManager::Provision
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
class ManageIQ::Providers::Openshift::InfraManager::ProvisionWorkflow < ManageIQ::Providers::Kubevirt::InfraManager::ProvisionWorkflow
def self.provider_model
ManageIQ::Providers::Openshift::InfraManager
end

def dialog_name_from_automate(message = 'get_dialog_name')
super(message, {'platform' => 'openshift'})
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class ManageIQ::Providers::Openshift::InfraManager::RefreshWorker < ManageIQ::Providers::BaseManager::RefreshWorker
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
require 'concurrent/atomic/atomic_boolean'

class ManageIQ::Providers::Openshift::InfraManager::RefreshWorker::Runner < ManageIQ::Providers::Kubevirt::InfraManager::RefreshWorker::Runner
private

def provider_class
ManageIQ::Providers::Openshift
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class ManageIQ::Providers::Openshift::InfraManager::Refresher < ManageIQ::Providers::Kubevirt::InfraManager::Refresher
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
ManageIQ::Providers::Kubevirt::InfraManager::Storage.include(ActsAsStiLeafClass)

class ManageIQ::Providers::Openshift::InfraManager::Storage < ManageIQ::Providers::Kubevirt::InfraManager::Storage
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
ManageIQ::Providers::Kubevirt::InfraManager::Template.include(ActsAsStiLeafClass)

class ManageIQ::Providers::Openshift::InfraManager::Template < ManageIQ::Providers::Kubevirt::InfraManager::Template
def self.display_name(number = 1)
n_('Template (OpenShift Virtualization)', 'Templates (OpenShift Virtualization)', number)
end
end
7 changes: 7 additions & 0 deletions app/models/manageiq/providers/openshift/infra_manager/vm.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
ManageIQ::Providers::Kubevirt::InfraManager::Vm.include(ActsAsStiLeafClass)

class ManageIQ::Providers::Openshift::InfraManager::Vm < ManageIQ::Providers::Kubevirt::InfraManager::Vm
def self.display_name(number = 1)
n_('Virtual Machine (OpenShift Virtualization)', 'Virtual Machines (OpenShift Virtualization)', number)
end
end
37 changes: 37 additions & 0 deletions app/models/manageiq/providers/openshift/inventory.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,39 @@
class ManageIQ::Providers::Openshift::Inventory < ManageIQ::Providers::Kubernetes::Inventory
def self.parser_class_for(ems, target)
if !ems.kind_of?(EmsInfra)
super
else
Comment on lines +3 to +5
Copy link
Member

Choose a reason for hiding this comment

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

We could also return super unless ems.kind_of?(EmsInfra) so the entire method isn't just one if/else block.

parser_type = if target_is_vm?(target)
"PartialTargetRefresh"
else
"FullRefresh"
end
Comment on lines +6 to +10
Copy link
Member

Choose a reason for hiding this comment

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

Really minor for a follow-up but something this simple taking up 5 lines could be a ternary operator instead on one line

Suggested change
parser_type = if target_is_vm?(target)
"PartialTargetRefresh"
else
"FullRefresh"
end
parser_type = target_is_vm?(target) ? "PartialTargetRefresh" : "FullRefresh"

I'm not usually a "hey you can turn this big multi-step thing into one looong line" guy but this is pretty straightforward

"ManageIQ::Providers::Openshift::Inventory::Parser::InfraManager::#{parser_type}".safe_constantize
end
end

def self.build(ems, target)
if !ems.kind_of?(EmsInfra)
super
else
Comment on lines +16 to +18
Copy link
Member

Choose a reason for hiding this comment

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

Same here,

Suggested change
if !ems.kind_of?(EmsInfra)
super
else
return super unless ems.kind_of?(EmsInfra)

collector_class = collector_class_for(ems, target)

collector = if target_is_vm?(target)
collector_class.new(ems, target)
else
collector_class.new(ems, ems)
end
Comment on lines +21 to +25
Copy link
Member

Choose a reason for hiding this comment

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

If this is the same collector class I wonder if we could handle this in the initialize there, e.g. target = ems unless target.kind_of?(Vm) or something like that


persister = persister_class_for(ems, target).new(ems, target)
new(
persister,
collector,
parser_classes_for(ems, target).map(&:new)
)
end
end

def self.target_is_vm?(target)
target.kind_of?(ManageIQ::Providers::Openshift::InfraManager::Vm)
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class ManageIQ::Providers::Openshift::Inventory::Collector::InfraManager < ManageIQ::Providers::Kubevirt::Inventory::Collector
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class ManageIQ::Providers::Openshift::Inventory::Parser::InfraManager < ManageIQ::Providers::Kubevirt::Inventory::Parser
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class ManageIQ::Providers::Openshift::Inventory::Parser::InfraManager::FullRefresh < ManageIQ::Providers::Kubevirt::Inventory::Parser::FullRefresh
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class ManageIQ::Providers::Openshift::Inventory::Parser::InfraManager::PartialRefresh < ManageIQ::Providers::Kubevirt::Inventory::Parser::PartialRefresh
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class ManageIQ::Providers::Openshift::Inventory::Parser::InfraManager::PartialTargetRefresh < ManageIQ::Providers::Kubevirt::Inventory::Parser::PartialTargetRefresh
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class ManageIQ::Providers::Openshift::Inventory::Persister::InfraManager < ManageIQ::Providers::Kubevirt::Inventory::Persister
end
1 change: 1 addition & 0 deletions config/settings.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,4 @@
:ems_metrics_collector_worker_openshift: {}
:ems_refresh_worker:
:ems_refresh_worker_openshift: {}
:ems_refresh_worker_openshift_infra: {}
7 changes: 7 additions & 0 deletions spec/factories/ext_management_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,11 @@
zone
end
end

factory :ems_openshift_infra,
:aliases => ["manageiq/providers/openshift/infra_manager"],
:class => "ManageIQ::Providers::Openshift::InfraManager",
:parent => :ems_kubevirt do
parent_manager { FactoryBot.create(:ems_openshift) }
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
describe ManageIQ::Providers::Openshift::InfraManager::Refresher do
context '#refresh' do
let(:ems) do
host = Rails.application.secrets.openshift[:hostname]
token = Rails.application.secrets.openshift[:token]
port = Rails.application.secrets.openshift[:port]
zone = EvmSpecHelper.local_miq_server.zone

FactoryBot.create(:ems_openshift_infra,
:name => "openshift Virtualization Manager",
:zone => zone).tap do |ems|
ems.parent_manager.authentications << FactoryBot.create(:authentication, {:authtype => :bearer,
:type => "AuthToken",
:auth_key => token,
:userid => "_"})
ems.parent_manager.default_endpoint.update!(:role => :default,
:hostname => host,
:port => port,
:security_protocol => "ssl-without-validation")
end
end

it 'works correctly with one node' do
Copy link
Member

Choose a reason for hiding this comment

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

This looks a little out of place, something like "Performs a full refresh" would probably be better.

Copy link
Member

Choose a reason for hiding this comment

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

On that note, a targeted refresh spec would be a good addition in the future

2.times do
VCR.use_cassette(described_class.name.underscore) do
EmsRefresh.refresh(ems)
end

assert_counts
assert_specific_vm
assert_specific_host
assert_specific_cluster
assert_specific_storage
end
end

def assert_counts
expect(ems.vms.count).to eq(1)
expect(ems.hosts.count).to eq(6)
expect(ems.clusters.count).to eq(1)
expect(ems.storages.count).to eq(1)
end

def assert_specific_vm
vm = ems.vms.find_by(:name => "fedora-gold-porcupine-50")
expect(vm).to have_attributes(
:ems_ref => "50c54ad2-c2a6-44ae-89f5-14d2f313882c",
:name => "fedora-gold-porcupine-50",
:type => "ManageIQ::Providers::Openshift::InfraManager::Vm",
:uid_ems => "50c54ad2-c2a6-44ae-89f5-14d2f313882c",
:vendor => "openshift_infra",
:power_state => "on",
:connection_state => "connected"
)
end

def assert_specific_host
host = ems.hosts.find_by(:ems_ref => "248af02e-7da9-49a4-b026-1dd1a341b0de")
expect(host).to have_attributes(
:connection_state => "connected",
:ems_ref => "248af02e-7da9-49a4-b026-1dd1a341b0de",
:type => "ManageIQ::Providers::Openshift::InfraManager::Host",
:uid_ems => "248af02e-7da9-49a4-b026-1dd1a341b0de",
:vmm_product => "OpenShift Virtualization",
:vmm_vendor => "openshift_infra",
:vmm_version => "0.1.0",
:ems_cluster => ems.ems_clusters.find_by(:ems_ref => "0")
)
end

def assert_specific_cluster
cluster = ems.ems_clusters.find_by(:ems_ref => "0")
expect(cluster).to have_attributes(
:ems_ref => "0",
:name => "openshift Virtualization Manager",
:uid_ems => "0",
:type => "ManageIQ::Providers::Openshift::InfraManager::Cluster"
)
end

def assert_specific_storage
storage = ems.storages.find_by(:ems_ref => "0")
expect(storage).to have_attributes(
:name => "openshift Virtualization Manager",
:total_space => 0,
:free_space => 0,
:ems_ref => "0",
:type => "ManageIQ::Providers::Openshift::InfraManager::Storage"
)
end
end
end
Loading
Loading