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

Restart jobs, tasks and allocs #122

Merged
merged 3 commits into from Jan 29, 2024

Conversation

cshintov
Copy link
Contributor

@cshintov cshintov commented Jan 21, 2024

Implements #120

Implemented:
- Add restart task on key t
- Load job tasks after restarting a task
TODO:

  • restart alloc
  • restart job

@robinovitch61
Copy link
Owner

Thanks for starting work on this @cshintov ! Check out some of my thoughts thoughts here on overall design for admin stuff and LMK what you think.

For this PR, consider incorporating some of that design - it would be cool if the "admin task" table had just a single entry for now, "Restart Job/Allocation " by the end of this work. You can do jobs and allocations/tasks separately over different PRs if you'd like.

For each PR, target the next branch, as that's where new feature development should live.

@cshintov cshintov changed the base branch from main to next January 22, 2024 17:29
@cshintov
Copy link
Contributor Author

Continuing here.

Thanks for these thoughts. I like the X -> admin menu idea. Makes it explicit and cautions the users.
But I think the typing yes to do the action, a little tedious in a scenario like @attachmentgenie got into. I like the k9s way. Opening a pop up window, press tab to cancel or ok buttons, default can be cancel, and press enter to do the action.

If we can implement the pop up window nicely, that sounds great!

Looked into how this can be implemented. Not natively supported by lip gloss. It's in the pipeline.

There's a work around though!
charmbracelet/lipgloss#102 (comment)

I think we can postpone this and go with your idea of typing yes for now. What do you think? Should I try to implement the pop up window like mentioned in the above comment instead?

@robinovitch61
Copy link
Owner

Continuing here.

Thanks for these thoughts. I like the X -> admin menu idea. Makes it explicit and cautions the users.

But I think the typing yes to do the action, a little tedious in a scenario like @attachmentgenie got into. I like the k9s way. Opening a pop up window, press tab to cancel or ok buttons, default can be cancel, and press enter to do the action.

If we can implement the pop up window nicely, that sounds great!

Looked into how this can be implemented. Not natively supported by lip gloss. It's in the pipeline.

There's a work around though!

charmbracelet/lipgloss#102 (comment)

I think we can postpone this and go with your idea of typing yes for now. What do you think? Should I try to implement the pop up window like mentioned in the above comment instead?

I'm happy either way! If the workaround is too complicated and/or doesn't handle edge cases well (eg changing the terminal size while the popup is open breaks things), let's stick with the "type yes" screen for now. If user expediency is a concern, we can accept just typing "y" too.

@cshintov
Copy link
Contributor Author

Btw, we can't restart a job. I Checked the api code. There's no such function. Probably why the UI also doesn't provide a button for this.

When I want to restart a job, I usually add a dummy meta block to simulate/trigger a job restart. Maybe we can do that here as well.

@robinovitch61
Copy link
Owner

Btw, we can't restart a job. I Checked the api code. There's no such function. Probably why the UI also doesn't provide a button for this.

When I want to restart a job, I usually add a dummy meta block to simulate/trigger a job restart. Maybe we can do that here as well.

Interesting! Could we issue a stop, poll for when it's stopped, then issue a start? I'm afk so can't look at the nomad api right now.

Don't feel you have to implement more than one admin action in this first PR. I would pick the one that's easiest and add just that to the list of admin actions for now.

@cshintov
Copy link
Contributor Author

Have implemented just restarting tasks for now, although Admin Menu lists others as well. Will remove.

The UI is not very nice, but does the job.

wander-restart-task.webm

@robinovitch61
Copy link
Owner

Have implemented just restarting tasks for now, although Admin Menu lists others as well. Will remove.

The UI is not very nice, but does the job.

wander-restart-task.webm

Nice! I'll give the code a review this weekend :)

@cshintov cshintov force-pushed the restart-jobs-tasks-allocs branch 3 times, most recently from e62ecfb to 5a00563 Compare January 27, 2024 09:32
Pressing `X` on `JobTasks` page will go to `Admin Menu` which will
list variuos admin tasks along with trigger keys. Pressing any of them
will go to a confirmation page; then either you can confirm by pressing `y`
or go back to `Admin Menu` by `esc`.

Also added utility MyDebug function to log to separate file
other than `wander.log`.
Copy link
Owner

Choose a reason for hiding this comment

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

I'd be open to modifying Debug to read an optional environment variable's value, WANDER_DEBUG_PATH, and falling back to wander.log if it's not set. That way we don't need multiple debug functions and devs can still specify their preferred logging path if not wander.log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention was to rather redirect specific temporary debug logs to a second file for ease of parsing the logs. But the second function can be added ad hoc, no need to keep it in the code. Have removed it.

Btw, what do you use for your debugging, delve or wander.log?

README.md Outdated Show resolved Hide resolved
@robinovitch61
Copy link
Owner

Remember to modify GetPageKeyHelp to include the help text for X admin. I think we'll want it to the right of ctrl+w toggle wrap on the second row of the relevant pages.

@robinovitch61
Copy link
Owner

@cshintov I played around a bit here: #124

check it out and steal whatever you'd like to from there into your work here (I don't plan on merging my PR, it's just for sharing purposes)

@cshintov
Copy link
Contributor Author

cshintov commented Jan 28, 2024

@cshintov I played around a bit here: #124

check it out and steal whatever you'd like to from there into your work here (I don't plan on merging my PR, it's just for sharing purposes)

Thanks, that looks more clean, so I've stolen it all 😂!

I've added the X -> Admin help and returning FailureMsg as you suggested.

Screenshare.-.2024-01-28.11.10.00.PM.webm

@cshintov cshintov marked this pull request as ready for review January 28, 2024 17:25
@robinovitch61
Copy link
Owner

@cshintov I played around a bit here: #124
check it out and steal whatever you'd like to from there into your work here (I don't plan on merging my PR, it's just for sharing purposes)

Thanks, that looks more clean, so I've stolen it all 😂!

I've added the X -> Admin help and returning FailureMsg as you suggested.

Screenshare.-.2024-01-28.11.10.00.PM.webm

Looks great! Will give this a final review today

Copy link
Owner

@robinovitch61 robinovitch61 left a comment

Choose a reason for hiding this comment

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

Looks great! Left some final comments for you to consider, but feel free to merge whenever you feel good about it. Nice job, thanks so much for this!

@cshintov cshintov force-pushed the restart-jobs-tasks-allocs branch 2 times, most recently from 93eabf7 to d5b673a Compare January 29, 2024 17:58
Add toast for failures in admin actions
@cshintov
Copy link
Contributor Author

Made changes according to the review. Think it's ready now.

but feel free to merge whenever you feel good about it.

But I don't have permission to merge!

@robinovitch61 robinovitch61 merged commit 86fb38c into robinovitch61:next Jan 29, 2024
1 check passed
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.

None yet

2 participants