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

Only fetch job queue in getCauseOfBlockage #2568

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

schiasileon
Copy link

Description

This fixes a performance issue on large Jenkins instances with huge queues.

Before this fix, blueocean would fetch all Jenkins queue items when cause of blockage is needed just to find out which item the run belongs to. While doing this, Jenkins has to execute a permission check on all queued Items, possibly leading to performance issues.

We documented and tested this issue on one of our Jenkins instances with 1000+ queue items. The permission check lead to the azure-ad plugin taking up a lot of CPU. Also see previous fix attempts here.

Unit test not needed as this doesn't change any behavior.

Submitter checklist

  • [ x ] Link to JIRA ticket in description, if appropriate.
  • [ x ] Change is code complete and matches issue description
  • [ x ] Appropriate unit or acceptance tests or explanation to why this change has no tests
  • [ x ] Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@schiasileon schiasileon requested a review from a team as a code owner July 30, 2024 12:18
@olamy
Copy link
Member

olamy commented Jul 30, 2024

Hi,
Thanks for your PR, this is going to the right direction.
I can reproduce the test failures locally as well. Can you look at those? Maybe it's possible the assert are not correct?

@schiasileon schiasileon force-pushed the bugfix/cause-of-blockage-queue-fetch branch from ab5f39a to 4c28f5d Compare July 31, 2024 12:48
@schiasileon
Copy link
Author

Hi, Thanks for your PR, this is going to the right direction. I can reproduce the test failures locally as well. Can you look at those? Maybe it's possible the assert are not correct?

Thx for looking into it. I replaced the approach by getting the items from a different list which does not check permissions and all tests are passing now.
Its now failing on npm install step. No idea why.

@schiasileon schiasileon force-pushed the bugfix/cause-of-blockage-queue-fetch branch from 4c28f5d to 2f47cfc Compare August 19, 2024 08:12
@schiasileon
Copy link
Author

@olamy
Tests are now all passing

@schiasileon
Copy link
Author

any feedback pls?
@olamy

@olamy
Copy link
Member

olamy commented Aug 31, 2024

Hi,
I'm not really clear if it's normal we are not checking permissions.
I need to check that

@olamy
Copy link
Member

olamy commented Sep 3, 2024

@schiasileon checking permissions is on purpose (see https://www.jenkins.io/security/advisory/2015-11-11/#queue-api-did-show-items-not-visible-to-the-current-user)
can you check permissions for those items returned but getBuildableItems or ensure we are in context where the current user have enough permissions?

@schiasileon
Copy link
Author

Hi, I cannot see where permissions would matter in this context. You are just looking for a Item the user already has access to to get cause of blockage.
Please correct me if I'm wrong @olamy

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