Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Commit f913714

Browse files
Random-LiuRandom-Liu
authored
Merge pull request #951 from Random-Liu/cherrypick-#949-release-1.2
Cherrypick #949 release 1.2
2 parents 4f939fc + 0250180 commit f913714

9 files changed

Lines changed: 188 additions & 48 deletions

File tree

‎integration/restart_test.go‎

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ package integration
1919
import (
2020
"sort"
2121
"testing"
22-
"time"
2322

2423
"github.com/containerd/containerd"
2524
"github.com/stretchr/testify/assert"
@@ -140,27 +139,8 @@ func TestContainerdRestart(t *testing.T) {
140139
imagesBeforeRestart, err := imageService.ListImages(nil)
141140
assert.NoError(t, err)
142141

143-
t.Logf("Kill containerd")
144-
require.NoError(t, KillProcess("containerd"))
145-
defer func() {
146-
assert.NoError(t, Eventually(func() (bool, error) {
147-
return ConnectDaemons() == nil, nil
148-
}, time.Second, 30*time.Second), "make sure containerd is running before test finish")
149-
}()
150-
151-
t.Logf("Wait until containerd is killed")
152-
require.NoError(t, Eventually(func() (bool, error) {
153-
pid, err := PidOf("containerd")
154-
if err != nil {
155-
return false, err
156-
}
157-
return pid == 0, nil
158-
}, time.Second, 30*time.Second), "wait for containerd to be killed")
159-
160-
t.Logf("Wait until containerd is restarted")
161-
require.NoError(t, Eventually(func() (bool, error) {
162-
return ConnectDaemons() == nil, nil
163-
}, time.Second, 30*time.Second), "wait for containerd to be restarted")
142+
t.Logf("Restart containerd")
143+
RestartContainerd(t)
164144

165145
t.Logf("Check sandbox and container state after restart")
166146
loadedSandboxes, err := runtimeService.ListPodSandbox(&runtime.PodSandboxFilter{})

‎integration/sandbox_clean_remove_test.go‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,14 +17,23 @@ limitations under the License.
1717
package integration
1818

1919
import (
20+
"encoding/json"
21+
"io/ioutil"
22+
"os"
23+
"path/filepath"
24+
"strings"
2025
"testing"
2126
"time"
2227

2328
"github.com/containerd/containerd"
29+
runtimespec "github.com/opencontainers/runtime-spec/specs-go"
2430
"github.com/stretchr/testify/assert"
2531
"github.com/stretchr/testify/require"
2632
"golang.org/x/net/context"
33+
"golang.org/x/sys/unix"
2734
runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
35+
36+
"github.com/containerd/cri/pkg/server"
2837
)
2938

3039
func TestSandboxCleanRemove(t *testing.T) {
@@ -66,3 +75,100 @@ func TestSandboxCleanRemove(t *testing.T) {
6675
assert.NoError(t, runtimeService.StopPodSandbox(sb))
6776
assert.NoError(t, runtimeService.RemovePodSandbox(sb))
6877
}
78+
79+
func TestSandboxRemoveWithoutIPLeakage(t *testing.T) {
80+
ctx := context.Background()
81+
const hostLocalCheckpointDir = "/var/lib/cni"
82+
83+
t.Logf("Make sure host-local ipam is in use")
84+
config, err := CRIConfig()
85+
require.NoError(t, err)
86+
fs, err := ioutil.ReadDir(config.NetworkPluginConfDir)
87+
require.NoError(t, err)
88+
require.NotEmpty(t, fs)
89+
f := filepath.Join(config.NetworkPluginConfDir, fs[0].Name())
90+
cniConfig, err := ioutil.ReadFile(f)
91+
require.NoError(t, err)
92+
if !strings.Contains(string(cniConfig), "host-local") {
93+
t.Skip("host-local ipam is not in use")
94+
}
95+
96+
t.Logf("Create a sandbox")
97+
sbConfig := PodSandboxConfig("sandbox", "remove-without-ip-leakage")
98+
sb, err := runtimeService.RunPodSandbox(sbConfig, *runtimeHandler)
99+
require.NoError(t, err)
100+
defer func() {
101+
// Make sure the sandbox is cleaned up in any case.
102+
runtimeService.StopPodSandbox(sb)
103+
runtimeService.RemovePodSandbox(sb)
104+
}()
105+
106+
t.Logf("Get pod information")
107+
client, err := RawRuntimeClient()
108+
require.NoError(t, err)
109+
resp, err := client.PodSandboxStatus(ctx, &runtime.PodSandboxStatusRequest{
110+
PodSandboxId: sb,
111+
Verbose: true,
112+
})
113+
require.NoError(t, err)
114+
status := resp.GetStatus()
115+
info := resp.GetInfo()
116+
ip := status.GetNetwork().GetIp()
117+
require.NotEmpty(t, ip)
118+
var sbInfo server.SandboxInfo
119+
require.NoError(t, json.Unmarshal([]byte(info["info"]), &sbInfo))
120+
require.NotNil(t, sbInfo.RuntimeSpec.Linux)
121+
var netNS string
122+
for _, n := range sbInfo.RuntimeSpec.Linux.Namespaces {
123+
if n.Type == runtimespec.NetworkNamespace {
124+
netNS = n.Path
125+
}
126+
}
127+
require.NotEmpty(t, netNS, "network namespace should be set")
128+
129+
t.Logf("Should be able to find the pod ip in host-local checkpoint")
130+
checkIP := func(ip string) bool {
131+
found := false
132+
filepath.Walk(hostLocalCheckpointDir, func(_ string, info os.FileInfo, _ error) error {
133+
if info != nil && info.Name() == ip {
134+
found = true
135+
}
136+
return nil
137+
})
138+
return found
139+
}
140+
require.True(t, checkIP(ip))
141+
142+
t.Logf("Kill sandbox container")
143+
require.NoError(t, KillPid(int(sbInfo.Pid)))
144+
145+
t.Logf("Unmount network namespace")
146+
// The umount will take effect after containerd is stopped.
147+
require.NoError(t, unix.Unmount(netNS, unix.MNT_DETACH))
148+
149+
t.Logf("Restart containerd")
150+
RestartContainerd(t)
151+
152+
t.Logf("Sandbox state should be NOTREADY")
153+
assert.NoError(t, Eventually(func() (bool, error) {
154+
status, err := runtimeService.PodSandboxStatus(sb)
155+
if err != nil {
156+
return false, err
157+
}
158+
return status.GetState() == runtime.PodSandboxState_SANDBOX_NOTREADY, nil
159+
}, time.Second, 30*time.Second), "sandbox state should become NOTREADY")
160+
161+
t.Logf("Network namespace should have been removed")
162+
_, err = os.Stat(netNS)
163+
assert.True(t, os.IsNotExist(err))
164+
165+
t.Logf("Should still be able to find the pod ip in host-local checkpoint")
166+
assert.True(t, checkIP(ip))
167+
168+
t.Logf("Should be able to remove the sandbox after properly stopped")
169+
assert.NoError(t, runtimeService.StopPodSandbox(sb))
170+
assert.NoError(t, runtimeService.RemovePodSandbox(sb))
171+
172+
t.Logf("Should not be able to find the pod ip in host-local checkpoint")
173+
assert.False(t, checkIP(ip))
174+
}

‎integration/test_utils.go‎

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,14 @@ import (
2424
"os/exec"
2525
"strconv"
2626
"strings"
27+
"testing"
2728
"time"
2829

2930
"github.com/containerd/containerd"
3031
"github.com/pkg/errors"
3132
"github.com/sirupsen/logrus"
33+
"github.com/stretchr/testify/assert"
34+
"github.com/stretchr/testify/require"
3235
"google.golang.org/grpc"
3336
"k8s.io/kubernetes/pkg/kubelet/apis/cri"
3437
runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
@@ -295,6 +298,15 @@ func KillProcess(name string) error {
295298
return nil
296299
}
297300

301+
// KillPid kills the process by pid. kill is used.
302+
func KillPid(pid int) error {
303+
output, err := exec.Command("kill", strconv.Itoa(pid)).CombinedOutput()
304+
if err != nil {
305+
return errors.Errorf("failed to kill %d - error: %v, output: %q", pid, err, output)
306+
}
307+
return nil
308+
}
309+
298310
// PidOf returns pid of a process by name.
299311
func PidOf(name string) (int, error) {
300312
b, err := exec.Command("pidof", name).CombinedOutput()
@@ -308,8 +320,8 @@ func PidOf(name string) (int, error) {
308320
return strconv.Atoi(output)
309321
}
310322

311-
// CRIConfig gets current cri config from containerd.
312-
func CRIConfig() (*criconfig.Config, error) {
323+
// RawRuntimeClient returns a raw grpc runtime service client.
324+
func RawRuntimeClient() (runtime.RuntimeServiceClient, error) {
313325
addr, dialer, err := kubeletutil.GetAddressAndDialer(*criEndpoint)
314326
if err != nil {
315327
return nil, errors.Wrap(err, "failed to get dialer")
@@ -320,8 +332,16 @@ func CRIConfig() (*criconfig.Config, error) {
320332
if err != nil {
321333
return nil, errors.Wrap(err, "failed to connect cri endpoint")
322334
}
323-
client := runtime.NewRuntimeServiceClient(conn)
324-
resp, err := client.Status(ctx, &runtime.StatusRequest{Verbose: true})
335+
return runtime.NewRuntimeServiceClient(conn), nil
336+
}
337+
338+
// CRIConfig gets current cri config from containerd.
339+
func CRIConfig() (*criconfig.Config, error) {
340+
client, err := RawRuntimeClient()
341+
if err != nil {
342+
return nil, errors.Wrap(err, "failed to get raw runtime client")
343+
}
344+
resp, err := client.Status(context.Background(), &runtime.StatusRequest{Verbose: true})
325345
if err != nil {
326346
return nil, errors.Wrap(err, "failed to get status")
327347
}
@@ -331,3 +351,21 @@ func CRIConfig() (*criconfig.Config, error) {
331351
}
332352
return config, nil
333353
}
354+
355+
func RestartContainerd(t *testing.T) {
356+
require.NoError(t, KillProcess("containerd"))
357+
358+
// Use assert so that the 3rd wait always runs, this makes sure
359+
// containerd is running before this function returns.
360+
assert.NoError(t, Eventually(func() (bool, error) {
361+
pid, err := PidOf("containerd")
362+
if err != nil {
363+
return false, err
364+
}
365+
return pid == 0, nil
366+
}, time.Second, 30*time.Second), "wait for containerd to be killed")
367+
368+
require.NoError(t, Eventually(func() (bool, error) {
369+
return ConnectDaemons() == nil, nil
370+
}, time.Second, 30*time.Second), "wait for containerd to be restarted")
371+
}

‎pkg/server/container_status.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,8 @@ func toCRIContainerStatus(container containerstore.Container, spec *runtime.Imag
9999
}
100100
}
101101

102-
type containerInfo struct {
102+
// ContainerInfo is extra information for a container.
103+
type ContainerInfo struct {
103104
// TODO(random-liu): Add sandboxID in CRI container status.
104105
SandboxID string `json:"sandboxID"`
105106
Pid uint32 `json:"pid"`
@@ -122,7 +123,7 @@ func toCRIContainerInfo(ctx context.Context, container containerstore.Container,
122123
status := container.Status.Get()
123124

124125
// TODO(random-liu): Change CRI status info to use array instead of map.
125-
ci := &containerInfo{
126+
ci := &ContainerInfo{
126127
SandboxID: container.SandboxID,
127128
Pid: status.Pid,
128129
Removing: status.Removing,

‎pkg/server/sandbox_status.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,9 @@ func toCRISandboxStatus(meta sandboxstore.Metadata, status sandboxstore.Status,
9999
}
100100
}
101101

102+
// SandboxInfo is extra information for sandbox.
102103
// TODO (mikebrow): discuss predefining constants structures for some or all of these field names in CRI
103-
type sandboxInfo struct {
104+
type SandboxInfo struct {
104105
Pid uint32 `json:"pid"`
105106
Status string `json:"processStatus"`
106107
NetNSClosed bool `json:"netNamespaceClosed"`
@@ -132,7 +133,7 @@ func toCRISandboxInfo(ctx context.Context, sandbox sandboxstore.Sandbox) (map[st
132133
processStatus = taskStatus.Status
133134
}
134135

135-
si := &sandboxInfo{
136+
si := &SandboxInfo{
136137
Pid: sandbox.Status.Get().Pid,
137138
RuntimeHandler: sandbox.RuntimeHandler,
138139
Status: string(processStatus),

‎pkg/server/sandbox_stop.go‎

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ limitations under the License.
1717
package server
1818

1919
import (
20-
"os"
2120
"time"
2221

2322
"github.com/containerd/containerd"
@@ -60,23 +59,21 @@ func (c *criService) StopPodSandbox(ctx context.Context, r *runtime.StopPodSandb
6059
}
6160

6261
// Teardown network for sandbox.
63-
if sandbox.NetNSPath != "" && sandbox.NetNS != nil {
64-
if _, err := os.Stat(sandbox.NetNSPath); err != nil {
65-
if !os.IsNotExist(err) {
66-
return nil, errors.Wrapf(err, "failed to stat network namespace path %s", sandbox.NetNSPath)
67-
}
68-
} else {
69-
if teardownErr := c.teardownPod(id, sandbox.NetNSPath, sandbox.Config); teardownErr != nil {
70-
return nil, errors.Wrapf(teardownErr, "failed to destroy network for sandbox %q", id)
71-
}
62+
if sandbox.NetNSPath != "" {
63+
netNSPath := sandbox.NetNSPath
64+
if sandbox.NetNS == nil || sandbox.NetNS.Closed() {
65+
// Use empty netns path if netns is not available. This is defined in:
66+
// https://github.com/containernetworking/cni/blob/v0.7.0-alpha1/SPEC.md
67+
netNSPath = ""
7268
}
73-
/*TODO:It is still possible that containerd crashes after we teardown the network, but before we remove the network namespace.
74-
In that case, we'll not be able to remove the sandbox anymore. The chance is slim, but we should be aware of that.
75-
In the future, once TearDownPod is idempotent, this will be fixed.*/
76-
77-
//Close the sandbox network namespace if it was created
78-
if err = sandbox.NetNS.Remove(); err != nil {
79-
return nil, errors.Wrapf(err, "failed to remove network namespace for sandbox %q", id)
69+
if err := c.teardownPod(id, netNSPath, sandbox.Config); err != nil {
70+
return nil, errors.Wrapf(err, "failed to destroy network for sandbox %q", id)
71+
}
72+
// Close the sandbox network namespace if it was created
73+
if sandbox.NetNS != nil {
74+
if err = sandbox.NetNS.Remove(); err != nil {
75+
return nil, errors.Wrapf(err, "failed to remove network namespace for sandbox %q", id)
76+
}
8077
}
8178
}
8279

‎pkg/store/sandbox/netns.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,13 @@ import (
2727
osinterface "github.com/containerd/cri/pkg/os"
2828
)
2929

30+
// The NetNS library assumes only containerd manages the lifecycle of the
31+
// network namespace mount. The only case that netns will be unmounted by
32+
// someone else is node reboot.
33+
// If this assumption is broken, NetNS won't be aware of the external
34+
// unmount, and there will be a state mismatch.
35+
// TODO(random-liu): Don't cache state, always load from the system.
36+
3037
// ErrClosedNetNS is the error returned when network namespace is closed.
3138
var ErrClosedNetNS = errors.New("network namespace is closed")
3239

‎vendor.conf‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ github.com/containerd/console c12b1e7919c14469339a5d38f2f8ed9b64a9de23
66
github.com/containerd/containerd 15f19d7a67fa322e6de0ef4c6a1bf9da0f056554
77
github.com/containerd/continuity bd77b46c8352f74eb12c85bdc01f4b90f69d66b4
88
github.com/containerd/fifo 3d5202aec260678c48179c56f40e6f38a095738c
9-
github.com/containerd/go-cni 6d7b509a054a3cb1c35ed1865d4fde2f0cb547cd
9+
github.com/containerd/go-cni 40bcf8ec8acd7372be1d77031d585d5d8e561c90
1010
github.com/containerd/go-runc 5a6d9f37cfa36b15efba46dc7ea349fa9b7143c3
1111
github.com/containerd/ttrpc 2a805f71863501300ae1976d29f0454ae003e85a
1212
github.com/containerd/typeurl a93fcdb778cd272c6e9b3028b2f42d813e785d40

‎vendor/github.com/containerd/go-cni/cni.go‎

Lines changed: 10 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)