Move Openshift specific code into its provider directory. #15523

lfu · 2017-07-06T21:52:12Z

Move Openshift specific code into its provider directory to be consistent with the standard file layouts for providers.

Depends on ManageIQ/manageiq-schema#35.
Blocks ManageIQ/manageiq-providers-openshift#28.
Blocks ManageIQ/manageiq-providers-kubernetes#79.

@miq-bot assign @gmcculloug
@miq-bot add_label refactoring, providers/containers

cc @bzwei @simon3z

gmcculloug · 2017-07-20T14:43:10Z

👍 After type ManageIQ/manageiq-schema#35 is merged.

bzwei · 2017-07-20T15:10:27Z

LGTM

lfu · 2017-07-20T15:42:03Z

@enoodle Please review the refactoring of ContainerImage.
Do we need to add ManageIQ::Providers::Openshift::ContainerManager::ContainerImage subclass into Openshift repo?

enoodle · 2017-07-20T15:48:57Z

@lfu ManageIQ/manageiq-providers-openshift#23 is taking care of it but is waiting on #15519

Second thought: This change should also wait on it. Introducing a new container image class will change the way SSA jobs are built. Also we will need to update in manageiq-provider-kubernetes to set the correct type for the new container images.

lfu · 2017-07-20T16:09:34Z

@enoodle Probably we want to take the ContainerImage refactoring out of this PR and leave it to you, maybe add it along with ManageIQ/manageiq-providers-openshift#23? So this PR can be merged without waiting and the ContainerImage functionality won't break. Thoughts?

enoodle · 2017-07-20T16:31:38Z

@lfu Yes, no problem

Fryguy · 2017-07-26T20:17:59Z

I merged ManageIQ/manageiq-schema#35, but then realized the PRs makes all of the ContainerTemplate objects OpenShift specific...couldn't it be Kubernetes as well?

Fryguy · 2017-07-26T20:18:33Z

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

@@ -0,0 +1,67 @@
+class ManageIQ::Providers::ContainerManager::ContainerTemplate < ContainerTemplate


If this is Kubernetes specific code, why is it going in ManageIQ::Providers::ContainerManager::ContainerTemplate ?

Thought these codes are for both Kubernetes and Openshift.

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

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

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

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

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)

lfu · 2017-07-26T21:08:42Z

I thought Openshift template is the only container template that is currently supported.

miq-bot · 2017-07-31T14:58:21Z

Checked commit lfu@7bb28aa with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

cben · 2017-08-03T14:37:28Z

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

@@ -0,0 +1,2 @@
+class ManageIQ::Providers::ContainerManager::ContainerTemplate < ContainerTemplate


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.

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.

@cben Good catch 👍

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

Follow-up of ManageIQ#15523.

miq-bot assigned gmcculloug Jul 6, 2017

miq-bot added providers/containers refactoring labels Jul 6, 2017

lfu force-pushed the refactor_container_manager branch from 90069df to 4aa9ed0 Compare July 7, 2017 16:08

This was referenced Jul 7, 2017

Add type to ContainerTemplate to allow subclassing. ManageIQ/manageiq-schema#35

Merged

Add ManageIQ::Providers::Openshift::ContainerManager::ContainerTemplate. ManageIQ/manageiq-providers-openshift#28

Merged

lfu force-pushed the refactor_container_manager branch from 4aa9ed0 to 72e4a51 Compare July 20, 2017 15:25

lfu force-pushed the refactor_container_manager branch from 72e4a51 to 457d48a Compare July 20, 2017 16:39

simon3z approved these changes Jul 21, 2017

View reviewed changes

Fryguy reviewed Jul 26, 2017

View reviewed changes

lfu changed the title ~~Move kube/openshift specific code into its provider directory.~~ Move Openshift specific code into its provider directory. Jul 26, 2017

Move kube/openshift specific template code into its provider directory.

7bb28aa

lfu force-pushed the refactor_container_manager branch from 457d48a to 7bb28aa Compare July 31, 2017 14:54

lfu mentioned this pull request Jul 31, 2017

Move kube/openshift specific template code into its provider directory. ManageIQ/manageiq-providers-kubernetes#79

Merged

Fryguy merged commit 1fdd17e into ManageIQ:master Aug 2, 2017

Fryguy added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 2, 2017

cben reviewed Aug 3, 2017

View reviewed changes

lfu added a commit to lfu/manageiq that referenced this pull request Aug 3, 2017

Add require_nested for container templates.

fbe046d

Follow-up of ManageIQ#15523.

lfu mentioned this pull request Aug 3, 2017

Add require_nested for container templates. #15726

Merged

lfu deleted the refactor_container_manager branch October 16, 2017 20:15

This was referenced Apr 8, 2020

Move container_template operations into Openshift ManageIQ/manageiq-providers-openshift#165

Merged

Remove ContainerTemplateMixin from Kubernetes ManageIQ/manageiq-providers-kubernetes#365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Openshift specific code into its provider directory. #15523

Move Openshift specific code into its provider directory. #15523

lfu commented Jul 6, 2017 •

edited

Loading

gmcculloug commented Jul 20, 2017

bzwei commented Jul 20, 2017

lfu commented Jul 20, 2017

enoodle commented Jul 20, 2017 •

edited

Loading

lfu commented Jul 20, 2017

enoodle commented Jul 20, 2017

Fryguy commented Jul 26, 2017

Fryguy Jul 26, 2017

lfu Jul 26, 2017

Fryguy Jul 26, 2017

Fryguy Jul 26, 2017

lfu Jul 27, 2017

Fryguy Jul 27, 2017

cben Jul 31, 2017

lfu commented Jul 26, 2017

miq-bot commented Jul 31, 2017

cben Aug 3, 2017

cben Aug 3, 2017 •

edited

Loading

lfu Aug 3, 2017

cben Aug 3, 2017 •

edited

Loading

		@@ -0,0 +1,67 @@
		class ManageIQ::Providers::ContainerManager::ContainerTemplate < ContainerTemplate

		@@ -0,0 +1,2 @@
		class ManageIQ::Providers::ContainerManager::ContainerTemplate < ContainerTemplate

Move Openshift specific code into its provider directory. #15523

Move Openshift specific code into its provider directory. #15523

Conversation

lfu commented Jul 6, 2017 • edited Loading

gmcculloug commented Jul 20, 2017

bzwei commented Jul 20, 2017

lfu commented Jul 20, 2017

enoodle commented Jul 20, 2017 • edited Loading

lfu commented Jul 20, 2017

enoodle commented Jul 20, 2017

Fryguy commented Jul 26, 2017

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

lfu commented Jul 26, 2017

miq-bot commented Jul 31, 2017

Choose a reason for hiding this comment

cben Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

lfu commented Jul 6, 2017 •

edited

Loading

enoodle commented Jul 20, 2017 •

edited

Loading

cben Aug 3, 2017 •

edited

Loading

cben Aug 3, 2017 •

edited

Loading