Add test_recorder implementation and integration with integ_test_suite #467

gaiksaya · 2021-09-13T23:02:42Z

Signed-off-by: Sayali Gaikawad gaiksaya@amazon.com

Description

Adds test_recorder implementation and integration with integ_test_suite.
Test_recorder will be instantiated by each test_suite. The test_recorder object will then be passed to test_publisher to store the results in a specific location (s3, long-lived-cluster, etc).

To-Do:

Integrate with other test_suite i.e. bwc-test, perf-test
Add UT

Issues Resolved

#340

Tests:

/tmp/tmp93i0ybyw/tests/2/integ-test/index-management/with-security ~  ls local_cluster_logs test_outcome
local_cluster_logs:
gc.log                                 opensearch_deprecation.json            opensearch_index_indexing_slowlog.log  opensearch_server.json
gc.log.00                              opensearch_deprecation.log             opensearch_index_search_slowlog.json   stderr.txt
opensearch.log                         opensearch_index_indexing_slowlog.json opensearch_index_search_slowlog.log    stdout.txt

test_outcome:
index-management.yml stderr.txt           stdout.txt


~ cat test_outcome/index-management.yml
component_name: index-management
log_file_location: S3
status: FAILED/ERROR
test_config: with-security
test_run_id: '2'
test_type: integ-test

Check List

Commits are signed per the DCO using --signoff

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.

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

codecov-commenter · 2021-09-13T23:04:35Z

Codecov Report

Merging #467 (95fffa4) into main (163a32a) will decrease coverage by 1.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #467      +/-   ##
==========================================
- Coverage   64.46%   63.44%   -1.02%     
==========================================
  Files          48       51       +3     
  Lines        1269     1488     +219     
==========================================
+ Hits          818      944     +126     
- Misses        451      544      +93

Impacted Files	Coverage Δ
bundle-workflow/src/run_integ_test.py	`0.00% <0.00%> (ø)`
bundle-workflow/src/test.py	`0.00% <ø> (ø)`
...w/src/test_workflow/integ_test/integ_test_suite.py	`0.00% <0.00%> (ø)`
...src/test_workflow/integ_test/local_test_cluster.py	`0.00% <0.00%> (ø)`
bundle-workflow/src/test_workflow/test_recorder.py	`0.00% <0.00%> (ø)`
bundle-workflow/src/manifests/manifest.py	`96.55% <0.00%> (-3.45%)`	⬇️
bundle-workflow/src/build.py	`0.00% <0.00%> (ø)`
bundle-workflow/src/build_workflow/builder.py	`100.00% <0.00%> (ø)`
bundle-workflow/src/manifests/input_manifest.py	`100.00% <0.00%> (ø)`
...orkflow/src/build_workflow/build_artifact_check.py	`90.90% <0.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 163a32a...95fffa4. Read the comment docs.

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

dblock

Mostly small stuff below.

There's a lot of passing 4-6 parameters around, consider some refactoring into configuration objects.

You should work off your own fork to avoid everyone bringing everybody's branches along.

bundle-workflow/src/run_integ_test.py

dblock · 2021-09-14T10:37:47Z

bundle-workflow/src/test_workflow/integ_test/integ_test_suite.py

@@ -73,11 +77,12 @@ def _is_security_enabled(self, config):
            return False

    def _setup_cluster_and_execute_test_config(self, config):


What's the convention for private methods? I think we've been doing two __

Not written by me but I can change if @setiah is okay with that or not making changes on his side

You may skip this. This (and more) will be refactored in a separate PR which adds UTs as well

bundle-workflow/src/test_workflow/integ_test/integ_test_suite.py

bundle-workflow/src/test_workflow/integ_test/local_test_cluster.py

dblock · 2021-09-14T10:43:02Z

bundle-workflow/src/test_workflow/test_recorder.py

+        else:
+            return "FAILED/ERROR"
+
+    def __generate_test_outcome_yml(self, component_name, component_test_config, exit_status, log_file_location):


log_file_location -> output_path

bundle-workflow/src/test_workflow/test_recorder.py

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

saratvemulapalli · 2021-09-14T22:19:57Z

bundle-workflow/src/run_integ_test.py

@@ -37,6 +38,9 @@ def parse_arguments():
    parser.add_argument(
        "--test-manifest", type=argparse.FileType("r"), help="Test Manifest file."
    )
+    parser.add_argument(
+        "--test-run-id", type=str, help="Test Run ID"


You might want to sync up with @setiah as he also added this param as part of #456

saratvemulapalli · 2021-09-14T22:22:54Z

bundle-workflow/src/run_integ_test.py

@@ -104,6 +108,7 @@ def main():
    bundle_manifest = BundleManifest.from_file(args.bundle_manifest)
    build_manifest = BuildManifest.from_file(args.build_manifest)
    test_manifest = TestManifest.from_file(args.test_manifest)
+    test_recorder = TestRecorder(args.test_run_id, "integ-test")


You might want to define a standard constants which defines each of the test suites.
We would want all the workflows to use same identifiers.

do you mean this? https://github.com/opensearch-project/opensearch-build/pull/467/files#diff-7466053dcf4632ffc67632378636660e4b2a95bec30137fca501ab304e298779R15

Yeah that would a part of it. I was getting towards something in common which could be used by all workflows not something which TestRecorder can understand. But this is a good start.

saratvemulapalli · 2021-09-14T22:33:01Z

bundle-workflow/src/test_workflow/test_recorder.py

+import shutil
+import tempfile
+
+import yaml


 class TestRecorder:


Make this class a singleton. With the design pattern we have, we'd like to have just one instance of TestRecorder for a given testsuite/jenkins job.
This also prevents other instantiations, avoids passing the test recorder instance in multiple places within the code base.

Suggested change

class TestRecorder:

class TestRecorder:

__instance = None

@staticmethod

def getInstance(test_run_id, test_type, location=None):

""" Static access method. """

if TestRecorder.__instance == None:

TestRecorder(test_run_id, test_type, location)

return TestRecorder.__instance

Python by default doesn't have private constructors, if you'd like to block instantiation outside of the class, you could raise an error in __init__ and use a different private method which could be called by getInstance(...)

Also rename getInstance to get_instance

saratvemulapalli · 2021-09-14T22:48:39Z

bundle-workflow/src/test_workflow/test_recorder.py

+        logging.info(f"Recording remote cluster logs for {component_name} in {os.path.realpath(dest_directory)}")
+        self.__generate_std_files(stdout, stderr, os.path.realpath(dest_directory))
+        exit_status = self.__get_exit_status(exit_code)
+        component_yml = self.__generate_test_outcome_yml(component_name, component_test_config, exit_status,


nit:

Suggested change

component_yml = self.__generate_test_outcome_yml(component_name, component_test_config, exit_status,

test_outcome_yml = self.__generate_test_outcome_yml(component_name, component_test_config, exit_status,

saratvemulapalli · 2021-09-14T22:50:50Z

bundle-workflow/src/test_workflow/test_recorder.py

+        for log_file in local_cluster_log_files:
+            dest_file = os.path.join(dest_directory, os.path.basename(log_file[0]))
+            shutil.copyfile(log_file[0], dest_file)


Looks like this could be stubbed out into another method. This code is repeated in record_remote_cluster_logs and record_test_outcome

saratvemulapalli · 2021-09-14T22:58:24Z

bundle-workflow/src/test_workflow/test_recorder.py

+            dest_file = os.path.join(dest_directory, os.path.basename(log_file[0]))
+            shutil.copyfile(log_file[0], dest_file)
+
+    def record_remote_cluster_logs(self, component_name, component_test_config, exit_code, stdout, stderr,


The flow doesn't look clean to me. Let me know if I didn't understand this correctly.

For test suite which uses local cluster, it uses both record_test_outcome and record_local_cluster_logs.
record_test_outcome generates the test outcome manifest.

For test suite which uses remote cluster, it just uses record_remote_cluster_logs . record_remote_cluster_logs generates the test outcome manifest.

If that is the case why don't we merge both remote and local test cluster into one function record_test_cluster which takes in local/remote as a parameter. The only logic different between them is creating a different directory.

Let me know what you think.

I agree. I'll work on this. One more logic that differs is remote cluster logs won't have log_files but just the location of the logs. Will see how to differentiate that.

record_local_cluster_logs also copies the local test cluster log files into the directory where remote cluster just gives the location of the logs. That will be an additional change but with right param it should be distinguishable

saratvemulapalli · 2021-09-15T00:19:38Z

bundle-workflow/src/test_workflow/test_recorder.py

+        if self.test_type not in self.ACCEPTED_TEST_TYPES:
+            raise ValueError(f"test_type is invalid. Acceptable test_type are {self.ACCEPTED_TEST_TYPES}")
+
+        if location is None:


We have to clean up the directory when TestRecorder goes out of scope. Tmp directory gives us a great feature which takes care of it. It's better to clean it up by ourself, we do not want to leave them on the system.

Lets only use temporary directory as the only option for TestRecorder and when it goes out of scope let it destroy the directory and add a keep parameter if caller wants the directory to be preserved.
Anyway we don't have a use case yet for a custom directory, avoids the hassle of cleaning up.

Makes sense. Regarding clean up. We cannot clean up the directory unless publisher is done publishing the result. So would it make sense that publisher deletes the directory when it is done publishing? or is there any other way?

lets leave the ownership to the test suite runners. They use test recorder, TestRecorder records them and deletes them if it goes out of scope.
Any way TestPublisher(aka TestResults) consumes TestRecorder so it is implicitly taken care of.

Sarat suggests that the topmost caller creates a temporary dir and it's passed down to anyone that needs to write files.

Yeah that is happening right now. Will see how to clean_up when the test_recorder object goes out of scope

gaiksaya · 2021-09-24T07:56:48Z

Closing this one. Please see: #587

gaiksaya added 4 commits September 13, 2021 15:31

Add test_recorder implementation and integration with integ_test_suite

3f7c97e

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

Fix flake

f512954

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

Remove testrecorder from test.py

652378a

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

Fix imports

a39cb30

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

gaiksaya mentioned this pull request Sep 14, 2021

Integtest input arguments changes and integration with s3 #456

Merged

1 task

Remove print

bb7f8aa

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

dblock requested changes Sep 14, 2021

View reviewed changes

gaiksaya added 2 commits September 14, 2021 15:17

Fix naming and structure

e50f6d5

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

Fix logger

95fffa4

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

gaiksaya requested review from saratvemulapalli and peternied September 14, 2021 22:43

saratvemulapalli reviewed Sep 14, 2021

View reviewed changes

saratvemulapalli reviewed Sep 15, 2021

View reviewed changes

owaiskazi19 mentioned this pull request Sep 20, 2021

Added Test Publisher #464

Closed

1 task

gaiksaya closed this Sep 24, 2021

gaiksaya mentioned this pull request Sep 24, 2021

Add test_recorder implementation and integrate with integ_test suite #587

Merged

1 task

peternied deleted the testrecorder branch November 1, 2021 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test_recorder implementation and integration with integ_test_suite #467

Add test_recorder implementation and integration with integ_test_suite #467

gaiksaya commented Sep 13, 2021 •

edited

Loading

codecov-commenter commented Sep 13, 2021 •

edited

Loading

dblock left a comment

dblock Sep 14, 2021

gaiksaya Sep 14, 2021

setiah Sep 16, 2021

dblock Sep 14, 2021

saratvemulapalli Sep 14, 2021

saratvemulapalli Sep 14, 2021

gaiksaya Sep 14, 2021

saratvemulapalli Sep 14, 2021

saratvemulapalli Sep 14, 2021

dblock Sep 14, 2021

saratvemulapalli Sep 14, 2021

saratvemulapalli Sep 14, 2021

saratvemulapalli Sep 14, 2021 •

edited

Loading

gaiksaya Sep 14, 2021

gaiksaya Sep 22, 2021

saratvemulapalli Sep 15, 2021

gaiksaya Sep 15, 2021

saratvemulapalli Sep 15, 2021

dblock Sep 15, 2021

gaiksaya Sep 15, 2021

gaiksaya commented Sep 24, 2021

		@@ -73,11 +77,12 @@ def _is_security_enabled(self, config):
		return False

		def _setup_cluster_and_execute_test_config(self, config):

-class TestRecorder:
+class TestRecorder:
+   __instance = None
+   @staticmethod
+   def getInstance(test_run_id, test_type, location=None):
+      """ Static access method. """
+      if TestRecorder.__instance == None:
+         TestRecorder(test_run_id, test_type, location)
+      return TestRecorder.__instance

	component_yml = self.__generate_test_outcome_yml(component_name, component_test_config, exit_status,
	test_outcome_yml = self.__generate_test_outcome_yml(component_name, component_test_config, exit_status,

Add test_recorder implementation and integration with integ_test_suite #467

Add test_recorder implementation and integration with integ_test_suite #467

Conversation

gaiksaya commented Sep 13, 2021 • edited Loading

Description

Issues Resolved

Tests:

Check List

codecov-commenter commented Sep 13, 2021 • edited Loading

Codecov Report

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saratvemulapalli Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaiksaya commented Sep 24, 2021

gaiksaya commented Sep 13, 2021 •

edited

Loading

codecov-commenter commented Sep 13, 2021 •

edited

Loading

saratvemulapalli Sep 14, 2021 •

edited

Loading