feature: add cgroup resources check #1375
Conversation
|
@Ace-Tang please create PR base on branch 0.5.x, thanks a lot !!! 💯 |
|
Hi, @HusterWan , the pr won't include in 0.5.0 |
Codecov Report
@@ Coverage Diff @@
## master #1375 +/- ##
===========================================
+ Coverage 16.23% 34.92% +18.69%
===========================================
Files 200 255 +55
Lines 13583 18329 +4746
===========================================
+ Hits 2205 6402 +4197
+ Misses 11224 11084 -140
- Partials 154 843 +689
|
pkg/system/cgroup.go
Outdated
| } | ||
|
|
||
| func getMemoryCgroupInfo(root string) *MemoryCgroupInfo { | ||
| path := root + "/" + "memory" |
There was a problem hiding this comment.
Change to path.Join is more better?
There was a problem hiding this comment.
Or root + "/memory" more single?
There was a problem hiding this comment.
Actually I do not think root + "/memory" is a good way. String append via + is not very readable and brings some difficulty to code review. While I think for path, we can use path.Join like @rudyfly suggested. For others, I suggested that we should use format to make it like: fmt.Sprintf("you told me %s", msg).
So I think we had better update this.
pkg/system/cgroup.go
Outdated
| } | ||
|
|
||
| func getCPUCgroupInfo(root string) *CPUCgroupInfo { | ||
| cpuPath := root + "/" + "cpu" |
| @@ -10,6 +10,8 @@ import ( | |||
| "github.com/alibaba/pouch/pkg/errtypes" | |||
| "github.com/alibaba/pouch/pkg/meta" | |||
| "github.com/alibaba/pouch/pkg/randomid" | |||
| "github.com/alibaba/pouch/pkg/system" | |||
There was a problem hiding this comment.
please put the github.com/sirupsen/logrus into next import group
| @@ -360,6 +360,12 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty | |||
| container.Snapshotter.Data["UpperDir"] = upperDir | |||
| } | |||
|
|
|||
| // validate container Config | |||
| warnings, err := validateConfig(config) | |||
There was a problem hiding this comment.
@allencloud the config has changed by ContainerManager).Create, like
if config.HostConfig.EnableLxcfs && lxcfs.IsLxcfsEnabled {
config.HostConfig.Binds = append(config.HostConfig.Binds, lxcfs.LxcfsParentDir+":/var/lib/lxc:shared")
sourceDir := lxcfs.LxcfsHomeDir + "/proc/"
destDir := "/proc/"
for _, procFile := range lxcfs.LxcfsProcFiles {
bind := fmt.Sprintf("%s%s:%s%s", sourceDir, procFile, destDir, procFile)
config.HostConfig.Binds = append(config.HostConfig.Binds, bind)
}
}
I don't think we can do validation in the API bridge level.
There was a problem hiding this comment.
@fuweid ,do you mean config is merged with something after ContainerManager).Create?
There was a problem hiding this comment.
Yes @Ace-Tang . If we can move the merge function into bridge, the validation can be done in bridge, too.
check cgroup resource valid in container create, discard un-support cgroup set related flag. Signed-off-by: Ace-Tang <[email protected]>
|
@allencloud @rudyfly @fuweid , updated as comment, but except |
|
CRI ci test fail |
|
LGTM |
pouchrobot
added
the
LGTM
check cgroup resource valid in container create,
discard un-support cgroup set related flag.
Signed-off-by: Ace-Tang [email protected]
Ⅰ. Describe what this PR did
Pouch run get lots of flags that whether these flags can be set depend on the
corresponding cgroup file is exist. Add check for the cgroup file is needed.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews