-
Notifications
You must be signed in to change notification settings - Fork 282
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
Adds PerformanceTestSuiteClass and PerformanceTestCluster for Performance Testing #314
Conversation
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.
Should have a new perf_test.sh
like the other executables and at least some minimum of documentation please?
bundle-workflow/src/perf_test.py
Outdated
|
||
workspace = os.getcwd() | ||
|
||
os.chdir(workspace) |
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.
Noop with the above line?
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.
Removed
os.chdir(self.work_dir) | ||
dir = os.getcwd() | ||
# Install the depedencies for the private repo | ||
subprocess.check_call('pip3 install boto3 requests setuptools retry dataclasses_json', cwd=dir, shell=True) |
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.
Can we move this into the repo itself / wrap up in a test.sh
? At least a TODO 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.
Moved the installation to the repo itself
# Install the depedencies for the private repo | ||
subprocess.check_call('pip3 install boto3 requests setuptools retry dataclasses_json', cwd=dir, shell=True) | ||
|
||
if self.security: |
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.
construct a command line, then append -s
if security
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.
Added
self.work_dir = 'tools/cdk/mensor/mensor_tests' | ||
self.endpoint = endpoint | ||
self.security = security | ||
self.command = f'python3 test_config.py -i {self.endpoint} -b {self.manifest.build.id}'\ |
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.
So this test_config.py comes from Mensor? It needs to be doing dependency management with pipenv or similar, this is the wrong place to be calling python3
.
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.
Yeah this file is present in the Infra repo. I'll change the alias name, it should be python
.
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.
No actually python3 is correct. But you see why this doesn't work well, it's precisely because this is setup specific. To make it setup-not-specific adopt pipenv in that tool, then this becomes pipenv install
and pipenv run ...
. All dependencies are under control and the environment is stable.
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.
Makes sense. Since I'm checking out to the infra repo, I've to install pipenv
before running pipenv install
https://github.com/opensearch-project/opensearch-build/pull/314/files#diff-207df8df9b6f387ef1a541037c5ed9ecda140695ec4c2ce16bdcbac0b41d260dR22
Codecov Report
@@ Coverage Diff @@
## main #314 +/- ##
==========================================
+ Coverage 53.63% 56.05% +2.42%
==========================================
Files 29 33 +4
Lines 785 892 +107
==========================================
+ Hits 421 500 +79
- Misses 364 392 +28
Continue to review full report at Codecov.
|
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 don't want to hold the PR, but I think you should do the yield suggestion at least. Also, tests.
bundle-workflow/src/perf_test.py
Outdated
return "https://github.com/opensearch-project/opensearch-infra.git" | ||
|
||
|
||
os.chdir(os.getcwd()) |
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.
Isn't this a noop?
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.
Removed
return 443 | ||
return 9200 | ||
|
||
def destroy(self): |
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.
Consider implementing a yield
version instead like https://github.com/opensearch-project/opensearch-build/blob/main/bundle-workflow/src/system/temporary_directory.py so that you can use with
.
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.
Are we talking about destroying multiple clusters here? May be I did't get this part
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 clarified in a comment above. You don't want to rely on the caller to always do a finally
block that calls .destroy()
. If the caller were to forget, you'd end up with a running cluster. But if you adopt the with ...
pattern, it would be taken care of by the garbage collector in the worst case scenario.
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.
Implemented a yield
version
self.output_file = 'output.json' | ||
self.ip_address = None | ||
self.security = 'enable' if security else 'disable' | ||
self.params = f'-c url={self.manifest.build.location} -c security_group_id={self.security_id} -c vpc_id={self.vpc_id}'\ |
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.
Maybe a dict that is joined with -c
?
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.
These params are for the cdk
command as we have to run it using subprocess
. Correct me if I missed something 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.
Every argument is -c
, make an array with each item on a line, and then join with "-c " as the separator. Avoids a lot of f{}
and will be easier to diff.
self.ip_address = load_output[self.stack_name]['PrivateIp'] | ||
print('Private IP:', self.ip_address) | ||
|
||
def endpoint(self): |
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.
Assign self.endpoint =
when you have it so you can use a property.
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.
Assigned self.cluster_endpoint
.
def endpoint(self): | ||
return self.ip_address | ||
|
||
def port(self): |
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.
Assign, same as above.
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.
Assigned self.cluster_port
6fae82c
to
3a6dc3a
Compare
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
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 is close! Good stuff.
The must have's are in tests where you are executing a command and getting a 127 (not found). Those tests shouldn't be actually executing anything, the shell execution has to be mocked. Otherwise on some machines tests start actually calling things like python3 -m pipenv install
and who knows what happens :)
Try to address the other should have's. Half are about using the with/yield
patter to automatically cleanup resources.
bundle-workflow/src/perf_test.py
Outdated
if component.name == 'security': | ||
security = True | ||
os.chdir(current_workspace) | ||
perf_cluster = PerformanceTestCluster(manifest, config, args.stack, security) |
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.
Implement PerformanceTestCluster like we implemented https://github.com/opensearch-project/opensearch-build/blob/main/bundle-workflow/src/system/temporary_directory.py. I want to be able to write
with PerformanceTestCluster(manifest, config, args.stack, security) as test_cluster:
And you won't need the finally
block.
Similar suggestion for os.chdir
, I want to do with WorkingDirectory(curret_workspace) as curdir:
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.
Changed it to with WorkingDirectory(current_workspace) as curdir
and with PerformanceTestCluster(manifest, config, args.stack, security).cluster() as test_cluster_endpoint
return 443 | ||
return 9200 | ||
|
||
def destroy(self): |
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 clarified in a comment above. You don't want to rely on the caller to always do a finally
block that calls .destroy()
. If the caller were to forget, you'd end up with a running cluster. But if you adopt the with ...
pattern, it would be taken care of by the garbage collector in the worst case scenario.
import os | ||
import sys | ||
|
||
sys.path.insert(0, os.path.join(os.path.dirname(__file__), "../../src")) |
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 if the driver is test.sh
at the src
level.
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 have a separate test.py for performance testing. So keeping this file
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.
Build, assemble and test don't invoke .py
files directly because precisely of this problem. A perf_test.sh
should be fixing its path and calling bundle-workflow/perf_test.py
. If it doesn't exist, add it. Or we can make test.sh --suite perf
and invoke different test suites that way too.
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.
Removed. As discussed with @setiah @saratvemulapalli we will call each test.py of all the 3 tests in Jenkins directly.
def setUp(self): | ||
os.chdir(os.path.dirname(__file__)) | ||
manifest_file_handle = open("data/test_manifest.yaml", "r") | ||
self.manifest = BundleManifest.from_file(manifest_file_handle) |
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 can use from_path
and no need to do open
yourself.
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.
Added
os.chdir(os.path.dirname(__file__)) | ||
manifest_file_handle = open("data/test_manifest.yaml", "r") | ||
self.manifest = BundleManifest.from_file(manifest_file_handle) | ||
config_file_handle = open("data/config.yml", "r") |
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.
Use with
so that the file closes right after being read. No need to hold onto handles.
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.
Removed the config file and hard-coded the values
manifest_file_handle = open("data/test_manifest.yaml", "r") | ||
self.manifest = BundleManifest.from_file(manifest_file_handle) | ||
config_file_handle = open("data/config.yml", "r") | ||
config = yaml.load(config_file_handle, Loader=yaml.FullLoader) |
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 think for a test you don't want to use a config, but hard-code well known values here. But furthermore, these values are only checked against once, so I would remove all self.whatever=
assignments that are done here and hardcode expected commands in the tests so we can catch actual bugs. Otherwise I'll change the config inadvertently and tests still pass.
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.
Hard-coded the values and removed config.yml
self.perf_test_cluster.create() | ||
mock_chdir.assert_called_once_with('tools/cdk/mensor/single-node/') | ||
|
||
self.assertEqual(process_ret.exception.returncode, 127) |
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 actually runs cdk deploy
but fails with a 127 (not found). If the machine has cdk
in the path, the test will start doing something else. You need to mock the execute 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.
Mocked the function
self.perf_test_suite = PerformanceTestSuite( | ||
bundle_manifest=self.manifest, endpoint=None, security=False | ||
) | ||
manifest_file_handle.close() |
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.
Use with
, or better use BundleManifest.from_path
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.
Used BundleManifest.from_path
with self.assertRaises(subprocess.CalledProcessError) as process_ret: | ||
self.perf_test_suite.execute() | ||
mock_chdir.assert_called_once_with('tools/cdk/mensor/single-node/') | ||
cmd = 'python3 -m pipenv install' |
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 is actually executed, needs to be mocked.
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.
Mocked the function
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
from test_workflow.perf_test_cluster import PerformanceTestCluster | ||
from test_workflow.perf_test_suite import PerformanceTestSuite | ||
|
||
parser = argparse.ArgumentParser(description="Test an OpenSearch Bundle") |
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.
Please add comments for how structure of this file should work since it doesn't follow the typical interface/inheritance pattern
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 added the comment and explained the required arguments for Perf Test.
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
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 a few more, close.
|
||
os.chdir(current_workspace) | ||
perf_test_suite = PerformanceTestSuite(manifest, test_cluster_endpoint, security, current_workspace) | ||
perf_test_suite.execute() |
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.
@saratvemulapalli This will be nice and easy with a base class with a prepare
and execute
.
import os | ||
from contextlib import contextmanager | ||
|
||
saved_path = os.getcwd() |
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.
Needs to be saved before yield
inside the code of WorkingDirectory
, otherwise your directory cd
calls don't stack.
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.
Added saved_path
before yield
self.output_file = 'output.json' | ||
self.ip_address = None | ||
self.security = 'enable' if security else 'disable' | ||
self.params = f'-c url={self.manifest.build.location} -c security_group_id={self.security_id} -c vpc_id={self.vpc_id}'\ |
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.
Every argument is -c
, make an array with each item on a line, and then join with "-c " as the separator. Avoids a lot of f{}
and will be easier to diff.
f' -c architecture={self.manifest.build.architecture} --require-approval=never --plugin cdk-assume-role-credential-plugin'\ | ||
f' -c assume-role-credentials:writeIamRoleName={self.role} -c assume-role-credentials:readIamRoleName={self.role}' | ||
|
||
@contextmanager |
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.
Extract into a class.
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.
Created a new class in the same file as this yield version is specific to perf test
|
||
def execute(self): | ||
try: | ||
os.chdir(self.work_dir) |
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.
Use your new directory class.
|
||
class TestPerformanceCluster(unittest.TestCase): | ||
def setUp(self): | ||
os.chdir(os.path.dirname(__file__)) |
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 needed, has side effects. Constructs paths relative to dirname(__file__)
and then load the manifest from there.
@@ -0,0 +1,28 @@ | |||
# SPDX-License-Identifier: Apache-2.0 |
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.
file name doesn't match class, either class should be TestPerfTestSuite or rename to test_performance_test_suite.py.
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.
Renamed the class name to TestPerfSuite
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 know it's weird to have "Test" twice, but the suite is called a "PerfTestSuite", so you want "TestPerfTestSuite", and test_perf_test_suite
.
from test_workflow.test_cluster import TestCluster | ||
|
||
|
||
class PerformanceTestCluster(TestCluster): |
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.
class doesn't match filename
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.
Renamed the class name to PerfTestCluster
import subprocess | ||
|
||
|
||
class PerformanceTestSuite: |
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.
class doesn't match filename
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.
Renamed the class name to PerfTestSuite
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
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.
LGTM, made suggestions that you can address before/after
from test_workflow.test_cluster import TestCluster | ||
|
||
|
||
class Cluster: |
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 should be a capability of any cluster, so PerfTestCluster
would inherit TestCluster
and the latter has a static def create(cls, ...)
method that is implemented by calling cls._init_
...
You can refactor this later.
self.output_file = 'output.json' | ||
self.ip_address = None | ||
self.security = 'enable' if security else 'disable' | ||
self.params_dict = { |
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.
Since this is not used elsewhere should just be a local var. Same for others.
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.
Created local variables
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Hi @dblock and @peternied it'll be great if you can approve the PR again as I addressed the last comment as well. Thank you. |
Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
@@ -0,0 +1,63 @@ | |||
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.
This is not test_manifest.yaml. Its bundle_manifest.yml, Rename it to avoid confusion. Here is the test-manifest.yml
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'll address it in the next PR. Thanks
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.
Addressed in #404
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.
Lets merge and fix the open issue in a fresh PR
* Performance Testing workflow Signed-off-by: Owais Kazi <owaiskazi19@gmail.com>
Description
Implemented PerformanceTestSuiteClass and PerformanceTestCluster.
Prerequisites
PYTHONPATH
Issues Resolved
#215
#282
Testing
Check List
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.