-
Notifications
You must be signed in to change notification settings - Fork 949
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
bugfix: cannot get restartCount and let restartCount++ after restart container #2132
bugfix: cannot get restartCount and let restartCount++ after restart container #2132
Conversation
We found this is your first time to contribute to Pouch, @knightXun |
030773a
to
cb154cb
Compare
/retest |
Actually we have not supported |
@knightXun CRI test case failed has been solved, please rebase your pr , thanks a lot. |
cb154cb
to
21d9355
Compare
@HusterWan thanks |
Codecov Report
@@ Coverage Diff @@
## master #2132 +/- ##
==========================================
+ Coverage 64.38% 64.44% +0.06%
==========================================
Files 209 209
Lines 16631 16638 +7
==========================================
+ Hits 10708 10723 +15
+ Misses 4595 4587 -8
Partials 1328 1328
|
@@ -796,8 +796,20 @@ func (mgr *ContainerManager) Restart(ctx context.Context, name string, timeout i | |||
} | |||
|
|||
logrus.Debugf("start container %s when restarting", c.ID) | |||
|
|||
//let restartCount++ |
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.
actually, we should add container lock here when we visit container object
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.
that's not a problem, i think
21d9355
to
6895e09
Compare
6895e09
to
a328bbc
Compare
@knightXun Thanks for your contribution. 🍻 |
Thanks a lot for your contribution. @knightXun Here I have a little bit confusion for you:
|
@allencloud yeah,manual restart should also be counted. |
LGTM |
Thanks a lot for your contribution. @knightXun This is quite fantastic for us. What is more, could you provide your email address to me? Maybe in the future, we could have more communication. |
@allencloud [email protected], that's my email. thanks |
Ⅰ. Describe what this PR did
pouch inspect -f {{.RestartCount}} can't get corrent restartCount.Add one line code to fix it.
the restartCount will not change after manually restart a container, this pr fix it
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews