Added approaches to move loganalyzer to pytest. #1

yvolynets-mlnx · 2019-07-05T16:14:27Z

Signed-off-by: Yuriy Volynets yuriyv@mellanox.com

Signed-off-by: Yuriy Volynets <yuriyv@mellanox.com>

wangxin · 2019-07-08T03:14:10Z

doc/loganalyzer_to_pytest.md

+ Module "loganalyzer.py" with class "Loganalyzer".
+ "Loganalyzer" class - executes shell commands on DUT.
+ **"Loganalyzer" class interface:**
+- __init__(match: list, expect: list, ignore: list)


While creating Loganalyzer instance, it would be better to pass the DUT information to it. DUT information is needed by subsequent methods.

wangxin · 2019-07-08T03:28:29Z

doc/loganalyzer_to_pytest.md

+- extract_syslog() - calls "extract_syslog" from "log_processor.py"
+- upload_search_files(dest) - upload match, expect, ignore files to the DUT
+- parse_summary_file() - convert generated on DUT "result.loganalysis.{TEST_NAME}..." file to json.
+- parse_result_file() - convert generated on DUT "summary.loganalysis.{TEST_NAME}" file to json.


For the methods that are internally used by Loganalyzer, we can consider hide them using the "_" prefix.

wangxin · 2019-07-08T03:30:34Z

doc/loganalyzer_to_pytest.md

+Usage example:
+
+ def test(localhost, ansible_adhoc, testbed):
+ loganalyzer = Loganalyzer(match="{PATH}/loganalyzer_common_match.txt", expect="{PATH}/loganalyzer_common_expect.txt", ignore="{PATH}/loganalyzer_common_ignore.txt")


The loganalyzer can load and copy the common match/expect/ignore files by default. And we can consider adding extra argument for skipping the common pattern files.

wangxin · 2019-07-08T03:33:27Z

doc/loganalyzer_to_pytest.md

+ result = loganalyzer.analyze()
+ if result["summary"]["TOTAL MATCHES"] != 0:
+ loganalyzer.fetch_fail_artifacts(dst="test/{}".format(loganalyzer.unic_test_name))
+ pytest.fail("SOME_MESSAGE\n{}\n{}".format(result["summary"], result["match_messages"]))


When test failed, I think we need to keep the capability of generating dump and copy related files to sonic-mgmt container for further analysis. While generating dump, we can consider collecting logs within 1 hour by default. If more logs are needed, the time range should can be passed in by an extra argument.

"When test failed, I think we need to keep the capability of generating dump and copy related files to sonic-mgmt container for further analysis." Agree but maybe better will be to have this functionality in some pytest fixture which has access to the DUT?

wangxin · 2019-07-08T03:34:20Z

doc/loganalyzer_to_pytest.md

+ loganalyzer.fetch_fail_artifacts(dst="test/{}".format(loganalyzer.unic_test_name))
+ pytest.fail("SOME_MESSAGE\n{}\n{}".format(result["summary"], result["match_messages"]))
+
+#### Approach 2


I vote for approach 1 because less work is required.

I've added new commit with udpdates but based on the approach 2. Because it will be more flexible and will require less work done on the DUT side.

stepanblyschak · 2019-07-08T09:02:50Z

doc/loganalyzer_to_pytest.md

+- init() - Add start marker to the DUT syslog.
+- analyze() - Extract syslog and copy it to ansible host. Analyze extracted syslog files localy. Return python dictionary object.
+Return example:
+ {"summary":


The formatting in rendered view is messed up

stepanblyschak · 2019-07-08T09:07:59Z

doc/loganalyzer_to_pytest.md

+ loganalyzer.init()
+
+ # Add search marker specific for None MAC test
+ loganalyzer.update_match(action="add", type="match", "Runtime error: can't parse mac address 'None'")


I don't think action='pop' is needed. I would better recreate the loganalyzer object rather than keeping in mind which matches I removed and which not

Disagree. 1 - Probably this class will be wrapped in pytets fixture, so object recreation does not fit.
2 - We need interface to add/remove specific regexp entries to tune search, so such interface will be ok in such case.
3 - If there is a need we can add "extend" flag to "load_common_config()" which can clear config:

if "extend is True", load common configuration and add to the existed.

if "extend is False", clear previous configuration and load common.

stepanblyschak · 2019-07-08T09:09:33Z

doc/loganalyzer_to_pytest.md

+ loganalyzer.init()
+
+ # Add search marker specific for None MAC test
+ loganalyzer.update_match(action="add", type="match", "Runtime error: can't parse mac address 'None'")


add_match/add_expect/add_ignore seems to me better interface than passing a type.
BTW, 'type' is a bad name for key/variable in python

Maybe it will be better but in such case also will be needed del_match/del_expect/del_ignore. So it increase number of methods for similar functionality.

stepanblyschak · 2019-07-08T09:10:16Z

doc/loganalyzer_to_pytest.md

+
+ from loganalyzer import Loganalyzer
+ def test(localhost, ansible_adhoc, testbed):
+ loganalyzer = Loganalyzer()


What is the default in this case, why load_common_config is a different method? What if the user will not call load_common_config/update_match?

In this approach, when loganalyzer is initializing:
loganalyzer = Loganalyzer()

It does not have configured search regexp (does not know what markers to search in the logs).
As we have common regular expressions for the DUT (now defined in match, expect and ignore files) they can be loaded/read to the loganalyzer object by calling "loganalyzer.load_common_config()".

In most cases we will need common regexp to be loaded so this can be moved to the "init".

"update_match()" - not required to be used. It can be used if there is a need to remove or update (add test specific) configured regular expressions to search.

stepanblyschak · 2019-07-08T09:10:18Z

doc/loganalyzer_to_pytest.md

+- init() - Add start marker to the DUT syslog.
+- analyze() - Extract syslog and copy it to ansible host. Analyze extracted syslog files localy. Return python dictionary object.
+Return example:
+ {"summary":


This is a dict with one key. What is the benefit of having everything under key "summary"?

stepanblyschak · 2019-07-08T09:12:20Z

doc/loganalyzer_to_pytest.md

+ loganalyzer.init()
+
+ # Add search marker specific for None MAC test
+ loganalyzer.update_match(action="add", type="match", "Runtime error: can't parse mac address 'None'")


having the option to load a list of regexp is good, but there are a lot of tests which have files with regexps, how can one load these files using "update_match"?

stepanblyschak · 2019-07-08T09:16:05Z

doc/loganalyzer_to_pytest.md

+ loganalyzer.init()
+
+ # Add search marker specific for None MAC test
+ loganalyzer.update_match(action="add", type="match", "Runtime error: can't parse mac address 'None'")


Does update_match overrides common or adds incrementaly?
How can one use, lets say, common 'match'/'ignore' but specific 'expect'?

stepanblyschak · 2019-07-08T09:18:23Z

I vote for approach #2, because there is no haste, so we can do things better than they are right now

Signed-off-by: Yuriy Volynets <yuriyv@mellanox.com>

wangxin · 2019-07-09T02:05:18Z

doc/loganalyzer_to_pytest.md

+- __init__(ansible_host: ansible_host)
+- load_common_config() - Load regular expressions from common configuration files: match, expect and ignore which are located in some configuration directory or with current module. Clear previous configured match, expect and ignore. Save loaded configuration.
+- parse_regexp_file(file_path) - read and parse regular expressions from specified file. Return list of strings of defined regular expressions.
+- run_cmd(callable, *args, **kwargs) - call function and analyze DUT logs, return the same result as "analyze" function


Why use a wrapper here rather than directly call the functions?

This is analog to the "roles/test/tasks/run_command_with_log_analyzer.yml" - add start marker, execute function, add stop marker, analyze logs.

Ah, I see. It looks good.

wangxin · 2019-07-09T02:17:02Z

doc/loganalyzer_to_pytest.md

+- analyze() - Extract syslog and copy it to ansible host. Analyze extracted syslog files localy. Return python dictionary object.
+Return example:
+{"counters": {"match": 1, "expected_match": 0, "expected_missing_match": 0},
+ "match_files": {"/tmp/syslog": {"match": 0, "expected_match": 32},


Because the log analyzer may be called mutliple times, the logs may be copied to sonic-mgmt container multiple times. I think we need to consider putting the copied files under different temp folders to avoid conflicts.

After analyzing is done, should we cleanup the copied temp log files?

"I think we need to consider putting the copied files under different temp folders to avoid conflicts." - Yes it is planned like this.
"After analyzing is done, should we cleanup the copied temp log files?" - Yes we should clean up temp log file.

@yvolynets-mlnx Can the loganalyzer print whole temp file with DEBUG level. I assume we will run with DEBUG on jenkins and this part of log that is analyzed will be visible in output. Useful I think

Depends on the DUT health, it colud take many lines. We need more comments about this to decide whether to add it automatically.

wangxin · 2019-07-09T02:22:27Z

doc/loganalyzer_to_pytest.md

+
+#### Development
+Module "loganalyzer.py" with class "Loganalyzer". Class is intendent to do:
+- Add start marker to the DUT syslog


How would the start marker be looked like? Should we consider haveing the start marker string contain testname? If so, testname need to be passed in to the init method as an argument.

Existed roles\test\files\tools\loganalyzer\loganalyzer.py will be reused, so logic with adding start/stop markers and processing the logs will be the same (will be reused). And yes, start marker is based on the test name.

wangxin · 2019-07-09T02:23:37Z

doc/loganalyzer_to_pytest.md

+- parse_regexp_file(file_path) - read and parse regular expressions from specified file. Return list of strings of defined regular expressions.
+- run_cmd(callable, *args, **kwargs) - call function and analyze DUT logs, return the same result as "analyze" function
+- init() - Add start marker to the DUT syslog.
+- analyze() - Extract syslog and copy it to ansible host. Analyze extracted syslog files localy. Return python dictionary object.


Extract syslog and copy it to sonic-mgmt container?

Logs will be stored to the host where pytest was started. For now it will be sonic-mgmt container. It will decrease work which are doing on the device side.

wangxin · 2019-07-09T02:34:39Z

doc/loganalyzer_to_pytest.md

+ "/tmp/syslog2": ["Message 1", "Message 2", "Message n"]
+ }
+}
+- save_full_log(file_path) - save downloaded DUT syslog logs to the Ansible host folder specified in 'path' input parameter.


Save log to DUT or to sonic-mgmt container?

To sonic-mgmt container

save_full_log(file_path) - will be better if this will download /tmp/syslog from the DUT to the Ansible host. Because temp logs which are composed by analyze method will be deleted.

Signed-off-by: Yuriy Volynets <yuriyv@mellanox.com>

stepanblyschak · 2019-07-11T11:16:52Z

doc/loganalyzer_to_pytest.md

+"Loganalyzer" class interface:
+- __init__(ansible_host: ansible_host, run_dir="/tmp")
+- load_common_config() - Clear previous configured match, expect and ignore. Load regular expressions from common configuration files: match, expect and ignore which are located in some configuration directory or with current module. Save loaded configuration to the self.match_regex, self.expect_regex and self.ignore_regex attributes.
+- parse_regexp_file(file_path) - Read and parse regular expressions from specified file. Return list of strings of defined regular expressions.


Probably a @classmethod ?

Update Interface-Link-bring-up-sequence.md

Added approaches to move loganalyzer to pytest.

431a0eb

Signed-off-by: Yuriy Volynets <yuriyv@mellanox.com>

wangxin reviewed Jul 8, 2019

View reviewed changes

stepanblyschak suggested changes Jul 8, 2019

View reviewed changes

Added updates, comments fixes.

e7436e5

Signed-off-by: Yuriy Volynets <yuriyv@mellanox.com>

wangxin reviewed Jul 9, 2019

View reviewed changes

Loganalyzer to pytest High level design

b0bb3ae

Signed-off-by: Yuriy Volynets <yuriyv@mellanox.com>

stepanblyschak reviewed Jul 11, 2019

View reviewed changes

liat-grozovik mentioned this pull request Oct 29, 2019

[Test] All drops on L2 interface HLD #2

Closed

yvolynets-mlnx closed this Nov 1, 2019

liat-grozovik pushed a commit to liat-grozovik/SONiC that referenced this pull request Jun 26, 2023

Merge pull request yvolynets-mlnx#1 from jaganbal-a/patch-2

17f1e86

Update Interface-Link-bring-up-sequence.md

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added approaches to move loganalyzer to pytest. #1

Added approaches to move loganalyzer to pytest. #1

yvolynets-mlnx commented Jul 5, 2019

wangxin Jul 8, 2019

wangxin Jul 8, 2019

wangxin Jul 8, 2019

wangxin Jul 8, 2019

yvolynets-mlnx Jul 8, 2019

wangxin Jul 8, 2019

yvolynets-mlnx Jul 8, 2019

stepanblyschak Jul 8, 2019

stepanblyschak Jul 8, 2019

yvolynets-mlnx Jul 8, 2019

stepanblyschak Jul 8, 2019

yvolynets-mlnx Jul 8, 2019

stepanblyschak Jul 8, 2019

yvolynets-mlnx Jul 8, 2019

stepanblyschak Jul 8, 2019

stepanblyschak Jul 8, 2019

stepanblyschak Jul 8, 2019

stepanblyschak commented Jul 8, 2019

wangxin Jul 9, 2019

yvolynets-mlnx Jul 9, 2019

wangxin Jul 10, 2019

wangxin Jul 9, 2019 •

edited

Loading

yvolynets-mlnx Jul 9, 2019

stepanblyschak Jul 10, 2019

yvolynets-mlnx Jul 10, 2019

wangxin Jul 9, 2019

yvolynets-mlnx Jul 9, 2019

wangxin Jul 9, 2019

yvolynets-mlnx Jul 9, 2019

wangxin Jul 9, 2019

yvolynets-mlnx Jul 9, 2019

yvolynets-mlnx Jul 9, 2019

stepanblyschak Jul 11, 2019

Added approaches to move loganalyzer to pytest. #1

Added approaches to move loganalyzer to pytest. #1

Conversation

yvolynets-mlnx commented Jul 5, 2019

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

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

stepanblyschak commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxin Jul 9, 2019 • 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

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

wangxin Jul 9, 2019 •

edited

Loading