Skip to content

Conversation

@zhuangqh
Copy link
Contributor

@zhuangqh zhuangqh commented Jan 22, 2019

Ⅰ. Describe what this PR did

Pause/Unpause container interface are added to CRI, but missing a PAUSE state from ContainerStatus or ListContainer to figure out the paused container.

Ⅱ. Does this pull request fix one issue?

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

cri test here.
alibaba-archive/cri-tools#11

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Signed-off-by: zhuangqh [email protected]

@codecov
Copy link

codecov bot commented Jan 22, 2019

Codecov Report

Merging #2681 into master will decrease coverage by 0.01%.
The diff coverage is 84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2681      +/-   ##
==========================================
- Coverage   69.32%   69.31%   -0.02%     
==========================================
  Files         285      285              
  Lines       18959    18948      -11     
==========================================
- Hits        13144    13134      -10     
+ Misses       4346     4345       -1     
  Partials     1469     1469
Flag Coverage Δ
#criv1alpha1test 31.35% <0%> (-0.01%) ⬇️
#criv1alpha2test 35.82% <72%> (-0.1%) ⬇️
#integrationtest 42.03% <0%> (-0.01%) ⬇️
#nodee2etest 32.08% <72%> (-0.07%) ⬇️
#unittest 27.01% <64%> (+0.05%) ⬆️
Impacted Files Coverage Δ
cri/v1alpha2/cri.go 72.43% <100%> (+0.29%) ⬆️
cri/v1alpha2/cri_utils.go 88.1% <81.81%> (-0.34%) ⬇️
apis/server/utils.go 71.15% <0%> (-3.85%) ⬇️
ctrd/container.go 56.81% <0%> (-0.39%) ⬇️
daemon/mgr/container.go 58.94% <0%> (ø) ⬆️

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XXL labels Jan 22, 2019
// * Case 2: container has failed to start; it has a zero finishedAt
// time, but a non-zero exit code.
// * Case 3: container has been created, but not started (yet).
finishTime, err := time.Parse(utils.TimeLayout, container.State.FinishedAt)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PouchContainer will promise the consistency about FinishedAt, CreatedAt, StartedAt and container state

@zhuangqh zhuangqh changed the title fix: CONTAINER_PAUSE state for CRI feature: CONTAINER_PAUSE state for CRI Jan 22, 2019
@zhuangqh zhuangqh removed the kind/bug This is bug report for project label Jan 22, 2019
@zhuangqh zhuangqh force-pushed the fix-cri-pause-state branch from 5aef036 to 6f6102e Compare January 22, 2019 07:41
@zhuangqh zhuangqh force-pushed the fix-cri-pause-state branch from 6f6102e to 07a0ee5 Compare January 22, 2019 09:52
return runtime.ContainerState_CONTAINER_RUNNING
case apitypes.StatusExited:
return runtime.ContainerState_CONTAINER_EXITED
func toCriContainerState(state *apitypes.ContainerState) (criState runtime.ContainerState, reason string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why just define the return type runtime.ContainerState, string and return the value in the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you mean the named return value?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@HusterWan
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 28, 2019
@HusterWan HusterWan merged commit 9df7eab into AliyunContainerService:master Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants