Merge pull request #46739 from kubernetes/revert-43879-fix-up-runtime-GetNetNS

Revert "kubelet/network: report but tolerate errors returned from GetNetNS()"

pkg/kubelet/dockershim/docker_sandbox.go

@@ -48,26 +48,6 @@ const (
 	runtimeName = "docker"
 )
 
-// Returns whether the sandbox network is ready, and whether the sandbox is known
-func (ds *dockerService) getNetworkReady(podSandboxID string) (bool, bool) {
-	ds.networkReadyLock.Lock()
-	defer ds.networkReadyLock.Unlock()
-	ready, ok := ds.networkReady[podSandboxID]
-	return ready, ok
-}
-
-func (ds *dockerService) setNetworkReady(podSandboxID string, ready bool) {
-	ds.networkReadyLock.Lock()
-	defer ds.networkReadyLock.Unlock()
-	ds.networkReady[podSandboxID] = ready
-}
-
-func (ds *dockerService) clearNetworkReady(podSandboxID string) {
-	ds.networkReadyLock.Lock()
-	defer ds.networkReadyLock.Unlock()
-	delete(ds.networkReady, podSandboxID)
-}
-
 // RunPodSandbox creates and starts a pod-level sandbox. Runtimes should ensure
 // the sandbox is in ready state.
 // For docker, PodSandbox is implemented by a container holding the network
@@ -102,8 +82,6 @@ func (ds *dockerService) RunPodSandbox(config *runtimeapi.PodSandboxConfig) (str
 		return "", fmt.Errorf("failed to create a sandbox for pod %q: %v", config.Metadata.Name, err)
 	}
 
-	ds.setNetworkReady(createResp.ID, false)
-
 	// Step 3: Create Sandbox Checkpoint.
 	if err = ds.checkpointHandler.CreateCheckpoint(createResp.ID, constructPodSandboxCheckpoint(config)); err != nil {
 		return createResp.ID, err
@@ -136,7 +114,6 @@ func (ds *dockerService) RunPodSandbox(config *runtimeapi.PodSandboxConfig) (str
 
 	// Do not invoke network plugins if in hostNetwork mode.
 	if nsOptions := config.GetLinux().GetSecurityContext().GetNamespaceOptions(); nsOptions != nil && nsOptions.HostNetwork {
-		ds.setNetworkReady(createResp.ID, true)
 		return createResp.ID, nil
 	}
 
@@ -149,15 +126,12 @@ func (ds *dockerService) RunPodSandbox(config *runtimeapi.PodSandboxConfig) (str
 	// recognized by the CNI standard yet.
 	cID := kubecontainer.BuildContainerID(runtimeName, createResp.ID)
 	err = ds.network.SetUpPod(config.GetMetadata().Namespace, config.GetMetadata().Name, cID, config.Annotations)
-	if err == nil {
+	if err != nil {
-		ds.setNetworkReady(createResp.ID, true)
-	} else {
 		// TODO(random-liu): Do we need to teardown network here?
 		if err := ds.client.StopContainer(createResp.ID, defaultSandboxGracePeriod); err != nil {
 			glog.Warningf("Failed to stop sandbox container %q for pod %q: %v", createResp.ID, config.Metadata.Name, err)
 		}
 	}
-
 	return createResp.ID, err
 }
 
@@ -225,10 +199,7 @@ func (ds *dockerService) StopPodSandbox(podSandboxID string) error {
 	errList := []error{}
 	if needNetworkTearDown {
 		cID := kubecontainer.BuildContainerID(runtimeName, podSandboxID)
-		err := ds.network.TearDownPod(namespace, name, cID)
+		if err := ds.network.TearDownPod(namespace, name, cID); err != nil {
-		if err == nil {
-			ds.setNetworkReady(podSandboxID, false)
-		} else {
 			errList = append(errList, err)
 		}
 	}
@@ -270,8 +241,6 @@ func (ds *dockerService) RemovePodSandbox(podSandboxID string) error {
 		errs = append(errs, err)
 	}
 
-	ds.clearNetworkReady(podSandboxID)
-
 	// Remove the checkpoint of the sandbox.
 	if err := ds.checkpointHandler.RemoveCheckpoint(podSandboxID); err != nil {
 		errs = append(errs, err)
@@ -289,6 +258,9 @@ func (ds *dockerService) getIPFromPlugin(sandbox *dockertypes.ContainerJSON) (st
 	cID := kubecontainer.BuildContainerID(runtimeName, sandbox.ID)
 	networkStatus, err := ds.network.GetPodNetworkStatus(metadata.Namespace, metadata.Name, cID)
 	if err != nil {
+		// This might be a sandbox that somehow ended up without a default
+		// interface (eth0). We can't distinguish this from a more serious
+		// error, so callers should probably treat it as non-fatal.
 		return "", err
 	}
 	if networkStatus == nil {
@@ -300,7 +272,7 @@ func (ds *dockerService) getIPFromPlugin(sandbox *dockertypes.ContainerJSON) (st
 // getIP returns the ip given the output of `docker inspect` on a pod sandbox,
 // first interrogating any registered plugins, then simply trusting the ip
 // in the sandbox itself. We look for an ipv4 address before ipv6.
-func (ds *dockerService) getIP(podSandboxID string, sandbox *dockertypes.ContainerJSON) (string, error) {
+func (ds *dockerService) getIP(sandbox *dockertypes.ContainerJSON) (string, error) {
 	if sandbox.NetworkSettings == nil {
 		return "", nil
 	}
@@ -309,18 +281,11 @@ func (ds *dockerService) getIP(podSandboxID string, sandbox *dockertypes.Contain
 		// reporting the IP.
 		return "", nil
 	}
-
+	if IP, err := ds.getIPFromPlugin(sandbox); err != nil {
-	// Don't bother getting IP if the pod is known and networking isn't ready
+		glog.Warningf("%v", err)
-	ready, ok := ds.getNetworkReady(podSandboxID)
+	} else if IP != "" {
-	if ok && !ready {
+		return IP, nil
-		return "", nil
 	}
-
-	ip, err := ds.getIPFromPlugin(sandbox)
-	if err == nil {
-		return ip, nil
-	}
-
 	// TODO: trusting the docker ip is not a great idea. However docker uses
 	// eth0 by default and so does CNI, so if we find a docker IP here, we
 	// conclude that the plugin must have failed setup, or forgotten its ip.
@@ -329,11 +294,7 @@ func (ds *dockerService) getIP(podSandboxID string, sandbox *dockertypes.Contain
 	if sandbox.NetworkSettings.IPAddress != "" {
 		return sandbox.NetworkSettings.IPAddress, nil
 	}
-	if sandbox.NetworkSettings.GlobalIPv6Address != "" {
+	return sandbox.NetworkSettings.GlobalIPv6Address, nil
-		return sandbox.NetworkSettings.GlobalIPv6Address, nil
-	}
-
-	return "", fmt.Errorf("failed to read pod IP from plugin/docker: %v", err)
 }
 
 // PodSandboxStatus returns the status of the PodSandbox.
@@ -356,7 +317,7 @@ func (ds *dockerService) PodSandboxStatus(podSandboxID string) (*runtimeapi.PodS
 	if r.State.Running {
 		state = runtimeapi.PodSandboxState_SANDBOX_READY
 	}
-	IP, err := ds.getIP(podSandboxID, r)
+	IP, err := ds.getIP(r)
 	if err != nil {
 		return nil, err
 	}

pkg/kubelet/dockershim/docker_sandbox_test.go

@@ -133,8 +133,6 @@ func TestSandboxStatus(t *testing.T) {
 	expected.State = runtimeapi.PodSandboxState_SANDBOX_NOTREADY
 	err = ds.StopPodSandbox(id)
 	assert.NoError(t, err)
-	// IP not valid after sandbox stop
-	expected.Network.Ip = ""
 	status, err = ds.PodSandboxStatus(id)
 	assert.Equal(t, expected, status)
 
@@ -145,49 +143,6 @@ func TestSandboxStatus(t *testing.T) {
 	assert.Error(t, err, fmt.Sprintf("status of sandbox: %+v", status))
 }
 
-// TestSandboxStatusAfterRestart tests that retrieving sandbox status returns
-// an IP address even if RunPodSandbox() was not yet called for this pod, as
-// would happen on kubelet restart
-func TestSandboxStatusAfterRestart(t *testing.T) {
-	ds, _, fClock := newTestDockerService()
-	config := makeSandboxConfig("foo", "bar", "1", 0)
-
-	// TODO: The following variables depend on the internal
-	// implementation of FakeDockerClient, and should be fixed.
-	fakeIP := "2.3.4.5"
-
-	state := runtimeapi.PodSandboxState_SANDBOX_READY
-	ct := int64(0)
-	hostNetwork := false
-	expected := &runtimeapi.PodSandboxStatus{
-		State:       state,
-		CreatedAt:   ct,
-		Metadata:    config.Metadata,
-		Network:     &runtimeapi.PodSandboxNetworkStatus{Ip: fakeIP},
-		Linux:       &runtimeapi.LinuxPodSandboxStatus{Namespaces: &runtimeapi.Namespace{Options: &runtimeapi.NamespaceOption{HostNetwork: hostNetwork}}},
-		Labels:      map[string]string{},
-		Annotations: map[string]string{},
-	}
-
-	// Create the sandbox.
-	fClock.SetTime(time.Now())
-	expected.CreatedAt = fClock.Now().UnixNano()
-
-	createConfig, err := ds.makeSandboxDockerConfig(config, defaultSandboxImage)
-	assert.NoError(t, err)
-
-	createResp, err := ds.client.CreateContainer(*createConfig)
-	assert.NoError(t, err)
-	err = ds.client.StartContainer(createResp.ID)
-	assert.NoError(t, err)
-
-	// Check status without RunPodSandbox() having set up networking
-	expected.Id = createResp.ID // ID is only known after the creation.
-	status, err := ds.PodSandboxStatus(createResp.ID)
-	assert.NoError(t, err)
-	assert.Equal(t, expected, status)
-}
-
 // TestNetworkPluginInvocation checks that the right SetUpPod and TearDownPod
 // calls are made when we run/stop a sandbox.
 func TestNetworkPluginInvocation(t *testing.T) {

pkg/kubelet/dockershim/docker_service.go

@@ -21,7 +21,6 @@ import (
 	"io"
 	"net/http"
 	"strconv"
-	"sync"
 	"time"
 
 	"github.com/blang/semver"
@@ -176,7 +175,6 @@ func NewDockerService(client libdocker.Interface, seccompProfileRoot string, pod
 		containerManager:  cm.NewContainerManager(cgroupsName, client),
 		checkpointHandler: checkpointHandler,
 		disableSharedPID:  disableSharedPID,
-		networkReady:      make(map[string]bool),
 	}
 
 	// check docker version compatibility.
@@ -250,13 +248,8 @@ type dockerService struct {
 	podSandboxImage    string
 	streamingRuntime   *streamingRuntime
 	streamingServer    streaming.Server
-
+	network            *network.PluginManager
-	network *network.PluginManager
+	containerManager   cm.ContainerManager
-	// Map of podSandboxID :: network-is-ready
-	networkReady     map[string]bool
-	networkReadyLock sync.Mutex
-
-	containerManager cm.ContainerManager
 	// cgroup driver used by Docker runtime.
 	cgroupDriver      string
 	checkpointHandler CheckpointHandler
@@ -322,7 +315,7 @@ func (ds *dockerService) GetNetNS(podSandboxID string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	return getNetworkNamespace(r)
+	return getNetworkNamespace(r), nil
 }
 
 // GetPodPortMappings returns the port mappings of the given podSandbox ID.

pkg/kubelet/dockershim/docker_service_test.go

@@ -46,14 +46,8 @@ func newTestDockerService() (*dockerService, *libdocker.FakeDockerClient, *clock
 	fakeClock := clock.NewFakeClock(time.Time{})
 	c := libdocker.NewFakeDockerClient().WithClock(fakeClock).WithVersion("1.11.2", "1.23")
 	pm := network.NewPluginManager(&network.NoopNetworkPlugin{})
-	return &dockerService{
+	return &dockerService{client: c, os: &containertest.FakeOS{}, network: pm,
-		client:            c,
+		legacyCleanup: legacyCleanupFlag{done: 1}, checkpointHandler: NewTestPersistentCheckpointHandler()}, c, fakeClock
-		os:                &containertest.FakeOS{},
-		network:           pm,
-		legacyCleanup:     legacyCleanupFlag{done: 1},
-		checkpointHandler: NewTestPersistentCheckpointHandler(),
-		networkReady:      make(map[string]bool),
-	}, c, fakeClock
 }
 
 func newTestDockerServiceWithVersionCache() (*dockerService, *libdocker.FakeDockerClient, *clock.FakeClock) {

pkg/kubelet/dockershim/helpers.go

@@ -266,12 +266,14 @@ func getApparmorSecurityOpts(sc *runtimeapi.LinuxContainerSecurityContext, separ
 	return fmtOpts, nil
 }
 
-func getNetworkNamespace(c *dockertypes.ContainerJSON) (string, error) {
+func getNetworkNamespace(c *dockertypes.ContainerJSON) string {
 	if c.State.Pid == 0 {
-		// Docker reports pid 0 for an exited container.
+		// Docker reports pid 0 for an exited container. We can't use it to
-		return "", fmt.Errorf("Cannot find network namespace for the terminated container %q", c.ID)
+		// check the network namespace, so return an empty string instead.
+		glog.V(4).Infof("Cannot find network namespace for the terminated container %q", c.ID)
+		return ""
 	}
-	return fmt.Sprintf(dockerNetNSFmt, c.State.Pid), nil
+	return fmt.Sprintf(dockerNetNSFmt, c.State.Pid)
 }
 
 // dockerFilter wraps around dockerfilters.Args and provides methods to modify

pkg/kubelet/network/cni/cni.go

@@ -251,11 +251,9 @@ func (plugin *cniNetworkPlugin) TearDownPod(namespace string, name string, id ku
 	if err := plugin.checkInitialized(); err != nil {
 		return err
 	}
-
-	// Lack of namespace should not be fatal on teardown
 	netnsPath, err := plugin.host.GetNetNS(id.ID)
 	if err != nil {
-		glog.Warningf("CNI failed to retrieve network namespace path: %v", err)
+		return fmt.Errorf("CNI failed to retrieve network namespace path: %v", err)
 	}
 
 	return plugin.deleteFromNetwork(plugin.getDefaultNetwork(), name, namespace, id, netnsPath)

pkg/kubelet/network/kubenet/kubenet_linux.go

@@ -741,9 +741,9 @@ func podIsExited(p *kubecontainer.Pod) bool {
 	return true
 }
 
-func (plugin *kubenetNetworkPlugin) buildCNIRuntimeConf(ifName string, id kubecontainer.ContainerID, needNetNs bool) (*libcni.RuntimeConf, error) {
+func (plugin *kubenetNetworkPlugin) buildCNIRuntimeConf(ifName string, id kubecontainer.ContainerID) (*libcni.RuntimeConf, error) {
 	netnsPath, err := plugin.host.GetNetNS(id.ID)
-	if needNetNs && err != nil {
+	if err != nil {
 		glog.Errorf("Kubenet failed to retrieve network namespace path: %v", err)
 	}
 
@@ -755,7 +755,7 @@ func (plugin *kubenetNetworkPlugin) buildCNIRuntimeConf(ifName string, id kubeco
 }
 
 func (plugin *kubenetNetworkPlugin) addContainerToNetwork(config *libcni.NetworkConfig, ifName, namespace, name string, id kubecontainer.ContainerID) (cnitypes.Result, error) {
-	rt, err := plugin.buildCNIRuntimeConf(ifName, id, true)
+	rt, err := plugin.buildCNIRuntimeConf(ifName, id)
 	if err != nil {
 		return nil, fmt.Errorf("Error building CNI config: %v", err)
 	}
@@ -769,7 +769,7 @@ func (plugin *kubenetNetworkPlugin) addContainerToNetwork(config *libcni.Network
 }
 
 func (plugin *kubenetNetworkPlugin) delContainerFromNetwork(config *libcni.NetworkConfig, ifName, namespace, name string, id kubecontainer.ContainerID) error {
-	rt, err := plugin.buildCNIRuntimeConf(ifName, id, false)
+	rt, err := plugin.buildCNIRuntimeConf(ifName, id)
 	if err != nil {
 		return fmt.Errorf("Error building CNI config: %v", err)
 	}

pkg/kubelet/network/plugins.go

@@ -144,8 +144,6 @@ type Host interface {
 // CNI plugin wrappers like kubenet.
 type NamespaceGetter interface {
 	// GetNetNS returns network namespace information for the given containerID.
-	// Runtimes should *never* return an empty namespace and nil error for
-	// a container; if error is nil then the namespace string must be valid.
 	GetNetNS(containerID string) (string, error)
 }