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

Changes cloud network list to follow availability zone rules #16688

Merged
merged 2 commits into from
Jan 10, 2018
Merged

Changes cloud network list to follow availability zone rules #16688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2784dce
Changes cloud network list to follow availability zone rules per http…
d-m-u Dec 19, 2017
5fa2fce
Adds tests for allowed cloud networks
d-m-u Jan 9, 2018
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
28 changes: 20 additions & 8 deletions app/models/manageiq/providers/cloud_manager/provision_workflow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,6 @@ def allowed_availability_zones(_options = {})
targets.each_with_object({}) { |az, h| h[az.id] = az.name }
end

def allowed_cloud_networks(_options = {})
return {} unless (src_obj = provider_or_tenant_object)

src_obj.all_cloud_networks.each_with_object({}) do |cn, hash|
hash[cn.id] = cn.cidr.blank? ? cn.name : "#{cn.name} (#{cn.cidr})"
end
end

def allowed_cloud_subnets(_options = {})
src = resources_for_ui
return {} if src[:cloud_network_id].nil?
Expand All @@ -33,6 +25,12 @@ def allowed_cloud_subnets(_options = {})
end
end

def allowed_cloud_networks(_options = {})
source = load_ar_obj(get_source_vm)
targets = get_targets_for_source(source, :cloud_filter, CloudNetwork, 'cloud_network_id')
allowed_ci(:cloud_network, [:availability_zone], targets.map(&:id))
Copy link
Contributor

Choose a reason for hiding this comment

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

@d-m-u OpenStack CI is now down, most probably because of this line, can you please run all cloud providers specs locally? I think we will need to do this change only in AWS code, to avoid breaking others.

Copy link
Contributor

@Ladas Ladas Jan 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@bronaghs this has probably broken also Azure (there is no spec for allowed_cloud_networks in azure), @d-m-u you have probably missed my comment #16688 (review) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ladas I think if you look at https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/cloud_manager/provision_workflow.rb#L30 you'll see that cloud_network_id is not the line that got merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

@d-m-u right I mean the allowed_ci(:cloud_network, [:availability_zone], targets.map(&:id)), for OpenStack it's always returning blank set. The same case might be for the other providers, but the specs are probably missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@d-m-u I haven't tried if this breaks the actual Provision dialog, so that is probably a thing for testing for all clouds. But I don't see that we have a relation of AvailabilityZone to cloud_subnets, while AvailabilityZone is being picked in the UI.

So this is being invoked https://github.com/Ladas/manageiq/blob/59ba714ccb71b2eb3cdd46157587e85329af4f3b/app/models/manageiq/providers/cloud_manager/provision_workflow.rb#L92 and always returns blank result.

I don't see AZ associated to subnet in refresh also in Google and Vcloud. Azure seems to have it.

Copy link
Contributor

@Ladas Ladas Jan 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

so back to my comment #16688 (comment)

filters for specific provider should reside in the provider code, if the behavior is not common to all of them. In this case the Amazon workflow file would have

def allowed_cloud_networks(_options = {})
  targets = super
  allowed_ci(:cloud_network, [:availability_zone], targets.map(&:id))
end

and the generic code for all cloud providers would not have allowed_ci(:cloud_network, [:availability_zone], targets.map(&:id))

end

def allowed_guest_access_key_pairs(_options = {})
source = load_ar_obj(get_source_vm)
targets = get_targets_for_ems(source, :cloud_filter, ManageIQ::Providers, 'key_pairs')
Expand Down Expand Up @@ -90,6 +88,20 @@ def allowed_ci(ci, relats, filtered_ids = nil)
super(ci, relats, sources, filtered_ids)
end

def availability_zone_to_cloud_network(src)
Copy link
Contributor

Choose a reason for hiding this comment

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

there seems to be a mapping, other way around in https://github.com/Ladas/manageiq-providers-amazon/blob/6b6cb06d25bd5d38e1c700fd34fcd87a6efec824/app/models/manageiq/providers/amazon/cloud_manager/provision_workflow.rb#L75

Anyway. This seems like Amazon specific code, so this should belong to amazon/cloud_manager/provision_workflow.rb. @bronaghs or do we have similar behavior for e.g. azure?

if src[:availability_zone]
load_ar_obj(src[:availability_zone]).cloud_subnets.each_with_object({}) do |cs, hash|
Copy link
Contributor

Choose a reason for hiding this comment

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

@d-m-u - Would there be a need for RBAC'd cloud_subnets here and on line 99?

Copy link
Member

Choose a reason for hiding this comment

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

@d-m-u @syncrou The caller passes in an rbac filtered list of cloud_networks here so there is no need for additional checking at this level. Only the IDs from the initial filtered list are possible return values.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gmcculloug - Ahh - missed the get_targets_for_source that built out the targets list.

cn = cs.cloud_network
hash[cn.id] = cn.name
end
else
return {} unless load_ar_obj(src[:ems]).cloud_subnets
load_ar_obj(src[:ems]).cloud_subnets.collect(&:cloud_network).each_with_object({}) do |cn, hash|
hash[cn.id] = cn.name
end
end
end

def get_source_and_targets(refresh = false)
Copy link

Choose a reason for hiding this comment

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

@d-m-u - from reading this method it sounds like this loads the cloud_networks associated with the availability zone of the source which is contrary to the PR description, yay or nay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Fixed. Sorry.

return @target_resource if @target_resource && refresh == false
result = super
Expand Down
12 changes: 12 additions & 0 deletions spec/models/manageiq/providers/cloud_manager/provision_workflow_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,18 @@
:ext_management_system => ems.network_manager)
end

context "#allowed_cloud_networks" do
it "without a zone", :skip_before do
expect(workflow.allowed_cloud_networks.length).to be_zero
end

it "with a zone" do
workflow.values[:placement_availability_zone] = [@az1.id, @az1.name]
expect(workflow.allowed_cloud_networks.length).to eq(1)
expect(workflow.allowed_cloud_networks).to eq(@cn1.id => @cn1.name)
end
end

context "#allowed_cloud_subnets" do
it "without a cloud_network", :skip_before do
expect(workflow.allowed_cloud_subnets.length).to be_zero
Expand Down