-
Notifications
You must be signed in to change notification settings - Fork 5
murdock: implement numeric priorities #18
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
Open
kaspar030
wants to merge
10
commits into
main
Choose a base branch
from
switch_to_priority_queue
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
e0c180e
murdock: drop fasttrack_queue, use PriorityQueue
kaspar030 31510be
introduce MurdockJob.priority
kaspar030 061cd13
add "priorities" to MurdockSettings
kaspar030 9f85877
apply config.priorities in `MurdockJob.calculate_priority()`
kaspar030 6797afa
fixup! add "priorities" to MurdockSettings
kaspar030 37b7d62
murdock/murdock.py: get_queued_jobs() sort before `job.model()`
kaspar030 3b09dbe
murdock/murdock.py: handle priority label changes
kaspar030 f3ab8a6
fixup! murdock/murdock.py: handle priority label changes
kaspar030 50b9627
fixup! fixup! murdock/murdock.py: handle priority label changes
kaspar030 429950f
murdock.py: drop obsolete `Database()`
kaspar030 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -75,7 +75,6 @@ def __init__( | |||||
| self.queued: MurdockJobList = MurdockJobList() | ||||||
| self.running: MurdockJobPool = MurdockJobPool(num_workers) | ||||||
| self.queue: asyncio.Queue = asyncio.Queue() | ||||||
| self.fasttrack_queue: asyncio.Queue = asyncio.Queue() | ||||||
| self.notifier = Notifier() | ||||||
| self.instrumentator = Instrumentator() | ||||||
| self.db = database(database_type) | ||||||
|
|
@@ -204,18 +203,13 @@ async def job_processing_task(self): | |||||
| structlog.contextvars.bind_contextvars(worker=current_task) | ||||||
| LOGGER.debug("Starting worker") | ||||||
| while True: | ||||||
| if self.fasttrack_queue.qsize(): | ||||||
| job = self.fasttrack_queue.get_nowait() | ||||||
| try: | ||||||
| job = await self.queue.get() | ||||||
| await self._process_job(job) | ||||||
| self.fasttrack_queue.task_done() | ||||||
| else: | ||||||
| try: | ||||||
| job = await self.queue.get() | ||||||
| await self._process_job(job) | ||||||
| self.queue.task_done() | ||||||
| except RuntimeError as exc: | ||||||
| LOGGER.warning("Exiting worker", exception=str(exc)) | ||||||
| break | ||||||
| self.queue.task_done() | ||||||
| except RuntimeError as exc: | ||||||
| LOGGER.warning(f"Exiting worker {current_task}: {exc}") | ||||||
| break | ||||||
|
|
||||||
| async def job_prepare(self, job: MurdockJob): | ||||||
| logger = LOGGER.bind(**job.logging_context) | ||||||
|
|
@@ -291,14 +285,13 @@ async def add_job_to_queue(self, job: MurdockJob): | |||||
| "target_url": self.base_url, | ||||||
| }, | ||||||
| ) | ||||||
| all_busy = all(running is not None for running in self.running.jobs) | ||||||
| self.queued.add(job) | ||||||
| job.state = "queued" | ||||||
| if all_busy and job.fasttracked: | ||||||
| self.fasttrack_queue.put_nowait(job) | ||||||
| else: | ||||||
| self.queue.put_nowait(job) | ||||||
| logger.info("Job added to queued jobs") | ||||||
|
|
||||||
| self.queue.put_nowait(job) | ||||||
|
|
||||||
| LOGGER.info(f"{job} added to queued jobs") | ||||||
|
|
||||||
| self.job_queue_counter.inc() | ||||||
| await self.reload_jobs() | ||||||
|
|
||||||
|
|
@@ -538,23 +531,36 @@ async def handle_pull_request_event(self, event: dict): | |||||
| await set_commit_status(job.commit.sha, status) | ||||||
| return | ||||||
|
|
||||||
| if action in ["unlabeled", "edited"]: | ||||||
| if action in ["edited"]: | ||||||
| return | ||||||
|
|
||||||
| if action in ["unlabeled"]: | ||||||
| label_name = event["label"]["name"] | ||||||
| if label_name not in config.priorities.labels.keys(): | ||||||
| return | ||||||
| elif not self.queued.search_by_pr_number(pull_request.number): | ||||||
| # skip re-queing if there's no queued job | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tiny typo:
Suggested change
|
||||||
| return | ||||||
|
|
||||||
| if action == "opened" and CI_CONFIG.ready_label in pull_request.labels: | ||||||
| # A PR opened with "Ready label" already set will be scheduled via | ||||||
| # the "labeled" action | ||||||
| return | ||||||
|
|
||||||
| if action == "labeled": | ||||||
| if event["label"]["name"] != CI_CONFIG.ready_label: | ||||||
| # return if the ready label was set for an already queued job, | ||||||
| # or if the label wouldn't change the job's priority (and thus | ||||||
| # queue position). | ||||||
| # otherwise, fall through to re-scheduling the job. | ||||||
| label_name = event["label"]["name"] | ||||||
| if label_name == CI_CONFIG.ready_label: | ||||||
| # Skip already queued jobs for ready label | ||||||
| if self.queued.search_by_pr_number(pull_request.number): | ||||||
| return | ||||||
| elif label_name not in config.priorities.labels.keys(): | ||||||
| return | ||||||
| # Skip already queued jobs | ||||||
| if event["label"][ | ||||||
| "name" | ||||||
| ] == CI_CONFIG.ready_label and self.queued.search_by_pr_number( | ||||||
| pull_request.number | ||||||
| ): | ||||||
| elif not self.queued.search_by_pr_number(pull_request.number): | ||||||
| # skip re-queing if there's no queued job | ||||||
| return | ||||||
|
|
||||||
| await self.schedule_job(job) | ||||||
|
|
@@ -640,13 +646,17 @@ async def reload_jobs(self): | |||||
| await self.notify_message_to_clients(json.dumps({"cmd": "reload"})) | ||||||
|
|
||||||
| def get_queued_jobs(self, query: JobQueryModel = JobQueryModel()) -> List[JobModel]: | ||||||
| return sorted( | ||||||
| [ | ||||||
| job.model() | ||||||
| for job in self.queued.search_with_query(query) | ||||||
| if query.states is None or "queued" in query.states | ||||||
| ], | ||||||
| key=lambda job: job.fasttracked, # type: ignore[return-value,arg-type] | ||||||
| return list( | ||||||
| map( | ||||||
| lambda job: job.model(), | ||||||
| sorted( | ||||||
| [ | ||||||
| job | ||||||
| for job in self.queued.search_with_query(query) | ||||||
| if query.states is None or "queued" in query.states | ||||||
| ] | ||||||
| ), | ||||||
| ) | ||||||
| ) | ||||||
|
|
||||||
| def get_running_jobs( | ||||||
|
|
||||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all these function parameters are already instance attributes. Thus there's no need for
calculate_priorityto be a staticmethod. A regular instance method, private, should be enoughdef _priority(self):and useself.config,self.pr,self.refandself.fastrackedinside.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that an
@propertymethod could also be used but it would recompute the priority each time it's accessed. Maybe that's better I don't knowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure, can safely use a class method within
__init__()?(Also it needs to be public so it can react to label changes.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, if the method is accessing already defined/initialized attributes.
then I think a property method is more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a property method would make e.g., "job.priority" actually call the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assuming the pr field of a job is a reference to updated PR data.
if that's not the case, it's easier to have this staticmethod and supply all needed data at the call site.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you requeue ? In the current state, a new MurdockJob instance is created and the previous matching ones are marked as cancelled.
I also thought that the display order must also be taken into account. This is something that is in another data structure, a simple list. So a sort on priority must be added there.
https://github.com/murdock-ng/murdock/blob/e62397d29e8ffe1ded2bbb2dd0a7f7a0c49ee853/murdock/murdock.py#L561-569
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add re-queueing here:
murdock/murdock/murdock.py
Line 478 in e62397d
(if the job is queued, re-calc its priority and compare to the queued one. if it has changed, don't "return" but fall through to "schedule_job()", which would cancel the old job and create a new, sorted into the queue by priority.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit lost. What is the use case exactly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR blew up from just replacing two unprioritized queues with one prioritized, to making full use of that change. :( I'll update the description.