Bug 1376360

Summary: [RFE] VMs list in the UI - don't pass all VM fields
Product: [oVirt] ovirt-engine Reporter: Oved Ourfali <oourfali>
Component: Frontend.WebAdminAssignee: bugs <bugs>
Status: CLOSED WONTFIX QA Contact: Pavel Stehlik <pstehlik>
Severity: high Docs Contact:
Priority: unspecified    
Version: 3.6.6CC: bugs, michal.skrivanek, tjelinek, vszocs
Target Milestone: ---Keywords: FutureFeature, Improvement, Performance
Target Release: ---Flags: michal.skrivanek: ovirt-future?
rule-engine: planning_ack?
rule-engine: devel_ack?
rule-engine: testing_ack?
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2018-06-04 07:21:07 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: Virt RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description Oved Ourfali 2016-09-15 08:35:46 UTC
Description of problem:
Currently, when going into the VMs main tab, the frontend calls a query to get all the VM data, although a lot of fields aren't used at all.

Please change it to return nulls in fields that aren't used (probably need to create a dedicated query for that one).

If, for example, the user uses payloads heavily then we will bring back even more data for no good reason.

Also, please inspect user portal with regards to that, as it even uses less fields.

Comment 1 Michal Skrivanek 2016-09-15 08:44:08 UTC
so what are the fields which are used? This improvement(to not send all data) is pending for years due to no consensus or design of hidden/shown columns, and what entitites to use in GWT UI.
There should be a consensus of maintainers first

Tomas, thoughts?

Comment 2 Tomas Jelinek 2016-09-29 12:15:35 UTC
After an offline discussion with Vojtech we came to this conclusions:
- there is an ongoing effort to solve memory leaks in webadmin and we both believe that this effort will bring huge performance benefits
- loading only a subset of the VM would be very risky since the whole frontend expects the full VM entity. It could be refactored, but the risks are big
- there is a chance to do it partially - e.g. the first load of all vms would load only the DTOs bringing speedup. After selecting a VM from the list, do an additional server call. This way the first load would be faster while still postponing the need to refactor the rest of the UI. The disadvantage of this approach is that after doing this, the first load would be faster but every click would be slowed down by additional server call. The UX advantage of this approach is therefor very questionable.
- the bigger issue than the initial load of the main VMs tab is the slowness of the dialogs. To fix this we came to the conclusion that the best approach would be to try to rewrite the dialogs to more modern technologies with performance and overall UX in mind and integrating them to webadmin using ui-plugins.

That said, I would propose to close this issue as "wont fix" and continue tracking this effort on https://bugzilla.redhat.com/show_bug.cgi?id=1378935 

@Vojtech, did I summarize it as we agreed? Do you want to add something?

Comment 3 Oved Ourfali 2016-09-29 15:46:05 UTC
I still think we should go in that approach, or at least test it. 
I don't think the leak fixes will fix everything, and if we want to keep a stable ui going forward we should invest time in approaches like that. 

I don't mind waiting a bit til we see the leak impact, but please don't close it.

Comment 4 Michal Skrivanek 2016-09-30 08:13:09 UTC
I do not see the point about stable UI, you won't get a more stable legacy UI by rewriting the fundamental principles of it. The assumption that fields are not used is not true for webadmin. For power user portal it is the same case (as soon as you have a "new vm" functionality you pretty much need them all). It can be done for basic user portal, but it's not widely used(or complained about) and we're investigating https://trello.com/c/blwDpjcJ instead.

I don't mind leaving it open, but I do not see that likely until we are confident that it is worth the effort and risk of destabilizing the current code base

Comment 5 Oved Ourfali 2016-09-30 12:15:03 UTC
I didn't suggest to rewrite anything, just to check whether returning the vm object with nulls for unused fields is feasible, and whether it improves things. 

I haven't heard yet neither that it isn't feasible, nor that it doesn't improve anything. 

Close it if you want, but just wanted to clarify this point.

Comment 6 Michal Skrivanek 2016-09-30 12:20:40 UTC
based on analysis by SMEs in comment #2 it is not feasible, at least not easily enough

Comment 7 Vojtech Szocs 2016-10-04 17:51:14 UTC
Sorry for late reply.

(In reply to Tomas Jelinek from comment #2)
> After an offline discussion with Vojtech we came to this conclusions:
> - there is an ongoing effort to solve memory leaks in webadmin and we both
> believe that this effort will bring huge performance benefits

Since the UI makes heavy use of dialogs, making sure that dialog-related resources (-> RAM) are properly reclaimed will solve the "UI performance degrades over time" issue, which is very important for end users.

> - loading only a subset of the VM would be very risky since the whole
> frontend expects the full VM entity. It could be refactored, but the risks
> are big

WebAdmin frontend was designed as monolithic application that uses backend business entities (which generally means it's tightly coupled with backend). Our GWT UI code still reflects that design.

Changing this design globally (via big refactor) is very challenging and risky. That's why I suggested scoping any (perf.) improvements to particular parts of UI code.

In general, it is hard to tell which VM fields are used and which aren't; the VM entity is heavily used across UI code. Therefore, the original idea was to optimize the "load VM list to show in VM grid" scenario only.

I admit that the risk is big and maybe not worth it.. we could do a quick measurement like:

- assume ideal conditions (Engine & browser running locally)
- measure time to de-serialize few (1-10) VMs
- measure time to de-serialize 100 VMs, evaluate how big the perf. impact is

I agree that every perf. improvement should weigh pros vs. cons and proceed only if pros > cons.

(Note: if we really want to see which VM fields are used for given query, we can hack into GWT de-serialization mechanism and see what VM setters are called. But this doesn't prevent anyone from using currently-unused fields in future, unfortunately.)

> - there is a chance to do it partially - e.g. the first load of all vms
> would load only the DTOs bringing speedup. After selecting a VM from the
> list, do an additional server call. This way the first load would be faster
> while still postponing the need to refactor the rest of the UI. The
> disadvantage of this approach is that after doing this, the first load would
> be faster but every click would be slowed down by additional server call.
> The UX advantage of this approach is therefor very questionable.

The assumption was that users have lots of entities to display in UI grid, but select only a few, so the extra "full entity load" request wouldn't hurt too much. But it's not a perfect solution, I agree.

> - the bigger issue than the initial load of the main VMs tab is the slowness
> of the dialogs. To fix this we came to the conclusion that the best approach
> would be to try to rewrite the dialogs to more modern technologies with
> performance and overall UX in mind and integrating them to webadmin using
> ui-plugins.

I like this idea, actually.

While devs can say that it fragments/breaks existing monolithic design (which is true), it also means:

- less GWT/Java code to compile (faster builds, smaller GWT-generated JS footprint)
- possibility to reuse common UI components developed as part of this effort

Dashboard is an example where modern JS technologies (React etc.) can be used within existing GWT UI frontend. We can decide to push this idea further, re-implementing dialogs in such JS technologies and using UI plugins as the extension mechanism. The downside to consider is having to maintain/debug both GWT-based core + specific UI extensions.

> 
> That said, I would propose to close this issue as "wont fix" and continue
> tracking this effort on https://bugzilla.redhat.com/show_bug.cgi?id=1378935 
> 
> @Vojtech, did I summarize it as we agreed? Do you want to add something?

I think we should wait with any further perf. improvements until we fix the memory leaks (originating from dialogs).

(In reply to Michal Skrivanek from comment #4)
> I don't mind leaving it open, but I do not see that likely until we are
> confident that it is worth the effort and risk of destabilizing the current
> code base

That's why I wrote about "quick measurement" above, it could give us a hint on how big/impactful this issue is.

Anyway, I agree that there are cons and there is no perfect solution (that wouldn't go against the existing design). So I think it's up to Virt team to make the decision here.

Comment 8 Yaniv Kaul 2018-05-21 10:46:17 UTC
I think it's a great idea, but we have not managed to make any progress on it for years. If we do not feel 4.3 is going to make it, I suggest closing.

Comment 9 Tomas Jelinek 2018-06-04 07:21:07 UTC
(In reply to Yaniv Kaul from comment #8)
> I think it's a great idea, but we have not managed to make any progress on
> it for years. If we do not feel 4.3 is going to make it, I suggest closing.

Also I don't particularly like the idea of having a separate API for webadmin and a separate one for everything else. I would not invest into enhancing an API we should avoid in the first place.