Dashboard: Processing erb on each request to generate the description information produces performance issues

[ericfranz]

Note: this should be introduced under a feature flag that disables the new behavior by default. That way it can be released in the patch release and be optionally enabled by interested sites. The default can change in a later release.

Updated April 26, 2021

The logic currently is to:

• render form.yml.erb (or load form.yml), and use title and description as provided, otherwise defaulting to the manifest

With this issue the new logic would be:

• for nav, always use the default from the manifest (or the titelized form of the app name if a manifest is not present)
• for the desktop apps or other subapps, a new manifest location would be added - next to the form.yml so /etc/ood/config/apps/bc_desktop/owens.yml.erb would use /etc/ood/config/apps/bc_desktop/owens.manifest.yml
• the filtering out of apps by "valid?" checks from the nav bar would be omitted

The technical design for this is as follows:

• add factory method and move the loading and rendering of the form.yml to this method and pass the resulting form config hash as an argument; will want to support passing in validation_reason errors as well
• add second factory method that passes in an empty hash and use this when working with the app object for navigation
• omit the calls to valid? on batch connect apps in OodApp class

With a configuration option we could switch it back to the previous functionality. We could also wait until someone complains that the previous functionality is necessary and worth the performance cost before adding back this support.

NOTE: before doing this, sub_app_list must be refactored out of all the places it is used and the knowledge of that relationship should only be in one place - the router that creates the OodApp objects. Then the new functionality will need to only create OodApp objects for those "apps" that have a manifest.yml (or subapp.manifest.yml). This is how we address the bug where currently a sub_app_list pulls in any yml/yml.erb as a subapp, and only after validating the rendered erb does it determine it is not (such as submit.yml.erb in /etc).

Old description

We load and process the batch connect form.yml.erb files to generate the title and description of the iHPC apps. Unfortunately, this means that any expensive parts of the form.yml.erb are also executed. If there is code to build a queue list from a command, for example, that will be executed for each iHPC app on every request to the dashboard. This is terrible for performance.

┆Issue is synchronized with this Asana task by Unito

[ericfranz]

A bigger problem though is the fact that the title and description can be set in the form.yml for ihpc apps, and if its a form.yml.erb, that erb could potentially affect the title and description.

Even with a solution like https://github.com/OSC/ood-dashboard/issues/418 the form.yml.erb will still be processed when building the cache, whether it is done every time a new Passenger app instance loads (which means it would be re-done every 5 minutes if a user is inactive for 5 minutes after accessing the dashboard).

We have several cases:

1. I have a form.yml. I'd prefer a cache for the menu and don't care if users have to restart their PUNs or wait for their Passenger apps to stop before seeing the yaml update.
2. I have a form.yml. I'd prefer caching but proper cache invalidation if the form.yml is modified.
3. I have a form.yml.erb that specifies a static title and description in the yaml, but contains expensive erb code for getting a list of queues, etc. when building the web form. I'm not doing app sharing or heavy app development. I want the title and description to display without the erb being processed for iHPC apps and I want to cache the apps list at PUN start.
4. I have a form.yml.erb that uses a dynamic title and description. I want the title and description to update on every request.
5. I have a form.yml.erb that uses a dynamic title and description. I want the title and description to update when the menu cache is built.
6. I have form.yml.erb that uses a dynamic title and description and other erb code that is expensive. I want the title and description to update when the menu cache is built but I only want to have the expensive erb processed when the web form is accessed.

[ericfranz]

This problem gets a little worse.

1. We have this concept of "subapps" in batch connect that is used solely by the bc_desktop app. So the subapp specifies a title and description in the form.yml.erb file i.e. for us at /etc/ood/config/apps/bc_desktop/ we have oakley.yml which defines title: "Oakley Desktop"
2. We have the idea of form validation where each form specifies a cluster (oakley, ruby) and then if you do not have access to that cluster (ruby is restricted) you do not see the app in the navigation
3. As a result of 1 & 2 above, we load the form config (which includes the erb processing), when building the navigation. This is currently done on each request.
4. We then have documentation like https://osc.github.io/ood-documentation/release-1.3/app-development/tutorials-interactive-apps/add-custom-queue/local-dynamic-list.html which encourages placing erb code in the form.yml.erb that does non trivial amount of code, such as building a list of queues from executing sinfo. This encourages admins to add erb code, intended to only be run when the user accesses the app, that inadvertently gets executed when building a navigation, every request to the dashboard. This is even worse when the code is duplicated across all of the dashboard sub apps because you want that queue dropdown to appear in each.

[ericfranz]

Should probably require a separated manifest for each ihpc app (or subapp) which has the title, description, and list of clusters.

Will have to think of how to support backwards compatibility while adding this. One way would be if the manifest does not specify a cluster (or array of clusters) we then load the form.yml to get this. If manifest has title, description, and cluster we just use that and avoid parsing the form.yml.

We could also optimize the code for building the navigation by using an in memory cache of apps users have access to.

[ericfranz]

Change for 1.5 release:

https://github.com/OSC/ood-dashboard/blob/910d96e4ab06328db1fa47a830dc8a5f5f0a29da/app/models/batch_connect/app.rb#L107

By using default_title and "" for the description, we avoid rendering erb. However, we still have the problem of "You do not have access to use this app." situation, like we have with ruby-vdi. A simple solution is to say we are not going to remove that from the menu and instead recommend using file permissions on this file. So 640 ruby-vdi and only users in the ruby group have access to it.

Enforcing authorization any other way is too expensive. Unless that authorization can be enforced by adding a new line to the manifest.yml file.

Of course, another option is to just introduce caching for the entire menu.

[ericfranz]

Maybe the solution for backwards compatibility in some form is to support a manifest.yml.erb.