From 7fb6c51822e6bfbdd00b4bec55b4ff9c41ac9640 Mon Sep 17 00:00:00 2001 From: mattjmcnaughton Date: Sat, 25 Apr 2020 15:07:17 -0400 Subject: [PATCH] Move DockerLegacyService interface out of `pkg/kubelet/dockershim` DockerLegacyService interface is used throughout `pkg/kubelet`. It used to live in the `pkg/kubelet/dockershim` package. While we would eventually like to remove it entirely, we need to give users some form of warning. By including the interface in `pkg/kubelet/legacy/logs.go`, we ensure the interface is available to `pkg/kubelet`, even when we are building with the `dockerless` tag (i.e. not compiling the dockershim). While the interface always exists, there will be no implementations of the interface when building with the `dockerless` tag. The lack of implementations should not be an issue, as we only expect `pkg/kubelet` code to need an implementation of the `DockerLegacyService` when we are using docker. If we are using docker, but building with the `dockerless` tag, than this will be just one of many things that breaks. `pkg/kubelet/legacy` might not be the best name for the package... I'm very open to finding a different package name or even an already existing package. --- pkg/kubelet/BUILD | 2 + pkg/kubelet/dockershim/BUILD | 1 + .../dockershim/docker_legacy_service.go | 22 +++----- pkg/kubelet/dockershim/docker_service.go | 3 +- pkg/kubelet/kubelet.go | 6 +-- pkg/kubelet/legacy/BUILD | 27 ++++++++++ pkg/kubelet/legacy/logs.go | 53 +++++++++++++++++++ 7 files changed, 95 insertions(+), 19 deletions(-) create mode 100644 pkg/kubelet/legacy/BUILD create mode 100644 pkg/kubelet/legacy/logs.go diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD index e44ceaf7e70..84a6190319e 100644 --- a/pkg/kubelet/BUILD +++ b/pkg/kubelet/BUILD @@ -62,6 +62,7 @@ go_library( "//pkg/kubelet/images:go_default_library", "//pkg/kubelet/kubeletconfig:go_default_library", "//pkg/kubelet/kuberuntime:go_default_library", + "//pkg/kubelet/legacy:go_default_library", "//pkg/kubelet/lifecycle:go_default_library", "//pkg/kubelet/logs:go_default_library", "//pkg/kubelet/metrics:go_default_library", @@ -298,6 +299,7 @@ filegroup( "//pkg/kubelet/kubeletconfig:all-srcs", "//pkg/kubelet/kuberuntime:all-srcs", "//pkg/kubelet/leaky:all-srcs", + "//pkg/kubelet/legacy:all-srcs", "//pkg/kubelet/lifecycle:all-srcs", "//pkg/kubelet/logs:all-srcs", "//pkg/kubelet/metrics:all-srcs", diff --git a/pkg/kubelet/dockershim/BUILD b/pkg/kubelet/dockershim/BUILD index 2ec3bfd14de..e9674de52fa 100644 --- a/pkg/kubelet/dockershim/BUILD +++ b/pkg/kubelet/dockershim/BUILD @@ -51,6 +51,7 @@ go_library( "//pkg/kubelet/dockershim/network/kubenet:go_default_library", "//pkg/kubelet/kuberuntime:go_default_library", "//pkg/kubelet/leaky:go_default_library", + "//pkg/kubelet/legacy:go_default_library", "//pkg/kubelet/server/streaming:go_default_library", "//pkg/kubelet/types:go_default_library", "//pkg/kubelet/util/cache:go_default_library", diff --git a/pkg/kubelet/dockershim/docker_legacy_service.go b/pkg/kubelet/dockershim/docker_legacy_service.go index 5026722303d..770b1dab9c8 100644 --- a/pkg/kubelet/dockershim/docker_legacy_service.go +++ b/pkg/kubelet/dockershim/docker_legacy_service.go @@ -32,25 +32,17 @@ import ( kubetypes "k8s.io/apimachinery/pkg/types" "k8s.io/klog" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" - "k8s.io/kubernetes/pkg/kubelet/kuberuntime" "k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker" ) -// DockerLegacyService interface embeds some legacy methods for backward compatibility. -// This file/interface will be removed in the near future. Do not modify or add -// more functions. -type DockerLegacyService interface { - // GetContainerLogs gets logs for a specific container. - GetContainerLogs(context.Context, *v1.Pod, kubecontainer.ContainerID, *v1.PodLogOptions, io.Writer, io.Writer) error - - // IsCRISupportedLogDriver checks whether the logging driver used by docker is - // supported by native CRI integration. - // TODO(resouer): remove this when deprecating unsupported log driver - IsCRISupportedLogDriver() (bool, error) - - kuberuntime.LegacyLogProvider -} +// We define `DockerLegacyService` in `pkg/kubelet/legacy`, instead of in this +// file. We make this decision because `pkg/kubelet` depends on +// `DockerLegacyService`, and we want to be able to build the `kubelet` without +// relying on `github.com/docker/docker` or `pkg/kubelet/dockershim`. +// +// See https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/20200205-build-kubelet-without-docker.md +// for details. // GetContainerLogs get container logs directly from docker daemon. func (d *dockerService) GetContainerLogs(_ context.Context, pod *v1.Pod, containerID kubecontainer.ContainerID, logOptions *v1.PodLogOptions, stdout, stderr io.Writer) error { diff --git a/pkg/kubelet/dockershim/docker_service.go b/pkg/kubelet/dockershim/docker_service.go index 227bef8211a..4cb8a1f31d9 100644 --- a/pkg/kubelet/dockershim/docker_service.go +++ b/pkg/kubelet/dockershim/docker_service.go @@ -40,6 +40,7 @@ import ( "k8s.io/kubernetes/pkg/kubelet/dockershim/network/cni" "k8s.io/kubernetes/pkg/kubelet/dockershim/network/hostport" "k8s.io/kubernetes/pkg/kubelet/dockershim/network/kubenet" + "k8s.io/kubernetes/pkg/kubelet/legacy" "k8s.io/kubernetes/pkg/kubelet/server/streaming" "k8s.io/kubernetes/pkg/kubelet/util/cache" @@ -97,7 +98,7 @@ type DockerService interface { http.Handler // For supporting legacy features. - DockerLegacyService + legacy.DockerLegacyService } // NetworkPluginSettings is the subset of kubelet runtime args we pass diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 4eeb7964c00..87e06a7a035 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -70,12 +70,12 @@ import ( "k8s.io/kubernetes/pkg/kubelet/config" "k8s.io/kubernetes/pkg/kubelet/configmap" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" - "k8s.io/kubernetes/pkg/kubelet/dockershim" "k8s.io/kubernetes/pkg/kubelet/events" "k8s.io/kubernetes/pkg/kubelet/eviction" "k8s.io/kubernetes/pkg/kubelet/images" "k8s.io/kubernetes/pkg/kubelet/kubeletconfig" "k8s.io/kubernetes/pkg/kubelet/kuberuntime" + "k8s.io/kubernetes/pkg/kubelet/legacy" "k8s.io/kubernetes/pkg/kubelet/lifecycle" "k8s.io/kubernetes/pkg/kubelet/logs" "k8s.io/kubernetes/pkg/kubelet/metrics" @@ -231,7 +231,7 @@ type Dependencies struct { RemoteRuntimeService internalapi.RuntimeService RemoteImageService internalapi.ImageManagerService criHandler http.Handler - dockerLegacyService dockershim.DockerLegacyService + dockerLegacyService legacy.DockerLegacyService // remove it after cadvisor.UsingLegacyCadvisorStats dropped. useLegacyCadvisorStats bool } @@ -1129,7 +1129,7 @@ type Kubelet struct { // dockerLegacyService contains some legacy methods for backward compatibility. // It should be set only when docker is using non json-file logging driver. - dockerLegacyService dockershim.DockerLegacyService + dockerLegacyService legacy.DockerLegacyService // StatsProvider provides the node and the container stats. *stats.StatsProvider diff --git a/pkg/kubelet/legacy/BUILD b/pkg/kubelet/legacy/BUILD new file mode 100644 index 00000000000..c420c57af3c --- /dev/null +++ b/pkg/kubelet/legacy/BUILD @@ -0,0 +1,27 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["logs.go"], + importpath = "k8s.io/kubernetes/pkg/kubelet/legacy", + visibility = ["//visibility:public"], + deps = [ + "//pkg/kubelet/container:go_default_library", + "//pkg/kubelet/kuberuntime:go_default_library", + "//staging/src/k8s.io/api/core/v1:go_default_library", + ], +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], + visibility = ["//visibility:public"], +) diff --git a/pkg/kubelet/legacy/logs.go b/pkg/kubelet/legacy/logs.go new file mode 100644 index 00000000000..d4b938a05c7 --- /dev/null +++ b/pkg/kubelet/legacy/logs.go @@ -0,0 +1,53 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package legacy + +import ( + "context" + "io" + + "k8s.io/api/core/v1" + kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" + "k8s.io/kubernetes/pkg/kubelet/kuberuntime" +) + +// DockerLegacyService interface is used throughout `pkg/kubelet`. +// It used to live in the `pkg/kubelet/dockershim` package. While we +// would eventually like to remove it entirely, we need to give users some form +// of warning. +// +// By including the interface in +// `pkg/kubelet/legacy/logs.go`, we ensure the interface is +// available to `pkg/kubelet`, even when we are building with the `dockerless` +// tag (i.e. not compiling the dockershim). +// While the interface always exists, there will be no implementations of the +// interface when building with the `dockerless` tag. The lack of +// implementations should not be an issue, as we only expect `pkg/kubelet` code +// to need an implementation of the `DockerLegacyService` when we are using +// docker. If we are using docker, but building with the `dockerless` tag, than +// this will be just one of many things that breaks. +type DockerLegacyService interface { + // GetContainerLogs gets logs for a specific container. + GetContainerLogs(context.Context, *v1.Pod, kubecontainer.ContainerID, *v1.PodLogOptions, io.Writer, io.Writer) error + + // IsCRISupportedLogDriver checks whether the logging driver used by docker is + // supported by native CRI integration. + // TODO(resouer): remove this when deprecating unsupported log driver + IsCRISupportedLogDriver() (bool, error) + + kuberuntime.LegacyLogProvider +}