Ofer/311468 filter event by storage system #8424

Oferlis · 2022-08-24T11:03:26Z

Added a new selection field in the events monitoring page to filter by a physical storage system.

Before:

After:

The new field will show the connected physical storage systems and will filter by the selection.

When entering the monitoring page from the storage manager page, selecting an empty set will present all the events available.
When entering the monitoring page from a specific physical storage, an empty set will present the current physical storage events.

GilbertCherrie · 2022-08-29T19:54:28Z

@agrare I don't have any storage managers with monitoring or timelines enabled so I couldn't fully test this. Although @Oferlis I did notice that this PR also added this field to the timelines page for VMS and instances, is that intentional?

Oferlis · 2022-08-30T09:08:21Z

Hey @GilbertCherrie, the addition of the storage system field for the VMS and instances was not intentional, as this PR was related to physical storage.
What is the preferred way of approaching this in your opinion?

GilbertCherrie · 2022-08-30T16:17:39Z

@Oferlis You can create a variable in the physical storage controller file and set it to true, such as
@isPhysicalStorage = True then in your haml file you check if this value is true to render that drop down or not. What you did works but it adds an extra haml file that we don't need. Let me know if what I said makes sense or not.

Oferlis · 2022-09-05T14:14:45Z

Hey @agrare @GilbertCherrie,
Do you have any comments regarding the PR?

agrare · 2022-09-06T13:40:40Z

I'd like a UI reviewer before I merge this, @jeffibm @GilbertCherrie @DavidResende0

GilbertCherrie · 2022-09-06T22:52:43Z

app/javascript/oldjs/controllers/timeline/timeline_options_controller.js

@@ -31,6 +33,14 @@ ManageIQ.angular.app.controller('timelineOptionsController', ['miqService', 'url
    vm.reportModel.tl_categories = [];
  };

+  vm.showFilterField = function() {
+    let bool = false;
+    if(vm.model === "PhysicalStorage" || vm.model === "ExtManagementSystem"){


@Oferlis The vm.model === "ExtManagementSystem" shouldn't be used because if you go into the monitoring page for a cloud provider it will have the value of "ExtManagementSystem" for vm.model and cause this field to be rendered there as shown in the screenshot.

Just changed it to rely on the Autosde model so any other model will not be effected.
Changes will be pushed today.

agrare · 2022-09-07T19:44:06Z

app/helpers/application_helper.rb

+    storages = PhysicalStorage.all.ids
+    storages.map{|k| [PhysicalStorage.find_by(:id=>k).name, k]}


Hm there is a lot wrong with this...

I don't think this should be in the base application_helper when it is storage_system specific

It queries for physical storages one-at-a-time rather than in batches

It is showing ALL physical storages regardless of tenant/group/RBAC

This needs a re-think IMO

I modified the function so it will be less problematic, please tell me if this is fine with you:

Also, where should I place it?
The timeline JS files are in the UI repo in this is relevant to it

If you want to return an array of [name, id] the way to do that in the most efficient way possible is PhysicalStorage.pluck(:name, :id)

I still don't think we can return all physical storages here though, if you have two tenants wouldn't tenant A see all physical storages in tenant B and vice versa?

Changed it to the pluck method you suggested, and it looks good.
For the other part, I can use the Autosde::PhysicalStorage, is this better? it will make sure only AutoSDE objects are returned

For the other part, I can use the Autosde::PhysicalStorage, is this better? it will make sure only AutoSDE objects are returned

No it really doesn't because two tenants can both have AutoSDE providers. Need help from the UI team here because I'm not sure what is common here but you have to use something that has a user context like a selected provider or an RBAC filtered list of physical_storages

Who can I contact in the UI team?

I would hope they are already reviewing this, but /cc @jeffibm @DavidResende0 @GilbertCherrie

agrare · 2022-09-07T19:45:20Z

app/helpers/application_helper/button/physical_storage_timeline.rb

+  def calculate_properties
+    super
+    self[:hidden] = true unless visible?
+  end
+
+  def visible?
+    @record.supports?(:timeline)
+  end


These are identical to the parent class, why override them?

This file was a reminder from the first commit, it is deleted.

agrare · 2022-09-07T19:46:12Z

app/helpers/application_helper/button/physical_storage_timeline.rb

+  end
+
+  def disabled?
+    @error_message.present?


Will this ever be true? The parent class sets @error_message in a conditional block then checks for .present? but this never sets it so how could it ever be true?

This file was a reminder from the first commit, it is deleted.

agrare · 2022-09-07T19:46:56Z

app/javascript/oldjs/controllers/timeline/timeline_options_controller.js

@@ -7,6 +7,7 @@ ManageIQ.angular.app.controller('timelineOptionsController', ['miqService', 'url
      tl_timerange: 'weeks',
      tl_timepivot: 'ending',
      tl_result: 'success',
+      tl_storage_systems: [''],


Is the rest of this provider specific? I don't see anything else here about infra hosts or cloud vms...

This is storage specific, is there a better solution in your eyes?

agrare · 2022-09-07T19:49:40Z

app/controllers/application_controller/timelines.rb

+        rec_cond, *rec_params = @tl_record.event_where_clause(@tl_options.evt_type)
+        parameters = rec_params + [from_dt, to_dt]
+      else
+        rec_cond = tl_map_storage_systems(params[:tl_storage_systems])


So it looks like if you aren't on a specific record, you want to show all events from all storage systems?
Would it make more sense to have @tl_record be the Autosde::StorageManager and keep using the same @tl_record.event_where_clause ?

I edited the function as you recommended, the function is related to the StorageManager and PhysicalStorage models now.
Changes will be pushed hopefully today.

agrare

Just going to throw a NAK on this since I have a couple serious concerns

agrare · 2022-09-13T15:22:59Z

app/controllers/application_controller/timelines.rb

@@ -93,10 +93,11 @@ def tl_build_timeline_report_options
      from_dt = create_time_in_utc("#{from_date.strftime} 00:00:00",
                                   session[:user_tz])

-      rec_cond, *rec_params = @tl_record.event_where_clause(@tl_options.evt_type)
-      conditions = [rec_cond, "timestamp >= ?", "timestamp <= ?"]
+      rec_cond, *rec_params = @tl_record.event_where_clause(@tl_options.evt_type, params[:tl_storage_systems])


So the comment seems to have been lost in the changes, but this is still an issue. We can't be adding in provider specifics like storage_systems here in the general timeline options.

I think it is better to have @tl_record be your EMS and you can return all the storages related to the ems (this also fixes your RBAC issue) and/or have @tl_record be a specific storage system (also resolves the RBAC issue)

The suggested change is the situation today (Master branch), where when using the EMS, all the physical storages are displayed, and when selecting specific storage only its events are presented.

The params are returned from the form the user filled, so not passing them will mean no filtering, which is the purpose of the PR.

As I see it, the change will have to add provider-specific code somewhere in the code (which I agree is not good), so the problem will be moved to another file or function. Is there a solution that you think might fit here?

@jeffibm @GilbertCherrie @DavidResende0 is there a way for the Autosde provider to override the timeline UI page without modifying the core options?

MelsHyrule · 2022-09-15T14:32:58Z

Hi just got sent this PR, catching up on reading all the comments. Currently this page is due in for a conversion to React and is currently underway. With this conversion we can add schema to the AutoSDE provider a la https://github.com/ManageIQ/manageiq-providers-openstack/blob/master/app/models/manageiq/providers/openstack/storage_manager/cinder_manager/cloud_volume.rb#L135-L146 and have that include the 'Select Storage System' field when is appropriate. This will leave the rest of the fields intact for all other cases.

GilbertCherrie · 2022-09-15T19:44:59Z

Hi just got sent this PR, catching up on reading all the comments. Currently this page is due in for a conversion to React and is currently underway. With this conversion we can add schema to the AutoSDE provider a la https://github.com/ManageIQ/manageiq-providers-openstack/blob/master/app/models/manageiq/providers/openstack/storage_manager/cinder_manager/cloud_volume.rb#L135-L146 and have that include the 'Select Storage System' field when is appropriate. This will leave the rest of the fields intact for all other cases.

@agrare I agree with @MelsHyrule here, it would probably be much easier to just handle this with a react conversion than figuring out how to make the form provider specific

agrare · 2022-09-15T20:50:32Z

@GilbertCherrie @MelsHyrule 👍 sounds good to me thanks

miq-bot · 2022-09-20T12:02:07Z

Checked commits Autosde/manageiq-ui-classic@a222e7d~...9954745 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
3 files checked, 1 offense detected

app/views/layouts/_tl_options.html.haml

⚠️ - Line 123 - Layout/TrailingEmptyLines: Final newline missing.

Oferlis · 2022-09-20T14:32:13Z

Hi, @GilbertCherrie @MelsHyrule,
Thank you for the update, when this conversion will be introduced? just so I get an approximation

MelsHyrule · 2022-09-20T15:17:17Z

@Oferlis The conversion should be introduced sometime before Q4 so within the next month or so

miq-bot · 2022-11-01T19:36:11Z

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

Oferlis · 2022-11-03T16:53:43Z

Hi @agrare @MelsHyrule,

I am back at this PR as I noticed the transition to React.
Regarding the approach @GilbertCherrie suggested, as I see it the textual functions are for textual values only.
I aim to add a field to the form to filter by physical storage, so manipulating the Ruby files (timelines.rb, and such, as I did before) is still the only solution I can see.

another approach will be to filter the JSON response in the miq_timeline.js file, but it will introduce provider-specific code in the UI files.

Any thoughts?

MelsHyrule · 2022-11-04T16:36:33Z

So poking around what I would do, is I would start testing to see how I would introduce the physical storage filter using react. In my original comment I pointed out params_for_attach so you could introduce the drop down with an API.options call. You'll need to rebase this branch onto latest since some of the files you've changed here no longer exist within the repo. We're already passing in the id of what we select (see here) so you can use that to your advantage.

Once the drop down is actually set up and behaving as expected then I would look into including the physical storage option into the ruby file logic. It would likely go in tl_build_timeline_report_options or tl_gen_timeline_data in timelines.rb but i'd need to get my hands in there with data that has providers with physical storages to test properly.

Hopefully this answers your question. Let me know if you need any further help!

Oferlis · 2022-11-08T09:23:56Z

I have opened another branch for the new changes, but the conversation is documented here

Thank you for the reply, I have managed to get to the point where I need to handle the Ruby files, (tl_build_timeline_report_options or tl_gen_timeline_data functions).

As @agrare commented in #8424 (comment)
I should avoid introducing provider-specific code in the general timeline option, but I cannot find a workaround - the filtering must be passed in these functions. Any thoughts?

MelsHyrule · 2022-11-15T18:33:34Z

So the steps to take would be the following:

In timeline-options.jsx we would need to change this line https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/javascript/components/timeline-options/timeline-options.jsx#L73 to use an API call (some discussion can be seen here Api for timeline charts. manageiq-api#1090) instead of sending the data to the ruby controller, this would allow the flexibility to include the tl_storage_systems we're looking to add
Once that's created we hit the problem of getting the data to the timeline component persay.
The ruby code currently takes the response from the database, converts it to JSON, and sets it to the variable tl_json (see here https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/application_controller/timelines.rb#L179) and tl_json gets sent to / is used by https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/views/layouts/_timeline.html.haml#L1-L3 which hands it off to https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/javascript/oldjs/miq_timeline.js#L175-L193 who takes all that data, parses it, and puts it all together in the timeline we currently see and the tables that appear when selecting items off the timeline.
Once the API call is implemented we need to get that data to miq_timeline.js. To do it right we would need to convert the page to react (the current page would need to be reworked regardless bc the API call returns the data in a different format than the current parsed JSON does). Converting this page to react would be tricky since there's no 'timeline' carbon component, the tables we can convert relatively simply.

Oferlis · 2022-11-16T09:29:09Z

I managed to use the API link in the browser to test the response to the call:

/api/event_streams?expand=resources&attributes=group,group_level,group_name,id,event_type,physical_storage_id,message,ems_id,type&filter[]=group_level=critical&filter[]=or%20physical_storage_id=[1] -> as shown in the discussion, with the physical storage filtration I added.

As I understand, that data (from the API call) needs to be passed to the miq_timeline.js to further filter the JSON response, but that file needs to be converted to React before that can happen. Is the file conversion planned to take action?

Oferlis · 2022-11-28T11:52:52Z

@MelsHyrule, I didn't tag you in the last comment, so you probably weren't notified. what you think? How should we proceed?

MelsHyrule · 2022-12-01T14:52:46Z

@Oferlis Hey! Just got back from vacation so only just saw your messages. I'll be reaching out to the UI team to discuss what's the best way to proceed further with this. Will ping you once we have an answer.

MelsHyrule · 2022-12-07T19:33:21Z

@Oferlis Conversion to react for the required app/javascript/oldjs/miq_timeline.js page is currently underway. You can find the PR here.

Oferlis · 2022-12-08T07:38:14Z

@Oferlis Conversion to react for the required app/javascript/oldjs/miq_timeline.js page is currently underway. You can find the PR here.

Thank you for the update.

miq-bot · 2023-03-13T00:01:37Z

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

agrare requested review from DavidResende0 and GilbertCherrie August 25, 2022 16:18

agrare added enhancement storage labels Aug 25, 2022

GilbertCherrie mentioned this pull request Aug 29, 2022

[WIP] Fixed reconfigure button #8428

Closed

Oferlis force-pushed the ofer/311468-filter_event_by_storage_system branch from 997e245 to 8da576e Compare August 30, 2022 12:06

Fryguy assigned agrare Aug 31, 2022

GilbertCherrie reviewed Sep 6, 2022

View reviewed changes

agrare reviewed Sep 7, 2022

View reviewed changes

agrare requested changes Sep 7, 2022

View reviewed changes

Oferlis mentioned this pull request Sep 12, 2022

Override where clause functions and added helpers dir ManageIQ/manageiq-providers-autosde#178

Closed

Oferlis added 10 commits September 12, 2022 14:55

WIP - filteration seems to work, but testing is needed

a222e7d

WIP - works for east cases - more testing is needed, removed breakpoints

9610c97

New field is now inline and filtration works

547d79e

Added physical_storage_timeline file and started modifications

7cb9ed9

added physical storage options file and made it work

25c0072

deleted extra file, field will now show/not show based on model type

815173c

removed redundent consolelog and fixed ;'s

c97bf76

added model property to spec

475e513

WIP fixed MIQ teams comments. More changes may arise

adc90a9

small helper modification

4235160

Oferlis added 3 commits September 13, 2022 10:40

fixed rubocop issues

74a5918

rubocop issues fixed

e579162

deleted redundent function, rubocop fixes

b5ffa78

agrare reviewed Sep 13, 2022

View reviewed changes

minor change, still waiting for page to be converted to React

9954745

Oferlis requested a review from a team as a code owner September 20, 2022 11:58

miq-bot added unmergeable and removed unmergeable labels Oct 13, 2022

MelsHyrule mentioned this pull request Oct 26, 2022

Timeline Conversion #8496

Merged

miq-bot added the unmergeable label Nov 1, 2022

miq-bot added the stale label Mar 13, 2023

miq-bot closed this Mar 13, 2023

Fryguy removed the stale label Jul 27, 2023

		storages = PhysicalStorage.all.ids
		storages.map{\|k\| [PhysicalStorage.find_by(:id=>k).name, k]}

Ofer/311468 filter event by storage system #8424

Ofer/311468 filter event by storage system #8424

Conversation

Oferlis commented Aug 24, 2022

GilbertCherrie commented Aug 29, 2022

Oferlis commented Aug 30, 2022

GilbertCherrie commented Aug 30, 2022 • edited Loading

Oferlis commented Sep 5, 2022

agrare commented Sep 6, 2022

GilbertCherrie Sep 6, 2022 • 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

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

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

agrare 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

MelsHyrule commented Sep 15, 2022

GilbertCherrie commented Sep 15, 2022

agrare commented Sep 15, 2022

miq-bot commented Sep 20, 2022

Oferlis commented Sep 20, 2022

MelsHyrule commented Sep 20, 2022

miq-bot commented Nov 1, 2022

Oferlis commented Nov 3, 2022

MelsHyrule commented Nov 4, 2022

Oferlis commented Nov 8, 2022 • edited Loading

MelsHyrule commented Nov 15, 2022

Oferlis commented Nov 16, 2022

Oferlis commented Nov 28, 2022

MelsHyrule commented Dec 1, 2022

MelsHyrule commented Dec 7, 2022

Oferlis commented Dec 8, 2022

miq-bot commented Mar 13, 2023

GilbertCherrie commented Aug 30, 2022 •

edited

Loading

GilbertCherrie Sep 6, 2022 •

edited

Loading

Oferlis commented Nov 8, 2022 •

edited

Loading