pages/Run/index.tsx: Add kill run button #51
Conversation
✅ Deploy Preview for pulpito ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Changes made:
TODO:
|
VallariAg
force-pushed
the
kill-run
branch
3 times, most recently
from
0771abe
to
2f683a1
Compare
|
To test this PR, follow instructions in the description of this PR: ceph/teuthology-api#47 Screen.Recording.2024-02-12.at.3.52.16.PM.mov |
There was a problem hiding this comment.
Sorry for the delay in retesting this!
I think we need to allow org admins to kill, and I'm getting errors trying to kill runs I own. I can't find details aside from the "Failed!" message that pops up in the UI.
If I dismiss the error dialog and click the button a second time, it skips the confirmation and displays the "Failed!" message again immediately, without making another API request.
|
I have added the logic for letting admins kill all runs on ceph/teuthology-api#55 I have improved error handling on t-api so it should not just show "Failed!" now and give more details, please let me know if it still doesn't work! Edit: To test this PR, use this docker-compose: ceph/teuthology#1957 Or to manually test this PR: Setup teuthology-api inside the teuthology container.
|
|
Hi Vallari, I had to remove Also, after executing the kill button, I get this error
However, even with the error above, we were still able to successfully kill the run we intended to kill. Not sure if I was doing something wrong, but let me know if you know anything about these errors. |
|
Here is a prettier version of the logs in the kill confirmation box above. |
|
Thank you for reviewing this, I'll take look at both the above problems! Looks like error handling can be improved more too. |
|
@kamoltat I have improved error handling so the logs are more readable. About the traceback, I think maybe your local teuthology branch might not be up-to-date with remote branch because Zack removed the nuke command from main which I see logged in that traceback. Maybe the latest teuthology commit might not have the problem. And for editing ".teuthology.yml" file: there's a note in t-api README that we can use
|
|
@VallariAg However, we |
|
@kamoltat both (running and waiting) seems to have a regression at the moment: https://tracker.ceph.com/issues/66440 Though I see in that screenshot that the log trace is not very readable. I have improved that in a recent commit in this PR. Pulling the latest commit of this branch should fix that. |
@VallariAg sounds good thanks |
|
Putting https://github.com/ceph/teuthology-api/pull/63/files |
|
I have updated the docker-compose file to only mount "teuthology_api/src": ceph/teuthology#1957 |
|
@VallariAg ceph/teuthology#1959 |
|
@kamoltat It'll resolve for running jobs, yes! For waiting there are more regressions (mentioned in the comments of that ticket). Update: we should be able to kill both running and queued runs. |
|
The kill button is greyed out for me, on this run: https://pulpito-ng-zmc.ceph.com/runs/zmc-2024-08-13_20:31:57-teuthology:no-ceph-main-distro-default-smithi Edit: Pointing at ceph/teuthology-api#55, the button is enabled, though the tooltip says I'm not an admin. Clicking it, and the confirmation after, shows hitting: https://teuthology.front.sepia.ceph.com:8999/kill?logs=true |
|
@zmc I think something might be not right with t-api deployment. I tried to sign in on the above pulpito URL and it showed "internal server error". Maybe some env variable is missing, I don't have access to t-api's traceback so I'm not sure why it failed. |
Signed-off-by: Devansh3712 <[email protected]>
1. create `Alert` and `KillButton` components
2. lib/teuthologyAPI: add useRunKill hook
(which uses `useMutation` to send POST req at /kill to t-api)
3. create lib/teuthologyAPI.d.ts and add 'KillRun'
Signed-off-by: Vallari Agrawal <[email protected]>
new features: 1. Disable kill-run button for finished runs 2. Hide kill-run button for users who don't own the run Signed-off-by: Vallari Agrawal <[email protected]>
This commit also removes "--user" from the t-api /kill route request. Signed-off-by: Vallari Agrawal <[email protected]>
Show error message received in response from api when killing a run. This commit also includes some minor UI improvements on KillButtonDialog. And removes "dry_run" query param from "useRunKill". Signed-off-by: Vallari Agrawal <[email protected]>
If the run belongs to the logged-in user, then it'll show "Kill" button. If not, then "Kill As Admin" btn. For now, pulpito-ng does not verify if user has admin privileges. That check happens on t-api after the request is sent. This commit also refreshes the mutation obj for kill-button, upon button click. This fixes the issue of retrying to kill runs by clicking the button again. Signed-off-by: Vallari Agrawal <[email protected]>
…logy" "scheduled_<username>@teuthology" is the default owner name if run is scheduled from teuthology CLI tool. This commit allows users of same github username to recognize it as their jobs. Signed-off-by: Vallari Agrawal <[email protected]>
Read "isUserAdmin" from useSession and disable "Kill as Admin" button if isUserAdmin is false Signed-off-by: Vallari Agrawal <[email protected]>
Signed-off-by: Vallari Agrawal <[email protected]>
Signed-off-by: Vallari Agrawal <[email protected]>
Request get redirected to /kill/?logs=true if trailing slash is missing. Signed-off-by: Vallari Agrawal <[email protected]>
Add getHelperMessage() to display better tooltip message on kill button. The message should help the user understand why the button is disabled and know if they are admin user. Signed-off-by: Vallari Agrawal <[email protected]>
Signed-off-by: Vallari Agrawal <[email protected]>
|
I was able to kill two runs that matched my github username: one with a queued job, and one with a running job. Both behaved properly. A tiny issue is that we don't refresh the job status on the page after killing, but we don't have to block this PR on that.
|
There was a problem hiding this comment.
Now that the feature works reliably and we've sorted out the CORS and other issues getting in the way, I'm reminded that we're still showing the button when we know it can't be used either because of permissions, or because the run has completed and there's nothing to kill.
We should hide the button when:
- The user is logged out
- The run is finished
- The user is not an admin, and the run belongs to someone else (90% sure on this one)
I think the above might actually all cases where the button would normally be greyed out.
Sorry to retract an approval @VallariAg, but I bet you'd agree that 1 and 2 are unhelpful, and likely 3 as well. Curious what @kamoltat thinks.
|
@zmc my logic for greying out the button is that I wanted to give a message to the user (with tooltip) explaining why they can't kill a run (otherwise they might wonder why they can't see the button). But I 100% agree that not having a button for 1 and 2 would be more intuitive. I can change that! I think for 3, benefits of having a disabled button is that:
|
|
Thanks @VallariAg, I think I agree with you about keeping the button in scenario 3 - adding the value to the tooltip is a good idea too. |
Fixes: ceph#51 (review) Signed-off-by: Vallari Agrawal <[email protected]>
Send Run query as KillButton props. Then do all processing of payload and ownership in KillButton component. This also refreshes Run query after killing a run. Signed-off-by: Vallari Agrawal <[email protected]>
There was a problem hiding this comment.
Thank you so much @Devansh3712 and @VallariAg for making this happen!
This PR solves issue #2 which adds a kill run button that will kill all jobs in the current run. It currently uses a mock API endpoint for success alert, needs to be connected to the teuthology-api
Screen.Recording.2024-02-12.at.3.09.07.PM.mov