Container SSA: warn if no smartproxy/state role #273

enoodle · 2017-01-30T11:20:03Z

As it is very common to forget to set the SmartProxy role for SmartState Analysis for Container Images I got feedback that a warning if the role is not set is needed. This will prevent the initiation of the job, by disabling the SSA button and will show the user a message with the problem.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1342790

enoodle · 2017-01-30T11:21:32Z

@moolitayer @zgalor Please take a look

zgalor · 2017-01-30T11:33:16Z

👍 LGTM

gshefer · 2017-01-30T11:46:36Z

Excellent addition!, I'll add test case for the integration tests once merged.

enoodle · 2017-01-30T11:49:01Z

@simon3z

simon3z · 2017-01-30T14:31:02Z

@enoodle I think you'll have to sync with @h-kataria @dclarizio and @roliveri to have this on all the SSA pages.

@miq-bot assign enoodle
@miq-bot add_label providers/containers

miq-bot · 2017-01-30T14:31:14Z

@simon3z enoodle is an invalid assignee, ignoring...

moolitayer · 2017-01-30T14:40:12Z

app/controllers/mixins/containers_common_mixin.rb

@@ -210,6 +211,14 @@ def process_check_compliance(model, ids)
    end
  end

+  def check_smart_roles
+    %w(SmartProxy SmartState).each do |role|
+      unless MiqServer.all.any? { |s| s.has_active_role?(role.downcase) }


I'm pretty sure we want to only iterate only the servers in our zone. @gmcculloug ?

I could add && s.in_active_region? . waiting on @gmcculloug to ack this.

Checking the zone sounds like the right way to go, but I have not been involved much with SSA and will defer to @roliveri.

@gmcculloug I think that's right. I don't recall, I believe we check for available proxies in zone.

@roliveri I am sorry for resurrecting this from the dead ;-)
I was trying to find out but couldn't actually find where we are making this decision. I think it might be in [1] but I am not sure because it seems too general of a function name.
I am asking because I want to answer to BZ [2] with a definitive answer.
I think what they need is to allow Jobs created in the UI zone that have no active roles to be moved to other zones, is that possible?

[1] https://github.com/ManageIQ/manageiq/blob/master/app/models/job_proxy_dispatcher.rb#L174
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1459702

moolitayer · 2017-01-30T14:40:16Z

app/controllers/mixins/containers_common_mixin.rb

+  def check_smart_roles
+    %w(SmartProxy SmartState).each do |role|
+      unless MiqServer.all.any? { |s| s.has_active_role?(role.downcase) }
+        add_flash("There is no server with the #{role} role enables", :warning)


s/enables/enabled/

moolitayer

Nice idea, I like the general direction.

moolitayer · 2017-01-30T14:42:16Z

And I completely agree we need something against this scenario which keeps happening 👍

simon3z · 2017-02-17T15:03:45Z

@miq-bot assign roliveri

miq-bot · 2017-02-17T15:04:29Z

@simon3z Cannot apply the following label because they are not recognized: providers/containers

miq-bot · 2017-02-17T15:04:30Z

@simon3z roliveri is an invalid assignee, ignoring...

dclarizio · 2017-02-22T17:00:55Z

@h-kataria can you take a quick look please? This looks like a great addition.

dclarizio · 2017-02-22T17:02:06Z

@enoodle I like this . . . failing test right now though.

h-kataria · 2017-02-22T17:10:46Z

@enoodle in my opinion we should not let user press SSA button if SmartProxy role is not enabled. Iit would be better to disable SSA button on list view & summary screen with a hover text that "There is no server with SmaprtProxy role enabled". You can move the whole logic to check for the enabled role in https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/application_helper/button/vm_instance_template_scan.rb or to an appropriate button helper class.

enoodle · 2017-02-22T18:21:03Z

@h-kataria ok, will do. I just pushed something to make the test green but I will rework this logic for the button type.

h-kataria · 2017-02-27T14:01:22Z

@enoodle does this disable SSA button on list view as well?

enoodle · 2017-02-27T15:22:07Z

@h-kataria I changed the button for container_images_center.rb too, but apparently if the only button in a selection is disabled it won't show this button, so the whole "configuration" button is missing if the role is not set.

h-kataria · 2017-02-27T20:55:28Z

@enoodle does this PR address issue mentioned here https://bugzilla.redhat.com/show_bug.cgi?id=1342790

enoodle · 2017-02-28T09:07:59Z

@h-kataria yes, I was not aware or this BZ when I created this PR.

h-kataria · 2017-02-28T15:51:07Z

@enoodle can you add a link to the BZ in your commit comment.

enoodle · 2017-02-28T16:17:27Z

rebased on master to try and fix Travis problems.

simon3z · 2017-03-01T12:02:40Z

app/helpers/application_helper/button/smart_state_scan.rb

+    my_zone = MiqServer.my_server.my_zone
+    SMART_ROLES.each do |role|
+      unless MiqServer.all.any? { |s| s.has_active_role?(role.downcase) && (s.my_zone == my_zone) }
+        @error_message = "There is no server with the #{role} role enabled"


@enoodle @dclarizio does this need i18n?

I believe so, I will fix it.

simon3z · 2017-03-01T12:03:56Z

app/helpers/application_helper/button/smart_state_scan.rb

+  def check_smart_roles
+    my_zone = MiqServer.my_server.my_zone
+    SMART_ROLES.each do |role|
+      unless MiqServer.all.any? { |s| s.has_active_role?(role.downcase) && (s.my_zone == my_zone) }


@enoodle if this is the only place where you're using SMART_ROLES why don't you keep them downcase'd in the constant?

I also use it in the message below (line 10).

simon3z · 2017-03-01T12:04:35Z

app/helpers/application_helper/button/smart_state_scan.rb

+class ApplicationHelper::Button::SmartStateScan < ApplicationHelper::Button::Basic
+  needs :@record
+
+  SMART_ROLES = %w(SmartProxy SmartState).freeze


@enoodle @roliveri should this constant live somewhere in the SmartState subsystem?

We are using this button for the VM instances too so this should be in a general location. Is there a central SSA class?

@enoodle who are you asking? I assume you asked @roliveri (although I am sure you're more than qualified to look at the code and find a relevant place on your own).

The only place I could think of is in MiqServer::ServerSmartProxy but I am also not sure about it.

[1] https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_server/server_smart_proxy.rb#L3

simon3z · 2017-03-01T12:05:31Z

Few comments on the PR but overall LGTM 👍
@dclarizio @h-kataria can you review/merge?

h-kataria · 2017-03-01T22:26:58Z

@simon3z @dclarizio overall changes look good to me, not sure if @enoodle is looking into addressing comments from @simon3z to move the constant to somewhere else.

enoodle · 2017-03-02T08:58:42Z

I am waiting on ManageIQ/manageiq#14127 to address the constant for the smart roles.

miq-bot · 2017-03-05T13:38:09Z

Checked commits enoodle/manageiq-ui-classic@40e6071~...afa1657 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 4 offenses detected

spec/helpers/application_helper/buttons/smart_state_scan.rb

⚠️ - Line 2, Col 9 - Lint/ParenthesesAsGroupedExpression - (...) interpreted as grouped expression.
❗ - Line 1, Col 1 - Metrics/BlockLength - Block has too many lines. [38/25]
❗ - Line 6, Col 3 - Metrics/BlockLength - Block has too many lines. [33/25]

spec/helpers/application_helper/buttons/vm_instance_scan_spec.rb

⚠️ - Line 2, Col 9 - Lint/ParenthesesAsGroupedExpression - (...) interpreted as grouped expression.

enoodle · 2017-03-05T13:54:36Z

@simon3z, @dclarizio PTAL, I fixed @simon3z 's last comments about the smart proxy roles constant location.

h-kataria · 2017-03-06T14:17:15Z

looks good.

enoodle force-pushed the no_smartproxy_warning_container_ssa branch from 9ef0e2b to fcc5aa0 Compare January 30, 2017 11:23

enoodle force-pushed the no_smartproxy_warning_container_ssa branch from fcc5aa0 to b05fbfc Compare January 30, 2017 11:48

miq-bot added the compute/containers label Jan 30, 2017

moolitayer reviewed Jan 30, 2017

View reviewed changes

moolitayer suggested changes Jan 30, 2017

View reviewed changes

enoodle force-pushed the no_smartproxy_warning_container_ssa branch from b05fbfc to 8dd8d8a Compare January 30, 2017 16:20

miq-bot assigned enoodle Feb 17, 2017

enoodle force-pushed the no_smartproxy_warning_container_ssa branch from 8dd8d8a to d23792b Compare February 22, 2017 14:34

dclarizio added enhancement euwe/no labels Feb 22, 2017

dclarizio assigned dclarizio and unassigned enoodle Feb 22, 2017

enoodle force-pushed the no_smartproxy_warning_container_ssa branch from d23792b to db01e3a Compare February 22, 2017 18:19

enoodle changed the title ~~Container SSA: warn if no smartproxy/state role~~ [WIP] Container SSA: warn if no smartproxy/state role Feb 22, 2017

miq-bot added the wip label Feb 22, 2017

enoodle force-pushed the no_smartproxy_warning_container_ssa branch from e951b67 to 33dfb5b Compare February 28, 2017 16:16

simon3z reviewed Mar 1, 2017

View reviewed changes

enoodle force-pushed the no_smartproxy_warning_container_ssa branch from 33dfb5b to 713e0ae Compare March 1, 2017 12:32

enoodle mentioned this pull request Mar 2, 2017

A constant detailing the SmartState roles ManageIQ/manageiq#14127

Merged

enoodle changed the title ~~Container SSA: warn if no smartproxy/state role~~ [WIP] Container SSA: warn if no smartproxy/state role Mar 2, 2017

miq-bot added the wip label Mar 2, 2017

enoodle force-pushed the no_smartproxy_warning_container_ssa branch from 713e0ae to 137b6f1 Compare March 2, 2017 12:48

enoodle changed the title ~~[WIP] Container SSA: warn if no smartproxy/state role~~ Container SSA: warn if no smartproxy/state role Mar 5, 2017

enoodle closed this Mar 5, 2017

enoodle reopened this Mar 5, 2017

miq-bot removed the wip label Mar 5, 2017

enoodle force-pushed the no_smartproxy_warning_container_ssa branch from 137b6f1 to 9436239 Compare March 5, 2017 13:08

Erez Freiberger added 2 commits March 5, 2017 15:34

Container SSA: warn if no smartproxy/state role

40e6071

disabeling from the button

afa1657

enoodle force-pushed the no_smartproxy_warning_container_ssa branch from 9436239 to afa1657 Compare March 5, 2017 13:36

h-kataria added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 6, 2017

h-kataria merged commit 99fc51b into ManageIQ:master Mar 6, 2017

moolitayer mentioned this pull request Aug 20, 2017

smart state: failure but no error received when missing smart state role ManageIQ/manageiq#5262

Closed

Container SSA: warn if no smartproxy/state role #273

Container SSA: warn if no smartproxy/state role #273

Conversation

enoodle commented Jan 30, 2017 • edited Loading

enoodle commented Jan 30, 2017

zgalor commented Jan 30, 2017

gshefer commented Jan 30, 2017

enoodle commented Jan 30, 2017

simon3z commented Jan 30, 2017

miq-bot commented Jan 30, 2017

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

moolitayer left a comment

Choose a reason for hiding this comment

moolitayer commented Jan 30, 2017

simon3z commented Feb 17, 2017

miq-bot commented Feb 17, 2017

miq-bot commented Feb 17, 2017

dclarizio commented Feb 22, 2017

dclarizio commented Feb 22, 2017

h-kataria commented Feb 22, 2017

enoodle commented Feb 22, 2017

h-kataria commented Feb 27, 2017

enoodle commented Feb 27, 2017

h-kataria commented Feb 27, 2017

enoodle commented Feb 28, 2017

h-kataria commented Feb 28, 2017

enoodle commented Feb 28, 2017

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

simon3z commented Mar 1, 2017

h-kataria commented Mar 1, 2017

enoodle commented Mar 2, 2017

miq-bot commented Mar 5, 2017

enoodle commented Mar 5, 2017

h-kataria commented Mar 6, 2017

enoodle commented Jan 30, 2017 •

edited

Loading