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

bugfix: cannot get restartCount and let restartCount++ after restart container #2132

Merged
merged 1 commit into from
Aug 23, 2018

Conversation

knightXun
Copy link
Contributor

@knightXun knightXun commented Aug 21, 2018

Ⅰ. 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?

  1. when pouch inspect a container, we will get the restartCount of this container.
  2. after restart a container, the restartCount++.

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project size/S labels Aug 21, 2018
@pouchrobot
Copy link
Collaborator

We found this is your first time to contribute to Pouch, @knightXun
👏 We really appreciate it.
Just remind that you have read the contribution guide: https://github.com/alibaba/pouch/blob/master/CONTRIBUTING.md
If you didn't, you should do that first. If done, welcome again and please enjoy hacking! 🍻

@CLAassistant
Copy link

CLAassistant commented Aug 21, 2018

CLA assistant check
All committers have signed the CLA.

@knightXun
Copy link
Contributor Author

/retest

@allencloud
Copy link
Collaborator

Actually we have not supported /retest comment to re-trigger test re-running. Add a issue pouchcontainer/pouchrobot#53 in pouchrobot.

@HusterWan
Copy link
Contributor

@knightXun CRI test case failed has been solved, please rebase your pr , thanks a lot.

@knightXun
Copy link
Contributor Author

@HusterWan thanks

@codecov-io
Copy link

codecov-io commented Aug 22, 2018

Codecov Report

Merging #2132 into master will increase coverage by 0.06%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#criv1alpha1test 32.97% <0%> (+0.01%) ⬆️
#criv1alpha2test 33.59% <0%> (+0.02%) ⬆️
#integrationtest 39.31% <87.5%> (+0.06%) ⬆️
#unittest 23.89% <0%> (-0.02%) ⬇️
Impacted Files Coverage Δ
apis/server/container_bridge.go 75.92% <100%> (+0.08%) ⬆️
daemon/mgr/container.go 56.71% <71.42%> (+0.26%) ⬆️
ctrd/watch.go 72.72% <0%> (-7.58%) ⬇️
cri/stream/httpstream/spdy/upgrade.go 54.28% <0%> (-5.72%) ⬇️
cri/v1alpha2/cri.go 65.09% <0%> (+0.33%) ⬆️
cri/v1alpha1/cri.go 64.04% <0%> (+0.34%) ⬆️
ctrd/image.go 78.94% <0%> (+3.94%) ⬆️
apis/server/utils.go 66.66% <0%> (+4.76%) ⬆️

@@ -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++
Copy link
Contributor

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

Copy link
Contributor Author

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

@pouchrobot
Copy link
Collaborator

@knightXun Thanks for your contribution. 🍻
Please sign off in each of your commits.

@allencloud
Copy link
Collaborator

Thanks a lot for your contribution. @knightXun Here I have a little bit confusion for you:

RestartCount counts all restart counts of a container including manual restart from user's trigger and restart from restart policy; or just from restart policy?

@knightXun
Copy link
Contributor Author

knightXun commented Aug 23, 2018

@allencloud yeah,manual restart should also be counted.

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Aug 23, 2018
@allencloud
Copy link
Collaborator

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 allencloud merged commit 90f165a into AliyunContainerService:master Aug 23, 2018
@knightXun
Copy link
Contributor Author

@allencloud [email protected], that's my email. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants