Merge pull request #66506 from verb/remove-docker-pid-sharing

Automatic merge from submit-queue (batch tested with PRs 62423, 66180, 66492, 66506, 65242). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove kubelet docker shared pid flag

**What this PR does / why we need it**:
The --docker-disable-shared-pid flag has been deprecated since 1.10 and
has been superceded by ShareProcessNamespace in the pod API, which is
scheduled for beta in 1.12.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #41938

**Special notes for your reviewer**:
/assign @yujuhong 

**Release note**:

```release-note
The --docker-disable-shared-pid kubelet flag has been removed. PID namespace sharing can instead be enable per-pod using the ShareProcessNamespace option.
```

cmd/kubelet/app/options/container_runtime.go

@@ -49,7 +49,6 @@ func NewContainerRuntimeOptions() *config.ContainerRuntimeOptions {
 		RedirectContainerStreaming: false,
 		DockerEndpoint:             dockerEndpoint,
 		DockershimRootDirectory:    "/var/lib/dockershim",
-		DockerDisableSharedPID:     true,
 		PodSandboxImage:            defaultPodSandboxImage,
 		ImagePullProgressDeadline:  metav1.Duration{Duration: 1 * time.Minute},
 		ExperimentalDockershim:     false,

cmd/kubelet/app/server.go

@@ -1161,7 +1161,7 @@ func RunDockershim(f *options.KubeletFlags, c *kubeletconfiginternal.KubeletConf
 
 	// Standalone dockershim will always start the local streaming server.
 	ds, err := dockershim.NewDockerService(dockerClientConfig, r.PodSandboxImage, streamingConfig, &pluginSettings,
-		f.RuntimeCgroups, c.CgroupDriver, r.DockershimRootDirectory, r.DockerDisableSharedPID, true /*startLocalStreamingServer*/)
+		f.RuntimeCgroups, c.CgroupDriver, r.DockershimRootDirectory, true /*startLocalStreamingServer*/)
 	if err != nil {
 		return err
 	}

pkg/kubelet/config/flags.go

@@ -48,11 +48,6 @@ type ContainerRuntimeOptions struct {
 	DockershimRootDirectory string
 	// Enable dockershim only mode.
 	ExperimentalDockershim bool
-	// This flag, if set, disables use of a shared PID namespace for pods running in the docker CRI runtime.
-	// A shared PID namespace is the only option in non-docker runtimes and is required by the CRI. The ability to
-	// disable it for docker will be removed unless a compelling use case is discovered with widespread use.
-	// TODO: Remove once we no longer support disabling shared PID namespace (https://issues.k8s.io/41938)
-	DockerDisableSharedPID bool
 	// PodSandboxImage is the image whose network/ipc namespaces
 	// containers in each pod will use.
 	PodSandboxImage string
@@ -93,8 +88,6 @@ func (s *ContainerRuntimeOptions) AddFlags(fs *pflag.FlagSet) {
 	fs.MarkHidden("experimental-dockershim")
 	fs.StringVar(&s.DockershimRootDirectory, "experimental-dockershim-root-directory", s.DockershimRootDirectory, "Path to the dockershim root directory.")
 	fs.MarkHidden("experimental-dockershim-root-directory")
-	fs.BoolVar(&s.DockerDisableSharedPID, "docker-disable-shared-pid", s.DockerDisableSharedPID, fmt.Sprintf("Setting this to false causes Kubernetes to create pods using a shared process namespace for containers in a pod when running with Docker 1.13.1 or higher. A future Kubernetes release will make this configurable instead in the API. %s", dockerOnlyWarning))
-	fs.MarkDeprecated("docker-disable-shared-pid", "will be removed in a future release. This option will be replaced by PID namespace sharing that is configurable per-pod using the API. See https://features.k8s.io/495")
 	fs.StringVar(&s.PodSandboxImage, "pod-infra-container-image", s.PodSandboxImage, fmt.Sprintf("The image whose network/ipc namespaces containers in each pod will use. %s", dockerOnlyWarning))
 	fs.StringVar(&s.DockerEndpoint, "docker-endpoint", s.DockerEndpoint, fmt.Sprintf("Use this for the docker endpoint to communicate with. %s", dockerOnlyWarning))
 	fs.DurationVar(&s.ImagePullProgressDeadline.Duration, "image-pull-progress-deadline", s.ImagePullProgressDeadline.Duration, fmt.Sprintf("If no pulling progress is made before this deadline, the image pulling will be cancelled. %s", dockerOnlyWarning))

pkg/kubelet/dockershim/docker_service.go

@@ -187,9 +187,8 @@ func NewDockerClientFromConfig(config *ClientConfig) libdocker.Interface {
 }
 
 // NOTE: Anything passed to DockerService should be eventually handled in another way when we switch to running the shim as a different process.
-func NewDockerService(config *ClientConfig, podSandboxImage string, streamingConfig *streaming.Config,
+func NewDockerService(config *ClientConfig, podSandboxImage string, streamingConfig *streaming.Config, pluginSettings *NetworkPluginSettings,
-	pluginSettings *NetworkPluginSettings, cgroupsName string, kubeCgroupDriver string, dockershimRootDir string,
+	cgroupsName string, kubeCgroupDriver string, dockershimRootDir string, startLocalStreamingServer bool) (DockerService, error) {
-	disableSharedPID, startLocalStreamingServer bool) (DockerService, error) {
 
 	client := NewDockerClientFromConfig(config)
 
@@ -210,7 +209,6 @@ func NewDockerService(config *ClientConfig, podSandboxImage string, streamingCon
 		},
 		containerManager:          cm.NewContainerManager(cgroupsName, client),
 		checkpointManager:         checkpointManager,
-		disableSharedPID:          disableSharedPID,
 		startLocalStreamingServer: startLocalStreamingServer,
 		networkReady:              make(map[string]bool),
 	}
@@ -304,11 +302,6 @@ type dockerService struct {
 	// version checking for some operations. Use this cache to avoid querying
 	// the docker daemon every time we need to do such checks.
 	versionCache *cache.ObjectCache
-	// This option provides an escape hatch to override the new default behavior for Docker under
-	// the CRI to use a shared PID namespace for all pods. It is temporary and will be removed.
-	// See proposals/pod-pid-namespace.md for details.
-	// TODO: Remove once the escape hatch is no longer used (https://issues.k8s.io/41938)
-	disableSharedPID bool
 	// startLocalStreamingServer indicates whether dockershim should start a
 	// streaming server on localhost.
 	startLocalStreamingServer bool

pkg/kubelet/dockershim/helpers_linux.go

@@ -120,7 +120,7 @@ func (ds *dockerService) updateCreateConfig(
 		if err := applyContainerSecurityContext(lc, podSandboxID, createConfig.Config, createConfig.HostConfig, securityOptSep); err != nil {
 			return fmt.Errorf("failed to apply container security context for container %q: %v", config.Metadata.Name, err)
 		}
-		modifyContainerPIDNamespaceOverrides(ds.disableSharedPID, apiVersion, createConfig.HostConfig, podSandboxID)
+		modifyContainerPIDNamespaceOverrides(apiVersion, createConfig.HostConfig, podSandboxID)
 	}
 
 	// Apply cgroupsParent derived from the sandbox config.

pkg/kubelet/dockershim/security_context.go

@@ -200,20 +200,13 @@ func modifyHostOptionsForContainer(nsOpts *runtimeapi.NamespaceOption, podSandbo
 	}
 }
 
-// modifyPIDNamespaceOverrides implements two temporary overrides for the default PID namespace sharing for Docker:
+// modifyPIDNamespaceOverrides implements a temporary override for the default PID namespace sharing for Docker:
 //     1. Docker engine prior to API Version 1.24 doesn't support attaching to another container's
 //        PID namespace, and it didn't stabilize until 1.26. This check can be removed when Kubernetes'
 //        minimum Docker version is at least 1.13.1 (API version 1.26).
-//     2. The administrator can override the API behavior by using the deprecated --docker-disable-shared-pid=false
-//        flag. Until this flag is removed, this causes pods to use NamespaceMode_POD instead of
-//        NamespaceMode_CONTAINER regardless of pod configuration.
 // TODO(verb): remove entirely once these two conditions are satisfied
-func modifyContainerPIDNamespaceOverrides(disableSharedPID bool, version *semver.Version, hc *dockercontainer.HostConfig, podSandboxID string) {
+func modifyContainerPIDNamespaceOverrides(version *semver.Version, hc *dockercontainer.HostConfig, podSandboxID string) {
-	if version.LT(semver.Version{Major: 1, Minor: 26}) {
+	if version.LT(semver.Version{Major: 1, Minor: 26}) && strings.HasPrefix(string(hc.PidMode), "container:") {
-		if strings.HasPrefix(string(hc.PidMode), "container:") {
+		hc.PidMode = ""
-			hc.PidMode = ""
-		}
-	} else if !disableSharedPID && hc.PidMode == "" {
-		hc.PidMode = dockercontainer.PidMode(fmt.Sprintf("container:%v", podSandboxID))
 	}
 }

pkg/kubelet/dockershim/security_context_test.go

@@ -362,90 +362,41 @@ func TestModifyContainerNamespaceOptions(t *testing.T) {
 func TestModifyContainerNamespacePIDOverride(t *testing.T) {
 	cases := []struct {
 		name            string
-		disable         bool
 		version         *semver.Version
 		input, expected dockercontainer.PidMode
 	}{
 		{
-			name:     "mode:CONTAINER docker:NEW flag:UNSET",
+			name:     "mode:CONTAINER docker:NEW",
-			disable:  true,
 			version:  &semver.Version{Major: 1, Minor: 26},
 			input:    "",
 			expected: "",
 		},
 		{
-			name:     "mode:CONTAINER docker:NEW flag:SET",
+			name:     "mode:CONTAINER docker:OLD",
-			disable:  false,
-			version:  &semver.Version{Major: 1, Minor: 26},
-			input:    "",
-			expected: "container:sandbox",
-		},
-		{
-			name:     "mode:CONTAINER docker:OLD flag:UNSET",
-			disable:  true,
 			version:  &semver.Version{Major: 1, Minor: 25},
 			input:    "",
 			expected: "",
 		},
 		{
-			name:     "mode:CONTAINER docker:OLD flag:SET",
+			name:     "mode:HOST docker:NEW",
-			disable:  false,
-			version:  &semver.Version{Major: 1, Minor: 25},
-			input:    "",
-			expected: "",
-		},
-		{
-			name:     "mode:HOST docker:NEW flag:UNSET",
-			disable:  true,
 			version:  &semver.Version{Major: 1, Minor: 26},
 			input:    "host",
 			expected: "host",
 		},
 		{
-			name:     "mode:HOST docker:NEW flag:SET",
+			name:     "mode:HOST docker:OLD",
-			disable:  false,
-			version:  &semver.Version{Major: 1, Minor: 26},
-			input:    "host",
-			expected: "host",
-		},
-		{
-			name:     "mode:HOST docker:OLD flag:UNSET",
-			disable:  true,
 			version:  &semver.Version{Major: 1, Minor: 25},
 			input:    "host",
 			expected: "host",
 		},
 		{
-			name:     "mode:HOST docker:OLD flag:SET",
+			name:     "mode:POD docker:NEW",
-			disable:  false,
-			version:  &semver.Version{Major: 1, Minor: 25},
-			input:    "host",
-			expected: "host",
-		},
-		{
-			name:     "mode:POD docker:NEW flag:UNSET",
-			disable:  true,
 			version:  &semver.Version{Major: 1, Minor: 26},
 			input:    "container:sandbox",
 			expected: "container:sandbox",
 		},
 		{
-			name:     "mode:POD docker:NEW flag:SET",
+			name:     "mode:POD docker:OLD",
-			disable:  false,
-			version:  &semver.Version{Major: 1, Minor: 26},
-			input:    "container:sandbox",
-			expected: "container:sandbox",
-		},
-		{
-			name:     "mode:POD docker:OLD flag:UNSET",
-			disable:  true,
-			version:  &semver.Version{Major: 1, Minor: 25},
-			input:    "container:sandbox",
-			expected: "",
-		},
-		{
-			name:     "mode:POD docker:OLD flag:SET",
-			disable:  false,
 			version:  &semver.Version{Major: 1, Minor: 25},
 			input:    "container:sandbox",
 			expected: "",
@@ -453,7 +404,7 @@ func TestModifyContainerNamespacePIDOverride(t *testing.T) {
 	}
 	for _, tc := range cases {
 		dockerCfg := &dockercontainer.HostConfig{PidMode: tc.input}
-		modifyContainerPIDNamespaceOverrides(tc.disable, tc.version, dockerCfg, "sandbox")
+		modifyContainerPIDNamespaceOverrides(tc.version, dockerCfg, "sandbox")
 		assert.Equal(t, tc.expected, dockerCfg.PidMode, "[Test case %q]", tc.name)
 	}
 }

pkg/kubelet/kubelet.go

@@ -602,8 +602,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration,
 		// Create and start the CRI shim running as a grpc server.
 		streamingConfig := getStreamingConfig(kubeCfg, kubeDeps, crOptions)
 		ds, err := dockershim.NewDockerService(kubeDeps.DockerClientConfig, crOptions.PodSandboxImage, streamingConfig,
-			&pluginSettings, runtimeCgroups, kubeCfg.CgroupDriver, crOptions.DockershimRootDirectory,
+			&pluginSettings, runtimeCgroups, kubeCfg.CgroupDriver, crOptions.DockershimRootDirectory, !crOptions.RedirectContainerStreaming)
-			crOptions.DockerDisableSharedPID, !crOptions.RedirectContainerStreaming)
 		if err != nil {
 			return nil, err
 		}