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

[WIP] extend relationships to define the appropriate class names #654

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+26 −49
Open

[WIP] extend relationships to define the appropriate class names #654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 18 additions & 43 deletions app/models/manageiq/providers/openstack/cloud_manager.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,17 @@ class ManageIQ::Providers::Openstack::CloudManager < ManageIQ::Providers::CloudM
:foreign_key => :parent_ems_id,
:class_name => "ManageIQ::Providers::Openstack::NetworkManager",
:autosave => true,
:dependent => :destroy
:dependent => :destroy,
:inverse_of => :parent_manager
has_many :storage_managers,
:foreign_key => :parent_ems_id,
:class_name => "ManageIQ::Providers::StorageManager",
:class_name => "ManageIQ::Providers::Openstack::StorageManager",
:autosave => true,
:dependent => :destroy
:dependent => :destroy,
:auto_save => true,
:inverse_of => :parent_manager
has_many :snapshots, :through => :vms_and_templates

include ManageIQ::Providers::Openstack::CinderManagerMixin
include ManageIQ::Providers::Openstack::SwiftManagerMixin
include ManageIQ::Providers::Openstack::ManagerMixin
Expand Down Expand Up @@ -67,9 +71,12 @@ class ManageIQ::Providers::Openstack::CloudManager < ManageIQ::Providers::CloudM
unsupported_reason_add(:swift_service, "Swift service unavailable") unless openstack_handle.detect_volume_service.name == :swift
end

# TODO: move to after_initialization
before_create :ensure_managers

# TODO: move to before_validation
before_update :ensure_managers_zone_and_provider_region
Copy link
Member

Choose a reason for hiding this comment

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

One thing to keep in mind, the tenant_id is set also in a before_validation callback so we need to make sure those play nicely together. Don't want to be setting the child.tenant_id = tenant_id before tenant_id has been set

https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/tenancy_mixin.rb#L5

# TODO: after fixing inverse_of, this may go away
after_save :refresh_parent_infra_manager

private_class_method def self.provider_id_options
Expand Down Expand Up @@ -445,51 +452,19 @@ def ensure_managers
ensure_network_manager
ensure_cinder_manager
ensure_swift_manager
# TODO: remove when this moves to before_initialization
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean before_validation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should create the objects at initialization time - empty vessels
then in before validation, make sure they have the correct values

I seem to remember that rails could auto build the has_one for us but can't find the references anymore.

But maybe before validation is where we will create these objects too

ensure_managers_zone_and_provider_region
end

def ensure_managers_zone_and_provider_region
if network_manager
network_manager.enabled = enabled
network_manager.zone_id = zone_id
network_manager.tenant_id = tenant_id
network_manager.provider_region = provider_region
end

if cinder_manager
cinder_manager.enabled = enabled
cinder_manager.zone_id = zone_id
cinder_manager.tenant_id = tenant_id
cinder_manager.provider_region = provider_region
end

if swift_manager
swift_manager.enabled = enabled
swift_manager.zone_id = zone_id
swift_manager.tenant_id = tenant_id
swift_manager.provider_region = provider_region
end
# sigh, methods like this always start out innocent enough
def child_manager_references
[network_manager, cinder_manager, swift_manager].compact
end

def ensure_network_manager
build_network_manager unless network_manager
end

def ensure_cinder_manager
build_cinder_manager unless cinder_manager
end

def ensure_swift_manager
build_swift_manager unless swift_manager
end

after_save :save_on_other_managers

def save_on_other_managers
storage_managers.update_all(:tenant_mapping_enabled => tenant_mapping_enabled)
if network_manager
network_manager.tenant_mapping_enabled = tenant_mapping_enabled
network_manager.save!
def ensure_managers_zone_and_provider_region
child_manager_references.each do |child_manager|
Copy link
Member

Choose a reason for hiding this comment

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

If we could work out the list of child managers generically we could do this in core right?

Copy link
Member Author

@kbrock kbrock Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I had child_managers but that messed with an already defined child_managers relation. I'm guessing that is what you are referring.

Were, we want to access the actual child managers that we are using.

If we want to stick with the child_managers association, then we could define swift_manager to be a select into child_managers or something like that?

just having a child managers relation and a different association for swift_manager messes things up.

propagate_child_manager_attributes(child_manager)
child_manager.tenant_mapping_enabled = tenant_mapping_enabled
end
end

Expand Down
14 changes: 8 additions & 6 deletions app/models/manageiq/providers/openstack/infra_manager.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,17 @@ class ManageIQ::Providers::Openstack::InfraManager < ManageIQ::Providers::InfraM

include ManageIQ::Providers::Openstack::ManagerMixin
include HasManyOrchestrationStackMixin
include HasNetworkManagerMixin
include HasNetworkManagerMixin # TODO: also included in parent class

has_one :network_manager,
:foreign_key => :parent_ems_id,
:class_name => "ManageIQ::Providers::Openstack::NetworkManager",
:autosave => true,
:dependent => :destroy

before_save :ensure_parent_provider
before_destroy :destroy_parent_provider
before_create :ensure_managers
before_create :ensure_managers # defined by HasNetworkManagerMixin
before_update :ensure_managers_zone_and_provider_region

supports :create
Expand Down Expand Up @@ -286,10 +292,6 @@ def self.params_for_create
}
end

def ensure_network_manager
build_network_manager(:type => 'ManageIQ::Providers::Openstack::NetworkManager') unless network_manager
end

def allow_targeted_refresh?
false
end
Expand Down
Loading