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

task/WP-189 Handle timeout exit code for interactive app jobs #851

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

shayanaijaz
Copy link
Contributor

@shayanaijaz shayanaijaz commented Aug 23, 2023

Overview

Fixes an issue where after timing out an interactive job is displayed as FAILED when in fact the job was successful and should have the FINISHED state because it timed out after reaching the maximum allotted time.

Related

Changes

The changes implemented here are on the portal side where a check is made to confirm if the job is interactive and whether or not it ended because of a timeout. On the tapis side the job state remains FAILED but we are able to use the exit code provided in the remoteResultInfo attribute of a job to determine if it was a timeout and display a success message on the portal side. This change is made in JobsView and JobsWebhookView endpoints

Testing

  1. Enable webhook notifications in order to see job status updates without page refresh
  2. Go to Applications inside the portal and select an interactive app such as Jupyter Lab HPC
  3. Set the Maximum Job Runtime to a low number such as 1
  4. Start the job and after successful submission go to the History tab to monitor job progress
  5. The job should go through the regular job states and should end with the FINISHED state after reaching the maximum runtime limit

UI

  • No UI changes

Notes

Follow up question: Does the last status message displayed in the job detail modal need to be modified as well? Currently it shows the timeout error message sent from Tapis

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #851 (d87a5eb) into main (6c40f35) will decrease coverage by 0.01%.
The diff coverage is 61.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #851      +/-   ##
==========================================
- Coverage   63.05%   63.05%   -0.01%     
==========================================
  Files         425      426       +1     
  Lines       12112    12133      +21     
  Branches     2475     2479       +4     
==========================================
+ Hits         7637     7650      +13     
- Misses       4275     4279       +4     
- Partials      200      204       +4     
Flag Coverage Δ
javascript 68.84% <ø> (ø)
unittests 57.31% <61.90%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage
server/portal/apps/workspace/api/views.py 20.00%
server/portal/apps/workspace/api/utils.py 69.23%
server/portal/apps/webhooks/views.py 100.00%

@shayanaijaz shayanaijaz marked this pull request as ready for review August 24, 2023 15:56
@shayanaijaz shayanaijaz marked this pull request as draft August 29, 2023 19:11
@shayanaijaz shayanaijaz marked this pull request as ready for review August 30, 2023 17:28
Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, nice work!

Copy link
Contributor

@van-go van-go left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rstijerina rstijerina merged commit e2e3e5b into main Aug 31, 2023
4 of 6 checks passed
@rstijerina rstijerina deleted the task/WP-189--intractive-apps-exit-code-handling branch August 31, 2023 19:41
chandra-tacc pushed a commit that referenced this pull request Sep 13, 2023
* added success message and state for interactive job on timeout

* linting

* created a central utility function, added logic to determine timeout

* remove commented code

* Added a comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants