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.

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

migrations for merging container_definition and container #24

Merged
merged 2 commits into from
Jul 25, 2017
Merged
Changes from 1 commit
Commits
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
169 changes: 169 additions & 0 deletions db/migrate/20170529142557_unify_container_definition_and_container.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
class UnifyContainerDefinitionAndContainer < ActiveRecord::Migration[5.0]
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
has_one :container, :class_name => 'UnifyContainerDefinitionAndContainer::Container'
end

class Container < ActiveRecord::Base
belongs_to :container_definition, :class_name => 'UnifyContainerDefinitionAndContainer::ContainerDefinition'
end

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

class ContainerEnvVar < ActiveRecord::Base
belongs_to :container_definition, :class_name => 'UnifyContainerDefinitionAndContainer::ContainerDefinition'
end

class ContainerPortConfig < ActiveRecord::Base
belongs_to :container_definition, :class_name => 'UnifyContainerDefinitionAndContainer::ContainerDefinition'
end

def up
# attributes
add_column :containers, :image, :string
add_column :containers, :image_pull_policy, :string
add_column :containers, :memory, :string
add_column :containers, :cpu_cores, :float
add_column :containers, :container_group_id, :bigint
add_column :containers, :privileged, :boolean
add_column :containers, :run_as_user, :bigint
add_column :containers, :run_as_non_root, :boolean
add_column :containers, :capabilities_add, :string
add_column :containers, :capabilities_drop, :string
add_column :containers, :command, :text

containers = Arel::Table.new(:containers)
definitions = Arel::Table.new(:container_definitions)
say_with_time("Copying over columns from container_definition to container") do
%w(image image_pull_policy memory cpu_cores container_group_id privileged
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})")

Choose a reason for hiding this comment

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

👍

end
end

say_with_time("switch container_definition_id with container_id for container_port_configs") do
port_configs = Arel::Table.new(:container_port_configs)
join_sql = containers.project(containers[:id])
.where(containers[:container_definition_id].eq(port_configs[:container_definition_id])).to_sql
ContainerPortConfig.update_all("container_definition_id = (#{join_sql})")
end

say_with_time("switch container_definition_id with container_id for container_port_configs") do
env_vars = Arel::Table.new(:container_env_vars)
join_sql = containers.project(containers[:id])
.where(containers[:container_definition_id].eq(env_vars[:container_definition_id])).to_sql
ContainerEnvVar.update_all("container_definition_id = (#{join_sql})")
end

say_with_time("switch container_definition_id with container_id for security_contexts") do
Copy link
Contributor

Choose a reason for hiding this comment

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

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

security_contexts = Arel::Table.new(:security_contexts)
join_sql = containers.project(containers[:id])
.where(containers[:container_definition_id].eq(security_contexts[:resource_id])
.and(security_contexts[:resource_type].eq(Arel::Nodes::Quoted.new('ContainerDefinition')))).to_sql
SecurityContext.where(:resource_type => 'ContainerDefinition').update_all("resource_type = 'Container', resource_id = (#{join_sql})")
end

# relationships
rename_column :container_port_configs, :container_definition_id, :container_id
rename_column :container_env_vars, :container_definition_id, :container_id

remove_column :containers, :container_definition_id
drop_table :container_definitions
end

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

@cben cben Jul 12, 2017

Choose a reason for hiding this comment

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

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.)

Sorry, something went wrong.

Copy link

Choose a reason for hiding this comment

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

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

t.bigint :old_ems_id
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

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

t.timestamp :deleted_on
t.string :name
Copy link
Contributor

Choose a reason for hiding this comment

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

see above, copying name back should be OK

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

t.string :image
t.string :image_pull_policy
t.string :memory
t.float :cpu_cores
t.belongs_to :container_group, :type => :bigint
t.boolean :privileged
t.bigint :run_as_user
t.boolean :run_as_non_root
t.string :capabilities_add
t.string :capabilities_drop
t.text :command
end

add_column :containers, :container_definition_id, :bigint

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

Choose a reason for hiding this comment

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

This is going to be # containers x 2 queries

Copy link
Member

Choose a reason for hiding this comment

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

maybe temporary column ContainerDefinition#container_id would make sense.

That way, this could be done in 2 queries.

Copy link
Author

Choose a reason for hiding this comment

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

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.

container_def = ContainerDefinition.create(
:ems_id => container.ems_id,
:ems_ref => container.ems_ref,
:old_ems_id => container.old_ems_id,
:deleted_on => container.deleted_on,
:name => container.name,
:image => container.image,
:image_pull_policy => container.image_pull_policy,
:memory => container.memory,
:cpu_cores => container.cpu_cores,
:container_group_id => container.container_group_id,
:privileged => container.privileged,
:run_as_user => container.run_as_user,
:run_as_non_root => container.run_as_non_root,
:capabilities_add => container.capabilities_add,
:capabilities_drop => container.capabilities_drop,
:command => container.command
)
container.update!(:container_definition_id => container_def.id)
end
end
end

containers = Arel::Table.new(:containers)
say_with_time("switch container_definition_id with container_id for container_port_configs") do
port_configs = Arel::Table.new(:container_port_configs)
join_sql = containers.project(containers[:container_definition_id])
.where(containers[:id].eq(port_configs[:container_id])).to_sql
ContainerPortConfig.update_all("container_id = (#{join_sql})")
end

say_with_time("switch container_definition_id with container_id for container_port_configs") do
Copy link
Contributor

Choose a reason for hiding this comment

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

for container_env_vars

env_vars = Arel::Table.new(:container_env_vars)
join_sql = containers.project(containers[:container_definition_id])
.where(containers[:id].eq(env_vars[:container_id])).to_sql
ContainerEnvVar.update_all("container_id = (#{join_sql})")
end

say_with_time("switch container_definition_id with container_id for security_contexts") do
Copy link
Contributor

Choose a reason for hiding this comment

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

again, resource_id and resource_type

security_contexts = Arel::Table.new(:security_contexts)
join_sql = containers.project(containers[:container_definition_id])
.where(containers[:id].eq(security_contexts[:resource_id])
.and(security_contexts[:resource_type].eq(Arel::Nodes::Quoted.new('Container')))).to_sql
SecurityContext.where(:resource_type => 'Container').update_all("resource_type = 'ContainerDefinition', resource_id = (#{join_sql})")
end

# relationships
rename_column :container_port_configs, :container_id, :container_definition_id
rename_column :container_env_vars, :container_id, :container_definition_id

# attributes
remove_column :containers, :image
remove_column :containers, :image_pull_policy
remove_column :containers, :memory
remove_column :containers, :cpu_cores
remove_column :containers, :container_group_id
remove_column :containers, :privileged
remove_column :containers, :run_as_user
remove_column :containers, :run_as_non_root
remove_column :containers, :capabilities_add
remove_column :containers, :capabilities_drop
remove_column :containers, :command
end
end
34 changes: 13 additions & 21 deletions db/schema.yml
Original file line number Diff line number Diff line change
@@ -643,24 +643,6 @@ container_conditions:
- reason
- message
- container_entity_type
container_definitions:
- id
- ems_ref
- name
- image
- image_pull_policy
- memory
- cpu_cores
- container_group_id
- privileged
- run_as_user
- run_as_non_root
- capabilities_add
- capabilities_drop
- command
- deleted_on
- ems_id
- old_ems_id
container_deployment_nodes:
- id
- address
@@ -693,7 +675,7 @@ container_env_vars:
- name
- value
- field_path
- container_definition_id
- container_id
container_groups:
- id
- ems_ref
@@ -804,7 +786,7 @@ container_port_configs:
- port
- host_port
- protocol
- container_definition_id
- container_id
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

- name
container_projects:
- id
@@ -948,7 +930,6 @@ containers:
- name
- backing_ref
- last_perf_capture_on
- container_definition_id
- type
- container_image_id
- reason
@@ -971,6 +952,17 @@ containers:
- request_memory_bytes
- limit_cpu_cores
- limit_memory_bytes
- image
- image_pull_policy
- memory
- cpu_cores
- container_group_id
- privileged
- run_as_user
- run_as_non_root
- capabilities_add
- capabilities_drop
- command
custom_attributes:
- id
- section
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
require_migration

