Subclass Kubevirt InfraManager to separate Openshift Virtualization provider #264

nasark · 2024-08-07T20:17:11Z

We can subclass Kubevirt::InfraManager to create a separate provider for Openshift Virtualization

Depends on:

Add OpenShift Virtualization vendor types for VMs and hosts manageiq#23165
Openshift Virtualization subclassing changes manageiq-providers-kubevirt#247
Separate OSV and Kubevirt virtualization options from form manageiq-providers-kubernetes#537

@miq-bot assign @agrare
@miq-bot add_label enhancement, radjabov/yes?
@miq-bot add_reviewer @agrare

nasark · 2024-08-07T20:18:18Z

@agrare WIP - need to test with an Openshift Virtualization provider and clean up specs. Feel free to review what is here though

Fryguy · 2024-08-07T21:31:56Z

app/models/manageiq/providers/openshift/container_manager.rb

@@ -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,


Does it makes sense for this to just live in the base class, or is this intentionally overriding the class name?

This is here because we have to override the class_name to be Openshift::InfraManager so the correct class gets created

Fryguy · 2024-08-07T21:32:49Z

app/models/manageiq/providers/openshift/infra_manager.rb

+ supports :provisioning
+
+ def self.ems_type
+ @ems_type ||= "openshift".freeze


I think this has to be different than the container manager, so maybe something like openshift_virt or openshift_infra?

👍 openshift_infra would be good (OpenStack undercloud/InfraManager is openstack_infra)

Fryguy · 2024-08-07T21:34:59Z

app/models/manageiq/providers/openshift/infra_manager/refresh_worker/runner.rb

+ # @return [ManageIQ::Providers::Kubevirt::InfraManager] The manager.
+ #
+ def manager
+ @manager ||= ManageIQ::Providers::Openshift::InfraManager.find(@cfg[:ems_id])


I'm surprised so much of this overriding is necessary. Makes me wonder if there's a better way, such as providing a method like manageiq_class, which returns ManageIQ::Providers::Openshift::InfraManager, and then the actual logic would be something like

@manager ||= manager_class.find(@cfg[:ems_id])

This is the simple example of course - the same comment really applies to all of the methods in this file.

Fryguy · 2024-08-07T21:36:09Z

Very cool! I like that we didn't need a new repository, and it's just included in the openshift repo.

Fryguy · 2024-08-07T21:36:59Z

One other thing I think we need is the form for adding a provider needs to change so you can only select OpenShift Virtualization under the Virtualization tab.

agrare · 2024-08-07T22:42:00Z

app/models/manageiq/providers/openshift/infra_manager.rb

+class ManageIQ::Providers::OpenShift::InfraManager < ManageIQ::Providers::Kubevirt::InfraManager
+ belongs_to :parent_manager,
+ :foreign_key => :parent_ems_id,
+ :class_name => "ManageIQ::Providers::ContainerManager"


This should be ManageIQ::Providers::Openshift::ContainerManager, then you can add an invere_of :infra_manager

agrare · 2024-08-07T22:42:14Z

app/models/manageiq/providers/openshift/infra_manager.rb

+ delegate :authentication_check,
+ :authentication_for_summary,
+ :authentication_token,
+ :authentications,
+ :endpoints,
+ :default_endpoint,
+ :zone,
+ :to => :parent_manager,
+ :allow_nil => true


I think all of these can be inherited from the base class

agrare · 2024-08-07T22:48:06Z

app/models/manageiq/providers/openshift/infra_manager.rb

+ end
+
+ def authentication_for_providers
+ authentications.where(:authtype => :openshift)


Not necessarily for this PR but if most of these overrides are for :authtype => :openshift we could use e.g. a DEFAULT_AUTH_TYPE constant and switch to that in the base Kubevirt class

e.g.

def connect(opts = {}) # Get the authentication token: token = opts[:token] || authentication_token(:kubevirt)

If we changed that to authentication_token(default_authentication_type)

agrare · 2024-08-07T22:48:33Z

app/models/manageiq/providers/openshift/infra_manager.rb

+ #
+ def connect(opts = {})
+ # Get the authentication token:
+ token = opts[:token] || authentication_token(:openshift)


This is a good example, this whole method is probably copy&paste except for this :openshift here

agrare · 2024-08-07T22:49:07Z

app/models/manageiq/providers/openshift/infra_manager.rb

+ def virtualization_endpoint
+ connection_configurations.kubevirt.try(:endpoint)
+ end


This looks identical to the base class

agrare · 2024-08-07T22:52:01Z

app/models/manageiq/providers/openshift/infra_manager/template.rb

