Skip to content

Conversation

@chuanchang
Copy link
Contributor

Ⅰ. Describe what this PR did

Add new checkpoints and make sure cgroup setting is valid in inspect of container, configuration of host and the container.

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

$ go test -test.v -check.f PouchRunBlkioSuite
=== RUN Test
OK: 3 passed
--- PASS: Test (24.99s)
PASS
ok github.com/alibaba/pouch/test 25.103s

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Sep 29, 2018

Codecov Report

Merging #2291 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2291      +/-   ##
=========================================
+ Coverage   66.75%   66.8%   +0.04%     
=========================================
  Files         208     208              
  Lines       16971   16971              
=========================================
+ Hits        11329   11337       +8     
+ Misses       4275    4270       -5     
+ Partials     1367    1364       -3
Flag Coverage Δ
#criv1alpha1test 32.56% <ø> (-0.03%) ⬇️
#criv1alpha2test 36.14% <ø> (-0.03%) ⬇️
#integrationtest 39.46% <ø> (-0.03%) ⬇️
#nodee2etest 33.46% <ø> (+0.18%) ⬆️
#unittest 23.75% <ø> (ø) ⬆️
Impacted Files Coverage Δ
daemon/mgr/snapshot.go 89.85% <0%> (-4.35%) ⬇️
cri/v1alpha2/cri_wrapper.go 61.2% <0%> (-1.2%) ⬇️
daemon/mgr/container.go 57.63% <0%> (+0.2%) ⬆️
cri/v1alpha2/cri_utils.go 90.57% <0%> (+0.28%) ⬆️
cri/v1alpha2/cri.go 66.76% <0%> (+0.58%) ⬆️
cri/ocicni/cni_manager.go 80% <0%> (+15%) ⬆️

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@chuanchang chuanchang requested a review from fuweid September 29, 2018 16:06
@chuanchang chuanchang force-pushed the add_checkpoints_for_PouchRunBlkioSuite branch 2 times, most recently from d6319df to d847b5d Compare September 29, 2018 16:33
@chuanchang chuanchang changed the title Add checkpoints for pouch run blkio suite Add checkpoints for PouchRunBlkioSuite Sep 29, 2018
@fuweid
Copy link
Contributor

fuweid commented Sep 29, 2018

basically, LGTM. But please rebase the commits into one 😄

@fuweid
Copy link
Contributor

fuweid commented Sep 29, 2018

please check the error

test/cli_run_blkio_test.go:116:19: cannot refer to unexported name util.getMajMinNumOfDevice
test/cli_run_blkio_test.go:116:19: undefined: util.getMajMinNumOfDevice

test.util.util: add new GetMajMinNumofDevice function to get
                major:minor device number
test.cli_run_blkio_test: add host and container checkpoints

Signed-off-by: Alex Jia <[email protected]>
@chuanchang chuanchang force-pushed the add_checkpoints_for_PouchRunBlkioSuite branch from d847b5d to 1e36dc8 Compare September 30, 2018 01:30
@chuanchang
Copy link
Contributor Author

@fuweid done, thanks for reviewing, and v2 patch is coming.

@allencloud allencloud changed the title Add checkpoints for PouchRunBlkioSuite test: Add checkpoints for PouchRunBlkioSuite Sep 30, 2018
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 fa85aa5 into AliyunContainerService:master Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants