Fix Nuage provider update #3337

sasoc · 2018-01-30T10:52:44Z

There were two problems when editing Nuage network provider. If
network that you edited had no event, there would still be AMQP
selected under the Events tab. That made Save button disabled
since hostname must not be empty and everything has to be verified.
Now None is selected in case of no event.
The second problem was that when a network was updated error occurred
while redirecting, since you would only get one argument and
ems_path required two. Now ems_path works same as
in the cloud provider controller.

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

sasoc · 2018-01-30T10:52:57Z

cc @miha-plesko

miha-plesko

@sasoc please see the two comments I have, otherwise it looks great and I can confirm it works as expected. Thanks!

miha-plesko · 2018-01-30T14:19:06Z

app/controllers/mixins/ems_common_angular.rb

+        return 'none' if amqp_auth.nil?
+        return 'amqp'
+      end
+      'ceilometer'


I don't like that 'ceilometer' is fallback when nothing else is true, can you please reorganize function to following shape:

return 'amqp' if ... return 'ceilometer' if ... return 'none'

Also, please add unit test for this function, e.g. in a new file spec/controllers/mixins/ems_common_angular_spec.rb.

miha-plesko · 2018-02-01T12:00:36Z

app/controllers/mixins/ems_common_angular.rb

-      return "amqp" if @ems.connection_configurations.ceilometer.try(:endpoint).nil? && @ems.connection_configurations.amqp.try(:endpoint)
-      "ceilometer"
+      amqp_auth = @ems.authentications.detect { |a| a.authtype == 'amqp' }
+      return 'amqp' if @ems.connection_configurations.ceilometer.try(:endpoint).nil? && @ems.connection_configurations.amqp.try(:endpoint) && !amqp_auth.nil?


return 'amqp' if @ems.connection_configurations.amqp&.endpoint&.hostname&.present?

I don't think we need to check both auth and endpoint, it should suffice to have a valid hostname where to connect to, please see example above.

miha-plesko · 2018-02-01T12:05:56Z

app/controllers/mixins/ems_common_angular.rb

-      "ceilometer"
+      amqp_auth = @ems.authentications.detect { |a| a.authtype == 'amqp' }
+      return 'amqp' if @ems.connection_configurations.ceilometer.try(:endpoint).nil? && @ems.connection_configurations.amqp.try(:endpoint) && !amqp_auth.nil?
+      return 'ceilometer' if @ems.connection_configurations.ceilometer.try(:endpoint) && @ems.connection_configurations.amqp.try(:endpoint).nil?


return 'ceilometer' if @ems.connection_configurations.ceilometer&.endpoint&.hostname&.present?

miha-plesko · 2018-02-01T12:14:21Z

spec/controllers/mixins/ems_common_angular_spec.rb

+    let(:ems_nuage) { FactoryGirl.create(:ems_nuage_network_with_authentication) }
+
+    it 'when amqp' do
+      ems_nuage.endpoints << Endpoint.create(:role => 'amqp')


Endpoint.create(:role => 'amqp', hostname => 'hostname')

miha-plesko · 2018-02-01T12:18:32Z

spec/controllers/mixins/ems_common_angular_spec.rb

+      expect(network_controller.send(:retrieve_event_stream_selection)).to eq('none')
+    end
+  end
+end


Can you please add also following tests:

AMQP when ceilometer has empty hostname

CEILOMETER when amqp has empty hostname

NONE when amqp and ceilometer have nil endpoints

NONE when amqp and cailometer have empty hostnames

sasoc · 2018-02-01T13:42:33Z

@miha-plesko I've committed changes. Can you take a look if I understood and implemented additional tests as you imagined.

miha-plesko

@sasoc thanks, yes, I had such additional tests in mind. Please apply the one little comment that I have now, then its LGTM

miha-plesko · 2018-02-01T14:35:20Z

spec/controllers/mixins/ems_common_angular_spec.rb

+      expect(network_controller.send(:retrieve_event_stream_selection)).to eq('amqp')
+    end
+
+    it 'none when amqp and ceilometer have nil endpoints' do


This is the very same as we already have in L51, please remove it.

miha-plesko · 2018-02-01T14:40:59Z

@AparnaKarve I kindly ask for a review/merge. The problem that we're addressing here is blocking integration tests for Nuage provider, because simple CRUD operations fail due to bug on the UI. Bug was there for a long time now, but no one noticed...

@miq-bot assign @AparnaKarve
@miq-bot add_label enhancement

/cc @juliancheal

miha-plesko

LGTM

AparnaKarve · 2018-02-02T02:59:11Z