describe UnifyContainerDefinitionAndContainer do
let(:container_stub) { migration_stub(:Container) }
let(:container_definition_stub) { migration_stub(:ContainerDefinition) }

let(:security_context_stub) { migration_stub(:SecurityContext) }
let(:container_env_var_stub) { migration_stub(:ContainerEnvVar) }
let(:container_port_config_stub) { migration_stub(:ContainerPortConfig) }

let(:definition_hash) do
{
:image => "my_image",
:image_pull_policy => "image_pull_policy",
:memory => "memory",
:cpu_cores => 1.1,
:container_group_id => 1,
:privileged => true,
:run_as_user => 2,
:run_as_non_root => true,
:capabilities_add => "capabilities_add",
:capabilities_drop => "capabilities_drop",
:command => "command"
}
end

let(:container_hash) do
{
:name => "mycontainer",
:ems_ref => "123",
:ems_id => 1,
:old_ems_id => 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?]

:deleted_on => Time.now.beginning_of_hour.utc
}
end

migration_context :up do
it "merges container_defintion and container" do
container_def = container_definition_stub.create!(definition_hash)
container = container_stub.create!(container_hash.merge(:container_definition => container_def))
container_env_var_stub.create!(:name => "REGISTRY_HTTP_ADDR",
:value => ":5000",
:container_definition_id => container_def.id)
container_port_config_stub.create!(:ems_ref => "2da0c9e4",
:port => 5000,
:protocol => "TCP",
:container_definition_id => container_def.id)
security_context_stub.create!(:se_linux_level => "s0:c1,c0",
:resource_id => container_def.id,
:resource_type => 'ContainerDefinition')

container_def2 = container_definition_stub.create!(definition_hash.merge(:image => "my_image2"))
container2 = container_stub.create!(container_hash.merge(:container_definition => container_def2, :name => "mycontainer2"))
container_def2.container_env_vars << container_env_var_stub.create!(:name => "REGISTRY_HTTP_ADDR",
:value => ":5000")
container_def2.container_port_configs << container_port_config_stub.create!(:ems_ref => "2da0c9e4",
:port => 5000,
:protocol => "TCP")
security_context_stub.create!(:se_linux_level => "s0:c1,c0",
:resource_id => container_def2.id,
:resource_type => 'ContainerDefinition')

migrate

expect(container_stub.first).to have_attributes(definition_hash.merge(container_hash))

expect(container_port_config_stub.first).to have_attributes(:container_id => container.id)
expect(container_env_var_stub.first).to have_attributes(:container_id => container.id)
expect(security_context_stub.first).to have_attributes(:resource_type => "Container",
:resource_id => container.id)

expect(container_port_config_stub.second).to have_attributes(:container_id => container2.id)
expect(container_env_var_stub.second).to have_attributes(:container_id => container2.id)
expect(security_context_stub.second).to have_attributes(:resource_type => "Container",
:resource_id => container2.id)
end
end

migration_context :down do
it "splits container_definition columns from container" do
container = container_stub.create!(definition_hash.merge(container_hash))
container_env_var_stub.create!(:name => "REGISTRY_HTTP_ADDR",
:value => ":5000", :field_path => nil,
:container_id => container.id)
container_port_config_stub.create!(:ems_ref => "2da0c9e4",
:port => 5000,
:protocol => "TCP",
:container_id => container.id)
security_context_stub.create!(:resource_type => "Container",
:resource_id => container.id,
:se_linux_level => "s0:c1,c0")

migrate

container_def = container_definition_stub.first
expect(container_def).to have_attributes(definition_hash)
expect(container_env_var_stub.first).to have_attributes(:container_definition_id => container_def.id)
expect(container_port_config_stub.first).to have_attributes(:container_definition_id => container_def.id)
expect(security_context_stub.first).to have_attributes(:resource_type => "ContainerDefinition",
:resource_id => container_def.id)
end
end
end