Adding ContainerImage subclasses #15386

enoodle · 2017-06-15T13:25:00Z

This will add ContainerImage subclasses: BasicContainerImage and OpenshiftContainerImage. This will allow to treat container images from different sources differently.

Openshift provider complementing PR: ManageIQ/manageiq-providers-openshift#23

Fryguy · 2017-06-16T15:23:29Z

db/migrate/20170613144708_create_openshift_image.rb

+  def change
+    add_column :container_images, :type, :string
+    ContainerImage.update_all(:type => "OpenshiftContainerImage")
+    ContainerImage.where(:command => nil).update_all(:type => "BasicContainerImage")


These 2 data migration bits cannot be in a change method as they are not reversible. Please split this into an up and a down.

Please wrap data migrations in a say_with_time like the other data migrations

Data migrations must have a migration spec

Fryguy · 2017-06-16T15:23:41Z

db/migrate/20170613144708_create_openshift_image.rb

+    add_column :container_images, :type, :string
+    ContainerImage.update_all(:type => "OpenshiftContainerImage")
+    ContainerImage.where(:command => nil).update_all(:type => "BasicContainerImage")
+    add_column :container_images, :ems_ref, :string


Why is this here twice?

First column adds type and here it adds ems_ref. they are different.

Ah thank you...totally misread. Can you just point those two add_columns together up above?

Fryguy · 2017-06-16T15:24:14Z

app/models/openshift_container_image.rb

@@ -0,0 +1,13 @@
+class OpenshiftContainerImage < ContainerImage


This seems openshift specific and probably should go into the openshift provider gem itself.

Fryguy · 2017-06-16T15:25:04Z

app/models/miq_action.rb

@@ -742,7 +742,7 @@ def action_container_image_annotate_deny_execution(action, rec, inputs)
      return
    end

-    unless rec.digest.present?
+    unless rec.kind_of?(OpenshiftContainerImage)


Since the original code is OpenShift specific already, maybe we can let this go, but it would be preferable if we could avoid provider-specifics. @simon3z Thoughts?

Fryguy · 2017-06-16T15:26:42Z

app/models/ems_refresh/save_inventory_container.rb

@@ -308,6 +308,7 @@ def save_container_images_inventory(ems, hashes, target = nil)

    hashes.each do |h|
      h[:container_image_registry_id] = h[:container_image_registry][:id] unless h[:container_image_registry].nil?
+      h[:type] = h[:ems_ref] ? 'OpenshiftContainerImage' : 'BasicContainerImage'


We cannot have provider specifics here.

This can possibly be done differently by setting the :type in the parser itself. Then here do h[:type] ||= 'BasicContainerImage'. Maybe? I'm not exactly sure what a BasicContainerImage even is and whether or not it belongs in core or in the OpenShift specific provider code.

Ok, will do. BasicContainerImage is not an Openshift related class.

Fryguy · 2017-06-16T15:28:31Z

app/models/container_image.rb

@@ -11,6 +11,7 @@ class ContainerImage < ApplicationRecord
  DOCKER_PULLABLE_PREFIX = "docker-pullable://".freeze
  DOCKER_PREFIXES = [DOCKER_IMAGE_PREFIX, DOCKER_PULLABLE_PREFIX].freeze

+  belongs_to :parent, :polymorphic => true


To me it sounds like what you are trying to get is genealogy, in which case, containers should use the genealogy aspects that we already have for VMs (i.e. we have a relationship tree that show which instance came from which image, and then which image came from another image all the way back) @simon3z I'm not sure if this is already being done elsewhere in the containers code, but if so, we should make it consistent.

I didn't mean to create a genealogy structure here, Just to have all container image types inherit from the same base so that they can be addressed the same and share code.

If so, then parent might be a bad name choice as it conflicts with the Relationship code, should that be used in the future.

Fryguy · 2017-06-16T15:29:04Z

app/models/basic_container_image.rb

@@ -0,0 +1,4 @@
+class BasicContainerImage < ContainerImage


What is a BasicContainerImage exactly?

It is a class to represent a container image not from a specific provider. This is so that it will be easy to know what is the source of the image.

@enoodle why do we need BasicContainerImage in addition to ContainerImage?

It will make it easy to address all the container images that don't have an entity in Openshift/OtherContainerProvider managing them. It will hopefully be handy with the UI.

moolitayer

@enoodle we need something similar to:
https://github.com/manageiq/manageiq-providers-kubernetes/blob/7a0136a5696f270dc50d992594dd392db72106dd/app/models/manageiq/providers/kubernetes/container_manager/container_node.rb
(but in the openshift repository)

Add type to ContainerImage, and have a ContainerImage under the Openshift Namespace

enoodle · 2017-06-18T13:20:29Z

@moolitayer done. PTAL at ManageIQ/manageiq-providers-openshift#23 too

moolitayer · 2017-06-19T07:45:24Z

app/models/container_image.rb

@@ -11,6 +11,7 @@ class ContainerImage < ApplicationRecord
  DOCKER_PULLABLE_PREFIX = "docker-pullable://".freeze
  DOCKER_PREFIXES = [DOCKER_IMAGE_PREFIX, DOCKER_PULLABLE_PREFIX].freeze

+  belongs_to :container_image, :polymorphic => true


I though we needed this line, but now I dont think so anymore. I will remove this.

#15386 (comment)

moolitayer · 2017-06-19T10:09:04Z

@enoodle after investigation I think we will be better of with only two classes, so that image.type is always ContainerImage or ManageIQ::Providers::Openshift::ContainerManager::ContainerImage.
I don't expect it to cause any problems in the UI side.

moolitayer · 2017-06-19T12:22:34Z

db/migrate/20170613144708_create_openshift_image.rb

+      add_column :container_images, :type, :string
+      ContainerImage.update_all(:type => "ManageIQ::Providers::Openshift::ContainerManager::ContainerImage")
+      ContainerImage.where(:author => nil).update_all(:type => "ContainerImage")
+      add_column :container_images, :ems_ref, :string


Do we have a value to update for the Openshift::ContainerManager::ContainerImage ?

If this is only available during refresh, please make sure it will be populated in the next refresh

moolitayer · 2017-06-19T12:23:41Z

app/models/manageiq/providers/container_manager.rb

@@ -13,6 +13,7 @@ class ContainerManager < BaseManager
    has_many :container_limits, :foreign_key => :ems_id, :dependent => :destroy
    has_many :container_image_registries, :foreign_key => :ems_id, :dependent => :destroy
    has_many :container_images, :foreign_key => :ems_id, :dependent => :destroy
+    has_many :basic_container_images, :foreign_key => :ems_id, :dependent => :destroy


We don't need this any longer right?

moolitayer · 2017-06-19T14:35:42Z

@Fryguy @simon3z Please review

moolitayer · 2017-06-19T14:36:26Z

Thanks @enoodle

Fryguy · 2017-06-19T18:57:32Z

app/models/miq_action.rb

@@ -742,7 +742,7 @@ def action_container_image_annotate_deny_execution(action, rec, inputs)
      return
    end

-    unless rec.digest.present?
+    unless red.respond_to?(:annotate_deny_execution)


Is red a typo? If so are there no tests that would pick this up?

I added a test for that.

Fryguy · 2017-06-19T18:58:18Z

db/migrate/20170613144708_add_sti_to_container_image.rb

+  end
+
+  def up
+    say_with_time("Adding type and ems_ref columns to ContainerImage and setting type") do


The say_with_time should only be around the data migration but...not the add_columns (otherwise you are hiding the logging of the add_column calls).

Fryguy · 2017-06-19T18:58:45Z

db/migrate/20170613144708_add_sti_to_container_image.rb

+  end
+
+  def down
+    say_with_time("Removing type and ems_ref columns from ContainerImage") do


No say_with_time needed on the down since you don't have a data migration.

Fryguy · 2017-06-19T18:59:59Z

spec/migrations/20170613144708_add_sti_to_container_image_spec.rb

+      expect(container_image_stub.find(container_image.id)).not_to respond_to(:type)
+      expect(container_image_stub.find(openshift_container_image.id)).not_to respond_to(:type)
+    end
+  end


No need for a down migration test, since there is no data migration. Otherwise this test is just testing Rails itself, which isn't necessary.

miq-bot · 2017-06-22T17:36:49Z

This pull request is not mergeable. Please rebase and repush.

Fryguy · 2017-06-22T17:45:30Z

Database migrations have now been moved to the https://github.com/ManageIQ/manageiq-schema repo. Please see http://talk.manageiq.org/t/new-split-repo-manageiq-schema/2478 for instructions on how to transfer your database migrations. If this PR contains only migrations, I will leave it open for a short time during the transition, after which I will close this if it has not been moved over.

Adds type and ems_ref columns to ContainerImage and enables STI. allow annotating only OpenshiftContainerImages and remove annotation function from container_image

miq-bot · 2017-06-25T08:43:00Z

Checked commit enoodle@a01680a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

Fryguy · 2017-07-03T15:55:36Z

Merged ManageIQ/manageiq-schema#21. Please see my note about possible record thrashing, which may require another data migration for the ems_ref: ManageIQ/manageiq-schema#21 (comment)

Fryguy · 2017-07-03T15:58:51Z

~~Marking as WIP as this is pending the backend change in ManageIQ/manageiq-providers-openshift#23~~ Scratch that...I had it backwards...the other PR depends on this one.

enoodle changed the title ~~Adding ContainerImage subclasses~~ [WIP] Adding ContainerImage subclasses Jun 15, 2017

miq-bot added sql migration wip labels Jun 15, 2017

This was referenced Jun 15, 2017

Add openshift image resource ManageIQ/manageiq-providers-kubernetes#41

Closed

Adding openshift_container_images ManageIQ/manageiq-providers-openshift#23

Merged

enoodle force-pushed the openshift_container_image_class_split branch from 39a30e4 to 8a917b6 Compare June 15, 2017 13:52

enoodle changed the title ~~[WIP] Adding ContainerImage subclasses~~ Adding ContainerImage subclasses Jun 15, 2017

miq-bot removed the wip label Jun 15, 2017

enoodle changed the title ~~Adding ContainerImage subclasses~~ [WIP] Adding ContainerImage subclasses Jun 15, 2017

miq-bot added the wip label Jun 15, 2017

enoodle mentioned this pull request Jun 15, 2017

identify openshift images through command variable #15022

Closed

chessbyte assigned Fryguy Jun 15, 2017

chessbyte requested review from moolitayer and simon3z June 15, 2017 15:53

enoodle changed the title ~~[WIP] Adding ContainerImage subclasses~~ Adding ContainerImage subclasses Jun 15, 2017

miq-bot removed the wip label Jun 15, 2017

enoodle force-pushed the openshift_container_image_class_split branch from d290442 to dac108a Compare June 15, 2017 16:10

Fryguy requested changes Jun 16, 2017

View reviewed changes

enoodle force-pushed the openshift_container_image_class_split branch 2 times, most recently from edb5ec7 to c7b2160 Compare June 18, 2017 12:04

moolitayer suggested changes Jun 18, 2017

View reviewed changes

enoodle force-pushed the openshift_container_image_class_split branch from c7b2160 to 3235f7b Compare June 18, 2017 13:19

moolitayer reviewed Jun 19, 2017

View reviewed changes

enoodle force-pushed the openshift_container_image_class_split branch from 3235f7b to 8383f02 Compare June 19, 2017 08:19

enoodle force-pushed the openshift_container_image_class_split branch 2 times, most recently from ef84739 to c0998e5 Compare June 19, 2017 12:14

moolitayer reviewed Jun 19, 2017

View reviewed changes

Fryguy reviewed Jun 19, 2017

View reviewed changes

enoodle force-pushed the openshift_container_image_class_split branch from ed32267 to 744de99 Compare June 20, 2017 11:55

miq-bot added the unmergeable label Jun 22, 2017

enoodle mentioned this pull request Jun 25, 2017

subclassing ContainerImage to Openshift::ContainerImage ManageIQ/manageiq-schema#21

Merged

subclassing ContainerImage to Openshift::ContainerImage

a01680a

Adds type and ems_ref columns to ContainerImage and enables STI. allow annotating only OpenshiftContainerImages and remove annotation function from container_image

enoodle force-pushed the openshift_container_image_class_split branch from 744de99 to a01680a Compare June 25, 2017 08:37

miq-bot removed the unmergeable label Jun 25, 2017

Fryguy closed this Jul 3, 2017

Fryguy reopened this Jul 3, 2017

Fryguy changed the title ~~Adding ContainerImage subclasses~~ [Depends on ManageIQ/manageiq-providers-openshift#23] Adding ContainerImage subclasses Jul 3, 2017

Fryguy approved these changes Jul 3, 2017

View reviewed changes

Fryguy changed the title ~~[Depends on ManageIQ/manageiq-providers-openshift#23] Adding ContainerImage subclasses~~ [WIP] Adding ContainerImage subclasses Jul 3, 2017

Fryguy changed the title ~~[WIP] Adding ContainerImage subclasses~~ Adding ContainerImage subclasses Jul 3, 2017

Fryguy merged commit d0cf206 into ManageIQ:master Jul 3, 2017

Fryguy added this to the Sprint 64 Ending Jul 3, 2017 milestone Jul 3, 2017

Fryguy added core providers/containers enhancement and removed sql migration core labels Jul 3, 2017

		@@ -0,0 +1,13 @@
		class OpenshiftContainerImage < ContainerImage

Adding ContainerImage subclasses #15386

Adding ContainerImage subclasses #15386

Conversation

enoodle commented Jun 15, 2017 • edited Loading

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 Jun 16, 2017 • edited Loading

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

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

moolitayer left a comment

Choose a reason for hiding this comment

enoodle commented Jun 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Jun 19, 2017

moolitayer commented Jun 19, 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

miq-bot commented Jun 22, 2017

Fryguy commented Jun 22, 2017

miq-bot commented Jun 25, 2017

Fryguy commented Jul 3, 2017

Fryguy commented Jul 3, 2017 • edited Loading

enoodle commented Jun 15, 2017 •

edited

Loading

Fryguy Jun 16, 2017 •

edited

Loading

Fryguy commented Jul 3, 2017 •

edited

Loading