Skip to content
This repository was archived by the owner on Jun 28, 2024. It is now read-only.

Add container restart case#2140

Merged
GabyCT merged 2 commits intokata-containers:masterfrom
darfux:add-container-restart-case
Dec 17, 2019
Merged

Add container restart case#2140
GabyCT merged 2 commits intokata-containers:masterfrom
darfux:add-container-restart-case

Conversation

@darfux
Copy link
Copy Markdown
Contributor

@darfux darfux commented Dec 4, 2019

This PR mainly adds a crictl cri integration test patch based container restart case as well as fix two cases that doesn't exist in the cri integration test.
I think it's better to add such case to https://github.com/containerd/cri/tree/master/integration and I'll try to raise a PR there about this later. If that case is merged then we can use it in future after vendor update :)

Fixes: #2138, #2139
Signed-off-by: Li Yuxuan liyuxuan04@baidu.com

@darfux darfux force-pushed the add-container-restart-case branch from e2ec530 to fdf5fd3 Compare December 4, 2019 10:56
Remove `TestClearContainersCreate` and correct
`TestContainerListStatsWithSandboxIdFilterd` which cause the test suit
to complain with "warning: no tests to run".

Fixes: kata-containers#2138
Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
@darfux darfux force-pushed the add-container-restart-case branch from fdf5fd3 to f10f357 Compare December 4, 2019 12:28
@chavafg
Copy link
Copy Markdown
Contributor

chavafg commented Dec 4, 2019

/test

@darfux darfux force-pushed the add-container-restart-case branch from f10f357 to 2e7c1d9 Compare December 4, 2019 15:19
@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Dec 4, 2019

It seems that not all the jobs run cri integration-tests with containerd configured. I've added a detection @chavafg

@egernst
Copy link
Copy Markdown
Member

egernst commented Dec 4, 2019

/test

@darfux darfux force-pushed the add-container-restart-case branch from 2e7c1d9 to 528beb0 Compare December 5, 2019 03:26
@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Dec 5, 2019

Still many jobs failed🤣 , so I decide to patch the cri integration test directly. PTAL @chavafg @egernst

@darfux darfux force-pushed the add-container-restart-case branch 6 times, most recently from cdb6a00 to 6c9ebc3 Compare December 5, 2019 05:38
@chavafg
Copy link
Copy Markdown
Contributor

chavafg commented Dec 5, 2019

/test

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Dec 6, 2019

CI seems happy now. I think the ARM job is not fail on this commit 😃

Copy link
Copy Markdown
Contributor

@chavafg chavafg left a comment

Choose a reason for hiding this comment

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

Thanks @darfux, please let us know when you submit the test to the containerd/cri repository.

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Dec 9, 2019

@chavafg OK:)

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Dec 13, 2019

Hi @chavafg, I've submitted to containerd/cri#1357. But as Liu said the case may be moved to cri-tools as well. Shall we merge this PR at first as kata-containers/runtime#2008 is waiting to be tested, and then switch to cri-tools or cri integration based one in future re-vendor? 😉

@chavafg
Copy link
Copy Markdown
Contributor

chavafg commented Dec 13, 2019

thanks @darfux, yeah, I think we can merge this, but need another ack.
ping @kata-containers/tests

Copy link
Copy Markdown
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

lgtm

See the License for the specific language governing permissions and
limitations under the License.
*/
// Copyright (c) 2018 Intel Corporation
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: you could add yourself/org to the copyright header maybe if you wanted.
And trim the apache license header down to just the SPDX style one, to match the rest of the kata code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks ! Updated to be consistent with the cri patch :)

Add a cri-containerd integration case that restart same container inside
a pod.

Fixes: kata-containers#2139
Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
@darfux darfux force-pushed the add-container-restart-case branch from 6c9ebc3 to 620681f Compare December 13, 2019 15:54
@chavafg
Copy link
Copy Markdown
Contributor

chavafg commented Dec 13, 2019

/test

@jodh-intel
Copy link
Copy Markdown

Restarted ARM CI which appeared to have failed due to a network timeout.

@GabyCT GabyCT merged commit 098ebf2 into kata-containers:master Dec 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cri-containerd case show "testing: warning: no tests to run"

6 participants