align docker and podman implementations

container/docker/factory.go

@@ -25,9 +25,12 @@ import (
 
 	"github.com/blang/semver/v4"
 	dockersystem "github.com/docker/docker/api/types/system"
-	"github.com/google/cadvisor/container/containerd"
+	dclient "github.com/docker/docker/client"
+	"golang.org/x/net/context"
+	"k8s.io/klog/v2"
 
 	"github.com/google/cadvisor/container"
+	"github.com/google/cadvisor/container/containerd"
 	dockerutil "github.com/google/cadvisor/container/docker/utils"
 	"github.com/google/cadvisor/container/libcontainer"
 	"github.com/google/cadvisor/devicemapper"
@@ -36,10 +39,6 @@ import (
 	"github.com/google/cadvisor/machine"
 	"github.com/google/cadvisor/watcher"
 	"github.com/google/cadvisor/zfs"
-
-	docker "github.com/docker/docker/client"
-	"golang.org/x/net/context"
-	"k8s.io/klog/v2"
 )
 
 var ArgDockerEndpoint = flag.String("docker", "unix:///var/run/docker.sock", "docker endpoint")
@@ -110,7 +109,7 @@ type dockerFactory struct {
 	storageDriver StorageDriver
 	storageDir    string
 
-	client           *docker.Client
+	client           *dclient.Client
 	containerdClient containerd.ContainerdClient
 
 	// Information about the mounted cgroup subsystems.
@@ -148,7 +147,7 @@ func (f *dockerFactory) NewContainerHandler(name string, metadataEnvAllowList []
 		dockerMetadataEnvAllowList = metadataEnvAllowList
 	}
 
-	handler, err = newDockerContainerHandler(
+	handler, err = newContainerHandler(
 		client,
 		f.containerdClient,
 		name,

container/docker/handler.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Handler for Docker containers.
+// Package docker implements a handler for Docker containers.
 package docker
 
 import (
@@ -24,6 +24,11 @@ import (
 	"strings"
 	"time"
 
+	dclient "github.com/docker/docker/client"
+	"github.com/opencontainers/cgroups"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"golang.org/x/net/context"
+
 	"github.com/google/cadvisor/container"
 	"github.com/google/cadvisor/container/common"
 	"github.com/google/cadvisor/container/containerd"
@@ -34,11 +39,6 @@ import (
 	"github.com/google/cadvisor/fs"
 	info "github.com/google/cadvisor/info/v1"
 	"github.com/google/cadvisor/zfs"
-	"github.com/opencontainers/cgroups"
-	"github.com/opencontainers/runtime-spec/specs-go"
-
-	docker "github.com/docker/docker/client"
-	"golang.org/x/net/context"
 )
 
 const (
@@ -51,7 +51,7 @@ const (
 	pathToContainersDir = "containers"
 )
 
-type dockerContainerHandler struct {
+type containerHandler struct {
 	// machineInfoFactory provides info.MachineInfo
 	machineInfoFactory info.MachineInfoFactory
 
@@ -80,10 +80,10 @@ type dockerContainerHandler struct {
 	// The IP address of the container
 	ipAddress string
 
-	includedMetrics container.MetricSet
+	metrics container.MetricSet
 
 	// the devicemapper poolname
-	poolName string
+	thinPoolName string
 
 	// zfsParent is the parent for docker zfs
 	zfsParent string
@@ -94,10 +94,10 @@ type dockerContainerHandler struct {
 	libcontainerHandler *containerlibcontainer.Handler
 
 	// the docker client is needed to inspect the container and get the health status
-	client docker.APIClient
+	client dclient.APIClient
 }
 
-var _ container.ContainerHandler = &dockerContainerHandler{}
+var _ container.ContainerHandler = &containerHandler{}
 
 func getRwLayerID(containerID, storageDir string, sd StorageDriver, dockerVersion []int) (string, error) {
 	const (
@@ -116,9 +116,9 @@ func getRwLayerID(containerID, storageDir string, sd StorageDriver, dockerVersio
 	return string(bytes), err
 }
 
-// newDockerContainerHandler returns a new container.ContainerHandler
-func newDockerContainerHandler(
-	client *docker.Client,
+// newContainerHandler returns a new container.ContainerHandler
+func newContainerHandler(
+	client *dclient.Client,
 	containerdClient containerd.ContainerdClient,
 	name string,
 	machineInfoFactory info.MachineInfoFactory,
@@ -129,7 +129,7 @@ func newDockerContainerHandler(
 	inHostNamespace bool,
 	metadataEnvAllowList []string,
 	dockerVersion []int,
-	includedMetrics container.MetricSet,
+	metrics container.MetricSet,
 	thinPoolName string,
 	thinPoolWatcher *devicemapper.ThinPoolWatcher,
 	zfsWatcher *zfs.ZfsWatcher,
@@ -189,39 +189,37 @@ func newDockerContainerHandler(
 	}
 
 	// Do not report network metrics for containers that share netns with another container.
-	metrics := common.RemoveNetMetrics(includedMetrics, ctnr.HostConfig.NetworkMode.IsContainer())
+	includedMetrics := common.RemoveNetMetrics(metrics, ctnr.HostConfig.NetworkMode.IsContainer())
 
-	// TODO: extract object mother method
-	handler := &dockerContainerHandler{
+	handler := &containerHandler{
 		machineInfoFactory: machineInfoFactory,
 		cgroupPaths:        cgroupPaths,
-		fsInfo:             fsInfo,
 		storageDriver:      storageDriver,
-		poolName:           thinPoolName,
+		fsInfo:             fsInfo,
 		rootfsStorageDir:   rootfsStorageDir,
 		envs:               make(map[string]string),
 		labels:             ctnr.Config.Labels,
-		includedMetrics:    metrics,
+		image:              ctnr.Config.Image,
+		metrics:            includedMetrics,
+		thinPoolName:       thinPoolName,
 		zfsParent:          zfsParent,
 		client:             client,
+		reference: info.ContainerReference{
+			// Add the name and bare ID as aliases of the container.
+			Id:        id,
+			Name:      name,
+			Aliases:   []string{strings.TrimPrefix(ctnr.Name, "/"), id},
+			Namespace: DockerNamespace,
+		},
+		libcontainerHandler: containerlibcontainer.NewHandler(cgroupManager, rootFs, ctnr.State.Pid, metrics),
 	}
+
 	// Timestamp returned by Docker is in time.RFC3339Nano format.
 	handler.creationTime, err = time.Parse(time.RFC3339Nano, ctnr.Created)
 	if err != nil {
-		// This should not happen, report the error just in case
 		return nil, fmt.Errorf("failed to parse the create timestamp %q for container %q: %v", ctnr.Created, id, err)
 	}
-	handler.libcontainerHandler = containerlibcontainer.NewHandler(cgroupManager, rootFs, ctnr.State.Pid, metrics)
-
-	// Add the name and bare ID as aliases of the container.
-	handler.reference = info.ContainerReference{
-		Id:        id,
-		Name:      name,
-		Aliases:   []string{strings.TrimPrefix(ctnr.Name, "/"), id},
-		Namespace: DockerNamespace,
-	}
-	handler.image = ctnr.Config.Image
-	// Only adds restartcount label if it's greater than 0
+
 	if ctnr.RestartCount > 0 {
 		handler.labels["restartcount"] = strconv.Itoa(ctnr.RestartCount)
 	}
@@ -235,11 +233,10 @@ func newDockerContainerHandler(
 		containerID := strings.TrimPrefix(networkMode, "container:")
 		c, err := client.ContainerInspect(context.Background(), containerID)
 		if err != nil {
-			return nil, fmt.Errorf("failed to inspect container %q: %v", id, err)
+			return nil, fmt.Errorf("failed to inspect container %q: %v", containerID, err)
 		}
 		ipAddress = c.NetworkSettings.IPAddress
 	}
-
 	handler.ipAddress = ipAddress
 
 	if includedMetrics.Has(container.DiskUsageMetrics) {
@@ -252,7 +249,7 @@ func newDockerContainerHandler(
 		}
 	}
 
-	// split env vars to get metadata map.
+	// Split env vars to get metadata map.
 	for _, exposedEnv := range metadataEnvAllowList {
 		if exposedEnv == "" {
 			// if no dockerEnvWhitelist provided, len(metadataEnvAllowList) == 1, metadataEnvAllowList[0] == ""
@@ -295,36 +292,27 @@ func DetermineDeviceStorage(storageDriver StorageDriver, storageDir string, rwLa
 	return
 }
 
-func (h *dockerContainerHandler) Start() {
-	if h.fsHandler != nil {
-		h.fsHandler.Start()
-	}
-}
-
-func (h *dockerContainerHandler) Cleanup() {
-	if h.fsHandler != nil {
-		h.fsHandler.Stop()
-	}
-}
-
-func (h *dockerContainerHandler) ContainerReference() (info.ContainerReference, error) {
+func (h *containerHandler) ContainerReference() (info.ContainerReference, error) {
 	return h.reference, nil
 }
 
-func (h *dockerContainerHandler) GetSpec() (info.ContainerSpec, error) {
-	hasFilesystem := h.includedMetrics.Has(container.DiskUsageMetrics)
-	hasNetwork := h.includedMetrics.Has(container.NetworkUsageMetrics)
+func (h *containerHandler) GetSpec() (info.ContainerSpec, error) {
+	hasFilesystem := h.metrics.Has(container.DiskUsageMetrics)
+	hasNetwork := h.metrics.Has(container.NetworkUsageMetrics)
 	spec, err := common.GetSpec(h.cgroupPaths, h.machineInfoFactory, hasNetwork, hasFilesystem)
+	if err != nil {
+		return info.ContainerSpec{}, err
+	}
 
 	spec.Labels = h.labels
 	spec.Envs = h.envs
 	spec.Image = h.image
 	spec.CreationTime = h.creationTime
 
-	return spec, err
+	return spec, nil
 }
 
-func (h *dockerContainerHandler) GetStats() (*info.ContainerStats, error) {
+func (h *containerHandler) GetStats() (*info.ContainerStats, error) {
 	// TODO(vmarmol): Get from libcontainer API instead of cgroup manager when we don't have to support older Dockers.
 	stats, err := h.libcontainerHandler.GetStats()
 	if err != nil {
@@ -342,48 +330,59 @@ func (h *dockerContainerHandler) GetStats() (*info.ContainerStats, error) {
 	}
 
 	// Get filesystem stats.
-	err = FsStats(stats, h.machineInfoFactory, h.includedMetrics, h.storageDriver,
-		h.fsHandler, h.fsInfo, h.poolName, h.rootfsStorageDir, h.zfsParent)
+	err = FsStats(stats, h.machineInfoFactory, h.metrics, h.storageDriver,
+		h.fsHandler, h.fsInfo, h.thinPoolName, h.rootfsStorageDir, h.zfsParent)
 	if err != nil {
 		return stats, err
 	}
 
 	return stats, nil
 }
 
-func (h *dockerContainerHandler) ListContainers(listType container.ListType) ([]info.ContainerReference, error) {
-	// No-op for Docker driver.
+func (h *containerHandler) ListContainers(container.ListType) ([]info.ContainerReference, error) {
 	return []info.ContainerReference{}, nil
 }
 
-func (h *dockerContainerHandler) GetCgroupPath(resource string) (string, error) {
+func (h *containerHandler) ListProcesses(container.ListType) ([]int, error) {
+	return h.libcontainerHandler.GetProcesses()
+}
+
+func (h *containerHandler) GetCgroupPath(resource string) (string, error) {
 	var res string
 	if !cgroups.IsCgroup2UnifiedMode() {
 		res = resource
 	}
-	path, ok := h.cgroupPaths[res]
+	cgroupPath, ok := h.cgroupPaths[res]
 	if !ok {
 		return "", fmt.Errorf("could not find path for resource %q for container %q", resource, h.reference.Name)
 	}
-	return path, nil
+	return cgroupPath, nil
 }
 
-func (h *dockerContainerHandler) GetContainerLabels() map[string]string {
+func (h *containerHandler) GetContainerLabels() map[string]string {
 	return h.labels
 }
 
-func (h *dockerContainerHandler) GetContainerIPAddress() string {
+func (h *containerHandler) GetContainerIPAddress() string {
 	return h.ipAddress
 }
 
-func (h *dockerContainerHandler) ListProcesses(listType container.ListType) ([]int, error) {
-	return h.libcontainerHandler.GetProcesses()
+func (h *containerHandler) Exists() bool {
+	return common.CgroupExists(h.cgroupPaths)
 }
 
-func (h *dockerContainerHandler) Exists() bool {
-	return common.CgroupExists(h.cgroupPaths)
+func (h *containerHandler) Cleanup() {
+	if h.fsHandler != nil {
+		h.fsHandler.Stop()
+	}
+}
+
+func (h *containerHandler) Start() {
+	if h.fsHandler != nil {
+		h.fsHandler.Start()
+	}
 }
 
-func (h *dockerContainerHandler) Type() container.ContainerType {
+func (h *containerHandler) Type() container.ContainerType {
 	return container.ContainerTypeDocker
 }

container/podman/factory.go

@@ -109,7 +109,7 @@ func (f *podmanFactory) String() string {
 }
 
 func (f *podmanFactory) NewContainerHandler(name string, metadataEnvAllowList []string, inHostNamespace bool) (handler container.ContainerHandler, err error) {
-	return newPodmanContainerHandler(name, f.machineInfoFactory, f.fsInfo,
+	return newContainerHandler(name, f.machineInfoFactory, f.fsInfo,
 		f.storageDriver, f.storageDir, f.cgroupSubsystem, inHostNamespace,
 		metadataEnvAllowList, f.metrics, f.thinPoolName, f.thinPoolWatcher, f.zfsWatcher)
 }