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: add lxcfs flag to cli #534

Merged
merged 1 commit into from
Jan 10, 2018

Conversation

CodeJuan
Copy link
Contributor

@CodeJuan CodeJuan commented Jan 10, 2018

1.Describe what this PR did
Add lxcfs flag to cli

2.Does this pull request fix one issue?

3.Describe how you did it

4.Describe how to verify it

  1. start pouchd with lxcfs
pouchd --enable-lxcfs=true --lxcfs=/usr/bin/lxcfs --lxcfs-home="/var/lib/lxc/lxcfs/"
  1. pouch create with lxcfs
./pouch create --enableLxcfs=true  docker.io/library/busybox:latest cat /proc/uptime
./pouch inspect [container ID]

the value of EnableLxcfs should be true.

  1. pouch run with lxcfs
pouch run --enableLxcfs=true  docker.io/library/busybox:latest cat /proc/uptime

the output should be equal to "0.0 0.0"

5.Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Jan 10, 2018

CLA assistant check
All committers have signed the CLA.

cli/container.go Outdated
@@ -91,5 +92,9 @@ func (c *container) config() (*types.ContainerCreateConfig, error) {
}
}

if c.enableLxcfs {
config.EnableLxcfs = true
Copy link
Collaborator

@allencloud allencloud Jan 10, 2018

Choose a reason for hiding this comment

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

use config.EnableLxcfs = c.enableLxcfs ?

@CodeJuan CodeJuan changed the title add lxcfs flag to cli feature: add lxcfs flag to cli Jan 10, 2018
@allencloud allencloud added this to the v0.1-Milestone milestone Jan 10, 2018
@@ -118,3 +118,23 @@ func (suite *PouchCreateSuite) TestCreateWithLabels(c *check.C) {
c.Errorf("failed to set label: %s", label)
}
}

// TestCreateEnableLxcfs tries to test create a container with lxcfs.
func (suite *PouchCreateSuite) TestCreateEnableLxcfs(c *check.C) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually currently pouchd has enabled lxcfs by default, right? @CodeJuan @Letty5411
Then it seems these test setting enablelxcfs does not work as expected, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's causeed by the environment.
The output of pouch run --enableLxcfs=true docker.io/library/busybox:latest cat /proc/uptime was 0.0 0.0 on my machine, so I set the expected value=0.0 0.0, but the output of CI was 0.00 0.00

@CodeJuan CodeJuan force-pushed the lxcfs_flag branch 2 times, most recently from 315dd37 to c0424cb Compare January 10, 2018 08:19
@codecov-io
Copy link

codecov-io commented Jan 10, 2018

Codecov Report

Merging #534 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #534   +/-   ##
======================================
  Coverage    17.2%   17.2%           
======================================
  Files          38      38           
  Lines        2040    2040           
======================================
  Hits          351     351           
  Misses       1653    1653           
  Partials       36      36

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 8b94d7d...f0465d0. Read the comment docs.

Signed-off-by: codejuan <[email protected]>
@@ -28,6 +28,7 @@ type container struct {
memorySwap string
memorySwappiness int64
devices []string
enableLxcfs bool
}

func (c *container) config() (*types.ContainerCreateConfig, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this function is a little bit in mess, and we need to make it more clear.

@allencloud
Copy link
Collaborator

LGTM

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

Successfully merging this pull request may close these issues.

5 participants