migrations for merging container_definition and container #24

zeari · 2017-06-25T09:20:10Z

moolitayer · 2017-06-25T11:22:36Z

missing db/schema.yml changes

moolitayer · 2017-06-25T12:01:25Z

Looks good, one thing i'm concerned about is iterating over all containers. That would amount to tens of thousands of queries in large environments. Take a look at:
https://github.com/ManageIQ/manageiq-schema/blob/64af9f368dc352686f2908dec6ca9ab4b66d44ec/db/migrate/20170115140217_update_ems_in_miq_alert_status.rb

cben · 2017-06-25T12:23:29Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+
+    say_with_time("Copying over columns from container_definition to container") do
+      Container.all.each do |container|
+        container_definition = ContainerDefinition.find(container.container_definition_id)


can this ever fail?

I dont think so. in order to have containers at all the inventory refresh has to go through and save provider->pod->container_def->container.

lpichler · 2017-06-26T10:37:02Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

@@ -0,0 +1,159 @@
+class UnifyContainerDefinitionAndContainer < ActiveRecord::Migration[5.0]
+  class ContainerDefinition < ActiveRecord::Base
+    has_many :container_port_configs


@zeari we need to add class_name for all relations:
:class_name => 'UnifyContainerDefinitionAndContainer::ContainerPortConfig'
because without it, it will not take models defined in migrations.

lpichler · 2017-06-26T10:41:08Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+
+  class ContainerEnvVar < ActiveRecord::Base
+    belongs_to :container_definition
+    self.inheritance_column = :_type_disabled


this in only for tables with type column and there is no ContainerEnvVar#type, same for ContainerDefinition, SecurityContext and ContainerPortConfig.

We were asked to put this in all models to prevent problems if we ever add the column in the future (I understand that is the current policy)

lpichler · 2017-06-26T10:44:02Z

is it possible to separate structural and data migration to standalone migrations ?

lpichler · 2017-06-26T10:54:22Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+    add_column :containers, :command,            :text
+
+    say_with_time("Copying over columns from container_definition to container") do
+      Container.all.each do |container|


I think we can save some N+1 queries by this and use batch processing by with find_each:

Container.includes(:container_definition => [:container_port_configs, :container_env_vars, :security_contexts]).find_each do |container| container.update!( :image => container_definition.image, ...) container.container_definition.container_port_configs.update_all(... container.container_definition.container_env_vars.update_all(... container.container_definition.security_context.update end

zeari · 2017-06-26T11:25:12Z

is it possible to separate structural and data migration to standalone migrations ?

Im not sure that would turn out so nice. we need to do structural changes before and after copying the data.

kbrock

It is nice to avoid each, especially with a bunch of includes() in a migration. It leads to a bunch of individual UPDATE or INSERT queries

kbrock · 2017-06-26T18:50:56Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+      end
+    end
+
+    Container.includes(:container_definition => [:container_port_configs, :container_env_vars, :security_context]).find_each do |container|


starting on the other side (e.g. ContainerEnvVars) would work better.
In general, removing belongs_to

our suggestion from gitter:

update security_contexts set resource_id = (select containers.id from containers,container_definitions where containers.container_definition_id = container_definitions.id and container_definition_id = security_contexts.resource_id and resource_type = 'ContainerDefinition')

zeari · 2017-06-29T10:55:52Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+          :capabilities_drop  => container.capabilities_drop,
+          :command            => container.command
+        )
+        container.update!(:container_definition_id => container_def.id)


@kbrock what do you think is a good way to bulk insert into container_definitions?
overall we need to do something like

INSERT INTO container_definitions ( ems_id, name, image.... ) SELECT ems_id, name, image.... FROM containers

and then update container_definition_id in containers

we might be able to do something like

add_column :container_definitions, :container_id, :bigint insert into container_definitions (container_id, columns...) values (select (id, columns...) from containers) update containers set container_definition_id = (select container_definition.id from container_definitions, containers) where container.id = container_definitions.container_id remove_column :container_definitions, :container_id

or maybe leverage RETURNING somehow -

INSERT INTO container_definitions VALUES (....) RETURNING (id,....)

@zeari Oh, I had thought that the container_definitions already existed for the containers.

The pure sql way is:

insert into container_Definitions (container_id, columns...) select id, columns.. from conatiners

Not sure the MIQ way of doing that. we tend to be a little sql shy

simon3z · 2017-07-03T10:07:18Z

@zeari @cben IIUC we need this PR to be reviewed/approved before we can move on with ManageIQ/manageiq#15393 and ManageIQ/manageiq-providers-kubernetes#42

@cben @moolitayer @agrare can you review/approve?

moolitayer · 2017-07-03T10:50:46Z

@zeari can we have tags or other relevant relations associated with a removed container_definitions ?
(anything that has a generic resource relation to container_definitions )

zeari · 2017-07-03T10:59:46Z

@zeari can we have tags or other relevant relations associated with a removed container_definitions ?
(anything that has a generic resource relation to container_definitions )

Looking at container_definition.rb, i cant say that we do. Metrics, events and such belong to container.

moolitayer · 2017-07-03T11:21:06Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+         run_as_user run_as_non_root capabilities_add capabilities_drop command).each do |column|
+        join_sql = definitions.project(definitions[column.to_sym])
+                              .where(definitions[:id].eq(containers[:container_definition_id])).to_sql
+        Container.update_all("#{column} = (#{join_sql})")


moolitayer

LGTM 👍
I don't think we should refactor according to codeclimate's suggstions

kbrock

Looks good.

I prefer the update_all calls. So yes, ignore the rubo cops

kbrock

Thanks for pointing out the 2N + 1 queries.
Put in a suggestion for a fix for that

kbrock · 2017-07-03T17:45:56Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+
+    say_with_time("splitting columns from container into container_definition") do
+      ContainerDefinition.transaction do
+        Container.all.each do |container|


This is going to be # containers x 2 queries

maybe temporary column ContainerDefinition#container_id would make sense.

That way, this could be done in 2 queries.

sure, but this means we go with pure sql on this. (either that or something similar like https://gist.github.com/janko-m/2b2cea3e8e21d9232fb9 which is similar)

I did try to mitigate this with ContainerDefinition.transaction but it still means the DB goes through a lot of INSERT INTO container_definitions VALUES (......).

So pure sql is all I can think of doing here.

simon3z · 2017-07-11T16:03:52Z

@cben @moolitayer this PR is missing your final review/approval.

cben · 2017-07-12T09:12:51Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+      ContainerEnvVar.update_all("container_definition_id = (#{join_sql})")
+    end
+
+    say_with_time("switch container_definition_id with container_id for security_contexts") do


swtich resource_id with container_id, resource_type to 'Container' for security_contexts

cben · 2017-07-12T09:23:04Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+  def down
+    create_table :container_definitions do |t|
+      t.belongs_to :ems, :type => :bigint
+      t.string     :ems_ref


definition ems_ref is lost in up. Would copying it back FROM containers restore old situation?
parser synthesizes "#{pod_id}_#{container_def.name}_#{container_def.image}",
for containers it synthesizes "#{pod_id}_#{container.name}_#{container.image}".

pod_id is same pod.metadata.uid for both.

name should be same, it's what parser cross-ref'd containers to container_definitions on.

is image necessarily same? cc @enoodle

(I don't know if 100% reversible migration is a blocker here, but want to understands things first.)

Yes, If I understand correctly the contianer definition gets its image thought the container.

cben · 2017-07-12T09:23:20Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+      t.string     :ems_ref
+      t.bigint     :old_ems_id
+      t.timestamp  :deleted_on
+      t.string     :name


see above, copying name back should be OK

Can you add comment that above columns were lost in up and are being reconstructed (by copying),
while following columns are moved back from containers table.

cben · 2017-07-12T09:28:16Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+    create_table :container_definitions do |t|
+      t.belongs_to :ems, :type => :bigint
+      t.string     :ems_ref
+      t.bigint     :old_ems_id


did containers and definitions always have same ems_id, old_ems_id? were they archived in sync?
(trying to understand if copying back is safe)

Theyre usually always archived in sync.

# from container_definition.rb def disconnect_inv return if ems_id.nil? _log.info "Disconnecting Container definition [#{name}] id [#{id}]" self.container.try(:disconnect_inv) self.deleted_on = Time.now.utc save end

cben

Nice maneuver with the temp column 🔀 :-)

cben · 2017-07-12T10:01:45Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+      t.string     :ems_ref
+      t.bigint     :old_ems_id
+      t.timestamp  :deleted_on
+      t.string     :name


Can you add comment that above columns were lost in up and are being reconstructed (by copying),
while following columns are moved back from containers table.

cben · 2017-07-12T10:01:47Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+
+    add_column :containers, :container_definition_id, :bigint
+
+    # populate container_definitions. use container_id to keep relation to containers


consider turning this comment to a say_with_time
also for UPDATE below

cben · 2017-07-12T10:08:49Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+      ContainerEnvVar.update_all("container_id = (#{join_sql})")
+    end
+
+    say_with_time("switch container_definition_id with container_id for security_contexts") do


again, resource_id and resource_type

cben · 2017-07-12T10:19:27Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+      ContainerPortConfig.update_all("container_id = (#{join_sql})")
+    end
+
+    say_with_time("switch container_definition_id with container_id for container_port_configs") do


for container_env_vars

cben · 2017-07-12T10:23:21Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+    ActiveRecord::Base.connection.execute(update_statement)
+
+    # finally, remove the temp column
+    remove_column :container_definitions, :container_id


idea: if you move this remove to the end, can you simplify the 3 "switch container_definition_id with container_id" below to not need a join?
personally I'm not so much concerned with performance as simplicity ;-)

if yes, perhaps it's worth creating a temp column even in up?

just an idea, feel free to keep as is.

cben · 2017-07-12T10:40:49Z

spec/migrations/20170529142557_unify_container_definition_and_container_spec.rb

+      :name       => "mycontainer",
+      :ems_ref    => "123",
+      :ems_id     => 1,
+      :old_ems_id => 1,


Please take realistic data from real DB. (that's not how ems_ref looks, not sure ems_id == old_ems_id == 1 is possible etc...)
And since I have some worries about the overlapping columns like ems_ref, consider spelling out full data for both AND the expected merge, without assuming definition_hash.merge(container_hash)).

[have I mentioned migrations scare me, perhaps unreasonably?]

zeari · 2017-07-17T15:02:59Z

db/migrate/20170529142557_unify_container_definition_and_container.rb

+      SecurityContext.where(:resource_type => 'ContainerDefinition').update_all("resource_type = 'Container', resource_id = (#{join_sql})")
+    end
+
+    MiqQueue.where(:method_name => "purge_timer", :class_name => 'ContainerDefinition').destroy_all


@cben @simon3z Destroying the purge jobs.

simon3z · 2017-07-17T16:02:57Z

Added test. im ignoring container_definitions without containers because i dont think theyre meaningful:

@zeari ok as long as we don't crash in case there is a container_definitions without containers.
That is why having a test seemed interesting (to show that we have no issues with that).

cben

👍 test checks it does not crash;
I wondered why test doesn't explicitly check container_def2 is deleted => there is nothing to check as the whole table is gone :-) 🔨

moolitayer · 2017-07-17T17:32:48Z

@zeari I think there are some relevant rubocop warnings

Fryguy · 2017-07-17T17:48:38Z

db/schema.yml

@@ -805,7 +787,7 @@ container_port_configs:
 - port
 - host_port
 - protocol
- container_definition_id
+- container_id


@carbonin Will this cause a problem since it's in the middle of the pack? I think it's ok, just want to verify.

Yeah, as long as it was implemented as a rename (not a delete and add).

"Yeah" as in it's okay.... just to clarify 👍

miq-bot · 2017-07-17T17:59:41Z

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

miq-bot · 2017-07-18T12:19:25Z

Checked commits zeari/manageiq-schema@5ab7b5f~...0738280 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 16 offenses detected

db/migrate/20170718111834_unify_container_definition_and_container.rb

spec/migrations/20170718111834_unify_container_definition_and_container_spec.rb

⚠️ - Line 102, Col 7 - Lint/UselessAssignment - Useless assignment to variable - port_config2. Did you mean port_config?
⚠️ - Line 106, Col 7 - Lint/UselessAssignment - Useless assignment to variable - security_context2. Did you mean security_context?
⚠️ - Line 99, Col 7 - Lint/UselessAssignment - Useless assignment to variable - env_var2. Did you mean env_var?

zeari · 2017-07-20T14:00:03Z

ping @Fryguy @carbonin Can we merge this one and ManageIQ/manageiq#15393 ?

moolitayer · 2017-07-25T17:46:08Z

cc @Loicavenel

Fryguy · 2017-07-25T21:30:36Z

db/migrate/20170718111834_unify_container_definition_and_container.rb

+  class ContainerDefinition < ActiveRecord::Base
+    has_many :container_port_configs, :class_name => 'UnifyContainerDefinitionAndContainer::ContainerPortConfig'
+    has_many :container_env_vars, :class_name => 'UnifyContainerDefinitionAndContainer::ContainerEnvVar'
+    has_one :security_context, :as => :resource


This doesn't look like it's being used, so can we remove it? I'm asking because polymorphic types end up causing issues when accessed through has_many/belongs_to, but luckily you are using resource_type/resource_id directly in the migrations below.

Fryguy · 2017-07-25T21:30:48Z

db/migrate/20170718111834_unify_container_definition_and_container.rb

+
+  class SecurityContext < ActiveRecord::Base
+    self.inheritance_column = :_type_disabled
+    belongs_to :resource, :polymorphic => true


Same comment as https://github.com/ManageIQ/manageiq-schema/pull/24/files#r129430898 (this is the other half)

Fryguy · 2017-07-25T21:32:44Z

db/migrate/20170718111834_unify_container_definition_and_container.rb

+      t.bigint     :container_id                # temp column
+    end
+
+    add_index :container_definitions, :deleted_on


You probably want to add the index after you've done the data migration, otherwise each insert needs to update the index individually making it slower overall.

Fryguy · 2017-07-25T21:34:39Z

db/migrate/20170718111834_unify_container_definition_and_container.rb

+    add_column :containers, :container_definition_id, :bigint
+
+    say_with_time("populate container_definitions. use container_id to keep relation to containers") do
+      insert_statement = "INSERT INTO container_definitions (container_id, ems_id, ems_ref, old_ems_id, deleted_on, name, image,


Minor, bit Prefer this format for building SQL

insert_statement = <<-SQL INSERT INTO container_defintions... SELECT id, ... FROM containers SQL

The reason is that a) it keeps the SQL on it's own lines, and b) some IDEs will see Ruby <<-SQL block and actually color the SQL statement with SQL syntax coloring.

Fryguy · 2017-07-25T21:36:16Z

LGTM, so I am going to merge as is to unstick the main PR. @zeari Can you create a follow up PR that cleans up the various things I mentioned above?

zeari force-pushed the unify_container_definition branch from 5c3d7f0 to 6142bf9 Compare June 25, 2017 11:22

zeari force-pushed the unify_container_definition branch from 6142bf9 to 4f29d38 Compare June 25, 2017 11:57

cben reviewed Jun 25, 2017

View reviewed changes

lpichler reviewed Jun 26, 2017

View reviewed changes

zeari force-pushed the unify_container_definition branch 2 times, most recently from fa40488 to c4c99f4 Compare June 26, 2017 13:22

kbrock reviewed Jun 26, 2017

View reviewed changes

zeari force-pushed the unify_container_definition branch 2 times, most recently from eb87281 to 9c541f1 Compare June 29, 2017 10:45

zeari commented Jun 29, 2017

View reviewed changes

zeari force-pushed the unify_container_definition branch 3 times, most recently from 4b6073a to be9e7a7 Compare July 2, 2017 11:18

moolitayer reviewed Jul 3, 2017

View reviewed changes

kbrock approved these changes Jul 3, 2017

View reviewed changes

kbrock reviewed Jul 3, 2017

View reviewed changes

cben reviewed Jul 12, 2017

View reviewed changes

zeari commented Jul 17, 2017

View reviewed changes

zeari force-pushed the unify_container_definition branch from 6f23e0c to a37a7bc Compare July 17, 2017 15:09

cben approved these changes Jul 17, 2017

View reviewed changes

Fryguy reviewed Jul 17, 2017

View reviewed changes

miq-bot added the unmergeable label Jul 17, 2017

migration for merging container and container_definition

5ab7b5f

zeari force-pushed the unify_container_definition branch from a37a7bc to 4d6ac3c Compare July 18, 2017 11:08

miq-bot removed the unmergeable label Jul 18, 2017

zeari force-pushed the unify_container_definition branch from 4d6ac3c to c7630f4 Compare July 18, 2017 11:19

pure sql bulk insert

0738280

zeari force-pushed the unify_container_definition branch from c7630f4 to 0738280 Compare July 18, 2017 12:18

simon3z approved these changes Jul 20, 2017

View reviewed changes

chessbyte added data enhancement labels Jul 25, 2017

chessbyte assigned Fryguy Jul 25, 2017

Fryguy reviewed Jul 25, 2017

View reviewed changes

Fryguy merged commit 41a7428 into ManageIQ:master Jul 25, 2017

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

Fryguy added the schema label Jul 25, 2017

himdel mentioned this pull request Jul 26, 2017

Fix travis - ContainerDefinition no longer exists ManageIQ/manageiq-ui-classic#1760

Merged


		add_column :containers, :container_definition_id, :bigint

		# populate container_definitions. use container_id to keep relation to containers

migrations for merging container_definition and container #24

migrations for merging container_definition and container #24

Conversation

zeari commented Jun 25, 2017

moolitayer commented Jun 25, 2017

moolitayer commented Jun 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpichler Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpichler commented Jun 26, 2017

Choose a reason for hiding this comment

zeari commented Jun 26, 2017 • edited Loading

kbrock 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

simon3z commented Jul 3, 2017

moolitayer commented Jul 3, 2017

zeari commented Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

moolitayer left a comment

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

kbrock 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

simon3z commented Jul 11, 2017

Choose a reason for hiding this comment

cben Jul 12, 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

cben 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z commented Jul 17, 2017 • edited Loading

cben left a comment

Choose a reason for hiding this comment

moolitayer commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jul 17, 2017

miq-bot commented Jul 18, 2017

zeari commented Jul 20, 2017

moolitayer commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Fryguy commented Jul 25, 2017

lpichler Jun 26, 2017 •

edited

Loading

zeari commented Jun 26, 2017 •

edited

Loading

zeari commented Jul 3, 2017 •

edited

Loading

cben Jul 12, 2017 •

edited

Loading

simon3z commented Jul 17, 2017 •

edited

Loading

Fryguy Jul 25, 2017 •

edited

Loading