+ supports :provisioning do
+ if ext_management_system
+ ext_management_system.unsupported_reason(:provisioning)
+ else
+ _('not connected to ems')
+ end
+ end


Is this different from the base class?

agrare · 2024-08-07T22:56:17Z

Not sure if you want to do this in another PR but we should make the "virtualization" endpoint dropdown option (https://github.com/ManageIQ/manageiq-providers-kubernetes/blob/master/app/models/manageiq/providers/kubernetes/container_manager.rb#L331-L342) in the Openshift::ContainerManager subclass only show "Openshift Virtualization"

agrare · 2024-08-08T13:49:02Z

I think we should get some VCR specs in here, kubevirt has some weird mocking that they do for their refresher specs I'm not sure I want to copy what they do but a "simple" refresher spec doing a full refresh should be sufficient.

agrare · 2024-08-15T13:31:51Z

app/models/manageiq/providers/openshift/container_manager.rb

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

+ def virtualization_options


Suggested change

def virtualization_options

def self.virtualization_options

agrare · 2024-08-27T18:09:36Z

app/models/manageiq/providers/openshift/infra_manager/refresh_worker.rb

@@ -0,0 +1,2 @@
+class ManageIQ::Providers::Openshift::InfraManager::RefreshWorker < ManageIQ::Providers::Kubevirt::InfraManager::RefreshWorker


We don't need to inherit anything from the Kubevirt RefreshWorker, and doing this nests the settings under kubevirt so we typically don't do this for simple workers (e.g. see Openshift::ContainerManager::RefreshWorker it doesn't < Kubernetes)

Suggested change

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

class ManageIQ::Providers::Openshift::InfraManager::RefreshWorker < ManageIQ::Providers::BaseManager::RefreshWorker

You also need to add a settings section for this new worker to config/settings.yml

diff --git a/config/settings.yml b/config/settings.yml index dc097791..448ed39b 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -33,3 +33,4 @@ :ems_metrics_collector_worker_openshift: {} :ems_refresh_worker: :ems_refresh_worker_openshift: {} + :ems_refresh_worker_openshift_infra: {}

Without this when running the full evmserverd it'll fail to start up:

adam@workstation:~/src/manageiq/manageiq$ ruby lib/workers/bin/evm_server.rb ** ManageIQ master, codename: Spassky /home/grare/adam/src/manageiq/manageiq/app/models/miq_worker.rb:224:in `block in fetch_worker_settings_from_options_hash': Missing config section ems_refresh_worker_openshift_infra (RuntimeError)

NOTE we'll also need to add .service and .target files for the new openshift-infra-refresh worker in the systemd/ directory here (specs will catch this but just want to head it off now)

agrare · 2024-08-27T18:12:33Z

When running the full worker I see the following error:

[----] I, [2024-08-27T14:07:56.049629#148737:8890]  INFO -- evm: MIQ(ManageIQ::Providers::Openshift::ContainerManager::EventCatcher::Runner#start_event_monitor) EMS [api.crc.testing] as [] Starting Event Monitor Thread
[----] I, [2024-08-27T14:07:56.050809#148737:8890]  INFO -- evm: MIQ(ManageIQ::Providers::Openshift::ContainerManager::EventCatcher::Runner#start_event_monitor) EMS [api.crc.testing] as [] Started Event Monitor Thread
[----] E, [2024-08-27T14:07:56.641440#148735:8890] ERROR -- evm: MIQ(ManageIQ::Providers::Openshift::ContainerManager::RefreshWorker::Runner#refresh_block) EMS [CRC], id: [6] Refresh failed: undefined method `vms' for #<ManageIQ::Providers::Openshift::Inventory::Collector::ContainerManager
[----] E, [2024-08-27T14:07:56.642961#148735:8890] ERROR -- evm: /home/grare/adam/src/manageiq/manageiq-providers-kubevirt/app/models/manageiq/providers/kubevirt/inventory/parser/full_refresh.rb:8:in `parse'
/home/grare/adam/src/manageiq/manageiq/app/models/manageiq/providers/inventory.rb:38:in `block in parse'
/home/grare/adam/src/manageiq/manageiq/app/models/manageiq/providers/inventory.rb:35:in `each'
/home/grare/adam/src/manageiq/manageiq/app/models/manageiq/providers/inventory.rb:35:in `parse'

When using >> EmsRefresh.refresh(ManageIQ::Providers::Openshift::InfraManager.first) I see the same ManageIQ::Providers::Kubevirt::InfraManager::Host is not a subclass of ManageIQ::Providers::Openshift::InfraManager::Host (ManageIQ::Providers::BaseManager::Refresher::PartialRefreshError) that you do.

agrare · 2024-08-29T18:18:21Z

app/models/manageiq/providers/openshift/container_manager.rb

+ :value => 'kubevirt',
+ :pivot => 'endpoints.kubevirt.hostname',


If our default_authentication_type is :openshift does this have to be endpoints.openshift.hostname ?

agrare · 2024-08-29T18:29:59Z

spec/vcr_cassettes/full_refresh.yml

Usually we like to namespace these under the provider name e.g. https://github.com/ManageIQ/manageiq-providers-openshift/tree/master/spec/vcr_cassettes/manageiq/providers/openshift/container_manager/refresher.yml that was it is obvious what class this is for. You can use described_class.name.underscore in the VCR.use_cassette call

agrare · 2024-08-29T18:36:13Z

spec/models/manageiq/providers/openshift/infra_manager/full_refresh_spec.rb

Prefer this matching the Refresher class name e.g. spec/models/manageiq/providers/openshift/container_manager/refresher_spec.rb

agrare · 2024-08-29T18:50:04Z

spec/models/manageiq/providers/openshift/infra_manager/full_refresh_spec.rb

+require 'json'
+require 'recursive_open_struct'
+
+describe ManageIQ::Providers::Openshift::Inventory::Parser::InfraManager::FullRefresh do


Suggested change

describe ManageIQ::Providers::Openshift::Inventory::Parser::InfraManager::FullRefresh do

describe ManageIQ::Providers::Openshift::InfraManager::Refresher do

agrare · 2024-08-29T18:52:27Z

spec/vcr_cassettes/full_refresh.yml

Hm seem to have lost my comment, but typically we want this to match the provider class namespace like spec/vcr_cassettes/manageiq/providers/openshift/container_manager/refresher.yml
You can use VCR.use_cassette(described_class.name.underscore)

agrare · 2024-08-29T18:59:09Z

@miq-bot cross-repo-tests ManageIQ/manageiq-providers-kubernetes#537 ManageIQ/manageiq-providers-kubevirt#247 ManageIQ/manageiq#23165

From Pull Request: ManageIQ/manageiq-providers-openshift#264

Fryguy · 2024-08-30T16:15:43Z

app/assets/images/svg/vendor-openshift_infra.svg

Should this live in manageiq-decorators? I prefer it in the plugin class, but wasn't sure if it was required over there.

On that note, in manageiq-decorators we use symlinks when there are multiple images that point to the same thing, so this could just be a symlink over there.

It doesn't seem like it's required in manageiq-decorators

miq-bot · 2024-09-10T14:55:34Z

Checked commit nasark@994682d with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
23 files checked, 0 offenses detected
Everything looks fine. ⭐

nasark · 2024-09-10T15:01:56Z

@miq-bot cross-repo-tests ManageIQ/manageiq-providers-kubernetes#537 ManageIQ/manageiq-providers-kubevirt#247 ManageIQ/manageiq#23165

From Pull Request: ManageIQ/manageiq-providers-openshift#264

nasark · 2024-09-10T15:23:45Z

@agrare @Fryguy cross repo tests are green ManageIQ/manageiq-cross_repo-tests#911

agrare · 2024-09-10T15:29:52Z

app/models/manageiq/providers/openshift/inventory.rb

+ parser_type = if target_is_vm?(target)
+ "PartialTargetRefresh"
+ else
+ "FullRefresh"
+ end


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

agrare · 2024-09-10T15:30:45Z

app/models/manageiq/providers/openshift/inventory.rb

+ if !ems.kind_of?(EmsInfra)
+ super
+ else


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

agrare · 2024-09-10T15:31:19Z

app/models/manageiq/providers/openshift/inventory.rb

+ if !ems.kind_of?(EmsInfra)
+ super
+ else


Same here,

Suggested change

if !ems.kind_of?(EmsInfra)

super

else

return super unless ems.kind_of?(EmsInfra)

agrare · 2024-09-10T15:33:13Z

app/models/manageiq/providers/openshift/inventory.rb

+ collector = if target_is_vm?(target)
+ collector_class.new(ems, target)
+ else
+ collector_class.new(ems, ems)
+ end


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

agrare · 2024-09-10T15:35:00Z

spec/models/manageiq/providers/openshift/infra_manager/refresher_spec.rb

+ end
+ end
+
+ it 'works correctly with one node' do


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

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

agrare

Looks good some minor things that can be follow-ups

nasark requested review from agrare, cben and Fryguy as code owners August 7, 2024 20:17

miq-bot assigned agrare Aug 7, 2024

miq-bot added enhancement radjabov/yes? labels Aug 7, 2024

miq-bot added the wip label Aug 7, 2024

Fryguy reviewed Aug 7, 2024

View reviewed changes

agrare reviewed Aug 7, 2024

View reviewed changes

nasark force-pushed the subclass_kubevirt branch from a7ad2c4 to 6918d9e Compare August 14, 2024 20:57

This was referenced Aug 14, 2024

Openshift Virtualization subclassing changes ManageIQ/manageiq-providers-kubevirt#247

Merged

Separate OSV and Kubevirt virtualization options from form ManageIQ/manageiq-providers-kubernetes#537

Merged

agrare reviewed Aug 15, 2024

View reviewed changes

nasark force-pushed the subclass_kubevirt branch 2 times, most recently from edce97e to 7236e7c Compare August 27, 2024 17:50

agrare reviewed Aug 27, 2024

View reviewed changes

nasark force-pushed the subclass_kubevirt branch from 7236e7c to 42365f0 Compare August 28, 2024 18:19

agrare reviewed Aug 29, 2024

View reviewed changes

nasark force-pushed the subclass_kubevirt branch from 6c1fc78 to a7a29b6 Compare August 29, 2024 18:33

agrare reviewed Aug 29, 2024

View reviewed changes

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Aug 29, 2024

Running tests for agrare

dd906a3

From Pull Request: ManageIQ/manageiq-providers-openshift#264

This was referenced Aug 29, 2024

[BOT] Cross repo test for ManageIQ/manageiq-providers-openshift#264 ManageIQ/manageiq-cross_repo-tests#903

Closed

[BOT] Cross repo test for ManageIQ/manageiq-providers-openshift#264 ManageIQ/manageiq-cross_repo-tests#904

Closed

nasark force-pushed the subclass_kubevirt branch from 0b05389 to 3cbdac2 Compare August 29, 2024 19:04

nasark changed the title ~~[WIP] Subclass Kubevirt InfraManager to separate Openshift Virtualization provider~~ Subclass Kubevirt InfraManager to separate Openshift Virtualization provider Aug 29, 2024

nasark force-pushed the subclass_kubevirt branch from 3cbdac2 to 1a61429 Compare August 30, 2024 15:31

Fryguy reviewed Aug 30, 2024

View reviewed changes

subclass kubevirt for OSV

994682d

nasark force-pushed the subclass_kubevirt branch from 1a61429 to 994682d Compare September 10, 2024 14:51

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Sep 10, 2024

Running tests for nasark

c8999e2

From Pull Request: ManageIQ/manageiq-providers-openshift#264

miq-bot mentioned this pull request Sep 10, 2024

[BOT] Cross repo test for ManageIQ/manageiq-providers-openshift#264 ManageIQ/manageiq-cross_repo-tests#911

Closed

agrare reviewed Sep 10, 2024

View reviewed changes

agrare approved these changes Sep 10, 2024

View reviewed changes

agrare merged commit 2d66f57 into ManageIQ:master Sep 10, 2024
1 of 4 checks passed

Fryguy added this to the Radjabov milestone Sep 10, 2024

Fryguy removed the wip label Sep 11, 2024

		@@ -0,0 +1,2 @@
		class ManageIQ::Providers::Openshift::InfraManager::RefreshWorker < ManageIQ::Providers::Kubevirt::InfraManager::RefreshWorker

		:value => 'kubevirt',
		:pivot => 'endpoints.kubevirt.hostname',

	describe ManageIQ::Providers::Openshift::Inventory::Parser::InfraManager::FullRefresh do
	describe ManageIQ::Providers::Openshift::InfraManager::Refresher do

Subclass Kubevirt InfraManager to separate Openshift Virtualization provider #264

Subclass Kubevirt InfraManager to separate Openshift Virtualization provider #264

Conversation

nasark commented Aug 7, 2024 • edited by agrare Loading

nasark commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Aug 7, 2024

Fryguy commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Aug 7, 2024

agrare commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Aug 27, 2024

Choose a reason for hiding this comment

agrare Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

agrare Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Aug 29, 2024

Fryguy Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Sep 10, 2024

nasark commented Sep 10, 2024

nasark commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare left a comment

Choose a reason for hiding this comment

nasark commented Aug 7, 2024 •

edited by agrare

Loading

agrare Aug 27, 2024 •

edited

Loading

agrare Aug 29, 2024 •

edited

Loading

agrare Aug 29, 2024 •

edited

Loading

Fryguy Aug 30, 2024 •

edited

Loading