enhance: split the containerd daamon control from ctrd #2318
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2318 +/- ##
==========================================
- Coverage 67.33% 67.29% -0.04%
==========================================
Files 213 215 +2
Lines 17517 17574 +57
==========================================
+ Hits 11795 11827 +32
- Misses 4327 4348 +21
- Partials 1395 1399 +4
|
ctrd/supervisord/daemon.go
Outdated
| return err | ||
| } | ||
|
|
||
| // reap the contaienrd process when it has been killed |
There was a problem hiding this comment.
%s/contaienrd/containerd
| ) | ||
| if err != nil { | ||
| logrus.Errorf("failed to start containerd: %v", err) | ||
| return nil |
There was a problem hiding this comment.
Why return nil ?
I think we should return an error when NewDaemon got an error, Maybe we should fix this in later PR
|
LGTM |
pouchrobot
added
the
LGTM
| @@ -3,30 +3,15 @@ package ctrd | |||
| import "fmt" | |||
|
|
|||
| type clientOpts struct { | |||
| startDaemon bool | |||
| debugLog bool | |||
| rpcAddr string | |||
There was a problem hiding this comment.
Could you help to add more illustration for all these fields in clientOpts.
There was a problem hiding this comment.
this part will be updated in the following pr because the ctrd will be updated. thanks
ctrd/supervisord/option.go
Outdated
| func WithOOMScore(score int) Opt { | ||
| return func(d *Daemon) error { | ||
| if score > 1000 || score < -1000 { | ||
| return fmt.Errorf("oom-score range should be (-1000, 1000)") |
There was a problem hiding this comment.
should it be [-1000, 1000], rather than (-1000,1000)?
daemon/daemon.go
Outdated
| @@ -31,7 +33,8 @@ import ( | |||
| type Daemon struct { | |||
| config *config.Config | |||
| containerStore *meta.Store | |||
| containerd ctrd.APIClient | |||
| ctrdDaemon *supervisord.Daemon | |||
There was a problem hiding this comment.
Could you add more details about the fields here?
|
|
||
| if err := d.containerd.Cleanup(); err != nil { | ||
| if err := d.ctrdDaemon.Stop(); err != nil { |
There was a problem hiding this comment.
If d.ctrdClient.Cleanup() returns non-nil as well, here we will overwrite the errMsg. I think this has some potential possibility to miss information. How about using string append way to still keep the message? @fuweid
There was a problem hiding this comment.
Yes. There is some issues about handing error. basically, I will update this part in following PR, not in this pr.
1. make ctrd client package simple 2. the default config of containerdV1.2 is different from V1.0.3. The supervisord package can help us to use configuration file to setup containerd. It's easy to maintain. Signed-off-by: Wei Fu <[email protected]>
|
LGTM and will merge this until the CI passes. |
Signed-off-by: Wei Fu [email protected]
Ⅰ. Describe what this PR did
prepare for containerdV1.2
supervisord package can help us to use configuration file to setup
containerd. It's easy to maintain.
Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
No need
Ⅳ. Describe how to verify it
When pouchd starts, it should connect to containerd.
When pouchd exits, it should shutdown the containerd.
Ⅴ. Special notes for reviews