Introduce virtualization manager #16721

masayag · 2017-12-31T11:36:49Z

In order to support kubevirt as a secondary manager of the
openshift/kubevirt provider, a new virtualization manager mixin is added to introduce it.

The corresponding PR of the container providers is ManageIQ/manageiq-providers-kubernetes#197

masayag · 2017-12-31T11:38:43Z

@miq-bot add_labels providers/containers, gaprindashvili/no, wip

cben · 2017-12-31T22:22:49Z

cc @moolitayer @zeari

moolitayer · 2018-01-01T08:31:10Z

@masayag

Does a container manager always have a virtualization manager?
Can that change during the lifecycle of the container manager? (e.g created without a virtualization manager but then edited to have one)

zeari · 2018-01-01T10:35:40Z

app/models/manageiq/providers/container_manager.rb

+ :foreign_key => :parent_ems_id,
+ :class_name => "ManageIQ::Providers::Kubevirt::InfraManager",
+ :autosave => true,
+ :dependent => :destroy


@moolitayer should this be in a provider repo like monitoring manager?
https://github.com/manageiq/manageiq-providers-openshift/blob/master/app/models/manageiq/providers/openshift/container_manager.rb#L19-19

@zeari due to the inverse reference from https://github.com/ManageIQ/manageiq-providers-kubevirt/pull/6/files#diff-ff17b05a7c525eec833f0ddb0134a0d9R30 - it should reference a common class iiuc.

masayag · 2018-01-01T11:12:04Z

@moolitayer

Does a container manager always have a virtualization manager?

No, only if kubevirt was deployed (or any potential future virtualization manager)

Can that change during the lifecycle of the container manager? (e.g created without a virtualization manager but then edited to have one)

Yes, I don't see a reason not to support that option. Kubevirt can be deployed on top of an existing kubernetes cluster and to expose the virtualization capabilities.

masayag · 2018-01-02T10:05:03Z

add detect logic to openshift provider - large: 2 weeks

moolitayer · 2018-01-02T10:49:45Z

Yes, I don't see a reason not to support that option. Kubevirt can be deployed on top of an existing kubernetes cluster and to expose the virtualization capabilities.

OK in that case the implementation would have to change according to #16635

You will see that if you add a kubevirt endpoint through the API the new manager isn't created.

I will come see you later today

masayag · 2018-01-02T11:11:14Z

OK in that case the implementation would have to change according to #16635

You will see that if you add a kubevirt endpoint through the API the new manager isn't created.

I will come see you later today

@moolitayer I'm aware of that - noticed the behavior of kubevirt endpoints and authentication are left behind once the kubevirt provider is removed. That prevents the kubevirt provider from being added again.

Also - added a comment about it to ManageIQ/manageiq-providers-kubernetes#197:

This PR depends on #16721 and should be updated once #16635 is merged.

hence, the wip label on this PR.

zeari · 2018-01-04T15:25:38Z

@masayag #16635 was merged

masayag · 2018-01-04T15:30:48Z

@zeari thanks - I was looking at it closely :-)

I'll start adapting this PR and ManageIQ/manageiq-providers-kubernetes#197 based on it.

moolitayer · 2018-01-08T10:02:08Z

@masayag how do the virt manager PRs change the dependency between repos?
(manageiq-providers-kubevirt is depending on manageiq-providers-kubernetes? we would want to add the dependency explicitly)

masayag · 2018-01-11T21:28:25Z

@masayag how do the virt manager PRs change the dependency between repos?
(manageiq-providers-kubevirt is depending on manageiq-providers-kubernetes? we would want to add the dependency explicitly)

@moolitayer that dependency should be defined on the manageiq-providers-kubevirt repository only, right ?

masayag · 2018-01-11T21:33:35Z

app/models/ext_management_system.rb

@@ -433,7 +433,7 @@ def self.ems_physical_infra_discovery_types

 def disable!
 _log.info("Disabling EMS [#{name}] id [#{id}].")
- update!(:enabled => false)
+ update_attribute("enabled", false)


@zeari @moolitayer I had to change this since without it, when an endpoint is destroyed, the callback to remove the virtualization provider is triggered. At that point of time there isn't an endpoint for the provider and the validations invoked by update_attribute("enabled", false) i.e. hostname_format_valid? fails since there is no endpoint to fetch the hostname from.

I dont think we need this. It doesnt always look like it in the code but the contianers provider is valid if it has a default endpoint. So when actually using UI\API you should never end up with that hostname issue.

@zeari thanks for the hint - this change is not required.

masayag · 2018-01-14T11:10:48Z

@miq-bot remove_label wip

masayag · 2018-01-15T13:01:56Z

@agrare Could you review please ?

agrare · 2018-01-15T13:24:18Z

app/models/mixins/has_virtualization_manager_mixin.rb

+
+ def ensure_virtualization_manager
+ if virtualization_manager.nil?
+ build_virtualization_manager(:parent_manager => self,


Where is this (build_virtualization_manager) defined? I don't see it in any of the three PRs

@agrare it's rails method, the name was chosen to reflect the nature of that sub-provider, in the same sense as monitoring_manager was chosen for https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/has_monitoring_manager_mixin.rb#L22

The purpose of 'virtualization_manager' is to reflect the ability to serve virtual machines by a container provider, by that secondary provider.

If this doesn't make sense to you, I'll modify it.

Right but in that case there actually is a manager type called MonitoringManager (https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/monitoring_manager.rb). This should match the type of manager that is being added.

agrare · 2018-01-15T13:27:58Z

Should be infra_manager everywhere here, not virtualization_manager. It is confusing to see has_virtualization_manager when that doesn't actually exist. I agree that we can do better than infra_manager for a name but this makes it more confusing.

agrare

The association and mixin should all match the name of the manager that is being created

agrare · 2018-01-15T20:11:18Z

app/models/manageiq/providers/container_manager.rb

@@ -46,6 +47,11 @@ class ContainerManager < BaseManager
 has_many :all_container_images, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "ContainerImage"
 has_many :all_container_nodes, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "ContainerNode"

+ has_one :virtualization_manager,


Should be has_one :infra_manager

bdunne · 2018-01-15T15:00:39Z

app/models/manageiq/providers/container_manager.rb

@@ -46,6 +47,11 @@ class ContainerManager < BaseManager
 has_many :all_container_images, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "ContainerImage"
 has_many :all_container_nodes, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "ContainerNode"

+ has_one :virtualization_manager,


Wouldn't it make more sense to have a Provider that has multiple managers / ext_management_systems, rather than a manager that has a manager?

The ContainerManager already follows the CloudManager pattern with the monitoring_manager so we would need to move that as well.

The CloudManager really drove this and most providers already use it, might make more sense to deal with this at a higher level across all the providers?

😭 I thought Openstack was the only one doing this

In order to support kubevirt as a secondary manager of the openshift/kubevirt provider, a new mixin is added to represent that capability.

masayag · 2018-01-16T14:46:33Z

@agrare fixed according to your comments.

miq-bot · 2018-01-16T14:47:33Z

Checked commit masayag@8ffb88c with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

app/models/manageiq/providers/container_manager.rb

❗ - Line 50, Col 5 - Rails/InverseOf - Specify an :inverse_of option.

masayag mentioned this pull request Dec 31, 2017

Support KubeVirt provider ManageIQ/manageiq-providers-kubernetes#197

Merged

miq-bot changed the title ~~Introduce virtualization manager~~ [WIP] Introduce virtualization manager Dec 31, 2017

miq-bot added gaprindashvili/no providers/containers wip labels Dec 31, 2017

zeari reviewed Jan 1, 2018

View reviewed changes

masayag force-pushed the virtualization_manager branch from 4e3c9bc to cee2de3 Compare January 11, 2018 21:14

masayag commented Jan 11, 2018

View reviewed changes

masayag changed the title ~~[WIP] Introduce virtualization manager~~ Introduce virtualization manager Jan 14, 2018

miq-bot removed the wip label Jan 14, 2018

masayag force-pushed the virtualization_manager branch from cee2de3 to 893e6bf Compare January 14, 2018 18:48

zeari approved these changes Jan 15, 2018

View reviewed changes

agrare reviewed Jan 15, 2018

View reviewed changes

agrare requested changes Jan 15, 2018

View reviewed changes

bdunne reviewed Jan 15, 2018

View reviewed changes

Introduce virtualization manager

8ffb88c

In order to support kubevirt as a secondary manager of the openshift/kubevirt provider, a new mixin is added to represent that capability.

masayag force-pushed the virtualization_manager branch from 893e6bf to 8ffb88c Compare January 16, 2018 14:36

agrare approved these changes Jan 17, 2018

View reviewed changes

agrare merged commit 257ae38 into ManageIQ:master Jan 17, 2018

agrare added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 17, 2018

masayag deleted the virtualization_manager branch June 27, 2018 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce virtualization manager #16721

Introduce virtualization manager #16721

masayag commented Dec 31, 2017 •

edited

Loading

masayag commented Dec 31, 2017

cben commented Dec 31, 2017

moolitayer commented Jan 1, 2018 •

edited

Loading

zeari Jan 1, 2018

masayag Jan 1, 2018

masayag commented Jan 1, 2018

masayag commented Jan 2, 2018

moolitayer commented Jan 2, 2018

masayag commented Jan 2, 2018

zeari commented Jan 4, 2018

masayag commented Jan 4, 2018

moolitayer commented Jan 8, 2018

masayag commented Jan 11, 2018

masayag Jan 11, 2018

zeari Jan 14, 2018 •

edited

Loading

masayag Jan 14, 2018

masayag commented Jan 14, 2018

masayag commented Jan 15, 2018

agrare Jan 15, 2018

masayag Jan 15, 2018

agrare Jan 15, 2018

agrare commented Jan 15, 2018

agrare left a comment

agrare Jan 15, 2018

bdunne Jan 15, 2018

agrare Jan 15, 2018

bdunne Jan 15, 2018

masayag commented Jan 16, 2018

miq-bot commented Jan 16, 2018

Introduce virtualization manager #16721

Introduce virtualization manager #16721

Conversation

masayag commented Dec 31, 2017 • edited Loading

masayag commented Dec 31, 2017

cben commented Dec 31, 2017

moolitayer commented Jan 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayag commented Jan 1, 2018

masayag commented Jan 2, 2018

moolitayer commented Jan 2, 2018

masayag commented Jan 2, 2018

zeari commented Jan 4, 2018

masayag commented Jan 4, 2018

moolitayer commented Jan 8, 2018

masayag commented Jan 11, 2018

Choose a reason for hiding this comment

zeari Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masayag commented Jan 14, 2018

masayag commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Jan 15, 2018

agrare left a 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

masayag commented Jan 16, 2018

miq-bot commented Jan 16, 2018

masayag commented Dec 31, 2017 •

edited

Loading

moolitayer commented Jan 1, 2018 •

edited

Loading

zeari Jan 14, 2018 •

edited

Loading