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

Move Openshift specific code into its provider directory. #15523

Merged
merged 1 commit into from
Aug 2, 2017
Merged

Move Openshift specific code into its provider directory. #15523

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
66 changes: 0 additions & 66 deletions app/models/container_template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,70 +12,4 @@ class ContainerTemplate < ApplicationRecord
serialize :objects, Array

acts_as_miq_taggable

MIQ_ENTITY_MAPPING = {
"Route" => ContainerRoute,
"Build" => ContainerBuildPod,
"BuildConfig" => ContainerBuild,
"Template" => ContainerTemplate,
"ResourceQuota" => ContainerQuota,
"LimitRange" => ContainerLimit,
"ReplicationController" => ContainerReplicator,
"PersistentVolumeClaim" => PersistentVolumeClaim,
"Pod" => ContainerGroup,
"Service" => ContainerService,
}.freeze

def instantiate(params, project = nil)
project ||= container_project.name
processed_template = process_template(ext_management_system.connect,
:metadata => {
:name => name,
:namespace => project
},
:objects => objects,
:parameters => params)
create_objects(processed_template['objects'], project)
@created_objects.each { |obj| obj[:miq_class] = MIQ_ENTITY_MAPPING[obj[:kind]] }
end

def process_template(client, template)
client.process_template(template)
rescue KubeException => e
raise MiqException::MiqProvisionError, "Unexpected Exception while processing template: #{e}"
end

def create_objects(objects, project)
@created_objects = []
objects.each { |obj| @created_objects << create_object(obj, project).to_h }
end

def create_object(obj, project)
obj = obj.symbolize_keys
obj[:metadata][:namespace] = project
method_name = "create_#{obj[:kind].underscore}"
begin
ext_management_system.connect_client(obj[:apiVersion], method_name).send(method_name, obj)
rescue KubeException => e
rollback_objects(@created_objects)
raise MiqException::MiqProvisionError, "Unexpected Exception while creating object: #{e}"
end
end

# rollback_objects cannot catch children objects created during the template instantiation and therefore those objects
# will remain in the cluster.
def rollback_objects(objects)
objects.each { |obj| rollback_object(obj) }
end

def rollback_object(obj)
method_name = "delete_#{obj[:kind].underscore}"
begin
ext_management_system.connect_client(obj[:apiVersion], method_name).send(method_name,
obj[:metadata][:name],
obj[:metadata][:namespace])
rescue KubeException => e
_log.error("Unexpected Exception while deleting object: #{e}")
end
end
end
2 changes: 2 additions & 0 deletions app/models/manageiq/providers/container_manager/container_template.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class ManageIQ::Providers::ContainerManager::ContainerTemplate < ContainerTemplate
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 Kubernetes specific code, why is it going in ManageIQ::Providers::ContainerManager::ContainerTemplate ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thought these codes are for both Kubernetes and Openshift.

Copy link
Member

Choose a reason for hiding this comment

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

Code in ManageIQ::Providers::ContainerManager is for any container manager...it could even be something like docker swarm in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Shared code for kubernetes and openshift should go into manageiq-providers-kubernetes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fryguy What is the relationship between ManageIQ::Providers::Kubernetes::ContainerManager::ContainerTemplate and ManageIQ::Providers::Openshift::ContainerManager::ContainerTemplate? Are they parallel?

Copy link
Member

Choose a reason for hiding this comment

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

I would expect them to be unrelated. They may share code via a shared module, but other than that, they are independent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifically templates are an Openshift addition, they don't exist in Kubernetes.
(they might in future, openshift concepts tend to get upstreamed, there is already a proposal)

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this (now empty) subclass had an interesting consequence (that's breaking openshift refresh).
In manageiq/providers/container_manager.rb,
ContainerTemplate resolves to this nested class, not the base ::ContainerTemplate.
And when we do

    has_many :container_templates, :foreign_key => :ems_id, :dependent => :destroy

it apparently does such a const lookup (?), and the query now constrains type, never matching any records:

(byebug) ems.container_templates.to_sql
"SELECT \"container_templates\".* FROM \"container_templates\" WHERE \"container_templates\".\"type\" IN ('ManageIQ::Providers::ContainerManager::ContainerTemplate') AND \"container_templates\".\"ems_id\" = 24000000000002"

I can do

    has_many :container_templates, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "::ContainerTemplate"

to get back the query

> ems.container_templates.to_sql
=> "SELECT \"container_templates\".* FROM \"container_templates\" WHERE \"container_templates\".\"ems_id\" = 3"

but I wonder what's the point of having both ::ContainerTemplate and ManageIQ::Providers::ContainerManager::ContainerTemplate?
It's not like any non-container providers will have ContainerTemplate, so why not axe this subclass and have kubernetes/openshift inherit from ::ContainerTemplate?

@lfu @Fryguy @kbrock @moolitayer please advice.

Copy link
Contributor

@cben cben Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Got curious if ContainerManager::OrchestrationStack relation is also affected.
ContainerManager doesn't include HasManyOrchestrationStackMixin, but if it did:

[2] pry(main)> ems.orchestration_stacks.to_sql
=> "SELECT \"orchestration_stacks\".* FROM \"orchestration_stacks\" WHERE \"orchestration_stacks\".\"type\" IN ('ManageIQ::Providers::ContainerManager::OrchestrationStack', 'ManageIQ::Providers::Openshift::ContainerManager::OrchestrationStack') AND \"orchestration_stacks\".\"ems_id\" = 3"

Strange, this correctly includes the openshift descendants so seems harmless. Why doesn't the templates query?

[5] pry(main)> ManageIQ::Providers::ContainerManager::ContainerTemplate.descendants
=> []

... some false steps later ...

[13] pry(main)> ManageIQ::Providers::Openshift::ContainerManager::ContainerTemplate
=> ManageIQ::Providers::ContainerManager::ContainerTemplate(...

OMG, the provider-specific subclasses are not loaded! Const lookup is satisfied by finding this class instead, so no autoload.

[16] pry(main)> require 'manageiq/providers/openshift/container_manager/container_template.rb'
=> true
[17] pry(main)> ManageIQ::Providers::Openshift::ContainerManager::ContainerTemplate
=> ManageIQ::Providers::Openshift::ContainerManager::ContainerTemplate(...
[18] pry(main)> ManageIQ::Providers::ContainerManager::ContainerTemplate.descendants
=> [ManageIQ::Providers::Openshift::ContainerManager::ContainerTemplate(...)]

=> Gonna add require_nested.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cben Good catch 👍

Copy link
Contributor

@cben cben Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

(yay we resolved it, given ManageIQ/manageiq-providers-kubernetes#81 now ManageIQ/manageiq-providers-openshift#28 passes.)

end