Skip to content

[port #5904 to sbserver] Setup pod network after creating the sandbox container #7426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
samuelkarp merged 4 commits into from
Nov 23, 2022
Merged

[port #5904 to sbserver] Setup pod network after creating the sandbox container #7426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1deaedd
sbserver: persist sandbox during partial teardown
samuelkarp Sep 23, 2022
085d8e6
integration: enable tests for sbserver
samuelkarp Sep 23, 2022
a74f7e9
sbserver: save netns in sandbox metadata on create
samuelkarp Oct 13, 2022
ac4af4d
integration: enable CNI slow test for sbserver
samuelkarp Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 56 additions & 31 deletions integration/sandbox_run_rollback_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package integration

import (
"context"
"encoding/json"
"fmt"
"os"
Expand All @@ -36,6 +37,7 @@ import (
"github.com/stretchr/testify/require"
criapiv1 "k8s.io/cri-api/pkg/apis/runtime/v1"

"github.com/containerd/containerd/pkg/cri/sbserver"
"github.com/containerd/containerd/pkg/cri/store/sandbox"
"github.com/containerd/containerd/pkg/failpoint"
"github.com/containerd/typeurl"
Expand Down Expand Up @@ -103,9 +105,6 @@ func TestRunPodSandboxWithShimDeleteFailure(t *testing.T) {
if runtime.GOOS != "linux" {
t.Skip()
}
if os.Getenv("ENABLE_CRI_SANDBOXES") != "" {
t.Skip()
}

testCase := func(restart bool) func(*testing.T) {
return func(t *testing.T) {
Expand Down Expand Up @@ -177,9 +176,6 @@ func TestRunPodSandboxWithShimStartAndTeardownCNIFailure(t *testing.T) {
if runtime.GOOS != "linux" {
t.Skip()
}
if os.Getenv("ENABLE_CRI_SANDBOXES") != "" {
t.Skip()
}

testCase := func(restart bool) func(*testing.T) {
return func(t *testing.T) {
Expand Down Expand Up @@ -243,9 +239,6 @@ func TestRunPodSandboxAndTeardownCNISlow(t *testing.T) {
if runtime.GOOS != "linux" {
t.Skip()
}
if os.Getenv("ENABLE_CRI_SANDBOXES") != "" {
t.Skip()
}

t.Log("Init PodSandboxConfig with specific key")
sbName := t.Name()
Expand Down Expand Up @@ -301,31 +294,63 @@ func TestRunPodSandboxAndTeardownCNISlow(t *testing.T) {
assert.Equal(t, sb.Metadata.Uid, sbConfig.Metadata.Uid)
assert.Equal(t, sb.Metadata.Attempt, sbConfig.Metadata.Attempt)

t.Log("Get sandbox info")
_, info, err := SandboxInfo(sb.Id)
require.NoError(t, err)
require.False(t, info.NetNSClosed)

var netNS string
for _, n := range info.RuntimeSpec.Linux.Namespaces {
if n.Type == runtimespec.NetworkNamespace {
netNS = n.Path
switch os.Getenv("ENABLE_CRI_SANDBOXES") {
Comment thread
samuelkarp marked this conversation as resolved.
case "":
// non-sbserver
t.Log("Get sandbox info (non-sbserver)")
_, info, err := SandboxInfo(sb.Id)
require.NoError(t, err)
require.False(t, info.NetNSClosed)
var netNS string
for _, n := range info.RuntimeSpec.Linux.Namespaces {
if n.Type == runtimespec.NetworkNamespace {
netNS = n.Path
}
}
assert.NotEmpty(t, netNS, "network namespace should be set")

t.Log("Get sandbox container")
c, err := GetContainer(sb.Id)
require.NoError(t, err)
any, ok := c.Extensions["io.cri-containerd.sandbox.metadata"]
require.True(t, ok, "sandbox metadata should exist in extension")
i, err := typeurl.UnmarshalAny(any)
require.NoError(t, err)
require.IsType(t, &sandbox.Metadata{}, i)
metadata, ok := i.(*sandbox.Metadata)
require.True(t, ok)
assert.Equal(t, netNS, metadata.NetNSPath, "network namespace path should be the same in runtime spec and sandbox metadata")
default:
// sbserver
t.Log("Get sandbox info (sbserver)")
_, info, err := sbserverSandboxInfo(sb.Id)
require.NoError(t, err)
require.False(t, info.NetNSClosed)

assert.NotEmpty(t, info.Metadata.NetNSPath, "network namespace should be set")
}
assert.NotEmpty(t, netNS, "network namespace should be set")

t.Log("Get sandbox container")
c, err := GetContainer(sb.Id)
require.NoError(t, err)
any, ok := c.Extensions["io.cri-containerd.sandbox.metadata"]
require.True(t, ok, "sandbox metadata should exist in extension")
i, err := typeurl.UnmarshalAny(any)
require.NoError(t, err)
require.IsType(t, &sandbox.Metadata{}, i)
metadata, ok := i.(*sandbox.Metadata)
require.True(t, ok)
assert.NotEmpty(t, metadata.NetNSPath)
assert.Equal(t, netNS, metadata.NetNSPath, "network namespace path should be the same in runtime spec and sandbox metadata")
}

// sbserverSandboxInfo gets sandbox info.
func sbserverSandboxInfo(id string) (*criapiv1.PodSandboxStatus, *sbserver.SandboxInfo, error) {
client, err := RawRuntimeClient()
if err != nil {
return nil, nil, fmt.Errorf("failed to get raw runtime client: %w", err)
}
resp, err := client.PodSandboxStatus(context.Background(), &criapiv1.PodSandboxStatusRequest{
PodSandboxId: id,
Verbose: true,
})
if err != nil {
return nil, nil, fmt.Errorf("failed to get sandbox status: %w", err)
}
status := resp.GetStatus()
var info sbserver.SandboxInfo
if err := json.Unmarshal([]byte(resp.GetInfo()["info"]), &info); err != nil {
return nil, nil, fmt.Errorf("failed to unmarshal sandbox info: %w", err)
}
return status, &info, nil
}

func ensureCNIAddRunning(t *testing.T, sbName string) error {
Expand Down
2 changes: 2 additions & 0 deletions pkg/cri/sbserver/podsandbox/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ const (
sandboxMetadataExtension = criContainerdPrefix + ".sandbox.metadata"
// runtimeRunhcsV1 is the runtime type for runhcs.
runtimeRunhcsV1 = "io.containerd.runhcs.v1"
// MetadataKey is the key used for storing metadata in the sandbox extensions
MetadataKey = "metadata"
)

const (
Expand Down
10 changes: 6 additions & 4 deletions pkg/cri/sbserver/podsandbox/sandbox_delete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,13 @@ func (c *Controller) Delete(ctx context.Context, sandboxID string) (*api.Control
}

// Delete sandbox container.
if err := sandbox.Container.Delete(ctx, containerd.WithSnapshotCleanup); err != nil {
if !errdefs.IsNotFound(err) {
return nil, fmt.Errorf("failed to delete sandbox container %q: %w", sandboxID, err)
if sandbox.Container != nil {
if err := sandbox.Container.Delete(ctx, containerd.WithSnapshotCleanup); err != nil {
if !errdefs.IsNotFound(err) {
return nil, fmt.Errorf("failed to delete sandbox container %q: %w", sandboxID, err)
}
log.G(ctx).Tracef("Sandbox controller Delete called for sandbox container %q that does not exist", sandboxID)
}
log.G(ctx).Tracef("Sandbox controller Delete called for sandbox container %q that does not exist", sandboxID)
}

return &api.ControllerDeleteResponse{}, nil
Expand Down
58 changes: 36 additions & 22 deletions pkg/cri/sbserver/podsandbox/sandbox_run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ import (
"errors"
"fmt"

"github.com/containerd/nri"
v1 "github.com/containerd/nri/types/v1"
Comment thread
samuelkarp marked this conversation as resolved.
"github.com/containerd/typeurl"
"github.com/davecgh/go-spew/spew"
"github.com/opencontainers/selinux/go-selinux"
runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
Comment thread
samuelkarp marked this conversation as resolved.

"github.com/containerd/containerd"
api "github.com/containerd/containerd/api/services/sandbox/v1"
containerdio "github.com/containerd/containerd/cio"
Expand All @@ -33,27 +40,33 @@ import (
ctrdutil "github.com/containerd/containerd/pkg/cri/util"
"github.com/containerd/containerd/protobuf"
"github.com/containerd/containerd/snapshots"
"github.com/containerd/nri"
v1 "github.com/containerd/nri/types/v1"
"github.com/containerd/typeurl"
"github.com/davecgh/go-spew/spew"
"github.com/opencontainers/selinux/go-selinux"
runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
)

func init() {
typeurl.Register(&sandboxstore.Metadata{},
"github.com/containerd/cri/pkg/store/sandbox", "Metadata")
}

func (c *Controller) Start(ctx context.Context, id string) (_ *api.ControllerStartResponse, retErr error) {
// Start creates resources required for the sandbox and starts the sandbox. If an error occurs, Start attempts to tear
// down the created resources. If an error occurs while tearing down resources, a zero-valued response is returned
// alongside the error. If the teardown was successful, a nil response is returned with the error.
// TODO(samuelkarp) Determine whether this error indication is reasonable to retain once controller.Delete is implemented.
func (c *Controller) Start(ctx context.Context, id string) (resp *api.ControllerStartResponse, retErr error) {
var cleanupErr error
defer func() {
if retErr != nil && cleanupErr != nil {
log.G(ctx).WithField("id", id).WithError(cleanupErr).Errorf("failed to fully teardown sandbox resources after earlier error: %s", retErr)
Comment thread
samuelkarp marked this conversation as resolved.
resp = &api.ControllerStartResponse{}
}
}()

sandboxInfo, err := c.client.SandboxStore().Get(ctx, id)
if err != nil {
return nil, fmt.Errorf("unable to find sandbox with id %q: %w", id, err)
}

var metadata sandboxstore.Metadata
if err := sandboxInfo.GetExtension("metadata", &metadata); err != nil {
if err := sandboxInfo.GetExtension(MetadataKey, &metadata); err != nil {
return nil, fmt.Errorf("failed to get sandbox %q metadata: %w", id, err)
}

Expand Down Expand Up @@ -137,11 +150,11 @@ func (c *Controller) Start(ctx context.Context, id string) (_ *api.ControllerSta
return nil, fmt.Errorf("failed to create containerd container: %w", err)
}
defer func() {
if retErr != nil {
if retErr != nil && cleanupErr == nil {
deferCtx, deferCancel := ctrdutil.DeferContext()
defer deferCancel()
if err := container.Delete(deferCtx, containerd.WithSnapshotCleanup); err != nil {
log.G(ctx).WithError(err).Errorf("Failed to delete containerd container %q", id)
if cleanupErr = container.Delete(deferCtx, containerd.WithSnapshotCleanup); cleanupErr != nil {
log.G(ctx).WithError(cleanupErr).Errorf("Failed to delete containerd container %q", id)
}
}
}()
Expand All @@ -153,10 +166,10 @@ func (c *Controller) Start(ctx context.Context, id string) (_ *api.ControllerSta
sandboxRootDir, err)
}
defer func() {
if retErr != nil {
if retErr != nil && cleanupErr == nil {
Comment thread
samuelkarp marked this conversation as resolved.
// Cleanup the sandbox root directory.
if err := c.os.RemoveAll(sandboxRootDir); err != nil {
log.G(ctx).WithError(err).Errorf("Failed to remove sandbox root directory %q",
if cleanupErr = c.os.RemoveAll(sandboxRootDir); cleanupErr != nil {
log.G(ctx).WithError(cleanupErr).Errorf("Failed to remove sandbox root directory %q",
sandboxRootDir)
}
}
Expand All @@ -168,10 +181,10 @@ func (c *Controller) Start(ctx context.Context, id string) (_ *api.ControllerSta
volatileSandboxRootDir, err)
}
defer func() {
if retErr != nil {
if retErr != nil && cleanupErr == nil {
Comment thread
samuelkarp marked this conversation as resolved.
// Cleanup the volatile sandbox root directory.
if err := c.os.RemoveAll(volatileSandboxRootDir); err != nil {
log.G(ctx).WithError(err).Errorf("Failed to remove volatile sandbox root directory %q",
if cleanupErr = c.os.RemoveAll(volatileSandboxRootDir); cleanupErr != nil {
log.G(ctx).WithError(cleanupErr).Errorf("Failed to remove volatile sandbox root directory %q",
volatileSandboxRootDir)
}
}
Expand All @@ -182,9 +195,9 @@ func (c *Controller) Start(ctx context.Context, id string) (_ *api.ControllerSta
return nil, fmt.Errorf("failed to setup sandbox files: %w", err)
}
defer func() {
if retErr != nil {
if err = c.cleanupSandboxFiles(id, config); err != nil {
log.G(ctx).WithError(err).Errorf("Failed to cleanup sandbox files in %q",
if retErr != nil && cleanupErr == nil {
if cleanupErr = c.cleanupSandboxFiles(id, config); cleanupErr != nil {
log.G(ctx).WithError(cleanupErr).Errorf("Failed to cleanup sandbox files in %q",
sandboxRootDir)
}
}
Expand All @@ -210,12 +223,13 @@ func (c *Controller) Start(ctx context.Context, id string) (_ *api.ControllerSta
return nil, fmt.Errorf("failed to create containerd task: %w", err)
}
defer func() {
if retErr != nil {
if retErr != nil && cleanupErr == nil {
Comment thread
samuelkarp marked this conversation as resolved.
deferCtx, deferCancel := ctrdutil.DeferContext()
defer deferCancel()
// Cleanup the sandbox container if an error is returned.
if _, err := task.Delete(deferCtx, WithNRISandboxDelete(id), containerd.WithProcessKill); err != nil && !errdefs.IsNotFound(err) {
log.G(ctx).WithError(err).Errorf("Failed to delete sandbox container %q", id)
cleanupErr = err
}
}
}()
Expand Down Expand Up @@ -245,7 +259,7 @@ func (c *Controller) Start(ctx context.Context, id string) (_ *api.ControllerSta
return nil, fmt.Errorf("failed to start sandbox container task %q: %w", id, err)
}

resp := &api.ControllerStartResponse{
resp = &api.ControllerStartResponse{
SandboxID: id,
Pid: task.Pid(),
CreatedAt: protobuf.ToTimestamp(info.CreatedAt),
Expand Down
5 changes: 3 additions & 2 deletions pkg/cri/sbserver/podsandbox/sandbox_stop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@ import (
"fmt"
"syscall"

"github.com/sirupsen/logrus"

eventtypes "github.com/containerd/containerd/api/events"
api "github.com/containerd/containerd/api/services/sandbox/v1"
"github.com/containerd/containerd/errdefs"
sandboxstore "github.com/containerd/containerd/pkg/cri/store/sandbox"
ctrdutil "github.com/containerd/containerd/pkg/cri/util"
"github.com/containerd/containerd/protobuf"
"github.com/sirupsen/logrus"
)

func (c *Controller) Stop(ctx context.Context, sandboxID string) (*api.ControllerStopResponse, error) {
Expand All @@ -44,7 +45,7 @@ func (c *Controller) Stop(ctx context.Context, sandboxID string) (*api.Controlle
// TODO: The Controller maintains its own Status instead of CRI's sandboxStore.
// Only stop sandbox container when it's running or unknown.
state := sandbox.Status.Get().State
if state == sandboxstore.StateReady || state == sandboxstore.StateUnknown {
if (state == sandboxstore.StateReady || state == sandboxstore.StateUnknown) && sandbox.Container != nil {
Comment thread
samuelkarp marked this conversation as resolved.
if err := c.stopSandboxContainer(ctx, sandbox); err != nil {
return nil, fmt.Errorf("failed to stop sandbox container %q in %q state: %w", sandboxID, state, err)
}
Expand Down
43 changes: 35 additions & 8 deletions pkg/cri/sbserver/restart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/containerd/containerd/errdefs"
containerdimages "github.com/containerd/containerd/images"
"github.com/containerd/containerd/log"
"github.com/containerd/containerd/pkg/cri/sbserver/podsandbox"
"github.com/containerd/containerd/platforms"
"github.com/containerd/typeurl"
"golang.org/x/sync/errgroup"
Expand Down Expand Up @@ -82,6 +83,28 @@ func (c *criService) recover(ctx context.Context) error {
return err
}

// Recover sandboxes in the new SandboxStore
storedSandboxes, err := c.client.SandboxStore().List(ctx)
if err != nil {
return fmt.Errorf("failed to list sandboxes from API: %w", err)
Comment thread
samuelkarp marked this conversation as resolved.
}
for _, sbx := range storedSandboxes {
if _, err := c.sandboxStore.Get(sbx.ID); err == nil {
continue
}
metadata := sandboxstore.Metadata{}
err := sbx.GetExtension(podsandbox.MetadataKey, &metadata)
if err != nil {
return fmt.Errorf("failed to get metadata for stored sandbox %q: %w", sbx.ID, err)
}
sb := sandboxstore.NewSandbox(metadata, sandboxstore.Status{State: sandboxstore.StateUnknown})
Comment thread
samuelkarp marked this conversation as resolved.
// Load network namespace.
sb.NetNS = getNetNS(&metadata)
if err := c.sandboxStore.Add(sb); err != nil {
return fmt.Errorf("failed to add stored sandbox %q to store: %w", sbx.ID, err)
}
}

// Recover all containers.
containers, err := c.client.Containers(ctx, filterLabel(containerKindLabel, containerKindContainer))
if err != nil {
Expand Down Expand Up @@ -427,20 +450,24 @@ func (c *criService) loadSandbox(ctx context.Context, cntr containerd.Container)
sandbox.Container = cntr

// Load network namespace.
sandbox.NetNS = getNetNS(meta)

// It doesn't matter whether task is running or not. If it is running, sandbox
// status will be `READY`; if it is not running, sandbox status will be `NOT_READY`,
// kubelet will stop the sandbox which will properly cleanup everything.
return sandbox, nil
}

func getNetNS(meta *sandboxstore.Metadata) *netns.NetNS {
if goruntime.GOOS != "windows" &&
meta.Config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetNetwork() == runtime.NamespaceMode_NODE {
// Don't need to load netns for host network sandbox.
return sandbox, nil
return nil
}
if goruntime.GOOS == "windows" && meta.Config.GetWindows().GetSecurityContext().GetHostProcess() {
return sandbox, nil
return nil
}
sandbox.NetNS = netns.LoadNetNS(meta.NetNSPath)

// It doesn't matter whether task is running or not. If it is running, sandbox
// status will be `READY`; if it is not running, sandbox status will be `NOT_READY`,
// kubelet will stop the sandbox which will properly cleanup everything.
return sandbox, nil
return netns.LoadNetNS(meta.NetNSPath)
}

// loadImages loads images from containerd.
Expand Down
Loading