Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[pytest] Test the feature of container checker. #2890
[pytest] Test the feature of container checker. #2890
Changes from 30 commits
2453fb3
b11e303
28e9da5
eab6ece
6dde433
91fcad3
1b9eae5
0f86170
e4ae303
7e0b948
622cace
206bc63
043aa71
619fe8f
f52b2e6
c25f23a
b02cdca
ce74aa2
5c2ac25
9fe97e3
dc43951
d8b2d7e
7280c23
b04c5a6
db478d4
c398d29
669b590
7b4b6b1
6a6696c
1dab708
47eecca
23d844b
e8131dc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@bingwang-ms @wangxin Please help me review this PR again. The previous PR (#2852) was reverted due to BGP sessions were down after all containers are restarted and then it failed the post-check on KVM testbed. The root reason is I should not restart the containers according to the sequence of stopped containers. The correct way is to use
config_reload(...)
to restart the containers since some containers are depended on others. After running theconfig_reload(...)
, the line 255 will check whether all stopped containers are running or not and it will restart some containers in case theconfig_reload(...)
did not restart them such as the containermgmt-framework
.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. Except for the
time.sleep(360)
that guohan has mentioned. Please update the monit config for container_checker and shorten the sleep time. 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.
Thanks, Bing! Please help me review the function
update_monit_service(...)
and the line 274.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.
Why do we have to do config_reload here? is config_reload_after_tests not enough?
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 script will first do the
config_reload(...)
to start the stopped containers and then do the post check to check whether BGP session are established and all critical processes are running,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.
Why not move config_reload and the checking below to config_reload_after_tests?
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.
@yxieca This pytest script will skip the DuTs which were installed 201911 image. If this pytest script was ran against a DuT which was installed public image and if I removed the
config_reload(...)
at here and only invokedconfig_reload(...)
in the fixtureconfig_reload_after_test(...)
, then themgmt-framework
container will not be restarted. That is why at there this script will first invokeconfig_reload(..)
and use the functioncheck_containers_status(...)
to restart the containers which can not be restarted byconfig_reload(...)
.