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

Delete archived entities when a container manager is deleted #14359

Merged
merged 1 commit into from
Jun 2, 2017
Merged

Delete archived entities when a container manager is deleted #14359

merged 1 commit into from
Jun 2, 2017

Conversation

zeari
Copy link

@zeari zeari commented Mar 16, 2017 •
edited
Loading

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1458552
Delete archived entities of a container manager when the ems is deleted. This causes various errors in reports that deal with archived entities

@moolitayer @enoodle Please review
cc @simon3z

@miq-bot add_label providers/containers, bug

@zeari zeari changed the title Delete archived entities on ems delete Delete archived entities when a container manager is deleted Mar 16, 2017
@zeari zeari force-pushed the delete_archived_entities_on_ems_delete branch from 3001e82 to f520da0 Compare March 16, 2017 13:46
@zeari zeari force-pushed the delete_archived_entities_on_ems_delete branch from f520da0 to 132148a Compare March 16, 2017 13:48
enoodle
enoodle reviewed Mar 16, 2017
View reviewed changes
app/models/manageiq/providers/container_manager.rb Outdated
@@ -22,6 +22,12 @@ class ContainerManager < BaseManager
has_one :container_deployment, :foreign_key => :deployed_ems_id, :inverse_of => :deployed_ems
has_many :computer_systems, :through => :container_nodes

# Archived entities to destroy when the container manager is deleted
has_many :old_container_groups, :foreign_key => :old_ems_id, :dependent => :destroy
Copy link

Choose a reason for hiding this comment

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

@zeari Maybe I am mistaken but it seems to me that you need to tell it the type of the class to use when it is not clear :class_name => "ContainerGroups"

Copy link
Author

Choose a reason for hiding this comment

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

@enoodle right

moolitayer
moolitayer reviewed Mar 16, 2017
View reviewed changes
app/models/manageiq/providers/container_manager.rb Outdated
# Archived entities to destroy when the container manager is deleted
has_many :old_container_groups, :foreign_key => :old_ems_id, :dependent => :destroy
has_many :old_containers, :foreign_key => :old_ems_id
has_many :old_container_definitions, :foreign_key => :old_ems_id
Copy link

Choose a reason for hiding this comment

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

Where are the two above needed if they have no dependent destroy?

Copy link
Author

@zeari zeari Mar 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@moolitayer destroying container_group that destroys container_definition that destroys container
I thought it was worth mentioning though

Copy link

Choose a reason for hiding this comment

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

So this is for documentation purposes?

Also do think you need

container_group.rb
...
has_many :old_container_definitions, :dependent => :destroy

container_definition.rb
...
has_many :old_containers, :dependent => :destroy ...

?

@zeari zeari force-pushed the delete_archived_entities_on_ems_delete branch 2 times, most recently from 7a4754f to 8eabf4a Compare March 16, 2017 16:19
@moolitayer
Copy link

moolitayer commented Mar 19, 2017 •
edited
Loading

So after some offline discussion with @zeari we came to a conclusion we should remove

has_many :old_containers, :foreign_key => :old_ems_id
has_many :old_container_definitions, :foreign_key => :old_ems_id

As both those entities are removed through hierarchy. For example when a container definition is archived, it remains linked to it's container group (whether the container group is archived or not)
and when the provider is destroyed it will be destroyed as well

@zeari zeari force-pushed the delete_archived_entities_on_ems_delete branch from 8eabf4a to c8ceb77 Compare March 19, 2017 13:35
@zeari
Copy link
Author

zeari commented Mar 19, 2017

So after some offline discussion with @zeari we came to a conclusion we should remove

has_many :old_containers, :foreign_key => :old_ems_id
has_many :old_container_definitions, :foreign_key => :old_ems_id

Done

@miq-bot
Copy link
Member

miq-bot commented Mar 20, 2017

Checked commit zeari@c8ceb77 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 👍

@zeari
Copy link
Author

zeari commented Apr 12, 2017 •
edited
Loading

ping @simon3z
Archived entities get "orphaned" when their provider is deleted

@simon3z
Copy link
Contributor

simon3z commented May 12, 2017

LGTM 👍
@miq-bot add_label ewue/yes
@miq-bot add_label fine/yes

@miq-bot assign blomquisg

@miq-bot
Copy link
Member

miq-bot commented May 12, 2017

@simon3z Cannot apply the following label because they are not recognized: ewue/yes

@miq-bot miq-bot assigned blomquisg and unassigned simon3z May 12, 2017
@simon3z
Copy link
Contributor

simon3z commented May 12, 2017

@miq-bot add_label euwe/yes

@chessbyte chessbyte assigned chessbyte and unassigned blomquisg Jun 2, 2017
@chessbyte chessbyte merged commit 48d0291 into ManageIQ:master Jun 2, 2017
@chessbyte chessbyte added this to the Sprint 62 Ending Jun 5, 2017 milestone Jun 2, 2017
@simaishi
Copy link
Contributor

simaishi commented Jun 2, 2017

@zeari Is there a BZ for this? Can you please create if it doesn't exist?

@zeari
Copy link
Author

zeari commented Jun 4, 2017

@simaishi https://bugzilla.redhat.com/show_bug.cgi?id=1458552

simaishi pushed a commit that referenced this pull request Jun 5, 2017
@chessbyte @simaishi
Merge pull request #14359 from zeari/delete_archived_entities_on_ems_…
d7ed364 
…delete

Delete archived entities when a container manager is deleted
(cherry picked from commit 48d0291)

https://bugzilla.redhat.com/show_bug.cgi?id=1458811
@simaishi
Copy link
Contributor

simaishi commented Jun 5, 2017

Euwe backport details:

$ git log -1
commit d7ed364c18d0f5f5498cc6d78458d128c586d383
Author: Oleg Barenboim <chessbyte@gmail.com>
Date:   Thu Jun 1 23:56:00 2017 -0400

    Merge pull request #14359 from zeari/delete_archived_entities_on_ems_delete
    
    Delete archived entities when a container manager is deleted
    (cherry picked from commit 48d0291098ab39b474cf4712d325d57260372fa0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1458811

simaishi pushed a commit that referenced this pull request Jun 9, 2017
@chessbyte @simaishi
Merge pull request #14359 from zeari/delete_archived_entities_on_ems_…
a2cc7af 
…delete

Delete archived entities when a container manager is deleted
(cherry picked from commit 48d0291)

https://bugzilla.redhat.com/show_bug.cgi?id=1460397
@simaishi
Copy link
Contributor

simaishi commented Jun 9, 2017

Fine backport details:

$ git log -1
commit a2cc7afe7426391b34016632aef4796e1526ba2a
Author: Oleg Barenboim <chessbyte@gmail.com>
Date:   Thu Jun 1 23:56:00 2017 -0400

    Merge pull request #14359 from zeari/delete_archived_entities_on_ems_delete
    
    Delete archived entities when a container manager is deleted
    (cherry picked from commit 48d0291098ab39b474cf4712d325d57260372fa0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460397

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enoodle enoodle enoodle left review comments

@moolitayer moolitayer moolitayer left review comments

@simon3z simon3z simon3z approved these changes

Assignees

@chessbyte chessbyte

Labels
bug euwe/backported fine/backported providers/containers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@zeari @moolitayer @miq-bot @simon3z @simaishi @enoodle @blomquisg @chessbyte