Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provisioning Workflows are being stored directly in session #7877

Open
jrafanie opened this issue Sep 24, 2021 · 10 comments
Open

Provisioning Workflows are being stored directly in session #7877

jrafanie opened this issue Sep 24, 2021 · 10 comments

Comments

@jrafanie
Copy link
Member

The contents of @edit are being stored into session. Included in @edit in the provisioning pages is the @edit[:wf] which is an instance of a provisioning workflow object.

This is used throughout the UI in these pages but most specifically throughout this file

We shouldn't be storing such large objects into session.

As an side effect, the following issues/PRs were raised because this large object stored very large lists of allowed_templates:
ManageIQ/manageiq#21455
ManageIQ/manageiq-providers-google#198

For example, when walking the code that checks for session bloat, I found that there were thousands of openstructs for each template in the list of templates within the wf object:

[22] pry(#<MiqRequestController>)> data['edit'][:wf].instance_variable_get(:@dialogs)[:dialogs][:service][:fields][:src_vm_id]
=> {:values_from=>{:options=>{:tag_filters=>[], :prov_field_name=>:src_vm_id}, :method=>:allowed_templates},
 :description=>"Name",
 :required=>true,
 :notes=>nil,
 :display=>:edit,
 :data_type=>:integer,
 :notes_display=>:show,
 :values=>
  [#<OpenStruct id=1, name="centos-6-v20131120", guid="c15a5418-9ede-417e-ae40-24e9ecbab959", uid_ems="11748647391859510935", platform="linux", logical_cpus=0, mem_cpu=0, allocated_disk_storage=nil, v_total_snapshots=0, evm_object_class=:Vm, operating_system=#<OpenStruct product_name="linux_centos">, ext_management_system=#<OpenStruct name="google">>,
   #<OpenStruct id=2, name="centos-6-v20140318", guid="4df26ed9-c38e-44e8-8ccb-d605f4625a3b", uid_ems="11743140967858608122", platform="linux", logical_cpus=0, mem_cpu=0, allocated_disk_storage=nil, v_total_snapshots=0, evm_object_class=:Vm, operating_system=#<OpenStruct product_name="linux_centos">, ext_management_system=#<OpenStruct name="google">>,
...

We shouldn't store the workflow object in the session at all.

@jrafanie jrafanie added the bug label Sep 24, 2021
@chessbyte
Copy link
Member

What is the recommended approach? Store the IDs in the session and look up the objects when needed?

@jrafanie
Copy link
Member Author

What is the recommended approach? Store the IDs in the session and look up the objects when needed?

That would be an ideal solution but it looks more complicated than that. The code is deeply tied together and I can't tell what parts of the workflow object the UI needs in session. The only way I can think of doing it is just not store it in session and see what breaks. Then, store just what you need in session in a new object to get past that error. Rinse/repeat.

There were 80 references to @edit[:wf] in https://github.com/ManageIQ/manageiq-ui-classic/blob/9f6ee57289614ce9ce62d1f4c25bf8f6ad3a0e2e/app/controllers/application_controller/miq_request_methods.rb alone. There are at least 4 types of workflows being stored so I don't expect this is an easy task.

@chessbyte
Copy link
Member

what if the setters to @edit[:wf] convert the object to a string like "class_name:id" and the getters parse that to find the object in the database? Maybe through 2 new methods workflow_object_to_string and workflow_string_to_object?

@jrafanie
Copy link
Member Author

what if the setters to @edit[:wf] convert the object to a string like "class_name:id" and the getters parse that to find the object in the database? Maybe through 2 new methods workflow_object_to_string and workflow_string_to_object?

Yeah, that would work but many areas in miq_request_methods try to access fields / dialogs and refresh data on demand so I think the default is to get much of the data up front in the wf object, cache it in the object stored to session and refresh it as needed. If this is the case, I'd imagine, the getter would need to either eager load the full object on load or lazily load the wf components on demand. Both of these could have performance implications and complexity.

Additionally, is it possible that we store wf objects that aren't yet persisted with an id? I'm not sure. We've done it before for authentications we need to verify before we create.

Basically, I have more questions than answers.

@Fryguy
Copy link
Member

Fryguy commented Sep 27, 2021

I think the basic problem is that the workflow is a state based object. As users change their selections, things like the allowed lists are updated, and then the UI needs to reflect that. If we throw that away, we probably need to recalculate all of the allowed lists over again. However, that may not be a bad thing. Ultimately this will go to react though, and the nice thing about react is the state is held on the page on the client side (as opposed to server side for a regular Rails page). There can be API calls then which optimize building/filtering the allowed lists. @agrare and I have discussed designing that; we should probably bump the priority on that.

@chessbyte
Copy link
Member

Wondering if this is related to #7144

@jrafanie
Copy link
Member Author

jrafanie commented Oct 6, 2021

Wondering if this is related to #7144

Yes, good find!

It looks like @agrare had large dialogs in #7144 bloating the workflow whereas we had a large number of deprecated templates for GCE bloating the workflow in ManageIQ/manageiq#21455 and it's related google provider issue: ManageIQ/manageiq-providers-google#198

Since both dialogs and templates are stored in the provisioning workflow, which is saved into session, the end result is the same, too much data and you can't save it in session.

@Fryguy
Copy link
Member

Fryguy commented Oct 7, 2021

So that means we can close #7144?

@jrafanie
Copy link
Member Author

jrafanie commented Oct 8, 2021

So that means we can close #7144?

yeah, I closed the other. This seems like the more general issue.

@miq-bot
Copy link
Member

miq-bot commented Feb 27, 2023

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

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

@Fryguy Fryguy removed the stale label Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants