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

Show max running jobs on /tests page #5279

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

perlpunk
Copy link
Contributor

@perlpunk perlpunk commented Aug 10, 2023

See individual commits

I think we could also show the actual limit value, e.g. x jobs are running (limited by server config: 250)
WDYT?

@perlpunk perlpunk force-pushed the show-max-running branch 2 times, most recently from e37b0fd to fc160ea Compare August 10, 2023 14:14
@perlpunk
Copy link
Contributor Author

I think we could also show the actual limit value, e.g. x jobs are running (limited by server config: 250) WDYT?

Oh wait, since this is only shown when the limit is reached I think this is redundant :)

Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

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

I'm fine with printing just "… (limited by server config)" in case the limit is effected. Otherwise it would be rather redundant. I'd also avoid a "x / y" format unless we are really sure not to end up with e.g. "201 / 200" in production (which would look rather buggy).

lib/OpenQA/Scheduler/Model/Jobs.pm Outdated Show resolved Hide resolved
assets/javascripts/tests.js Outdated Show resolved Hide resolved
assets/javascripts/tests.js Outdated Show resolved Hide resolved
lib/OpenQA/WebAPI/Controller/Test.pm Outdated Show resolved Hide resolved
If it is always a number, it is easier to handle.

Issue: https://progress.opensuse.org/issues/129619
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #5279 (5707d4e) into master (75a911b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5279   +/-   ##
=======================================
  Coverage   98.31%   98.31%           
=======================================
  Files         389      389           
  Lines       37184    37190    +6     
=======================================
+ Hits        36558    36564    +6     
  Misses        626      626           
Files Changed Coverage Δ
lib/OpenQA/Setup.pm 100.00% <ø> (ø)
t/config.t 100.00% <ø> (ø)
lib/OpenQA/Scheduler/Model/Jobs.pm 97.40% <100.00%> (ø)
lib/OpenQA/WebAPI/Controller/Test.pm 95.56% <100.00%> (+0.03%) ⬆️
t/ui/01-list.t 99.26% <100.00%> (+<0.01%) ⬆️

@perlpunk
Copy link
Contributor Author

perlpunk commented Aug 11, 2023

So I investigated the problem with max_running_jobs not always being set to the default value, causing warnings.
That is actually a problem of the tests, and it wouldn't happen in the productive code.
The problem is that apparently OpenQA::Setup::read_config is not always called when we create app instances in tests.

I think that's unfortunate because we have to cover for that in code by ensuring a default value in several places, although that should be done by OpenQA::Setup in just one place.
It would be good to make sure in tests we read the config and fill in defaults.

I will create a ticket because I had a quick look, and it looks to me like it's not fixable very fast.

If the limit is reached, people will see that in the header of the running
jobs table.

We do a COUNT because the job query can actually be limited by several
parameters, not giving us the complete count.

Issue: https://progress.opensuse.org/issues/129619
@mergify mergify bot merged commit 42529be into os-autoinst:master Aug 11, 2023
36 checks passed
@perlpunk perlpunk deleted the show-max-running branch August 15, 2023 09:47
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

5 participants