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: set pid of stopped container to 0 #2193

Merged
merged 1 commit into from
Sep 3, 2018

Conversation

HusterWan
Copy link
Contributor

Signed-off-by: Michael Wan [email protected]

Ⅰ. Describe what this PR did

We set pid of stopped container to -1 before, it's ok that we do not check value of pid but the status of container when we want to check whether the container is stopped or not.

But now, pouchd supported CRI, the k8s will check the value of pid when the container stopped and the pid is 0 when container stopped.

So, in order to stay with moby and kubernetes, we also should change the pid to 0 when the container is stopped.

Ⅱ. Does this pull request fix one issue?

none

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

added TestStopPidValue test case

Ⅳ. Describe how to verify it

root@osboxes:test (zr/pid-fix *) -> pouch run -d -t  centos top
d42b8e1c8c45dbdbf53f1fb2432ba6a0c2d730007601c8d47f2bdf94508f8c58
root@osboxes:test (zr/pid-fix) -> pouch ps
Name     ID       Status         Created         Image                                           Runtime
d42b8e   d42b8e   Up 3 seconds   4 seconds ago   registry.hub.docker.com/library/centos:latest   runc
root@osboxes:test (zr/pid-fix) -> pouch stop -t 3 d42b8e
d42b8e
root@osboxes:test (zr/pid-fix) -> pouch inspect -f '{{.State.Pid}}' d42b8e
0

Ⅴ. Special notes for reviews

None

@pouchrobot pouchrobot added kind/bug This is bug report for project size/S labels Sep 3, 2018
@@ -126,3 +126,21 @@ func (suite *PouchStopSuite) TestStopMultiContainers(c *check.C) {
c.Assert(string(result[0].State.Status), check.Equals, "stopped")

}

// TestStopPidValue ensure stopped container's pid is 0
func (suite *PouchStopSuite) TestStopPidValue(c *check.C) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add one case for the process exits by itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense

@rudyfly
Copy link
Collaborator

rudyfly commented Sep 3, 2018

LGTM

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

You have filled up the beautiful pull request description. @HusterWan

@codecov-io
Copy link

codecov-io commented Sep 3, 2018

Codecov Report

Merging #2193 into master will increase coverage by 5.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2193      +/-   ##
==========================================
+ Coverage   59.91%   64.97%   +5.06%     
==========================================
  Files         209      209              
  Lines       16739    16739              
==========================================
+ Hits        10029    10877     +848     
+ Misses       5488     4515     -973     
- Partials     1222     1347     +125
Flag Coverage Δ
#criv1alpha1test 32.73% <100%> (+0.02%) ⬆️
#criv1alpha2test 33.61% <100%> (?)
#integrationtest 39.96% <100%> (+0.01%) ⬆️
#unittest 23.95% <0%> (ø) ⬆️
Impacted Files Coverage Δ
daemon/mgr/container_state.go 87.23% <100%> (ø) ⬆️
daemon/mgr/container.go 56.98% <0%> (+0.61%) ⬆️
ctrd/container.go 53.82% <0%> (+0.95%) ⬆️
daemon/mgr/snapshot.go 94.2% <0%> (+4.34%) ⬆️
cri/stream/runtime.go 68.23% <0%> (+5.88%) ⬆️
ctrd/watch.go 80.3% <0%> (+7.57%) ⬆️
cri/stream/remotecommand/httpstream.go 44.04% <0%> (+9.32%) ⬆️
cri/v1alpha2/cri_utils.go 82.55% <0%> (+22.55%) ⬆️
cri/criservice.go 64.7% <0%> (+24.99%) ⬆️
cri/v1alpha2/cri_wrapper.go 52.71% <0%> (+52.71%) ⬆️
... and 5 more

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fuweid fuweid merged commit 4e03146 into AliyunContainerService:master Sep 3, 2018
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