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

job_queue: add logging of long build queue #20

Merged
merged 1 commit into from
May 13, 2024
Merged

Conversation

carlocab
Copy link
Member

This will make it easier to monitor whether the new queueing system is
behaving as expected.

@mutex = Mutex.new
@queue = Hash.new { |h, k| h[k] = [] }
@queue_type = queue_type
@condvar = ConditionVariable.new
@logger = logger
Copy link
Member Author

Choose a reason for hiding this comment

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

This feels a bit like violence, but I wasn't sure how else to do it. Suggestions welcome.

Copy link
Member

Choose a reason for hiding this comment

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

Seems fine. Could pull in Rails logging gems if you wanted something better?

Reminds me that Sorbet feels like it would be a really good fit for ci-orchestrator.

Copy link
Member

Choose a reason for hiding this comment

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

Reminds me that Sorbet feels like it would be a really good fit for ci-orchestrator.

Yeah, orka_api_client supports Sorbet very well but doesn't work with it being a git gem.

However I might be swapping out that for a K8s backed solution now that the Orka 3 changed things, so watch this space.

Comment on lines +32 to +33
job = @queue[:long].shift
@logger.call("Long build slot available. Scheduling #{job.runner_name} for deployment...")
break job
Copy link
Member

Choose a reason for hiding this comment

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

This is already implied by the existing Deploying VM for job *-long tbh

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that, but not quite, since other things can happen between a job getting popped and and VM being deployed.

src/job_queue.rb Outdated
Comment on lines 28 to 29
@logger.call("Running Long Build Jobs: #{running_long_build_count}")
@logger.call("Available Long Build Slots: #{long_build_slots}")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe having a webpage tab with numbers might be more useful here, including total number of jobs deployed and slots available.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, a dashboard would be useful, but I think logs are also important so I can see how these values evolve over time. Not sure if you also had a log in mind with a separate tab.

Copy link
Member

@Bo98 Bo98 May 13, 2024

Choose a reason for hiding this comment

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

Ok, fair. Thoughts on combining these two into one line so it doesn't fill up the log history limit as much?

e.g. Long builds: x running, x available

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

This will make it easier to monitor whether the new queueing system is
behaving as expected.
@Bo98 Bo98 merged commit bdce7b2 into main May 13, 2024
4 checks passed
@Bo98 Bo98 deleted the verbose-long-build-logs branch May 13, 2024 14:46
@Bo98
Copy link
Member

Bo98 commented May 13, 2024

Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants