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.
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 performance test scripts #1671
Add performance test scripts #1671
Changes from 3 commits
7affa4d
e93c8c7
7247194
440051d
1347f5c
5513bbf
9fef51e
7648533
4f06122
57c8977
e65e19e
eee81d0
dea8876
f9f5674
c01ea40
75e8d53
74bf99a
a19db11
fad13db
f79d5bd
a2a40d5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We already have a configuration file, and we have a manifest that has or does not have security, why are we promoting something so specific to a top level feature/option and how is it going to add up with that?
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 am trying to parallelize the two jobs (security/non security) at the top level to get clear status and result visibility. Moving it at the python script level will require additional async logic to be added in to make the script execute in parallel.
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 tooling is trying hard not to be too specific for OpenSearch/OpenSearch Dashboards so I'd think about this twice. My concern though is that with this change we have 2 ways to say "with security" and "without security", you should collapse it in 1 way as part of this change.
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 have updated the config to be more appropriate with the general use case. I can work on abstracting it out/utilizing a test config for example, when we move to add more components to this script.
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.
Shouldn't we get security from bundle manifest?
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.
That would prevent us from parallelizing the tests in the future. We want to kick off two tests for the security based bundles, which can be done from the Jenkins pipeline
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 feel like security enabled/disabled should be decided based on
security
param inmanifest.components
. To parallelize the test we can pass 2 different bundle manifest, one with security enabled and other disabled.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.
We will have to publish these manifests to our buckets and distributions, which we do not currently.
Moving it out additionally gives more flexibility for extensibility moving forward for other use cases rather than updating/adding manifests for every build.
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.
We do publish manifests to our buckets.
without-security
tests is something that will run irrespective of what's in the manifest but forwith-security
can we add a check in the codebase and then start the test only if the security component is present?If the performance tests are to be run nightly it is highly possible that security component is not present initially in the manifest.
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.
Refer to the comment below - #1671 (comment)
What I was trying to say is - we have a single manifest which might or might not have security in the components. By enabling the logic at the Jenkins level, we can execute the two runs in parallel, giving us more control over the test runs and statuses.
Also, it is based directly on the manifest. Look here for more - https://github.com/opensearch-project/opensearch-build/pull/1671/files#diff-4debf5e3ece07145d8395f15df88f49b8b784a274cead94e2a94c8b7152c11efR75
All I am doing is pulling out that logic for better control at the top level of job execution.
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.
This shouldn't be necessary. Dependencies needed by the tools should go into Pipfile.
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 is because of the nature in which the packages are separated and it being pulled at run time.
pipenv
does not support nested module pipfile installation, which is why I had to resort to installing it via this script.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.
You should fix this in the other project. Implement a similar wrapper as
test.sh
, aPipfile
and have the .sh script runpipenv install
to get these dependencies. It's not on the caller's responsibility to ensure that the dependencies are met, or you'll be constantly chasing changes in that project here.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 can work on this in a follow up PR. I want to get the changes going as they are blocking other performance test related integrations and tests.