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

feature: CONTAINER_PAUSE state for CRI #2681

Merged

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 zhuangqhc@gmail.com

@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
Signed-off-by: zhuangqh <zhuangqhc@gmail.com>
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