feature: take over old containerd instance when pouchd restart #1275

HusterWan · 2018-05-07T06:25:09Z

Signed-off-by: Michael Wan zirenwan@gmail.com

Ⅰ. Describe what this PR did

When pouchd restart, if old containerd is alive, we take it over.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

fuweid · 2018-05-07T07:52:45Z

ctrd/client.go

@@ -166,3 +185,125 @@ func (c *Client) Version(ctx context.Context) (containerd.Version, error) {

 	return cli.client.Version(ctx)
 }
+
+func (c *Client) runContainerdDaemon(homeDir string, copts clientOpts) error {
+	if homeDir == "" {


Could we use os.Stat to check homeDir here? If the homeDir doesn't exist, we should return error right away.
WDYT?

the home-dir existence is checked in main.go, so we no need check here.
but we should check the stateDir here, thanks for your reminder, :)

fuweid · 2018-05-07T08:02:44Z

ctrd/client.go

+	}
+	logrus.Infof("ctrd: new containerd process, pid: %d", cmd.Process.Pid)
+
+	if err := utils.SetOOMScore(cmd.Process.Pid, -17); err != nil {


I remember the containerd has the OOM score settings in the configuration. In my opinion, it's better if we maintain configuration for the pouchd. The configuration can help us to locate the problem in the future. How do you think?

Agree, reasonable !!!

codecov-io · 2018-05-07T08:18:40Z

Codecov Report

Merging #1275 into master will decrease coverage by 0.2%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1275      +/-   ##
==========================================
- Coverage   15.52%   15.31%   -0.21%     
==========================================
  Files         173      174       +1     
  Lines       10868    11014     +146     
==========================================
  Hits         1687     1687              
- Misses       9062     9208     +146     
  Partials      119      119

Impacted Files	Coverage Δ
daemon/config/config.go	`7.89% <ø> (ø)`	⬆️
ctrd/client_opts.go	`0% <0%> (ø)`
pkg/utils/utils.go	`69.79% <0%> (-7.24%)`	⬇️
ctrd/client.go	`0% <0%> (ø)`	⬆️
ctrd/wrapper_client.go	`0% <0%> (ø)`	⬆️

fuweid · 2018-05-07T14:33:04Z

ctrd/client.go

+		os.RemoveAll(c.socketPath)
+	}
+
+	// Start a new containerd instance


I think we can use a function to wrap the following functions

allencloud · 2018-05-07T16:24:26Z

daemon/daemon.go

+		errMsg = fmt.Sprintf("%s\n", err.Error())
+	}
+
+	fmt.Println("Start cleanup containerd...")


I am afraid we should use logrus to catch this line of message.
Rather than fmt.Println.

allencloud · 2018-05-07T16:26:30Z

daemon/daemon.go

@@ -216,7 +225,23 @@ func (d *Daemon) Run() error {

 // Shutdown stops daemon.
 func (d *Daemon) Shutdown() error {
-	return d.server.Stop()
+	var errMsg string


I am wondering if the variable errMsg is redundant.
I think with the current implementation, we can just return the returned err, right?

@allencloud i want to execute all stop function even occurred an error when calling one of the stop function

HusterWan · 2018-05-08T02:47:25Z

Updated! PTAL @allencloud @fuweid

allencloud · 2018-05-08T12:39:53Z

I am wondering if we could add an integration test to cover this case? @HusterWan
Also cc @Letty5411 @fuweid for more feedbacks.

We only have api and cli integration test and unit test, for other kinds of test, shall we include them in test code as well?

HusterWan · 2018-05-08T13:45:07Z

Maybe we can add test case in daemon test, here is test steps in my compute:

first start containerd, and get the pid of it.
echo -n pid of containerd to pidfile
check if we can start pouchd normally
kill pouchd and check if old containerd can exited normally

@Letty5411 can we add test case as my described ?

fuweid · 2018-05-09T01:05:22Z

ctrd/client.go

+	// Start a new containerd instance
+	args := []string{
+		"-a", c.rpcAddr,
+		"--root", path.Join(c.homeDir, "containerd/root"),


Sorry. I don't make my suggestion clear in the last comment.

The containerd has own the configuration. The link is here. Maybe we can marshal the settings in the file so that we can use the file to check the information about pouchd.

actually, the containerd core Opts have been in the poucd configuration file, and i think we no need keep much containerd configuration opts in pouchd code, if you want to add more configurations , use the containerd configuration file outside, DYA?

fuweid · 2018-05-09T01:08:40Z

ctrd/client_opts.go

+type clientOpts struct {
+	startDaemon            bool
+	debugLog               bool
+	rpcAddr                string


Should we have default value for the option? I'm wondering that the WithXXX is not necessary for every time. WDYT?

Not call WithXXX will set the Opts with default value.

Hi @HusterWan

// set default value for parameters if copts.rpcAddr == "" { copts.rpcAddr = unixSocketPath } client.rpcAddr = copts.rpcAddr if copts.grpcClientPoolCapacity <= 0 { copts.grpcClientPoolCapacity = defaultGrpcClientPoolCapacity } if copts.maxStreamsClient <= 0 { copts.maxStreamsClient = defaultMaxStreamsClient }

For this part, I means we can use the following code to handle

var defaultClientOpts = clientOpts{ rpcAddr: unixSocketPath, grpcClientPoolCapacity: defaultGrpcClientPoolCapacity, maxStreamsClient: defaultMaxStreamsClient }

With the default value, we don't need to have the if-else code. :)

Hope it can help

I am afraid, i can not agree with you, default value take effect when not specified or invalid params

Signed-off-by: Michael Wan <zirenwan@gmail.com>

fuweid · 2018-05-09T04:57:56Z

LGTM

pouchrobot added kind/feature size/XL labels May 7, 2018

HusterWan force-pushed the zr/containerd-dev branch from 451686b to 58c1400 Compare May 7, 2018 07:12

fuweid reviewed May 7, 2018

View reviewed changes

HusterWan force-pushed the zr/containerd-dev branch from 58c1400 to 662553d Compare May 7, 2018 08:05

HusterWan force-pushed the zr/containerd-dev branch from 662553d to d21aaf3 Compare May 7, 2018 08:56

fuweid reviewed May 7, 2018

View reviewed changes

allencloud reviewed May 7, 2018

View reviewed changes

HusterWan force-pushed the zr/containerd-dev branch from d21aaf3 to cb430da Compare May 8, 2018 02:27

HusterWan force-pushed the zr/containerd-dev branch 3 times, most recently from 9588676 to 002d016 Compare May 8, 2018 08:57

fuweid reviewed May 9, 2018

View reviewed changes

pouchrobot added the conflict/needs-rebase label May 9, 2018

feature: support take over old containerd instance when pouchd restart

7bf5abc

Signed-off-by: Michael Wan <zirenwan@gmail.com>

HusterWan force-pushed the zr/containerd-dev branch from 002d016 to 7bf5abc Compare May 9, 2018 04:23

pouchrobot removed the conflict/needs-rebase label May 9, 2018

allencloud merged commit 86bfa6e into AliyunContainerService:master May 9, 2018

pouchrobot mentioned this pull request May 10, 2018

Weekly Report of Pouch from 2018-05-03 to 2018-05-10 #1297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: take over old containerd instance when pouchd restart #1275

feature: take over old containerd instance when pouchd restart #1275

HusterWan commented May 7, 2018

fuweid May 7, 2018

HusterWan May 7, 2018

fuweid May 7, 2018

HusterWan May 7, 2018

codecov-io commented May 7, 2018 •

edited

Loading

fuweid May 7, 2018

allencloud May 7, 2018

allencloud May 7, 2018

allencloud May 7, 2018

HusterWan May 8, 2018

HusterWan commented May 8, 2018

allencloud commented May 8, 2018

HusterWan commented May 8, 2018

fuweid May 9, 2018

HusterWan May 9, 2018

fuweid May 9, 2018

fuweid May 9, 2018

HusterWan May 9, 2018

fuweid May 9, 2018

HusterWan May 9, 2018

fuweid commented May 9, 2018

feature: take over old containerd instance when pouchd restart #1275

feature: take over old containerd instance when pouchd restart #1275

Conversation

HusterWan commented May 7, 2018

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HusterWan commented May 8, 2018

allencloud commented May 8, 2018

HusterWan commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented May 9, 2018

codecov-io commented May 7, 2018 •

edited

Loading