From c39f63eaf49c5ec1161749907f4f81ebb9a796e0 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Tue, 9 Oct 2018 23:13:25 -0700 Subject: [PATCH 1/3] Teardown pod network even if the network namespace is closed Signed-off-by: Lantao Liu --- pkg/server/sandbox_stop.go | 31 ++++++++++++++----------------- pkg/store/sandbox/netns.go | 7 +++++++ 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/pkg/server/sandbox_stop.go b/pkg/server/sandbox_stop.go index 2d5b3ca33..e0f203309 100644 --- a/pkg/server/sandbox_stop.go +++ b/pkg/server/sandbox_stop.go @@ -17,7 +17,6 @@ limitations under the License. package server import ( - "os" "time" "github.com/containerd/containerd" @@ -60,23 +59,21 @@ func (c *criService) StopPodSandbox(ctx context.Context, r *runtime.StopPodSandb } // Teardown network for sandbox. - if sandbox.NetNSPath != "" && sandbox.NetNS != nil { - if _, err := os.Stat(sandbox.NetNSPath); err != nil { - if !os.IsNotExist(err) { - return nil, errors.Wrapf(err, "failed to stat network namespace path %s", sandbox.NetNSPath) - } - } else { - if teardownErr := c.teardownPod(id, sandbox.NetNSPath, sandbox.Config); teardownErr != nil { - return nil, errors.Wrapf(teardownErr, "failed to destroy network for sandbox %q", id) - } + if sandbox.NetNSPath != "" { + netNSPath := sandbox.NetNSPath + if sandbox.NetNS == nil || sandbox.NetNS.Closed() { + // Use empty netns path if netns is not available. This is defined in: + // https://github.com/containernetworking/cni/blob/v0.7.0-alpha1/SPEC.md + netNSPath = "" } - /*TODO:It is still possible that containerd crashes after we teardown the network, but before we remove the network namespace. - In that case, we'll not be able to remove the sandbox anymore. The chance is slim, but we should be aware of that. - In the future, once TearDownPod is idempotent, this will be fixed.*/ - - //Close the sandbox network namespace if it was created - if err = sandbox.NetNS.Remove(); err != nil { - return nil, errors.Wrapf(err, "failed to remove network namespace for sandbox %q", id) + if err := c.teardownPod(id, netNSPath, sandbox.Config); err != nil { + return nil, errors.Wrapf(err, "failed to destroy network for sandbox %q", id) + } + // Close the sandbox network namespace if it was created + if sandbox.NetNS != nil { + if err = sandbox.NetNS.Remove(); err != nil { + return nil, errors.Wrapf(err, "failed to remove network namespace for sandbox %q", id) + } } } diff --git a/pkg/store/sandbox/netns.go b/pkg/store/sandbox/netns.go index a96eb6aad..8a08194cb 100644 --- a/pkg/store/sandbox/netns.go +++ b/pkg/store/sandbox/netns.go @@ -27,6 +27,13 @@ import ( osinterface "github.com/containerd/cri/pkg/os" ) +// The NetNS library assumes only containerd manages the lifecycle of the +// network namespace mount. The only case that netns will be unmounted by +// someone else is node reboot. +// If this assumption is broken, NetNS won't be aware of the external +// unmount, and there will be a state mismatch. +// TODO(random-liu): Don't cache state, always load from the system. + // ErrClosedNetNS is the error returned when network namespace is closed. var ErrClosedNetNS = errors.New("network namespace is closed") From 1f1e92e4a423521f074125aa04829dd12e8b9498 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Tue, 9 Oct 2018 23:13:40 -0700 Subject: [PATCH 2/3] Update go-cni to 40bcf8ec8acd7372be1d77031d585d5d8e561c90. Signed-off-by: Lantao Liu --- vendor.conf | 2 +- vendor/github.com/containerd/go-cni/cni.go | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/vendor.conf b/vendor.conf index 084b27dfd..121470af0 100644 --- a/vendor.conf +++ b/vendor.conf @@ -6,7 +6,7 @@ github.com/containerd/console c12b1e7919c14469339a5d38f2f8ed9b64a9de23 github.com/containerd/containerd 15f19d7a67fa322e6de0ef4c6a1bf9da0f056554 github.com/containerd/continuity bd77b46c8352f74eb12c85bdc01f4b90f69d66b4 github.com/containerd/fifo 3d5202aec260678c48179c56f40e6f38a095738c -github.com/containerd/go-cni 6d7b509a054a3cb1c35ed1865d4fde2f0cb547cd +github.com/containerd/go-cni 40bcf8ec8acd7372be1d77031d585d5d8e561c90 github.com/containerd/go-runc 5a6d9f37cfa36b15efba46dc7ea349fa9b7143c3 github.com/containerd/ttrpc 2a805f71863501300ae1976d29f0454ae003e85a github.com/containerd/typeurl a93fcdb778cd272c6e9b3028b2f42d813e785d40 diff --git a/vendor/github.com/containerd/go-cni/cni.go b/vendor/github.com/containerd/go-cni/cni.go index 3993395fc..31350e304 100644 --- a/vendor/github.com/containerd/go-cni/cni.go +++ b/vendor/github.com/containerd/go-cni/cni.go @@ -18,6 +18,7 @@ package cni import ( "fmt" + "strings" "sync" cnilibrary "github.com/containernetworking/cni/libcni" @@ -127,6 +128,15 @@ func (c *libcni) Remove(id string, path string, opts ...NamespaceOpts) error { } for _, network := range c.networks { if err := network.Remove(ns); err != nil { + // Based on CNI spec v0.7.0, empty network namespace is allowed to + // do best effort cleanup. However, it is not handled consistently + // right now: + // https://github.com/containernetworking/plugins/issues/210 + // TODO(random-liu): Remove the error handling when the issue is + // fixed and the CNI spec v0.6.0 support is deprecated. + if path == "" && strings.Contains(err.Error(), "no such file or directory") { + continue + } return err } } From 84775d2c10ecd751f36866145883b3e3155514f6 Mon Sep 17 00:00:00 2001 From: Lantao Liu Date: Tue, 9 Oct 2018 23:13:48 -0700 Subject: [PATCH 3/3] Add integration test. Signed-off-by: Lantao Liu --- integration/restart_test.go | 24 +---- integration/sandbox_clean_remove_test.go | 106 +++++++++++++++++++++++ integration/test_utils.go | 46 +++++++++- pkg/server/container_status.go | 5 +- pkg/server/sandbox_status.go | 5 +- 5 files changed, 156 insertions(+), 30 deletions(-) diff --git a/integration/restart_test.go b/integration/restart_test.go index 2c9f149ca..676bfa500 100644 --- a/integration/restart_test.go +++ b/integration/restart_test.go @@ -19,7 +19,6 @@ package integration import ( "sort" "testing" - "time" "github.com/containerd/containerd" "github.com/stretchr/testify/assert" @@ -140,27 +139,8 @@ func TestContainerdRestart(t *testing.T) { imagesBeforeRestart, err := imageService.ListImages(nil) assert.NoError(t, err) - t.Logf("Kill containerd") - require.NoError(t, KillProcess("containerd")) - defer func() { - assert.NoError(t, Eventually(func() (bool, error) { - return ConnectDaemons() == nil, nil - }, time.Second, 30*time.Second), "make sure containerd is running before test finish") - }() - - t.Logf("Wait until containerd is killed") - require.NoError(t, Eventually(func() (bool, error) { - pid, err := PidOf("containerd") - if err != nil { - return false, err - } - return pid == 0, nil - }, time.Second, 30*time.Second), "wait for containerd to be killed") - - t.Logf("Wait until containerd is restarted") - require.NoError(t, Eventually(func() (bool, error) { - return ConnectDaemons() == nil, nil - }, time.Second, 30*time.Second), "wait for containerd to be restarted") + t.Logf("Restart containerd") + RestartContainerd(t) t.Logf("Check sandbox and container state after restart") loadedSandboxes, err := runtimeService.ListPodSandbox(&runtime.PodSandboxFilter{}) diff --git a/integration/sandbox_clean_remove_test.go b/integration/sandbox_clean_remove_test.go index 395987a0e..ee4843f94 100644 --- a/integration/sandbox_clean_remove_test.go +++ b/integration/sandbox_clean_remove_test.go @@ -17,14 +17,23 @@ limitations under the License. package integration import ( + "encoding/json" + "io/ioutil" + "os" + "path/filepath" + "strings" "testing" "time" "github.com/containerd/containerd" + runtimespec "github.com/opencontainers/runtime-spec/specs-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/context" + "golang.org/x/sys/unix" runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" + + "github.com/containerd/cri/pkg/server" ) func TestSandboxCleanRemove(t *testing.T) { @@ -66,3 +75,100 @@ func TestSandboxCleanRemove(t *testing.T) { assert.NoError(t, runtimeService.StopPodSandbox(sb)) assert.NoError(t, runtimeService.RemovePodSandbox(sb)) } + +func TestSandboxRemoveWithoutIPLeakage(t *testing.T) { + ctx := context.Background() + const hostLocalCheckpointDir = "/var/lib/cni" + + t.Logf("Make sure host-local ipam is in use") + config, err := CRIConfig() + require.NoError(t, err) + fs, err := ioutil.ReadDir(config.NetworkPluginConfDir) + require.NoError(t, err) + require.NotEmpty(t, fs) + f := filepath.Join(config.NetworkPluginConfDir, fs[0].Name()) + cniConfig, err := ioutil.ReadFile(f) + require.NoError(t, err) + if !strings.Contains(string(cniConfig), "host-local") { + t.Skip("host-local ipam is not in use") + } + + t.Logf("Create a sandbox") + sbConfig := PodSandboxConfig("sandbox", "remove-without-ip-leakage") + sb, err := runtimeService.RunPodSandbox(sbConfig, *runtimeHandler) + require.NoError(t, err) + defer func() { + // Make sure the sandbox is cleaned up in any case. + runtimeService.StopPodSandbox(sb) + runtimeService.RemovePodSandbox(sb) + }() + + t.Logf("Get pod information") + client, err := RawRuntimeClient() + require.NoError(t, err) + resp, err := client.PodSandboxStatus(ctx, &runtime.PodSandboxStatusRequest{ + PodSandboxId: sb, + Verbose: true, + }) + require.NoError(t, err) + status := resp.GetStatus() + info := resp.GetInfo() + ip := status.GetNetwork().GetIp() + require.NotEmpty(t, ip) + var sbInfo server.SandboxInfo + require.NoError(t, json.Unmarshal([]byte(info["info"]), &sbInfo)) + require.NotNil(t, sbInfo.RuntimeSpec.Linux) + var netNS string + for _, n := range sbInfo.RuntimeSpec.Linux.Namespaces { + if n.Type == runtimespec.NetworkNamespace { + netNS = n.Path + } + } + require.NotEmpty(t, netNS, "network namespace should be set") + + t.Logf("Should be able to find the pod ip in host-local checkpoint") + checkIP := func(ip string) bool { + found := false + filepath.Walk(hostLocalCheckpointDir, func(_ string, info os.FileInfo, _ error) error { + if info != nil && info.Name() == ip { + found = true + } + return nil + }) + return found + } + require.True(t, checkIP(ip)) + + t.Logf("Kill sandbox container") + require.NoError(t, KillPid(int(sbInfo.Pid))) + + t.Logf("Unmount network namespace") + // The umount will take effect after containerd is stopped. + require.NoError(t, unix.Unmount(netNS, unix.MNT_DETACH)) + + t.Logf("Restart containerd") + RestartContainerd(t) + + t.Logf("Sandbox state should be NOTREADY") + assert.NoError(t, Eventually(func() (bool, error) { + status, err := runtimeService.PodSandboxStatus(sb) + if err != nil { + return false, err + } + return status.GetState() == runtime.PodSandboxState_SANDBOX_NOTREADY, nil + }, time.Second, 30*time.Second), "sandbox state should become NOTREADY") + + t.Logf("Network namespace should have been removed") + _, err = os.Stat(netNS) + assert.True(t, os.IsNotExist(err)) + + t.Logf("Should still be able to find the pod ip in host-local checkpoint") + assert.True(t, checkIP(ip)) + + t.Logf("Should be able to remove the sandbox after properly stopped") + assert.NoError(t, runtimeService.StopPodSandbox(sb)) + assert.NoError(t, runtimeService.RemovePodSandbox(sb)) + + t.Logf("Should not be able to find the pod ip in host-local checkpoint") + assert.False(t, checkIP(ip)) +} diff --git a/integration/test_utils.go b/integration/test_utils.go index 8a802cdf7..a2e598f77 100644 --- a/integration/test_utils.go +++ b/integration/test_utils.go @@ -24,11 +24,14 @@ import ( "os/exec" "strconv" "strings" + "testing" "time" "github.com/containerd/containerd" "github.com/pkg/errors" "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "google.golang.org/grpc" "k8s.io/kubernetes/pkg/kubelet/apis/cri" runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2" @@ -295,6 +298,15 @@ func KillProcess(name string) error { return nil } +// KillPid kills the process by pid. kill is used. +func KillPid(pid int) error { + output, err := exec.Command("kill", strconv.Itoa(pid)).CombinedOutput() + if err != nil { + return errors.Errorf("failed to kill %d - error: %v, output: %q", pid, err, output) + } + return nil +} + // PidOf returns pid of a process by name. func PidOf(name string) (int, error) { b, err := exec.Command("pidof", name).CombinedOutput() @@ -308,8 +320,8 @@ func PidOf(name string) (int, error) { return strconv.Atoi(output) } -// CRIConfig gets current cri config from containerd. -func CRIConfig() (*criconfig.Config, error) { +// RawRuntimeClient returns a raw grpc runtime service client. +func RawRuntimeClient() (runtime.RuntimeServiceClient, error) { addr, dialer, err := kubeletutil.GetAddressAndDialer(*criEndpoint) if err != nil { return nil, errors.Wrap(err, "failed to get dialer") @@ -320,8 +332,16 @@ func CRIConfig() (*criconfig.Config, error) { if err != nil { return nil, errors.Wrap(err, "failed to connect cri endpoint") } - client := runtime.NewRuntimeServiceClient(conn) - resp, err := client.Status(ctx, &runtime.StatusRequest{Verbose: true}) + return runtime.NewRuntimeServiceClient(conn), nil +} + +// CRIConfig gets current cri config from containerd. +func CRIConfig() (*criconfig.Config, error) { + client, err := RawRuntimeClient() + if err != nil { + return nil, errors.Wrap(err, "failed to get raw runtime client") + } + resp, err := client.Status(context.Background(), &runtime.StatusRequest{Verbose: true}) if err != nil { return nil, errors.Wrap(err, "failed to get status") } @@ -331,3 +351,21 @@ func CRIConfig() (*criconfig.Config, error) { } return config, nil } + +func RestartContainerd(t *testing.T) { + require.NoError(t, KillProcess("containerd")) + + // Use assert so that the 3rd wait always runs, this makes sure + // containerd is running before this function returns. + assert.NoError(t, Eventually(func() (bool, error) { + pid, err := PidOf("containerd") + if err != nil { + return false, err + } + return pid == 0, nil + }, time.Second, 30*time.Second), "wait for containerd to be killed") + + require.NoError(t, Eventually(func() (bool, error) { + return ConnectDaemons() == nil, nil + }, time.Second, 30*time.Second), "wait for containerd to be restarted") +} diff --git a/pkg/server/container_status.go b/pkg/server/container_status.go index 1def0d417..a02454690 100644 --- a/pkg/server/container_status.go +++ b/pkg/server/container_status.go @@ -99,7 +99,8 @@ func toCRIContainerStatus(container containerstore.Container, spec *runtime.Imag } } -type containerInfo struct { +// ContainerInfo is extra information for a container. +type ContainerInfo struct { // TODO(random-liu): Add sandboxID in CRI container status. SandboxID string `json:"sandboxID"` Pid uint32 `json:"pid"` @@ -122,7 +123,7 @@ func toCRIContainerInfo(ctx context.Context, container containerstore.Container, status := container.Status.Get() // TODO(random-liu): Change CRI status info to use array instead of map. - ci := &containerInfo{ + ci := &ContainerInfo{ SandboxID: container.SandboxID, Pid: status.Pid, Removing: status.Removing, diff --git a/pkg/server/sandbox_status.go b/pkg/server/sandbox_status.go index 9f50a7b24..7965a1a3f 100644 --- a/pkg/server/sandbox_status.go +++ b/pkg/server/sandbox_status.go @@ -99,8 +99,9 @@ func toCRISandboxStatus(meta sandboxstore.Metadata, status sandboxstore.Status, } } +// SandboxInfo is extra information for sandbox. // TODO (mikebrow): discuss predefining constants structures for some or all of these field names in CRI -type sandboxInfo struct { +type SandboxInfo struct { Pid uint32 `json:"pid"` Status string `json:"processStatus"` NetNSClosed bool `json:"netNamespaceClosed"` @@ -132,7 +133,7 @@ func toCRISandboxInfo(ctx context.Context, sandbox sandboxstore.Sandbox) (map[st processStatus = taskStatus.Status } - si := &sandboxInfo{ + si := &SandboxInfo{ Pid: sandbox.Status.Get().Pid, RuntimeHandler: sandbox.RuntimeHandler, Status: string(processStatus),