add support logs for namespace k8s.io #1847

zhaojizhuang · 2023-01-13T11:10:34Z

Fixes: #1844
Support logs in namespace k8s.io

zhaojizhuang · 2023-01-13T11:10:58Z

ping @AkihiroSuda

pkg/logging/text_logger.go

go.mod

cmd/nerdctl/logs.go

pkg/logging/log_viewer.go

zhaojizhuang · 2023-01-16T06:43:47Z

Can we drop this dependency by copying containerd/pkg/cri/store/container/metadata.go?

one question left, yes we can drop this dependency by copying containerd/pkg/cri/store/container/metadata.go,
but copyping these codes, may cause more upgrade problems, compared with referencing through go mod.

What's your idea？@AkihiroSuda

AkihiroSuda · 2023-01-16T08:00:53Z

Can we drop this dependency by copying containerd/pkg/cri/store/container/metadata.go?

one question left, yes we can drop this dependency by copying containerd/pkg/cri/store/container/metadata.go, but copyping these codes, may cause more upgrade problems, compared with referencing through go mod.

What's your idea？@AkihiroSuda

Maybe we should just refactor containerd/pkg/cri/store/container to minimize its dependencies, and then we can avoid copypasta

zhaojizhuang · 2023-01-16T11:27:48Z

Can we drop this dependency by copying containerd/pkg/cri/store/container/metadata.go?

one question left, yes we can drop this dependency by copying containerd/pkg/cri/store/container/metadata.go, but copyping these codes, may cause more upgrade problems, compared with referencing through go mod.
What's your idea？@AkihiroSuda

Maybe we should just refactor containerd/pkg/cri/store/container to minimize its dependencies, and then we can avoid copypasta

agree, so, lets create a another issue to refactor containerd/pkg/cri/store/container ？
and any other issue about this PR？

pkg/logging/log_viewer.go

pkg/logging/text_logger.go

pkg/logging/cri_logger.go

pkg/logging/log_viewer.go

AkihiroSuda · 2023-01-17T05:00:59Z

cmd/nerdctl/logs.go

+		return "", fmt.Errorf("get extensions for container %s,failed: %#v", container.ID(), err)
+	}
+	metaData := extensions[k8slabels.ContainerMetadataExtension]
+	var meta containerstore.Metadata


Probably we just need this to avoid importing the CRI deps

type CRIContainerVersionedMetadata struct { Version string Metadata CRIContainerMetadata } type CRIContainerMetadata struct { LogPath string }

looks like a good idea

AkihiroSuda · 2023-01-17T07:24:33Z

pkg/api/types/containerd_types.go

+type metadataInternal Metadata
+
+// Metadata is the unversioned container metadata.
+type Metadata struct {


Suggested change

type Metadata struct {

type CRIContainerMetadata struct {

Or split this to a new package like pkg/api/types/critypes

create a new package "github.com/containerd/nerdctl/pkg/api/types/cri"

pkg/api/types/containerd_types.go

AkihiroSuda · 2023-01-17T07:26:03Z

pkg/api/types/containerd_types.go

@@ -0,0 +1,76 @@
+/*


containerd_types.go

cri_types.go might be more descriptive

AkihiroSuda

Please make sure to make struct names descriptive
https://github.com/containerd/nerdctl/pull/1847/files#r1071829430

LGTM other than that, thanks!

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

AkihiroSuda

Thanks

cmd/nerdctl/logs.go

AkihiroSuda reviewed Jan 13, 2023

View reviewed changes

pkg/logging/text_logger.go Show resolved Hide resolved

AkihiroSuda reviewed Jan 13, 2023

View reviewed changes

pkg/logging/text_logger.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jan 13, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

zhaojizhuang force-pushed the lognerdctl branch from f29a2f2 to d3efcf5 Compare January 13, 2023 11:57

AkihiroSuda added the area/logging label Jan 14, 2023

fahedouch reviewed Jan 15, 2023

View reviewed changes

cmd/nerdctl/logs.go Outdated Show resolved Hide resolved

pkg/logging/log_viewer.go Outdated Show resolved Hide resolved

zhaojizhuang force-pushed the lognerdctl branch 2 times, most recently from 4ff631c to fdeec39 Compare January 16, 2023 06:38

zhaojizhuang force-pushed the lognerdctl branch from fdeec39 to d81f916 Compare January 16, 2023 06:45

AkihiroSuda reviewed Jan 16, 2023

View reviewed changes

pkg/logging/log_viewer.go Show resolved Hide resolved

AkihiroSuda reviewed Jan 16, 2023

View reviewed changes

pkg/logging/text_logger.go Outdated Show resolved Hide resolved

zhaojizhuang force-pushed the lognerdctl branch 2 times, most recently from 64cd106 to 4f803f5 Compare January 16, 2023 11:48

fahedouch reviewed Jan 16, 2023

View reviewed changes

pkg/logging/cri_logger.go Outdated Show resolved Hide resolved

pkg/logging/log_viewer.go Outdated Show resolved Hide resolved

pkg/logging/log_viewer.go Outdated Show resolved Hide resolved

zhaojizhuang force-pushed the lognerdctl branch 4 times, most recently from c8c8a03 to 837a68a Compare January 17, 2023 03:18

AkihiroSuda reviewed Jan 17, 2023

View reviewed changes

AkihiroSuda added the kind/feature label Jan 17, 2023

zhaojizhuang force-pushed the lognerdctl branch from 837a68a to 7f793cd Compare January 17, 2023 06:36

AkihiroSuda added this to the v1.2.0 milestone Jan 17, 2023

AkihiroSuda reviewed Jan 17, 2023

View reviewed changes

pkg/api/types/containerd_types.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jan 17, 2023

View reviewed changes

pkg/api/types/containerd_types.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jan 17, 2023

View reviewed changes

AkihiroSuda requested changes Jan 17, 2023

View reviewed changes

zhaojizhuang force-pushed the lognerdctl branch 2 times, most recently from 646c770 to 4684a3c Compare January 17, 2023 09:29

add support logs for namespace k8s.io

ad635da

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

zhaojizhuang force-pushed the lognerdctl branch from 4684a3c to ad635da Compare January 17, 2023 09:34

AkihiroSuda approved these changes Jan 17, 2023

View reviewed changes

fahedouch reviewed Jan 17, 2023

View reviewed changes

cmd/nerdctl/logs.go Show resolved Hide resolved

fahedouch approved these changes Jan 18, 2023

View reviewed changes

AkihiroSuda merged commit 744ab88 into containerd:main Jan 19, 2023

AkihiroSuda added the area/kubernetes label Jan 30, 2023

This was referenced Jan 30, 2023

CI: test the CRI log viewer #1957

Open

Mark cri log viewer as experimental #1958

Merged

add support logs for namespace k8s.io #1847

add support logs for namespace k8s.io #1847

Uh oh!

Conversation

zhaojizhuang commented Jan 13, 2023

Uh oh!

zhaojizhuang commented Jan 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhaojizhuang commented Jan 16, 2023

Uh oh!

AkihiroSuda commented Jan 16, 2023

Uh oh!

zhaojizhuang commented Jan 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants