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

Start resources on demand #118

Merged
merged 5 commits into from
Aug 9, 2023
Merged

Start resources on demand #118

merged 5 commits into from
Aug 9, 2023

Conversation

praiskup
Copy link
Owner

@praiskup praiskup commented Jun 30, 2023

Missing stuff

  • test is missing
  • documentation is missing

Copy link
Collaborator

@FrostyX FrostyX left a comment

Choose a reason for hiding this comment

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

There is nothing that I have a problem with, so +1 from me.

But I must be missing something because the PR is much more complicated than I would expect.

As "resources on demand" I would expect something like --on-demand parameter or --tag on-demand for the resalloc ticket command. Which would basically just set high priority and ignore all limits to spawn the resource ASAP.

The linked Copr issue says "Provide a few high-performance builders (for demanding software like chromium)" which is IMHO a different concept than on-demand resources. But in this case, I would expect only a new pool configuration that would provide beefy machines.

This PR seems to do more things. Maybe let's discuss on some standup.

else:
assert False
tags.clear()
tags.extend(new_tags)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function deserves some tests. It is very testing-friendly.

@praiskup praiskup force-pushed the on-demand branch 4 times, most recently from ed2f7d2 to 6c37d0c Compare August 2, 2023 08:33
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@praiskup praiskup force-pushed the on-demand branch 3 times, most recently from c3ad2ee to 49b7a68 Compare August 2, 2023 13:37
resallocserver/priority_queue.py Dismissed Show dismissed Hide dismissed
@@ -2,6 +2,11 @@
Python priority queue implementation.
"""

# TODO: rename "Task" to "Item" if we want to make this a library, we can
# place any kind of resource into the Queue (Pools, Tickets, etc.).
# TODO: enforce the PriorityQueueTask use. Using the default __repr__ for

Check warning

Code scanning / vcs-diff-lint

TODO: enforce the PriorityQueueTask use. Using the default repr for Warning

TODO: enforce the PriorityQueueTask use. Using the default __repr__ for
Copy link
Owner Author

Choose a reason for hiding this comment

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

I'd prefer keeping these TODO entries.

resallocserver/logic.py Fixed Show fixed Hide fixed
@praiskup praiskup force-pushed the on-demand branch 2 times, most recently from 952a31d to de9be38 Compare August 3, 2023 12:25
praiskup added a commit to praiskup/copr that referenced this pull request Aug 5, 2023
praiskup added a commit to praiskup/copr that referenced this pull request Aug 7, 2023
praiskup added a commit to praiskup/copr that referenced this pull request Aug 7, 2023
praiskup added a commit to praiskup/copr that referenced this pull request Aug 7, 2023
praiskup added a commit to praiskup/copr that referenced this pull request Aug 7, 2023
praiskup added a commit to praiskup/copr that referenced this pull request Aug 8, 2023
Drop the unused class for now, and add some TODO items.  With those many
use-cases we have for PriorityQueue, we should have better method naming
and typechecking.
On-demand tickets need to be prioritized over the normal tickets
(priority queue), otherwise we risk that normal tickets take "on demand"
resources.

If we want to prefer one "on demand" pool over the other "on demand"
(e.g. spot AWS over normal instances), we need to sort them through a
priority queue based on tag priority.  The fallback for spawn failures
(e.g. if SPOT instances don't start) isn't resolved yet, though.

Fixes: #24
praiskup added a commit to praiskup/copr that referenced this pull request Aug 8, 2023
praiskup added a commit to fedora-copr/copr that referenced this pull request Aug 9, 2023
@praiskup praiskup requested a review from FrostyX August 9, 2023 22:20
Copy link
Collaborator

@FrostyX FrostyX left a comment

Choose a reason for hiding this comment

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

Oh, sorry, I started reviewing but forgot to finish it.

But I must be missing something because the PR is much more complicated than I would expect.

We discussed it last week on 1on1 call, so it makes more sense now.

@praiskup praiskup merged commit b19d446 into main Aug 9, 2023
2 of 3 checks passed
@praiskup praiskup deleted the on-demand branch January 10, 2024 15:59
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.

2 participants