Add caching capability for frequently used facts by wangxin · Pull Request #2789 · sonic-net/sonic-mgmt

wangxin · 2021-01-12T09:08:24Z

Description of PR

Summary:
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Approach

What is the motivation for this PR?

To run test scripts, we frequently need to gather facts from various devices again and again. Most of the facts gatherings need to run some commands on remote devices through SSH connection, parse the commands output and return the results. Most of the time, the facts to be gathered are unchanged, like DUT HWSKU, platform, etc. For the less frequently changed facts, we can cache them for quicker access to save a lot of overhead for gathering them each time. Then we can improve the overall time required for running all the tests.

How did you do it?

This change added caching capability for frequently used facts. For more details, please refer to the documentation included in this change.

How did you verify/test it?

Currently caching has been enabled for duthost.facts and duthost.get_extended_minigraph_facts.

Created a simple script like below to verify that caching works as expected:

def test_cache1(duthost, tbinfo):
    mg_facts = duthost.get_extended_minigraph_facts(tbinfo)

def test_cache2(duthost, tbinfo):
    mg_facts = duthost.get_extended_minigraph_facts(tbinfo)

Run testbed-cli.sh deploy-mg to verify that cached json files of current DUT are cleaned.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

To run test scripts, we frequently need to gather facts from various devices again and again. Most of the facts gatherings need to run some commands on remote devices through SSH connection, parse the commands output and return the results. Most of the time, the facts to be gathered are unchanged, like DUT HWSKU, platform, etc. For the less frequently changed facts, we can cache them for quicker access to save a lot of overhead for gathering them each time. Then we can improve the overall time required for running all the tests. This change added caching capability for frequently used facts. For more details, please refer to the documentation included in this change. Signed-off-by: Xin Wang <[email protected]>

bingwang-ms · 2021-01-12T09:33:39Z

tests/common/cache/facts_cache.py

+    cache = FactsCache()
+    def decorator(target):
+        def wrapper(*args, **kwargs):
+            hostname = args[0].hostname


I suggest adding a check for existence for hostname in args[0] in case this wrapper is used at none subclass of AnsibleHostBase. If hostname not exists, just call target directly.

ansible/config_sonic_basedon_testbed.yml

lolyu

Should we limit the cache size(entries count or memory size)?

wangxin · 2021-01-13T04:18:02Z

Should we limit the cache size(entries count or memory size)?

I think that's a nice to have feature. Currently we are just using it to cache a few dictionaries in json file. It's OK for now. We can enhance it in the future when storage size is becoming a real issue.

lolyu · 2021-01-13T05:53:27Z

Should we limit the cache size(entries count or memory size)?

I think that's a nice to have feature. Currently we are just using it to cache a few dictionaries in json file. It's OK for now. We can enhance it in the future when storage size is becoming a real issue.

Agreed, for the future multiple DUTs in one testbed, the memory consumption might be a challenge for the test running host. It would be nicer if we could limit how many cache entries we store in memory and store all other caches that exceed a certain threshold only to files.

wangxin · 2021-01-13T06:49:11Z

Should we limit the cache size(entries count or memory size)?

I think that's a nice to have feature. Currently we are just using it to cache a few dictionaries in json file. It's OK for now. We can enhance it in the future when storage size is becoming a real issue.

Agreed, for the future multiple DUTs in one testbed, the memory consumption might be a challenge for the test running host. It would be nicer if we could limit how many cache entries we store in memory and store all other caches that exceed a certain threshold only to files.

Added cache usage check.

wangxin · 2021-01-14T02:11:54Z

retest vsimage please

wangxin · 2021-01-15T00:09:33Z

retest vsimage please

…xtures (#2811) What is the motivation for this PR? pytest_generate_tests dynamically adds enum_* fixtures that select DUTs on which the tests are to be parameterized. In order to select the DUTs on which the tests are to run, we need to get the TestbedInfo and also all variables defined in the inventory file for each DUT. These operations are time consuming - like creating TestbedInfo taking 1-9 seconds, and getting variables from inventory taking 3-5 seconds. pytest_generate_tests is called for all the tests/fixtures that needs to run in a pytest session. This was adding a long time when trying to execute many tests. This issue was reported in issue #2790 - pytest_generate_tests in tests/conftest.py takes much more time than before How did you do it? To fix this, we need to create the TestbedInfo only once, store it, and then use the stored value in the next execution of pytest_generate_tests, rather than re-creating TestbedInfo. Similary, the variables for all the DUTs in the testbed should be read once from the inventory files, stored, and the re-used in the next execution of pytest_generate_tests. PR #2789 Added caching capability to store any facts using pickle. PR #2856 added caching capability for TestbedInfo PR #2873 added caching capability for variables for DUTs in the inventory files. We use the cached TestbedInfo and DUT variables in the selection of DUTs in pytest_generate_tests. How did you verify/test it? Ran tests that use the dut selection fixtures and validated that the delay is seen only once, and not in every call to pytest_generate_tests. Validated the execution time reduced significantly when using caching when executing iface_namingmode test cases which have ~30 tests.

…lly (sonic-net#15387) src/sonic-swss * 2aec547 - (HEAD -> 202205, origin/202205) [pfcwd] Enhance DLR_INIT based recovery and DLR_PACKET_ACTION for broadcom platforms (sonic-net#2807) (4 days ago) [Neetha John] * fa4acd3 - Fix to substract the macsec sectag size from port MTU during InitializePort (sonic-net#2789) (5 days ago) [judyjoseph]

wangxin requested a review from a team January 12, 2021 09:08

bingwang-ms reviewed Jan 12, 2021

View reviewed changes

ansible/config_sonic_basedon_testbed.yml Show resolved Hide resolved

lolyu reviewed Jan 12, 2021

View reviewed changes

wangxin added 2 commits January 13, 2021 06:15

Add checking of 'hostname' attribute.

b3b2776

Add cache usage check

a71cdc4

bingwang-ms approved these changes Jan 14, 2021

View reviewed changes

wangxin merged commit 3af49cc into sonic-net:master Jan 19, 2021

wangxin deleted the cache-facts branch February 1, 2021 08:22

sanmalho-git mentioned this pull request Feb 4, 2021

Use cached testbedInfo and host_vars in inventory in dut selection fixtures #2811

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add caching capability for frequently used facts#2789

Add caching capability for frequently used facts#2789
wangxin merged 3 commits intosonic-net:masterfrom
wangxin:cache-facts

wangxin commented Jan 12, 2021

Uh oh!

bingwang-ms Jan 12, 2021

Uh oh!

Uh oh!

lolyu left a comment

Uh oh!

wangxin commented Jan 13, 2021

Uh oh!

lolyu commented Jan 13, 2021 •

edited

Loading

Uh oh!

wangxin commented Jan 13, 2021

Uh oh!

wangxin commented Jan 14, 2021

Uh oh!

wangxin commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

wangxin commented Jan 12, 2021

Description of PR

Type of change

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Uh oh!

bingwang-ms Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lolyu left a comment

Choose a reason for hiding this comment

Uh oh!

wangxin commented Jan 13, 2021

Uh oh!

lolyu commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wangxin commented Jan 13, 2021

Uh oh!

wangxin commented Jan 14, 2021

Uh oh!

wangxin commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lolyu commented Jan 13, 2021 •

edited

Loading