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

Clean up mapped tenants after a CloudManager is destroyed #17866

Merged

Conversation

mansam
Copy link
Contributor

@mansam mansam commented Aug 15, 2018

Tenants created by cloud tenant mapping are not destroyed when their provider is destroyed, which prevents tenant mapping from working if the provider is readded with the same name. This PR causes the mapped tenant tree to be destroyed along with the provider.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1547740

@mansam mansam force-pushed the cleanup-mapped-tenants-after-ems-destroy branch from 526b7d0 to b1c8d42 Compare August 23, 2018 19:03
@mansam
Copy link
Contributor Author

mansam commented Oct 3, 2018

@agrare do you know who could review this?

@@ -42,6 +42,8 @@ class << model_name
include HasNetworkManagerMixin
include HasManyOrchestrationStackMixin

after_destroy { |record| record.destroy_mapped_tenants }
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to just do after_destroy :destroy_mapped_tenants

def destroy_mapped_tenants
if source_tenant
source_tenant.all_subtenants.destroy_all
source_tenant.all_subprojects.destroy_all
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for these to be shared by any other providers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe so, the mappings should be unique to one provider as far as I understand it.

@agrare
Copy link
Member

agrare commented Oct 3, 2018

@mansam could you just add a :dependent => :destroy to the has_one :source_tenant and let cascade deletion take care of the rest?

@mansam
Copy link
Contributor Author

mansam commented Oct 3, 2018

@agrare I'll give that a shot, I hadn't considered it.

@mansam mansam force-pushed the cleanup-mapped-tenants-after-ems-destroy branch from b1c8d42 to f58bfc9 Compare October 8, 2018 21:23
@mansam
Copy link
Contributor Author

mansam commented Oct 8, 2018

Turns out I had actually already tried that when I first worked on this in August and just forgot. Trying to use :dependent => :destroy hits a constraint on destroying mapped tenants, hence having to destroy the subtenants first. I'm not sure if there would be a better way to get around this.

     Failure/Error: raise _("A tenant created by tenant mapping cannot be deleted") if source
     
     RuntimeError:
       A tenant created by tenant mapping cannot be deleted
     # ./app/models/tenant.rb:330:in `ensure_can_be_destroyed'
     # ./app/models/ext_management_system.rb:496:in `destroy'
     # ./spec/models/manageiq/providers/cloud_manager_spec.rb:200:in `block (5 levels) in <top (required)>'

@miq-bot
Copy link
Member

miq-bot commented Oct 12, 2018

Checked commit mansam@f58bfc9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@agrare agrare self-assigned this Oct 12, 2018
@agrare
Copy link
Member

agrare commented Oct 12, 2018

Hm okay so you'd have to have some before_destroy to clear the sub-tenants first, and there isn't an actual relation for the sub tenants so I was hoping we could dependent destroy those as well but I guess not.

@agrare agrare closed this Oct 15, 2018
@agrare agrare reopened this Oct 15, 2018
@mansam
Copy link
Contributor Author

mansam commented Oct 17, 2018

@miq-bot add_label hammer/yes

@agrare agrare merged commit 89a1ebf into ManageIQ:master Oct 17, 2018
@agrare agrare added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 17, 2018
@agrare
Copy link
Member

agrare commented Oct 17, 2018

Sorry @mansam I kicked the tests then forgot to come back and check if it passed

@mansam
Copy link
Contributor Author

mansam commented Oct 17, 2018

@agrare no worries, I figured you'd get to it eventually :) thanks for the merge!

simaishi pushed a commit that referenced this pull request Oct 17, 2018
…s-destroy

Clean up mapped tenants after a CloudManager is destroyed

(cherry picked from commit 89a1ebf)

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

Hammer backport details:

$ git log -1
commit 051d05f4ea7f061721720b8202bd3b7a96ec6dce
Author: Adam Grare <[email protected]>
Date:   Wed Oct 17 14:26:15 2018 -0400

    Merge pull request #17866 from mansam/cleanup-mapped-tenants-after-ems-destroy
    
    Clean up mapped tenants after a CloudManager is destroyed
    
    (cherry picked from commit 89a1ebf0dadecf532b557f74adbb68dce82c1dae)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1547740

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants