Add kubevirt provider #3143

masayag · 2017-12-28T07:59:27Z

kubevirt provider is added as a secondary provider to be
managed by the openshift/kubernetes provider.

It will have also representation as an infrastructure provider, but its
management will be done under openshift/kubernetes, as it shares the
same endpoint, and might differ only by its token.

The token might be different than the default token in order to allow
the specific virtualization capabilities.

Since kubevirt share the same endpoint as the parent provider,
the 'detection' button will not be shown.

The virtualization label for the kubevirt provider:

The virtualization tab for the kubevirt provider:

Changing token for the virtualization manager:

'Add provider' drop-down list without 'kubevirt':

masayag · 2017-12-28T08:03:20Z

@h-kataria @Fryguy @jhernand @martinpovolny the current PR designed to replace #3051

It is more conventional and introduces provider specifics to multi_auth_credentials.html.haml
and endpoints_angular.html.haml as suggested on #3051 (comment)

masayag · 2017-12-28T08:06:50Z

@miq-bot add_labels enhacement graphics gaprindashvili/no

miq-bot · 2017-12-28T08:07:10Z

@masayag Cannot apply the following label because they are not recognized: enhacement graphics gaprindashvili/no

masayag · 2017-12-28T08:12:22Z

@serenamarie125 please review

masayag · 2017-12-28T08:13:07Z

add detect logic to openshift provider - large: 2 weeks

masayag · 2017-12-28T08:15:24Z

@miq-bot add_labels enhacement, graphics, gaprindashvili/no

miq-bot · 2017-12-28T08:16:15Z

@masayag Cannot apply the following label because they are not recognized: enhacement

masayag · 2017-12-28T08:43:53Z

@miq-bot add_label enhancement

h-kataria · 2018-01-03T16:12:44Z

@AparnaKarve please review.

AparnaKarve · 2018-01-03T19:02:05Z

@masayag Can you replace all == instances in this PR to === in the JS code?

AparnaKarve · 2018-01-03T19:06:19Z

Why is the Security Protocol field disabled in the Add screens?

masayag · 2018-01-03T19:46:45Z

@AparnaKarve

@masayag Can you replace all == instances in this PR to === in the JS code?

Will do.

Why is the Security Protocol field is disabled in the Add screens?

The option to add the kubevirt provider should be blocked. Management (add/remove or enable/disable) of the kubevirt provider is supposed to be done under the container manager as a secondary provider.

So I need to somehow hide the kubevirt provider from the suggested providers list when trying to add new provider and also to block the possibility to remove it.

Is it acceptable from UX point of view (@serenamarie125 ) ? If so, can you guide what is the place in code to control it ?

AparnaKarve · 2018-01-03T20:32:13Z

The option to add the kubevirt provider should be blocked. Management (add/remove or enable/disable) of the kubevirt provider is supposed to be done under the container manager as a secondary provider.

Would you be creating a separate UI PR for the kubevirt provider Management tasks - Add/Remove/Enable/Disable ?

zeari · 2018-01-04T15:18:33Z

app/controllers/ems_container_controller.rb

+    return "disabled" if @ems.connection_configurations.try(:kubevirt).nil?
+    "kubevirt"
+  end
+


how about @ems.connection_configurations.try(:kubevirt).nil? ? "disabled" : "kubevirt"
or
@ems.connection_configurations.try(:kubevirt).present? ? "kubevirt" : "disabled"

AparnaKarve · 2018-01-04T22:09:25Z

So I need to somehow hide the kubevirt provider from the suggested providers list when trying to add new provider and also to block the possibility to remove it.

Based on the screenshots in the top comment, editing a kubevirt provider seems to be a supported operation. Hence I was thinking - do you want to consider disabling the kubevirt option in the dropdown for Add screen, as shown below?

A disabled entry would indicate that a kubevirt provider cannot be added from the provider Add screen, but is supported in the product.

miq-bot · 2018-01-10T17:43:55Z

This pull request is not mergeable. Please rebase and repush.

masayag · 2018-01-11T21:26:23Z

@h-kataria

Would you be creating a separate UI PR for the kubevirt provider Management tasks - Add/Remove/Enable/Disable ?

These actions are already part of this PR. From Compute --> Containers --> Providers --> Select 'kubernetes' provider with virtualization manager --> change 'virtualization' tab to 'disabled' - The admin will be able to remove the 'kubevirt' provider from the system.

Based on the screenshots in the top comment, editing a kubevirt provider seems to be a supported operation. Hence I was thinking - do you want to consider disabling the kubevirt option in the dropdown for Add screen, as shown below?

Yes, I'd like to be able to do this as you showed in the attached screenshot of your comment.
How can it be done ?

AparnaKarve · 2018-01-12T00:20:54Z

I'd like to be able to do this as you showed in the attached screenshot of your comment.

diff --git a/app/views/ems_infra/_form.html.haml b/app/views/ems_infra/_form.html.haml
index d0e0e80da..31b8e9ef2 100644
--- a/app/views/ems_infra/_form.html.haml
+++ b/app/views/ems_infra/_form.html.haml
@@ -24,7 +24,7 @@
         = _('Type')
       .col-md-8
         = select_tag('emstype',
-                     options_for_select([["<#{_('Choose')}>", nil]] + @ems_types, disabled: ["<#{_('Choose')}>", nil]),
+                     options_for_select([["<#{_('Choose')}>", nil]] + @ems_types, disabled: ["<#{_('Choose')}>", nil, 'Kubevirt', 'kubevirt']),
                      "ng-if"                       => "newRecord",
                      "ng-model"                    => "emsCommonModel.emstype",
                      "ng-change"                   => "providerTypeChanged()",

This will disable Kubevirt.

However, a proper approach would be to create an array in the controller - @disabled_ems_infra_types

@disabled_ems_infra_types = ['Kubevirt', 'kubevirt']

and then in the haml, you could concat this array with ["<#{_('Choose')}>", nil]

options_for_select([["<#{_('Choose')}>", nil]] + @ems_types, disabled: ["<#{_('Choose')}>", nil].concat(@disabled_ems_infra_types)),

AparnaKarve · 2018-01-12T01:20:36Z

app/assets/javascripts/controllers/ems_common/ems_common_form_controller.js

@@ -445,6 +490,8 @@ ManageIQ.angular.app.controller('emsCommonFormController', ['$http', '$scope', '
      $scope.emsCommonModel.metrics_api_port = "5432";
      $scope.emsCommonModel.default_api_port = $scope.getDefaultApiPort($scope.emsCommonModel.emstype);
      $scope.emsCommonModel.metrics_database_name = "ovirt_engine_history";
+    } else if ($scope.emsCommonModel.emstype === 'kubevirt') {


Should this be $scope.emsCommonModel.virtualization_selection === 'kubevirt'?

since for a Container Provider, emstype is either 'kubernetes' or 'openshift' and I believe virtualization_selection is kubevirt

If the above is true, can you check all emstype/kubevirt references below and change them appropriately?

@AparnaKarve I think this should be completed removed. I can't think of a scenario in which we'll allow modifying the type of the 'kubevirt' provider or when we'll allow the user to add that provider explicitly and not via the openshift/kubernetes provider.

masayag · 2018-01-16T15:12:05Z

@AparnaKarve I've incorporated your comments into the PR.

masayag · 2018-01-24T10:22:45Z

@serenamarie125

One more question for ya @masayag ... are the change stored token & clear stored tokens links on the Virtualization tab a duplicate of the way that it works on the General tab as well?

Yes, they are the same. Updating the token under 'Infra' --> 'Providers' --> 'KubeVirt' --> 'Edit' is supposed to be simpler flow to the user.

masayag · 2018-02-05T13:23:15Z

@AparnaKarve I've added specs to this PR, please review and let me know if further fixes are needed.
If not, can we have it merged ?

AparnaKarve · 2018-02-07T17:25:49Z

spec/controllers/ems_common_controller_spec.rb

@@ -191,6 +191,82 @@ def test_setting_few_fields
          expect(@ems.options[:image_inspector_options][:http_proxy]).to eq("example.com")
        end
      end
+
+      context "update container provider with kubevirt provider" do


@masayag Would it be possible to move this test out of ems_common, and move it to ems_container_controller_spec instead?
In fact, all the other ems_container specific tests before this PR also need to be moved if possible.
(but not in this PR)

Can you also include a test for editing/updating the kubevirt provider in ems_infra_controller_spec?

@AparnaKarve fixed the specs and added missing ones.

@moolitayer @zeari please see comment about pulling ems_container related specs to ems_container_controller_spec

AparnaKarve · 2018-02-08T20:02:01Z

spec/controllers/ems_infra_controller_spec.rb

+
+    render_views
+
+    it 'creates on post' do


@masayag Just wondering if we need the 'creates on post' spec at all, since in the UI we disallow creating an Infra Provider of this type, and only allow the user to update the Provider.

Instead of this -

kubevirt = ManageIQ::Providers::Kubevirt::InfraManager.where(:name => "openshift_with_kubevirt Virtualization Manager").first

is it possible to work with a FactoryGirl record for the 'updates on post' spec?

dclarizio · 2018-02-08T21:56:34Z

@serenamarie125 from #3143 (comment)

Based on the screenshots in the top comment, editing a kubevirt provider seems to be a supported operation. Hence I was thinking - do you want to consider disabling the kubevirt option in the dropdown for Add screen, as shown below?

Do you think it's better to just not even have Kubevirt in the drop down, rather than have it there disabled, since that type can't be added from the Infrastructure area? I'm thinking it might be confusing to the user, making them think there is some way to get that enabled.

dclarizio · 2018-02-08T21:59:10Z

@Loicavenel @masayag is there a design document or something somewhere that explains why we want to show the kubevirt providers in 2 places in the UI? I'm just thinking it might get confusing both codewise and to the users. I'm sure someone has thought this out, but coming in from the outside, I'd like to understand the rationale behind this update before we merge. Thx, Dan

masayag · 2018-02-11T08:45:05Z

@dclarizio there is no design document for it. The idea was to add a secondary virtualization provider to the ems container. However, it behaves as an infra manager, therefore, once enabled, it can be managed as an infra manager. Changing the endpoints cannot be done for the infra manager, only by changing the ems container endpoint, since they are basically the same (differ by api url, but same endpoint), and also the only property that can be changed is the token (authentication's password).

We'd like to have the kubevirt as infra manager, to show its dashboard, list its resources and more.
As for managing it, it seems more intuitive to allow updating its token by editing it directly, else, the user will have to find to which ems container it belongs (be default, it can be figured out by the name, but the name can also be updated).

@serenamarie125 could you share your thoughts on this item as well ?

serenamarie125 · 2018-02-19T14:23:47Z

@AparnaKarve @dclarizio @masayag Agree that we should not have disabled items in a drop down. Rather than disable it, please do not show it.

serenamarie125 · 2018-02-19T14:26:14Z

@dclarizio @masayag there was no design doc for this, the premise that the new provider was supposed to be consistent with others.

Seems like the only reason we are allowing a kubevirt provider on the infra side (hadn't realized this til it was pointed out to me) is to allow for it to be renamed. Having both is confusing from a UX perspective, and I'd rather be constrained to not allow them to rename the provider (assuming it's not a requirement).

masayag · 2018-02-20T19:10:55Z

@AparnaKarve latest version contains the removal of 'kubevirt' from 'Add Infra Provider' dialog.
See attached screenshot to PR message.

Disable 'Edit Provider' for kubevirt is still needed for this PR to be complete.

serenamarie125 · 2018-02-21T14:21:23Z

After just looking at this with @masayag , I think the current flow is fine, so disregard my above comment.

Please keep the KubeVirt provider in Infrastructure, and allow for editing.

One thing that we could do with a follow up PR is including field level help on the Security Protocol, host name & API port which would provide a tooltip with an instruction stating if they want to edit that value, they'd need to do so by going to the associated Container Provider.

serenamarie125

LGTM @masayag ! I've added a possible improvement that can be done in a future PR, but approving for now!

kubevirt provider is added as a secondary provider to be managed by the openshift/kubernetes provider. It will have also representation as an infrastructure provider, but its management will be done under openshift/kubernetes, as it shares the same endpoint, and might differ only by its token. The token might be different than the default token in order to allow the specific virtualization capabilities. Since kubevirt share the same endpoint as the parent provider, the 'detection' button will not be shown.

The kubevirt provider should be added as a sub-manager of the openshift/kubernetes provider, and as such we should block the option to add it from the UI as infrastructure manager.

miq-bot · 2018-02-21T16:29:29Z

Checked commits masayag/manageiq-ui-classic@421c2e5~...e373ef2 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
11 files checked, 4 offenses detected

**

💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:

warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.5-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

app/controllers/ems_infra_controller.rb

❗ - Line 39, Col 34 - Style/WordArray - Use %w or %W for an array of words.

app/controllers/mixins/ems_common_angular.rb

❗ - Line 694, Col 7 - Style/SymbolArray - Use %i or %I for an array of symbols.

spec/controllers/ems_infra_controller_spec.rb

⚠️ - Line 749, Col 7 - Lint/AmbiguousBlockAssociation - Parenthesize the param change { Authentication.count } to make sure that the block will be associated with the change method call.

AparnaKarve · 2018-02-21T22:01:29Z

@masayag UI changes look good.

As for the specs, I think what I had mentioned before is still valid #3143 (comment)
-- reason being, the use case for adding a kubevirt provider in the Infrastructure space does not exist.
Feel free to make the spec adjustment in a follow-up - I think you may have to add the kubevirt ems_infra factory in the core manageiq repo to do the adjustment.

Overall LGTM!

@dclarizio This is good to go.

pkliczewski · 2018-02-22T08:02:13Z

🎉

zeari · 2018-02-22T08:55:40Z

🎉

masayag · 2018-02-22T09:37:54Z

As for the specs, I think what I had mentioned before is still valid #3143 (comment)
-- reason being, the use case for adding a kubevirt provider in the Infrastructure space does not exist.
Feel free to make the spec adjustment in a follow-up - I think you may have to add the kubevirt ems_infra factory in the core manageiq repo to do the adjustment.

@AparnaKarve please note that in the create request on that spec file I'm not creating the 'kubevirt' provider alone (as one might not do from the UI), since the backend doesn't support it.

Instead I'm creating 'openshift' provider with 'kubevirt' endpoint, which simulates the only supported method of adding 'kubevirt' provider into the system.

AparnaKarve · 2018-02-22T22:26:06Z

@masayag Thanks for the above clarification.

masayag force-pushed the virtualization_manager branch from f70573d to bc25a2c Compare December 28, 2017 08:10

miq-bot added gaprindashvili/no graphics labels Dec 28, 2017

miq-bot added the enhancement label Dec 28, 2017

masayag mentioned this pull request Jan 1, 2018

Support KubeVirt provider ManageIQ/manageiq-providers-kubernetes#197

Merged

zeari reviewed Jan 4, 2018

View reviewed changes

miq-bot added the unmergeable label Jan 10, 2018

masayag force-pushed the virtualization_manager branch from bc25a2c to 12db17c Compare January 11, 2018 21:14

miq-bot removed the unmergeable label Jan 11, 2018

AparnaKarve suggested changes Jan 12, 2018

View reviewed changes

masayag force-pushed the virtualization_manager branch from 12db17c to 2352369 Compare January 14, 2018 19:41

masayag force-pushed the virtualization_manager branch from 2352369 to f320911 Compare January 17, 2018 16:59

masayag force-pushed the virtualization_manager branch from a995e23 to 94a9786 Compare February 5, 2018 13:17

AparnaKarve reviewed Feb 7, 2018

View reviewed changes

masayag force-pushed the virtualization_manager branch from 94a9786 to 5fc7366 Compare February 8, 2018 14:37

AparnaKarve reviewed Feb 8, 2018

View reviewed changes

masayag force-pushed the virtualization_manager branch from 5fc7366 to 323af1f Compare February 20, 2018 19:07

serenamarie125 approved these changes Feb 21, 2018

View reviewed changes

masayag added 4 commits February 21, 2018 17:39

Replace == with ===

31b5398

Hide kubevirt in 'Add Infra Provider' dialog

4944f37

The kubevirt provider should be added as a sub-manager of the openshift/kubernetes provider, and as such we should block the option to add it from the UI as infrastructure manager.

Added specs

e373ef2

masayag force-pushed the virtualization_manager branch from 323af1f to e373ef2 Compare February 21, 2018 16:26

dclarizio self-assigned this Feb 21, 2018

dclarizio merged commit 3c5dafd into ManageIQ:master Feb 21, 2018

dclarizio added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 21, 2018

dclarizio added the compute/infrastructure label Feb 21, 2018

Add kubevirt provider #3143

Add kubevirt provider #3143

Conversation

masayag commented Dec 28, 2017 • edited Loading

masayag commented Dec 28, 2017 • edited Loading

masayag commented Dec 28, 2017

miq-bot commented Dec 28, 2017

masayag commented Dec 28, 2017

masayag commented Dec 28, 2017

masayag commented Dec 28, 2017

miq-bot commented Dec 28, 2017

masayag commented Dec 28, 2017

h-kataria commented Jan 3, 2018

AparnaKarve commented Jan 3, 2018

AparnaKarve commented Jan 3, 2018 • edited Loading

masayag commented Jan 3, 2018

AparnaKarve commented Jan 3, 2018

zeari Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

AparnaKarve commented Jan 4, 2018

miq-bot commented Jan 10, 2018

masayag commented Jan 11, 2018

AparnaKarve commented Jan 12, 2018

AparnaKarve Jan 12, 2018

Choose a reason for hiding this comment

AparnaKarve Jan 12, 2018

Choose a reason for hiding this comment

masayag Jan 14, 2018

Choose a reason for hiding this comment

masayag commented Jan 16, 2018

masayag commented Jan 24, 2018

masayag commented Feb 5, 2018

AparnaKarve Feb 7, 2018

Choose a reason for hiding this comment

AparnaKarve Feb 7, 2018

Choose a reason for hiding this comment

masayag Feb 8, 2018

Choose a reason for hiding this comment

AparnaKarve Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

dclarizio commented Feb 8, 2018

dclarizio commented Feb 8, 2018

masayag commented Feb 11, 2018

serenamarie125 commented Feb 19, 2018

serenamarie125 commented Feb 19, 2018

masayag commented Feb 20, 2018 • edited Loading

serenamarie125 commented Feb 21, 2018

serenamarie125 left a comment

Choose a reason for hiding this comment

miq-bot commented Feb 21, 2018

AparnaKarve commented Feb 21, 2018

pkliczewski commented Feb 22, 2018

zeari commented Feb 22, 2018

masayag commented Feb 22, 2018

AparnaKarve commented Feb 22, 2018

masayag commented Dec 28, 2017 •

edited

Loading

masayag commented Dec 28, 2017 •

edited

Loading

AparnaKarve commented Jan 3, 2018 •

edited

Loading

zeari Jan 4, 2018 •

edited

Loading

AparnaKarve Feb 8, 2018 •

edited

Loading

masayag commented Feb 20, 2018 •

edited

Loading