feature: implement the feature of reopen log about cri #2447

starnop · 2018-11-07T12:39:56Z

Signed-off-by: Starnop [email protected]

Ⅰ. Describe what this PR did

Thanks for the PR of IO redesign , we could implement the function of ReopenContainerLog.

BTW, delete the skip test about ReopenContainerLog .

Ⅱ. Does this pull request fix one issue?

None.

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

None.

Ⅳ. Describe how to verify it

use critest

critest --runtime-endpoint /var/run/pouchcri.sock --ginkgo.focus="runtime should support reopening container log"

Ⅴ. Special notes for reviews

codecov · 2018-11-07T12:39:58Z

Codecov Report

Merging #2447 into master will decrease coverage by 0.08%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #2447      +/-   ##
==========================================
- Coverage   69.24%   69.15%   -0.09%     
==========================================
  Files         278      278              
  Lines       18528    18552      +24     
==========================================
  Hits        12829    12829              
- Misses       4242     4252      +10     
- Partials     1457     1471      +14

Flag	Coverage Δ
#criv1alpha1test	`31.28% <0%> (-0.03%)`	⬇️
#criv1alpha2test	`35.44% <30.55%> (+0.02%)`	⬆️
#integrationtest	`40.57% <0%> (-0.04%)`	⬇️
#nodee2etest	`32.53% <11.11%> (-0.16%)`	⬇️
#unittest	`26.81% <0%> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
cri/v1alpha2/cri_types.go	`100% <ø> (ø)`	⬆️
daemon/mgr/container.go	`58.77% <0%> (+0.08%)`	⬆️
pkg/errtypes/errors.go	`91.3% <0%> (-8.7%)`	⬇️
cri/v1alpha2/cri.go	`68.34% <37.5%> (-1.6%)`	⬇️
pkg/streams/utils.go	`82.14% <0%> (-9.53%)`	⬇️
ctrd/watch.go	`84.5% <0%> (-2.82%)`	⬇️
pkg/meta/store.go	`67.44% <0%> (-1.56%)`	⬇️
daemon/mgr/container_utils.go	`83.92% <0%> (-1.2%)`	⬇️
... and 4 more

fuweid · 2018-11-07T14:55:49Z

cri/v1alpha2/cri.go

+		return nil, fmt.Errorf("failed to get container %q with error: %v", containerID, err)
+	}
+	if !container.IsRunning() {
+		return nil, fmt.Errorf("container is not running")


I was thinking that CRI should use the right status code for the error. For this one, we should use ErrPreconditionFail something like that. How do you think about this?

Yeah, Agree with that. THX.

fuweid · 2018-11-07T14:56:52Z

cri/v1alpha2/cri.go

@@ -180,6 +183,20 @@ func NewCriManager(config *config.Config, ctrMgr mgr.ContainerMgr, imgMgr mgr.Im
 		return nil, fmt.Errorf("failed to create sandbox meta store: %v", err)
 	}

+	c.ContainerStore, err = meta.NewStore(meta.Config{


could you provide more detail about this design? I think it should be described in the PR.

HusterWan · 2018-11-08T01:43:55Z

cri/v1alpha2/cri.go

 			return nil, fmt.Errorf("failed to remove container %q of sandbox %q: %v", container.ID, podSandboxID, err)
 		}
+
+		if err = c.ContainerStore.Remove(container.ID); err != nil {


weird, we should redesign this feature

Maybe a mapping is better. I will redesign it. THX.

fuweid · 2018-11-20T11:31:43Z

cri/v1alpha2/cri.go

+		ID:              id,
+		Config:          config,
+		Runtime:         config.Annotations[anno.KubernetesRuntime],
+		ContainerLogMap: containerLogMap,


could we use ContainerLogMap: make(map[string]string) to do initialization? since the containerLogMap is only used for assignment.

fuweid · 2018-11-20T11:33:51Z

cri/v1alpha2/cri.go

@@ -758,6 +761,12 @@ func (c *CriManager) CreateContainer(ctx context.Context, r *runtime.CreateConta
 		if err := c.ContainerMgr.AttachCRILog(ctx, containerID, logPath); err != nil {
 			return nil, err
 		}
+
+		sandboxMeta.ContainerLogMap[containerID] = logPath


could we update the data before AttachCRILog? because we might miss the data.

allencloud · 2018-11-25T04:26:26Z

cri/v1alpha2/cri.go

@@ -489,28 +494,26 @@ func (c *CriManager) RemovePodSandbox(ctx context.Context, r *runtime.RemovePodS

 	// Remove all containers in the sandbox.
 	for _, container := range containers {
-		err = c.ContainerMgr.Remove(ctx, container.ID, &apitypes.ContainerRemoveOptions{Volumes: true, Force: true})
-		if err != nil {
+		if err = c.ContainerMgr.Remove(ctx, container.ID, &apitypes.ContainerRemoveOptions{Volumes: true, Force: true}); err != nil {


I suggest that you could use if err := c.ContainerMgr.Remove(); err != nil rather than if err = c.ContainerMgr.Remove(); err != nil. The only difference is that replace = with :=.

Here is my reason:

you should try to control the variable effect localization well. If you use =, then the effect area is very large, sometime it will override the whole context's variables. if := is enough, then we use it, you should only care about the single block's scope. This will reduce much confusions in the code. Since when I read your code, I am still thinking that if the = is on purpose and if the err in err = c.ContainerMgr.Remove() is related to the whole scope of function RemovePodSandbox , which consumes more energy of mine.

@starnop WDYT?

starnop · 2018-11-27T14:37:24Z

All mentioned above have been updated. PTAL. THX.

zhuangqh · 2018-12-11T02:25:23Z

cri/v1alpha2/cri.go

@@ -765,6 +764,11 @@ func (c *CriManager) CreateContainer(ctx context.Context, r *runtime.CreateConta
 	// Get container log.
 	if config.GetLogPath() != "" {
 		logPath := filepath.Join(sandboxConfig.GetLogDirectory(), config.GetLogPath())
+		sandboxMeta.ContainerLogMap[containerID] = logPath


@starnop containerLogMap has not been initialized

THX, I have updated it.

zhuangqh · 2018-12-11T06:56:45Z

cri/v1alpha2/cri.go

+	}
+	logPath, ok := sandboxMeta.ContainerLogMap[containerID]
+	if !ok {
+		logrus.Infof("log path of container: %q is empty", containerID)


Why did you just print a log here. Empty logPath is not acceptable in attachCRILog.
Please also add a precheck at https://github.com/alibaba/pouch/blob/master/daemon/mgr/container.go#L975
if logPath is empty, immediately return error

For cri, it should not return error here when logPath is empty, in addition, we should return here straightly.

Signed-off-by: Starnop <[email protected]>

zhuangqh

also cc @fuweid

fuweid

LGTM

fuweid · 2018-12-11T08:09:47Z

cri/v1alpha2/cri.go

@@ -499,28 +501,26 @@ func (c *CriManager) RemovePodSandbox(ctx context.Context, r *runtime.RemovePodS

 	// Remove all containers in the sandbox.
 	for _, container := range containers {
-		err = c.ContainerMgr.Remove(ctx, container.ID, &apitypes.ContainerRemoveOptions{Volumes: true, Force: true})
-		if err != nil {
+		if err := c.ContainerMgr.Remove(ctx, container.ID, &apitypes.ContainerRemoveOptions{Volumes: true, Force: true}); err != nil {
 			return nil, fmt.Errorf("failed to remove container %q of sandbox %q: %v", container.ID, podSandboxID, err)


I think we can use errors.Wrap here because the Remove might return the 404 or specific error.

We can handle this in next pr. it is not blocked issue.

fuweid · 2018-12-11T08:24:48Z

Two things should be done after this PR:

use errors.Wrap in the gRPC service level because we can use Cause to check the specific error type.
Missing recovery after restart pouch daemon

cc @starnop @zhuangqh

pouchrobot added areas/log kind/feature size/L labels Nov 7, 2018

fuweid reviewed Nov 7, 2018

View reviewed changes

HusterWan reviewed Nov 8, 2018

View reviewed changes

starnop force-pushed the cri-reopen-log branch from 2974c5a to c65a77a Compare November 8, 2018 03:55

pouchrobot added the conflict/needs-rebase label Nov 8, 2018

starnop force-pushed the cri-reopen-log branch from c65a77a to 3be703a Compare November 8, 2018 04:22

pouchrobot added size/M and removed size/L labels Nov 8, 2018

starnop force-pushed the cri-reopen-log branch from 3be703a to 6f0fc8f Compare November 8, 2018 04:28

pouchrobot removed the conflict/needs-rebase label Nov 8, 2018

starnop force-pushed the cri-reopen-log branch from 6f0fc8f to dac6165 Compare November 8, 2018 05:54

pouchrobot added size/L and removed size/M labels Nov 8, 2018

starnop force-pushed the cri-reopen-log branch 2 times, most recently from d0f3510 to 08a101b Compare November 12, 2018 09:05

fuweid reviewed Nov 20, 2018

View reviewed changes

allencloud reviewed Nov 25, 2018

View reviewed changes

starnop force-pushed the cri-reopen-log branch 4 times, most recently from 8b2b31f to d8a23bc Compare November 27, 2018 08:15

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Nov 28, 2018

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Nov 29, 2018

starnop force-pushed the cri-reopen-log branch from d8a23bc to 65af91b Compare December 3, 2018 01:57

pouchrobot added size/M and removed size/L conflict/needs-rebase labels Dec 3, 2018

zhuangqh reviewed Dec 11, 2018

View reviewed changes

starnop force-pushed the cri-reopen-log branch from 65af91b to 87f7ff2 Compare December 11, 2018 05:03

zhuangqh reviewed Dec 11, 2018

View reviewed changes

feature: implement the feature of reopen log about cri

8467cbf

Signed-off-by: Starnop <[email protected]>

starnop force-pushed the cri-reopen-log branch from 87f7ff2 to 8467cbf Compare December 11, 2018 07:21

pouchrobot added size/L and removed size/M labels Dec 11, 2018

zhuangqh approved these changes Dec 11, 2018

View reviewed changes

fuweid approved these changes Dec 11, 2018

View reviewed changes

fuweid merged commit f1d832a into AliyunContainerService:master Dec 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: implement the feature of reopen log about cri #2447

feature: implement the feature of reopen log about cri #2447

starnop commented Nov 7, 2018

codecov bot commented Nov 7, 2018 •

edited

Loading

fuweid Nov 7, 2018

starnop Nov 8, 2018

fuweid Nov 7, 2018

HusterWan Nov 8, 2018 •

edited

Loading

starnop Nov 8, 2018

fuweid Nov 20, 2018

fuweid Nov 20, 2018 •

edited

Loading

allencloud Nov 25, 2018

starnop commented Nov 27, 2018

zhuangqh Dec 11, 2018

starnop Dec 11, 2018

zhuangqh Dec 11, 2018

starnop Dec 11, 2018

zhuangqh left a comment

fuweid left a comment

fuweid Dec 11, 2018

fuweid commented Dec 11, 2018

feature: implement the feature of reopen log about cri #2447

feature: implement the feature of reopen log about cri #2447

Conversation

starnop commented Nov 7, 2018

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Nov 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

HusterWan Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starnop commented Nov 27, 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

zhuangqh left a comment

Choose a reason for hiding this comment

fuweid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid commented Dec 11, 2018

codecov bot commented Nov 7, 2018 •

edited

Loading

HusterWan Nov 8, 2018 •

edited

Loading

fuweid Nov 20, 2018 •

edited

Loading