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: sysctl configuration for both cri manager and container manager #654

Merged
merged 1 commit into from
Jan 30, 2018

Conversation

YaoZengzeng
Copy link
Contributor

@YaoZengzeng YaoZengzeng commented Jan 26, 2018

Signed-off-by: YaoZengzeng [email protected]

1.Describe what this PR did

2.Does this pull request fix one issue?

fixes part of #635

3.Describe how you did it

4.Describe how to verify it

[root@pouch-test monster]# pouch run --sysctl kernel.shm_rmid_forced=1 docker.io/library/busybox:latest
[root@pouch-test monster]# pouch exec 7ee857 cat /proc/sys/kernel/shm_rmid_forced
1

5.Special notes for reviews

@YaoZengzeng
Copy link
Contributor Author

@allencloud already passed corresponding CRI test.

@Letty5411 PTAL.

@codecov-io
Copy link

codecov-io commented Jan 26, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@24e3f35). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #654   +/-   ##
=========================================
  Coverage          ?   10.61%           
=========================================
  Files             ?       70           
  Lines             ?     3277           
  Branches          ?        0           
=========================================
  Hits              ?      348           
  Misses            ?     2890           
  Partials          ?       39
Impacted Files Coverage Δ
cli/container.go 35.59% <0%> (ø)
cli/run.go 0% <0%> (ø)
cli/create.go 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24e3f35...d9a5e93. Read the comment docs.

@YaoZengzeng
Copy link
Contributor Author

@allencloud already add the CRI related tests into CI.

@allencloud
Copy link
Collaborator

@YaoZengzeng CI fails with gofmt error:

cli/container.go
please format Go code with 'gofmt -s -w'
Makefile:42: recipe for target 'fmt' failed
make: *** [fmt] Error 1
make: *** [test] Error 2

@Letty5411
Copy link
Contributor

Please also add a cli_run_test.

cli/container.go Outdated
for _, sysctl := range sysctls {
fields := strings.SplitN(sysctl, "=", 2)
if len(fields) != 2 {
return nil, fmt.Errorf("invalid sysctl: %s", sysctl)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you provide more invalid reasons? @YaoZengzeng
I think here we tell user the invalidness of input, but we have not told them why it is invalid which will increase his confusion and time in finding ways to solve this. Maybe more time for us to support these cases. But if we add the invalid reasons. It may reduce much potential work no matter in developing or supporting. How about the format fmt.Errorf("invalid sysctl %s: sysctl must be in format of key=value", sysctl).

I have to say many places have the same issue. While we need to improve that places. In addition, I think we need to update the cli flag illustration as well, since they are not so readable for users. We code them just in developer's perspectives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allencloud can't agree more.

@YaoZengzeng
Copy link
Contributor Author

@allencloud @Letty5411 updated.

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 30, 2018
@allencloud allencloud merged commit 830114d into AliyunContainerService:master Jan 30, 2018
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/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants