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

iHPC apps to submit to multiple clusters #424

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ericfranz
Copy link
Contributor

commented Dec 12, 2018

Fixes #282

Client side:

  1. if there is only one depdendent cluster the user can submit jobs to, that adapter is used for the "format" for client side rendering (i.e. 'format' for the bc_* attributes like bc_num_slots)
  2. if there are multiple clusters of same adapter, that is used for "format" for client side rendering
  3. if there are multiple clusters of different adapters, the first in the list is used (BUG - not currently fixed nor warned - also rarer edge case)

Server side:

  1. server side if there is only one depdendent cluster the user can submit jobs to, that adapter is used for "format" and that id is used for cluster_id
  2. server side if there is multiple dependent clusters available, and the id of one is specified in the web form submission paramter name "batch_connect_session_context[cluster]" that is used for adapter "format" and cluster_id
  3. server side, if there is multiple dependent clusters available, and one is not specified, or if there are no dependent clusters available, an error is thrown

The result?

In the cluster config, in place of "cluster: owens" you can do "cluster: owens, pitzer, ruby, oakely, quick". Then if the iHCP app specifies an attribute named "cluster" whose value is one of the ids in this list when the form is submitted, that cluster will be used during job submission, and that cluster's adapter will be used when generating the submit hash for bc_num_slots.

ericfranz added some commits Dec 12, 2018

ihpc apps to submit to multiple clusters
if there is only one depdendent cluster the user can submit jobs to, that adapter is used for the "format" for client side rendering
if there are multiple clusters of same adapter, that is used for "format" for client side rendering
if there are multiple clusters of different adapters, the first in the list is used (BUG - not currently fixed nor warned - also rarer edge case)

server side if there is only one depdendent cluster the user can submit jobs to, that adapter is used for "format" and that id is used for cluster_id
server side if there is multiple dependent clusters available, and the id of one is specified in the web form submission paramter name "batch_connect_session_context[cluster]" that is used for adapter "format" and cluster_id
server side, if there is multiple dependent clusters available, and one is not specified, or if there are no dependent clusters available, an error is thrown
remove test that is no longer relevant
accidentally included a test for uncommitted code that attempted
to define a clusters dropdown when multiple clusters were declared
this method would be providing that:

default_cluster_attribute_options

postponing that solution at this time
@ericfranz

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@MorganRodgers this has not been thoroughly tested yet; also this omits two things:

  1. doesn't address if you declare dependencies on multiple clusters of different adapter types (torque and slurm in our future case)
  2. doesn't attempt to auto create a select element that let the user set the cluster

@ericfranz ericfranz requested a review from MorganRodgers Dec 12, 2018

@ericfranz

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@MorganRodgers this has not been well tested nor are there comprehensive tests added. I wouldn't recommend merging this right away unless you are confident of these changes (or at least that these changes don't break anything). Regardless the code is ugly - that is partially reflective of the complexity of the code that was modified.

@ericfranz

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

Some of @MorganRodgers thoughts from an Asana task:

  • Some conditionals are incredibly long and should be extracted into methods
  • Better automated tests would be great
  • A Vagrant setup where we could test multiple clusters / queues would be great (quick with Slurm, slower with SGE)
@MorganRodgers
Copy link
Contributor

left a comment

This does not yet appear to be functioning. A test OnDemand environment with SGE and Slurm installed is available for developing this against a live system: https://github.com/MorganRodgers/sge-slurm-workbench

Other comments inline.

if @app.cluster_dependencies.one?
@cluster = @app.cluster_dependencies.first
elsif @app.cluster_dependencies.any? && permitted_params[:batch_connect_session_context] && permitted_params[:batch_connect_session_context][:cluster]
cluster = @app.cluster_dependencies.find { |cluster| cluster.id == permitted_params[:batch_connect_session_context][:cluster] }

This comment has been minimized.

Copy link
@MorganRodgers

MorganRodgers Apr 23, 2019

Contributor

Shouldn't this be @cluster?


if @app.cluster_dependencies.one?
@cluster = @app.cluster_dependencies.first
elsif @app.cluster_dependencies.any? && permitted_params[:batch_connect_session_context] && permitted_params[:batch_connect_session_context][:cluster]

This comment has been minimized.

Copy link
@MorganRodgers

MorganRodgers Apr 23, 2019

Contributor

As noted in Asana discussion this is a long conditional because of the keys, ideally it would be broken up into one or more methods.

# The clusters that the batch connect app uses
# invalid cluster ids are ignored
# @return [Array<OodCore::Cluster>] clusters that app uses
def cluster_dependencies

This comment has been minimized.

Copy link
@MorganRodgers

MorganRodgers Apr 23, 2019

Contributor

I would prefer to call these something other than 'dependencies', like 'options'. 'Dependencies' makes me think that the job will not run if any of the listed clusters are not available.

return @validation_reason if @validation_reason

if !cluster_id
"This app does not specify a cluster."

This comment has been minimized.

Copy link
@MorganRodgers

MorganRodgers Apr 23, 2019

Contributor

What replaces this check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.