@miha-plesko Need a little clarification here.
Is this PR applicable to Nuage Network Providers only?
Or is it applicable to Nuage and Amazon? ( I believe in the BZ you have mentioned Amazon)

Incidentally, I can open the edit page for Amazon Network Provider (even without this PR)
But the form does not populate all the fields.

miha-plesko · 2018-02-02T07:56:20Z

@AparnaKarve I think the "Edit NetworkProvider" option should be disabled for any other provider but Nuage. Probably we've accidentaly enabled it for all providers when we were integrating Nuage.

We've also tested if editing works for Amazon NetworkManager but it didn't (it's broken on many places, as you've correctly noticed) therefore we were brave enough to assume def ems_path(*args) in ems_network_controller.rb is obsolete because no one has used it for a while now - hence this PR.

May I suggest following procedure:

this PR: make Nuage NetworkManager editing work
followup PR: disable editing for all other NetworkManagers but Nuage

Wdyt?

P.S.: Not sure how familiar are you with Nuage provider, so let me quickly put some context for you. Nuage Provider is "one of a kind" among providers because it no CloudManager, only NetworkManager. Other providers (like Amazon) only allow you to add a new CloudProvider and then Network-, Storage-, ... managers are added automatically (you are not even able to edit them). When you remove Amazon CloudProvider, also all its Managers are removed. Well, this is not the case with Nuage, where there is no CloudProvider, only a standalone NetworkManager -> and that's the source of our problem, because UI does not assume Manager editing. Luckily, the fixes seem to be quite simple 😄

Please let me know if this makes any sense.

AparnaKarve · 2018-02-03T00:08:31Z

app/controllers/mixins/ems_common_angular.rb

-      "ceilometer"
+      return 'amqp' if @ems.connection_configurations.amqp&.endpoint&.hostname&.present?
+      return 'ceilometer' if @ems.connection_configurations.ceilometer&.endpoint&.hostname&.present?
+      'none'


Noticed that a hostname&.present? condition has been added which did not exist before.
Should not matter for OpenStack Cloud/Infra Provider I think (where this method is used), but just wanted to know if it's absolutely needed for Nuage?

Could you post a screenshot of the Nuage Edit Network Provider page?
Does it have AMQP/Ceilometer Radio buttons too?

I think the above change may affect how we display the Radio buttons for OpenStack.

OpenStack selects the Ceilometer radiobutton by default if the AMQP endpoint does not exist.
But with the above change, there is an explicit check for the Ceilometer endpoint.
If both AMQP and Ceilometer endpoints do not exist, none of the radiobuttons would be selected -- which is not what we want.

Let's keep the OpenStack UI unaffected by this change.

AparnaKarve · 2018-02-03T00:21:47Z

@miha-plesko Thanks for the detailed explanation above - it did make sense :)

I have a few inline comments.

There were two problems when editing Nuage network provider. If network that you edited had no event, there would still be AMQP selected under the Events tab. That made Save button disabled since hostname must not be empty and everything has to be verified. Now None is selected in case of no event. The second problem was that when a network was updated error occurred while redirecting, since you would only get one argument and `ems_path` required two. Now `ems_path` works same as in the cloud provider controller. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1534457

sasoc · 2018-02-06T10:43:50Z

@AparnaKarve hostname&.present? was added because there is a bug when you create new Nuage network. When created without endpoints there is still endpoint added to network, which has empty attributes. And before update retrieve_event_stream_selection would return 'amqp' and amqp radio button would be selected in edit page. And since all attributes are empty and
form fields are required, you would have to go to events tab and select none so you would be allowed to save changes(as seen in screenshot). Which creates some unnecessary work.

I've changed retrieve_event_stream_selection to return ceilometer by default for OpenStack. I've also added test for this usecase.

Screenshot of Nuage network edit page, where you can see that there is oly AMQP radio button:

miq-bot · 2018-02-06T10:49:13Z

Checked commit sasoc@c525ab7 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

miha-plesko

I've tested this again (for Nuage provider) and it works like a charm, thanks @sasoc.

@AparnaKarve unfortuantely I don't have OpenStack testbed at hand to test if everything works also there, but looking at implementation I'm confident it should. I'm sorry to push you, but may I ask for another pass of review? We really need to be able to update Nuage provider from GUI in integration tests and we're catching 5.9.1 code-freeze date...

AparnaKarve · 2018-02-09T22:41:21Z

@sasoc @miha-plesko This looks good now.

@dclarizio Can we merge this?

AparnaKarve · 2018-02-09T22:57:21Z

@miha-plesko Regarding the follow-up PR as mentioned in #3337 (comment) -- is that something you would be able to address?
Let us know. Thanks.

miha-plesko · 2018-02-10T10:42:26Z

Thanks for merging. @AparnaKarve yes, let me prepare followup PR for you. I think it should be a matter of a single IF statement, I'll take a look on Monday.

Fix Nuage provider update (cherry picked from commit 02e1a70) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1552895

simaishi · 2018-03-08T12:47:27Z

Gaprindashvili backport details:

$ git log -1
commit 7bc1af0116223f5ff9eb5aa239ac4b0ba77b9621
Author: Dan Clarizio <dclarizi@redhat.com>
Date:   Fri Feb 9 14:52:50 2018 -0800

    Merge pull request #3337 from sasoc/fix-nuage-provider-update
    
    Fix Nuage provider update
    (cherry picked from commit 02e1a7098b58cff9b7cc309b18e5fda7fe56e17e)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1552895

miha-plesko suggested changes Jan 30, 2018

View reviewed changes

sasoc force-pushed the fix-nuage-provider-update branch from 551ed14 to a9d90fb Compare February 1, 2018 11:04

miha-plesko suggested changes Feb 1, 2018

View reviewed changes

sasoc force-pushed the fix-nuage-provider-update branch from a9d90fb to 3b4ea7b Compare February 1, 2018 13:38

miha-plesko reviewed Feb 1, 2018

View reviewed changes

sasoc force-pushed the fix-nuage-provider-update branch from 3b4ea7b to f17b43a Compare February 1, 2018 14:38

miq-bot assigned AparnaKarve Feb 1, 2018

miq-bot added the enhancement label Feb 1, 2018

miha-plesko approved these changes Feb 1, 2018

View reviewed changes

AparnaKarve reviewed Feb 3, 2018

View reviewed changes

sasoc force-pushed the fix-nuage-provider-update branch from f17b43a to c525ab7 Compare February 6, 2018 10:41

miha-plesko approved these changes Feb 9, 2018

View reviewed changes

dclarizio added networks gaprindashvili/no and removed gaprindashvili/no labels Feb 9, 2018

dclarizio merged commit 02e1a70 into ManageIQ:master Feb 9, 2018

dclarizio added this to the Sprint 79 Ending Feb 12, 2018 milestone Feb 9, 2018

miha-plesko mentioned this pull request Feb 12, 2018

Disable editing of NetworkProviders except Nuage #3394

Merged

simaishi pushed a commit that referenced this pull request Mar 8, 2018

Merge pull request #3337 from sasoc/fix-nuage-provider-update

7bc1af0

Fix Nuage provider update (cherry picked from commit 02e1a70) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1552895

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Mar 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Nuage provider update #3337

Fix Nuage provider update #3337

sasoc commented Jan 30, 2018

sasoc commented Jan 30, 2018

miha-plesko left a comment

miha-plesko Jan 30, 2018

miha-plesko Jan 30, 2018

miha-plesko Feb 1, 2018

miha-plesko Feb 1, 2018

miha-plesko Feb 1, 2018

miha-plesko Feb 1, 2018

miha-plesko Feb 1, 2018

sasoc commented Feb 1, 2018

miha-plesko left a comment

miha-plesko Feb 1, 2018

miha-plesko commented Feb 1, 2018

miha-plesko left a comment

AparnaKarve commented Feb 2, 2018

miha-plesko commented Feb 2, 2018

AparnaKarve Feb 3, 2018

AparnaKarve Feb 3, 2018

AparnaKarve Feb 3, 2018

AparnaKarve commented Feb 3, 2018

sasoc commented Feb 6, 2018

miq-bot commented Feb 6, 2018

miha-plesko left a comment

AparnaKarve commented Feb 9, 2018

AparnaKarve commented Feb 9, 2018

miha-plesko commented Feb 10, 2018

simaishi commented Mar 8, 2018

Fix Nuage provider update #3337

Fix Nuage provider update #3337

Conversation

sasoc commented Jan 30, 2018

sasoc commented Jan 30, 2018

miha-plesko left a comment

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

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

sasoc commented Feb 1, 2018

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented Feb 1, 2018

miha-plesko left a comment

Choose a reason for hiding this comment

AparnaKarve commented Feb 2, 2018

miha-plesko commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented Feb 3, 2018

sasoc commented Feb 6, 2018

miq-bot commented Feb 6, 2018

miha-plesko left a comment

Choose a reason for hiding this comment

AparnaKarve commented Feb 9, 2018

AparnaKarve commented Feb 9, 2018

miha-plesko commented Feb 10, 2018

simaishi commented Mar 8, 2018