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

Jump to bottom

Soft delete instead of disconnect for containers models #15251

Merged
merged 9 commits into from
Jul 5, 2017
Merged

Soft delete instead of disconnect for containers models #15251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0ec229a
Scope ems relations to show only :deleted => false
Ladas May 30, 2017
719c919
Do soft delete instead of disconnecting ems
Ladas May 30, 2017
606b675
Add scopes for deleted not deleted to the archived mixing
Ladas May 30, 2017
050f1f9
Alias archived? to deleted?
Ladas May 30, 2017
1295547
Use :active, :archived scopes and methods for soft-delete
Ladas Jun 5, 2017
50b4153
Move dependent destroy to _all relations
Ladas Jun 6, 2017
7d72385
Remove duplicate :container_definitions relation
Ladas Jun 6, 2017
44ef947
Use deleted boolean for active/archived scopes
Ladas Jun 7, 2017
d4448cb
Keep :deleted_on as the only only source of truth for disconnect
Ladas Jun 22, 2017
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
2 changes: 0 additions & 2 deletions app/models/container.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@ def disconnect_inv
return if ems_id.nil?
_log.info "Disconnecting Container [#{name}] id [#{id}] from EMS "
self.deleted_on = Time.now.utc
self.old_ems_id = self.ems_id
self.ems_id = nil
save
end
end
2 changes: 0 additions & 2 deletions app/models/container_definition.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ def disconnect_inv
_log.info "Disconnecting Container definition [#{name}] id [#{id}]"
self.container.try(:disconnect_inv)
self.deleted_on = Time.now.utc
self.old_ems_id = self.ems_id
self.ems_id = nil
save
end
end
2 changes: 0 additions & 2 deletions app/models/container_group.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,6 @@ def disconnect_inv
_log.info "Disconnecting Pod [#{name}] id [#{id}] from EMS [#{ext_management_system.name}]" \
"id [#{ext_management_system.id}] "
self.container_definitions.each(&:disconnect_inv)
self.old_ems_id = ems_id
self.ext_management_system = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

A few lines below, it still does

self.old_container_project_id = self.container_project_id
self.container_project_id = nil

I suppose for consistency we'll also want to stop that and retain the normal relation, just with one or both sides archived. And project.container_groups association would default to -> { active } to preserve current behavior.
@zakiva @zeari what do you think? Is this the desired end state?

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked with @zakiva, we're also clearing self.container_node_id = nil which will make less sense after ManageIQ/manageiq-schema#22 starts archiving nodes.
I think let's get both this and ManageIQ/manageiq-schema#22 merged, then revisit these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It make sense to unify the behavior and not delete the foreign keys. It will be a zombie-free land at some point :-)

self.container_node_id = nil
self.container_services = []
self.container_replicator_id = nil
Expand Down
2 changes: 0 additions & 2 deletions app/models/container_image.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,6 @@ def disconnect_inv
_log.info "Disconnecting Image [#{name}] id [#{id}] from EMS [#{ext_management_system.name}]" \
"id [#{ext_management_system.id}] "
self.container_image_registry = nil
self.old_ems_id = ems_id
self.ext_management_system = nil
self.deleted_on = Time.now.utc
save
end
Expand Down
2 changes: 0 additions & 2 deletions app/models/container_project.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ def disconnect_inv
return if ems_id.nil?
_log.info "Disconnecting Container Project [#{name}] id [#{id}] from EMS [#{ext_management_system.name}]" \
"id [#{ext_management_system.id}] "
self.old_ems_id = ems_id
self.ext_management_system = nil
self.deleted_on = Time.now.utc
save
end
Expand Down
21 changes: 12 additions & 9 deletions app/models/manageiq/providers/container_manager.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,16 @@ class ContainerManager < BaseManager
include SupportsFeatureMixin

has_many :container_nodes, :foreign_key => :ems_id, :dependent => :destroy
has_many :container_groups, :foreign_key => :ems_id, :dependent => :destroy
has_many :container_groups, -> { active }, :foreign_key => :ems_id
has_many :container_services, :foreign_key => :ems_id, :dependent => :destroy
has_many :container_replicators, :foreign_key => :ems_id, :dependent => :destroy
has_many :containers, :foreign_key => :ems_id
has_many :container_projects, :foreign_key => :ems_id, :dependent => :destroy
has_many :containers, -> { active }, :foreign_key => :ems_id
has_many :container_definitions, -> { active }, :foreign_key => :ems_id
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cben you've defined has_many :container_definitions, :through => :container_groups in the https://github.com/ManageIQ/manageiq/pull/15298/files

but there is :ems_id on the container_definitions

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. Both old and graph refresh set this ems_id, we have a test. Please replace my definion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in bb2f07a

Copy link
Member

Choose a reason for hiding this comment

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

are you intentionally removing the :dependent => :destroy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbrock yes that is defined on all_xy relations, that include also soft-deleted items

has_many :container_projects, -> { active }, :foreign_key => :ems_id
has_many :container_quotas, :foreign_key => :ems_id, :dependent => :destroy
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 :container_images, -> { active }, :foreign_key => :ems_id
has_many :persistent_volumes, :as => :parent, :dependent => :destroy
has_many :persistent_volume_claims, :foreign_key => :ems_id, :dependent => :destroy
has_many :container_component_statuses, :foreign_key => :ems_id, :dependent => :destroy
Expand All @@ -26,7 +27,6 @@ class ContainerManager < BaseManager
has_many :computer_system_hardwares, :through => :computer_systems, :source => :hardware
has_many :computer_system_operating_systems, :through => :computer_systems, :source => :operating_system
has_many :container_volumes, :through => :container_groups
has_many :container_definitions, :through => :container_groups
has_many :container_port_configs, :through => :container_definitions
has_many :container_env_vars, :through => :container_definitions
has_many :security_contexts, :through => :container_definitions
Expand All @@ -36,10 +36,13 @@ class ContainerManager < BaseManager
has_many :container_limit_items, :through => :container_limits
has_many :container_template_parameters, :through => :container_templates

# Archived entities to destroy when the container manager is deleted
has_many :old_container_groups, :foreign_key => :old_ems_id, :dependent => :destroy, :class_name => "ContainerGroup"
has_many :old_container_projects, :foreign_key => :old_ems_id, :dependent => :destroy, :class_name => "ContainerProject"
has_many :old_container_images, :foreign_key => :old_ems_id, :dependent => :destroy, :class_name => "ContainerImage"
# Archived and active entities to destroy when the container manager is deleted
has_many :all_containers, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "Container"
has_many :all_container_groups, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "ContainerGroup"
has_many :all_container_projects, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "ContainerProject"
has_many :all_container_images, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "ContainerImage"
has_many :all_container_definitions, :foreign_key => :ems_id, :dependent => :destroy, :class_name => "ContainerDefinition"


virtual_column :port_show, :type => :string

Expand Down
9 changes: 8 additions & 1 deletion app/models/mixins/archived_mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,18 @@ module ArchivedMixin
extend ActiveSupport::Concern

included do
scope :archived, -> { where.not(:deleted_on => nil) }
scope :active, -> { where(:deleted_on => nil) }

belongs_to :old_ext_management_system, :foreign_key => :old_ems_id, :class_name => 'ExtManagementSystem'
end

def archived?
ems_id.nil?
!active?
end

def active?
deleted_on.nil?
end

# Needed for metrics
Expand Down