feature: gc unused exec processes #1129
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1129 +/- ##
==========================================
+ Coverage 15.19% 15.62% +0.42%
==========================================
Files 173 173
Lines 9705 9846 +141
==========================================
+ Hits 1475 1538 +63
- Misses 8125 8197 +72
- Partials 105 111 +6
|
daemon/mgr/container.go
Outdated
| @@ -1564,6 +1566,35 @@ func (mgr *ContainerManager) setBaseFS(ctx context.Context, meta *ContainerMeta, | |||
| meta.BaseFS = filepath.Join(mgr.Config.HomeDir, "containerd/state", "io.containerd.runtime.v1.linux", namespaces.Default, info.Name, "rootfs") | |||
| } | |||
|
|
|||
| // execProcessGC cleans unused exec processes config every 5 minutes. | |||
| func (mgr *ContainerManager) execProcessGC() { | |||
| for range time.Tick(5 * time.Minute) { | |||
There was a problem hiding this comment.
Please do not hard code here in the detailed implementation. Maybe we can define this variable in the front of some file, and make this a const variable.
WDYT? @Ace-Tang
There was a problem hiding this comment.
Actually, I do not think time clock need to be a variable.
There was a problem hiding this comment.
I like this feature very much, and it indeed brings much benefit to improve memory efficiency. While I totally agree with the initial point. While to be honest, I am not on a tiny point of this part. How about making this 5 * time.Minute to be a const variable of DefaultExecGCInterval?
/cc @rudyfly @Letty5411
There was a problem hiding this comment.
I have already update that, but forgot to push. @allencloud .
And I make it a variable, not const.
daemon/mgr/container.go
Outdated
| for id, v := range execProcesses { | ||
| execConfig, ok := v.(*ContainerExecConfig) | ||
| if !ok { | ||
| logrus.Warnf("get uncorrect exec config: %v", v) |
| @@ -25,6 +25,19 @@ func (m *SafeMap) Get(k string) *Value { | |||
| return &Value{v, ok} | |||
| } | |||
|
|
|||
| // Values returns all key-values stored in map | |||
| func (m *SafeMap) Values() map[string]interface{} { | |||
There was a problem hiding this comment.
If m.inner equals to nil, do we need to return nil instead of map[string]interface{}?
Just a little confused on this.
There was a problem hiding this comment.
m.inner can not be nil, since
func NewSafeMap() *SafeMap {
return &SafeMap{
inner: make(map[string]interface{}, 16),
}
}
and if inner is not initialled, it won't be panic.
There was a problem hiding this comment.
Someone has some potential possibility that they do not use NewSafeMap() to create a SafeMap. If they create it via safetyMap := &SafeMap{}, it works fine. But, if they call function safetyMap.Values(), it could panic. Right?
There was a problem hiding this comment.
The same reason with #1132 .
for range a nil map will never cause panic, since nil value will convert to a nil map(map[] ), that add a nil check is useless.
|
updated, and add more test for collect pkg, @allencloud |
add gc to clean unused exec processes add Values funtion to collect pkg, and make the pkg more robust. Signed-off-by: Ace-Tang <[email protected]>
|
Thanks a lot for your update. LGTM |
pouchrobot
added
the
LGTM
add gc to clean unused exec processes
Signed-off-by: Ace-Tang [email protected]
Ⅰ. Describe what this PR did
add gc to clean unused exec processes, gc process will be triggered every 5 minutes.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews