Skip to content
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

[device map] fix device map creation issue and caching issue #2978

Merged
merged 1 commit into from
Feb 16, 2021

Conversation

yxieca
Copy link
Collaborator

@yxieca yxieca commented Feb 11, 2021

Description of PR

Summary:

Type of change

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

Approach

What is the motivation for this PR?

lag_2 was failing on dualtor testbed with 3 tests. 3 particular lag always fail. Investigation shows that the cached ptf port map was not right.

How did you do it?

Device map creation could be aborted in the middle and leave map half updated if an extra interface was not defined in the
topology but show up in the minigraph.

And because we cache the host variables on the file system, the cache could prevent a correct map being generated.

Add a new pretest to remove cache. It would still be good to generate cache for each nightly test.

Remove cache removal from kvmtest.sh to leave it for pretest.

Signed-off-by: Ying Xie [email protected]

How did you verify/test it?

Verified that the new pretest will remove _cache if exists.

Verified that newly generated port map for dualtor testbed is correct.

lag_2 passes with the change.

Device map creation could be aborted in the middle and leave
map half updated if an extra interface was not defined in the
topology but show up in the minigraph.

And because we cache the host variables on the file system,
the cache could prevent a correct map being generated.

Add a new pretest to remove cache. It would still be good to
generate cache for each nightly test.

Update kvmtest.sh to stop removing cache since the pretest
will do it now.

Signed-off-by: Ying Xie <[email protected]>
@sanmalho-git
Copy link
Contributor

@yxieca I have a PR #2958 that addresses cache cleanup at the end of a pytest session - would that work for you instead of test_cache_cleanup in test_pretest. My changes only delete the cache for the DUT's, but not the whole cache directory. Maybe that can be changed to cleanup the whole directory as well.

@yxieca
Copy link
Collaborator Author

yxieca commented Feb 11, 2021

@yxieca I have a PR #2958 that addresses cache cleanup at the end of a pytest session - would that work for you instead of test_cache_cleanup in test_pretest. My changes only delete the cache for the DUT's, but not the whole cache directory. Maybe that can be changed to cleanup the whole directory as well.

@sanmalho-git Sorry I didn't see your change earlier. So your change works if we run the test in group mode, in this mode, there will be 3 sessions: pretest group, all actual tests, post test group. However, in practice, we use individual mode more than group mode so that we have logs for each individual test. In individual mode, your change will cleanup cache after each test file. Please assess accordingly.

I added the cleanup in pretest, so that the cleanup will give nightly test a fresh start but don't impose performance penalty for the nightly test suite.

@sanmalho-git
Copy link
Contributor

@yxieca I had put it per session because of the following reasons:

  • During test case development, if we change the inventory and run of an individual test, then we were picking up stale cache data. This was just for convenience to deleting the _cache directory manually;
  • In our express/sanity run, we we are running in 'group' mode and not running pretest or posttest.

But, I see your point with individual mode in a regression run, we are cleaning up the cache for no reason. I will remove my fixture from my PR #2958

@yxieca yxieca merged commit e22724d into sonic-net:master Feb 16, 2021
@yxieca yxieca deleted the cache branch February 16, 2021 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants