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

Add new jenkins job for benchmarking pull request #4831

Merged
merged 4 commits into from
Jul 10, 2024

Conversation

rishabh6788
Copy link
Collaborator

@rishabh6788 rishabh6788 commented Jul 9, 2024

Description

This PR:

  • Adds --results-file flag to opensearch-benchmark command to write the final markdown format output to a file. This file content will be read and published to PR comment in jenkinsfile.
  • Add new job for running benchmarks initiated by pull requests. The jenkins job is a slightly modified version of existing benchmark-test.jenkinsfile

Issues Resolved

#4804
opensearch-project/OpenSearch#14553

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

rishabh6788 and others added 3 commits July 1, 2024 13:13
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.07%. Comparing base (aefb420) to head (fb724f7).
Report is 23 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4831      +/-   ##
==========================================
+ Coverage   92.05%   92.07%   +0.02%     
==========================================
  Files         193      193              
  Lines        6416     6445      +29     
==========================================
+ Hits         5906     5934      +28     
- Misses        510      511       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[key: 'CAPTURE_NODE_STAT', value: '$.CAPTURE_NODE_STAT'],
[key: 'TELEMETRY_PARAMS', value: '$.TELEMETRY_PARAMS']
],
tokenCredentialId: 'jenkins-pr-benchmark-generic-webhook-token',
Copy link
Member

Choose a reason for hiding this comment

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

Token is already created for this job?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, will create a PR in opensearch-ci repo for this.

Copy link
Member

Choose a reason for hiding this comment

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

👍

jenkins/opensearch/benchmark-pull-request.jenkinsfile Outdated Show resolved Hide resolved
Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

Couple of questions:

  1. Can we repurpose or make the current benchmark test generic enough to handle PR as well instead of maintaining an identical version?
  2. Little confused with overall workflow. I understand the PR is the trigger but it is on every commit or synchronize or something else?
  3. If not PR what is the other trigger
  4. From security point of view, can you please list how to avoid multi-runs for same PR?

if (currentBuild.rawBuild.getCauses().toString().contains("GenericCause")) {
currentBuild.description = "Benchmark initiated by PR:${pull_request} on ${repository}"
}
else {
Copy link
Member

Choose a reason for hiding this comment

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

If not pull request what is the other cause? Ad-hoc run?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ad-hoc.

Copy link
Member

Choose a reason for hiding this comment

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

Should the cause string be differed accordingly then?

Copy link
Collaborator Author

@rishabh6788 rishabh6788 Jul 9, 2024

Choose a reason for hiding this comment

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

Yes, it will be set by Jenkins I believe.
something like Started by user <user-id>.

Copy link
Member

Choose a reason for hiding this comment

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

Okay got it. The cause string only applies if we use generic trigger plugin

tests/jenkins/TestBenchmarkPullRequest.groovy Show resolved Hide resolved
@rishabh6788
Copy link
Collaborator Author

Couple of questions:

  1. Can we repurpose or make the current benchmark test generic enough to handle PR as well instead of maintaining an identical version?
  2. Little confused with overall workflow. I understand the PR is the trigger but it is on every commit or synchronize or something else?
  3. If not PR what is the other trigger
  4. From security point of view, can you please list how to avoid multi-runs for same PR?
  1. Yes we can but since it runs more than 100+ jobs per day it will become difficult to separate put the pr runs from nightly runs. Also too much if-else will reduce flexibility to easily modify the functionality.
  2. The workflow will be triggered when a user will add a comment on the PR in a certain format and a maintainer will have to approve the run before jenkins job gets triggered.
  3. No other trigger for now, apart from admin running it on ad-hoc basis.
  4. Multi runs for the same PR should be allowed as the user might push more commits onto the same PR. To prevent the abuse the maintainer approval will be required for each invocation.

Signed-off-by: Rishabh Singh <sngri@amazon.com>
Copy link
Member

@gaiksaya gaiksaya left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@rishabh6788 rishabh6788 merged commit a84bb81 into opensearch-project:main Jul 10, 2024
10 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants