Changes cloud network list to follow availability zone rules #16688

d-m-u · 2017-12-19T17:45:54Z

Previously, cloud network list was only updated after selection of security group, or change of tab, or a select set of other actions that completely voided the list of parameters passed to the filtering method. This adds a change so that availability zone updates the selection of cloud network, preventing incorrect cloud networks from showing up in the after the zone dropdown changes.

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

d-m-u · 2017-12-19T17:46:47Z

@miq-bot assign @gmcculloug

d-m-u · 2017-12-19T17:46:58Z

@miq-bot add_label bug

syncrou · 2017-12-19T21:57:37Z

app/models/manageiq/providers/cloud_manager/provision_workflow.rb

@@ -31,6 +23,12 @@ def allowed_cloud_subnets(_options = {})
    end
  end

+  def allowed_cloud_networks(_options = {})
+    source = load_ar_obj(get_source_vm)
+    targets = get_targets_for_ems(source, :cloud_filter, CloudNetwork, 'cloud_networks')


@d-m-u - I think you may just need to use get_targets_for_source here as opposed to the _ems variant.

The original code iterated off the src_obj without the need to lookup the ems so you shouldn't need to lookup the ems with this fix as well.

bronaghs · 2017-12-20T20:48:13Z

app/models/manageiq/providers/cloud_manager/provision_workflow.rb

+      end
+    end
+  end
+
  def get_source_and_targets(refresh = false)


@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?

Yeah. Fixed. Sorry.

syncrou · 2017-12-21T20:21:29Z

@d-m-u - If you want - you can add tests now here.

d-m-u · 2017-12-21T20:23:11Z

@syncrou cool, thanks! can this wait till i'm back?

Ladas · 2018-01-02T11:02:31Z

app/models/manageiq/providers/cloud_manager/provision_workflow.rb

@@ -90,6 +88,19 @@ def allowed_ci(ci, relats, filtered_ids = nil)
    super(ci, relats, sources, filtered_ids)
  end

+  def availability_zone_to_cloud_network(src)


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?

…s://bugzilla.redhat.com/show_bug.cgi?id=1518847

d-m-u · 2018-01-09T14:17:01Z

@syncrou can ya take a look at this please for me?

miq-bot · 2018-01-09T14:21:43Z

Checked commits d-m-u/manageiq@2784dce~...5fa2fce with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

syncrou · 2018-01-09T16:52:40Z

app/models/manageiq/providers/cloud_manager/provision_workflow.rb

@@ -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)
+    if src[:availability_zone]
+      load_ar_obj(src[:availability_zone]).cloud_subnets.each_with_object({}) do |cs, hash|


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

@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.

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

syncrou

👍 LGTM

Changes cloud network list to follow availability zone rules (cherry picked from commit c11b14e) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533277

simaishi · 2018-01-10T23:27:40Z

Gaprindashvili backport details:

$ git log -1
commit 68f3d0c9abaf092577bee21669777c0fee07259a
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed Jan 10 14:03:44 2018 -0500

    Merge pull request #16688 from d-m-u/bz_1518847
    
    Changes cloud network list to follow availability zone rules
    (cherry picked from commit c11b14e956902f79be68ae457d5b98a3d574db93)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533277

Ladas · 2018-01-15T09:02:49Z

app/models/manageiq/providers/cloud_manager/provision_workflow.rb

+  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))


@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.

@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) ?

@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.

@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.

@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.

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))

d-m-u · 2018-01-15T13:43:56Z

@Ladas GM and I and @syncrou had whole big discussion about this and the outcome of that was that this code isn't specific to a particular provider.

Ladas · 2018-01-15T13:56:57Z

@d-m-u seems like OpenStack networks support AvailabilityZones now https://developer.openstack.org/api-ref/network/v2/index.html#show-network-details , but it's has_many relation to network, not belongs to subnet. So probably a thing for OpenStack team to extend cc @aufi , although we need to extend the model.

Anyway at minimum, OpenStack behavior is specific. I am not 100% sure about others, not sure if we tested impact on all providers changing this shared code?

d-m-u changed the title ~~Changes cloud network list to follow availability zone rules~~ [WIP] Changes cloud network list to follow availability zone rules Dec 19, 2017

miq-bot assigned gmcculloug Dec 19, 2017

miq-bot added bug wip labels Dec 19, 2017

syncrou suggested changes Dec 19, 2017

View reviewed changes

d-m-u force-pushed the bz_1518847 branch 2 times, most recently from 85ff45a to 079c8fb Compare December 19, 2017 22:25

d-m-u mentioned this pull request Dec 20, 2017

[WIP] Adds spec for cloud network with availability zone ManageIQ/manageiq-providers-amazon#380

Closed

d-m-u changed the title ~~[WIP] Changes cloud network list to follow availability zone rules~~ Changes cloud network list to follow availability zone rules Dec 20, 2017

miq-bot removed the wip label Dec 20, 2017

bronaghs reviewed Dec 20, 2017

View reviewed changes

Ladas reviewed Jan 2, 2018

View reviewed changes

d-m-u added 2 commits January 9, 2018 09:15

Changes cloud network list to follow availability zone rules per http…

2784dce

…s://bugzilla.redhat.com/show_bug.cgi?id=1518847

Adds tests for allowed cloud networks

5fa2fce

d-m-u force-pushed the bz_1518847 branch from 079c8fb to 5fa2fce Compare January 9, 2018 14:16

syncrou reviewed Jan 9, 2018

View reviewed changes

syncrou approved these changes Jan 10, 2018

View reviewed changes

gmcculloug merged commit c11b14e into ManageIQ:master Jan 10, 2018

gmcculloug added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 10, 2018

gmcculloug added lifecycle/provisioning gaprindashvili/yes labels Jan 10, 2018

d-m-u deleted the bz_1518847 branch January 10, 2018 19:06

simaishi pushed a commit that referenced this pull request Jan 10, 2018

Merge pull request #16688 from d-m-u/bz_1518847

68f3d0c

Changes cloud network list to follow availability zone rules (cherry picked from commit c11b14e) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533277

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 10, 2018

This was referenced Jan 11, 2018

[WIP] Fixes logic for allowed_cloud_networks #16804

Closed

Change get_targets_for_source to use correct params #16811

Merged

Add cloud_subnets to availability zone test ManageIQ/manageiq-content#241

Merged

Ladas reviewed Jan 15, 2018

View reviewed changes

lfu mentioned this pull request Jan 26, 2018

Add back the missing IP address range in Virtual Private Cloud name. #16898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes cloud network list to follow availability zone rules #16688

Changes cloud network list to follow availability zone rules #16688

d-m-u commented Dec 19, 2017 •

edited

Loading

d-m-u commented Dec 19, 2017

d-m-u commented Dec 19, 2017

syncrou Dec 19, 2017

bronaghs Dec 20, 2017

d-m-u Dec 20, 2017

syncrou commented Dec 21, 2017 •

edited

Loading

d-m-u commented Dec 21, 2017

Ladas Jan 2, 2018

d-m-u commented Jan 9, 2018

miq-bot commented Jan 9, 2018

syncrou Jan 9, 2018

gmcculloug Jan 9, 2018

syncrou Jan 10, 2018

syncrou left a comment

simaishi commented Jan 10, 2018

Ladas Jan 15, 2018

Ladas Jan 15, 2018 •

edited

Loading

d-m-u Jan 15, 2018

Ladas Jan 15, 2018

Ladas Jan 15, 2018

Ladas Jan 15, 2018 •

edited

Loading

d-m-u commented Jan 15, 2018

Ladas commented Jan 15, 2018 •

edited

Loading

Changes cloud network list to follow availability zone rules #16688

Changes cloud network list to follow availability zone rules #16688

Conversation

d-m-u commented Dec 19, 2017 • edited Loading

d-m-u commented Dec 19, 2017

d-m-u commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syncrou commented Dec 21, 2017 • edited Loading

d-m-u commented Dec 21, 2017

Choose a reason for hiding this comment

d-m-u commented Jan 9, 2018

miq-bot commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syncrou left a comment

Choose a reason for hiding this comment

simaishi commented Jan 10, 2018

Choose a reason for hiding this comment

Ladas Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

d-m-u commented Jan 15, 2018

Ladas commented Jan 15, 2018 • edited Loading

d-m-u commented Dec 19, 2017 •

edited

Loading

syncrou commented Dec 21, 2017 •

edited

Loading

Ladas Jan 15, 2018 •

edited

Loading

Ladas Jan 15, 2018 •

edited

Loading

Ladas commented Jan 15, 2018 •

edited

Loading