Merge pull request #1120 from liubin/fix/1119-revert-cleanupcontainer-api

virtcontainers: revert CleanupContainer from PR 1079

src/runtime/containerd-shim-v2/service.go

@@ -312,17 +312,17 @@ func (s *service) Cleanup(ctx context.Context) (_ *taskAPI.DeleteResponse, err e
 
 	switch containerType {
 	case vc.PodSandbox:
-		err = cleanupContainer(ctx, s.sandbox, s.id, path)
+		err = cleanupContainer(ctx, s.id, s.id, path)
 		if err != nil {
 			return nil, err
 		}
 	case vc.PodContainer:
-		_, err := oci.SandboxID(ociSpec)
+		sandboxID, err := oci.SandboxID(ociSpec)
 		if err != nil {
 			return nil, err
 		}
 
-		err = cleanupContainer(ctx, s.sandbox, s.id, path)
+		err = cleanupContainer(ctx, sandboxID, s.id, path)
 		if err != nil {
 			return nil, err
 		}

src/runtime/containerd-shim-v2/utils.go

@@ -31,10 +31,10 @@ func cReap(s *service, status int, id, execid string, exitat time.Time) {
 	}
 }
 
-func cleanupContainer(ctx context.Context, sandbox vc.VCSandbox, cid, bundlePath string) error {
+func cleanupContainer(ctx context.Context, sandboxID, cid, bundlePath string) error {
 	shimLog.WithField("service", "cleanup").WithField("container", cid).Info("Cleanup container")
 
-	err := vci.CleanupContainer(ctx, sandbox, cid, true)
+	err := vci.CleanupContainer(ctx, sandboxID, cid, true)
 	if err != nil {
 		shimLog.WithError(err).WithField("container", cid).Warn("failed to cleanup container")
 		return err

src/runtime/virtcontainers/api.go

@@ -7,7 +7,6 @@ package virtcontainers
 
 import (
 	"context"
-	"fmt"
 	"runtime"
 
 	deviceApi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/api"
@@ -136,24 +135,28 @@ func createSandboxFromConfig(ctx context.Context, sandboxConfig SandboxConfig, f
 // CleanupContainer is used by shimv2 to stop and delete a container exclusively, once there is no container
 // in the sandbox left, do stop the sandbox and delete it. Those serial operations will be done exclusively by
 // locking the sandbox.
-func CleanupContainer(ctx context.Context, sandbox VCSandbox, containerID string, force bool) error {
+func CleanupContainer(ctx context.Context, sandboxID, containerID string, force bool) error {
 	span, ctx := trace(ctx, "CleanupContainer")
 	defer span.Finish()
+
+	if sandboxID == "" {
+		return vcTypes.ErrNeedSandboxID
+	}
+
 	if containerID == "" {
 		return vcTypes.ErrNeedContainerID
 	}
 
-	s, ok := sandbox.(*Sandbox)
-	if !ok {
-		return fmt.Errorf("not a Sandbox reference")
-	}
-
-	unlock, err := rwLockSandbox(s.id)
+	unlock, err := rwLockSandbox(sandboxID)
 	if err != nil {
 		return err
 	}
 	defer unlock()
 
+	s, err := fetchSandbox(ctx, sandboxID)
+	if err != nil {
+		return err
+	}
 	defer s.Release()
 
 	_, err = s.StopContainer(containerID, force)

src/runtime/virtcontainers/api_test.go

@@ -307,7 +307,7 @@ func TestCleanupContainer(t *testing.T) {
 	}
 
 	for _, c := range p.GetAllContainers() {
-		CleanupContainer(ctx, p, c.ID(), true)
+		CleanupContainer(ctx, p.ID(), c.ID(), true)
 	}
 
 	s, ok := p.(*Sandbox)

src/runtime/virtcontainers/implementation.go

@@ -38,6 +38,6 @@ func (impl *VCImpl) CreateSandbox(ctx context.Context, sandboxConfig SandboxConf
 // CleanupContainer is used by shimv2 to stop and delete a container exclusively, once there is no container
 // in the sandbox left, do stop the sandbox and delete it. Those serial operations will be done exclusively by
 // locking the sandbox.
-func (impl *VCImpl) CleanupContainer(ctx context.Context, sandbox VCSandbox, containerID string, force bool) error {
-	return CleanupContainer(ctx, sandbox, containerID, force)
+func (impl *VCImpl) CleanupContainer(ctx context.Context, sandboxID, containerID string, force bool) error {
+	return CleanupContainer(ctx, sandboxID, containerID, force)
 }

src/runtime/virtcontainers/interfaces.go

@@ -24,7 +24,7 @@ type VC interface {
 	SetFactory(ctx context.Context, factory Factory)
 
 	CreateSandbox(ctx context.Context, sandboxConfig SandboxConfig) (VCSandbox, error)
-	CleanupContainer(ctx context.Context, sandbox VCSandbox, containerID string, force bool) error
+	CleanupContainer(ctx context.Context, sandboxID, containerID string, force bool) error
 }
 
 // VCSandbox is the Sandbox interface

src/runtime/virtcontainers/persist.go

@@ -9,6 +9,8 @@ import (
 	"errors"
 
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/api"
+	exp "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/experimental"
+	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist"
 	persistapi "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/api"
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/types"
 )
@@ -426,3 +428,117 @@ func (c *Container) Restore() error {
 	c.loadContMounts(cs)
 	return nil
 }
+
+func loadSandboxConfig(id string) (*SandboxConfig, error) {
+	store, err := persist.GetDriver()
+	if err != nil || store == nil {
+		return nil, errors.New("failed to get fs persist driver")
+	}
+
+	ss, _, err := store.FromDisk(id)
+	if err != nil {
+		return nil, err
+	}
+
+	savedConf := ss.Config
+	sconfig := &SandboxConfig{
+		ID:             id,
+		HypervisorType: HypervisorType(savedConf.HypervisorType),
+		NetworkConfig: NetworkConfig{
+			NetNSPath:         savedConf.NetworkConfig.NetNSPath,
+			NetNsCreated:      savedConf.NetworkConfig.NetNsCreated,
+			DisableNewNetNs:   savedConf.NetworkConfig.DisableNewNetNs,
+			InterworkingModel: NetInterworkingModel(savedConf.NetworkConfig.InterworkingModel),
+		},
+
+		ShmSize:             savedConf.ShmSize,
+		SharePidNs:          savedConf.SharePidNs,
+		SystemdCgroup:       savedConf.SystemdCgroup,
+		SandboxCgroupOnly:   savedConf.SandboxCgroupOnly,
+		DisableGuestSeccomp: savedConf.DisableGuestSeccomp,
+		Cgroups:             savedConf.Cgroups,
+	}
+
+	for _, name := range savedConf.Experimental {
+		sconfig.Experimental = append(sconfig.Experimental, *exp.Get(name))
+	}
+
+	hconf := savedConf.HypervisorConfig
+	sconfig.HypervisorConfig = HypervisorConfig{
+		NumVCPUs:                hconf.NumVCPUs,
+		DefaultMaxVCPUs:         hconf.DefaultMaxVCPUs,
+		MemorySize:              hconf.MemorySize,
+		DefaultBridges:          hconf.DefaultBridges,
+		Msize9p:                 hconf.Msize9p,
+		MemSlots:                hconf.MemSlots,
+		MemOffset:               hconf.MemOffset,
+		VirtioMem:               hconf.VirtioMem,
+		VirtioFSCacheSize:       hconf.VirtioFSCacheSize,
+		KernelPath:              hconf.KernelPath,
+		ImagePath:               hconf.ImagePath,
+		InitrdPath:              hconf.InitrdPath,
+		FirmwarePath:            hconf.FirmwarePath,
+		MachineAccelerators:     hconf.MachineAccelerators,
+		CPUFeatures:             hconf.CPUFeatures,
+		HypervisorPath:          hconf.HypervisorPath,
+		HypervisorPathList:      hconf.HypervisorPathList,
+		HypervisorCtlPath:       hconf.HypervisorCtlPath,
+		HypervisorCtlPathList:   hconf.HypervisorCtlPathList,
+		JailerPath:              hconf.JailerPath,
+		JailerPathList:          hconf.JailerPathList,
+		BlockDeviceDriver:       hconf.BlockDeviceDriver,
+		HypervisorMachineType:   hconf.HypervisorMachineType,
+		MemoryPath:              hconf.MemoryPath,
+		DevicesStatePath:        hconf.DevicesStatePath,
+		EntropySource:           hconf.EntropySource,
+		SharedFS:                hconf.SharedFS,
+		VirtioFSDaemon:          hconf.VirtioFSDaemon,
+		VirtioFSDaemonList:      hconf.VirtioFSDaemonList,
+		VirtioFSCache:           hconf.VirtioFSCache,
+		VirtioFSExtraArgs:       hconf.VirtioFSExtraArgs[:],
+		BlockDeviceCacheSet:     hconf.BlockDeviceCacheSet,
+		BlockDeviceCacheDirect:  hconf.BlockDeviceCacheDirect,
+		BlockDeviceCacheNoflush: hconf.BlockDeviceCacheNoflush,
+		DisableBlockDeviceUse:   hconf.DisableBlockDeviceUse,
+		EnableIOThreads:         hconf.EnableIOThreads,
+		Debug:                   hconf.Debug,
+		MemPrealloc:             hconf.MemPrealloc,
+		HugePages:               hconf.HugePages,
+		FileBackedMemRootDir:    hconf.FileBackedMemRootDir,
+		FileBackedMemRootList:   hconf.FileBackedMemRootList,
+		Realtime:                hconf.Realtime,
+		Mlock:                   hconf.Mlock,
+		DisableNestingChecks:    hconf.DisableNestingChecks,
+		DisableImageNvdimm:      hconf.DisableImageNvdimm,
+		HotplugVFIOOnRootBus:    hconf.HotplugVFIOOnRootBus,
+		PCIeRootPort:            hconf.PCIeRootPort,
+		BootToBeTemplate:        hconf.BootToBeTemplate,
+		BootFromTemplate:        hconf.BootFromTemplate,
+		DisableVhostNet:         hconf.DisableVhostNet,
+		EnableVhostUserStore:    hconf.EnableVhostUserStore,
+		VhostUserStorePath:      hconf.VhostUserStorePath,
+		VhostUserStorePathList:  hconf.VhostUserStorePathList,
+		GuestHookPath:           hconf.GuestHookPath,
+		VMid:                    hconf.VMid,
+		RxRateLimiterMaxRate:    hconf.RxRateLimiterMaxRate,
+		TxRateLimiterMaxRate:    hconf.TxRateLimiterMaxRate,
+		SGXEPCSize:              hconf.SGXEPCSize,
+		EnableAnnotations:       hconf.EnableAnnotations,
+	}
+
+	sconfig.AgentConfig = KataAgentConfig{
+		LongLiveConn: savedConf.KataAgentConfig.LongLiveConn,
+	}
+
+	for _, contConf := range savedConf.ContainerConfigs {
+		sconfig.Containers = append(sconfig.Containers, ContainerConfig{
+			ID:          contConf.ID,
+			Annotations: contConf.Annotations,
+			Resources:   contConf.Resources,
+			RootFs: RootFs{
+				Target: contConf.RootFs,
+			},
+		})
+	}
+	return sconfig, nil
+}

src/runtime/virtcontainers/pkg/vcmock/mock.go

@@ -50,9 +50,9 @@ func (m *VCMock) CreateSandbox(ctx context.Context, sandboxConfig vc.SandboxConf
 	return nil, fmt.Errorf("%s: %s (%+v): sandboxConfig: %v", mockErrorPrefix, getSelf(), m, sandboxConfig)
 }
 
-func (m *VCMock) CleanupContainer(ctx context.Context, sandbox vc.VCSandbox, containerID string, force bool) error {
+func (m *VCMock) CleanupContainer(ctx context.Context, sandboxID, containerID string, force bool) error {
 	if m.CleanupContainerFunc != nil {
-		return m.CleanupContainerFunc(ctx, sandbox, containerID, true)
+		return m.CleanupContainerFunc(ctx, sandboxID, containerID, true)
 	}
-	return fmt.Errorf("%s: %s (%+v): sandbox: %v", mockErrorPrefix, getSelf(), m, sandbox)
+	return fmt.Errorf("%s: %s (%+v): sandboxID: %v", mockErrorPrefix, getSelf(), m, sandboxID)
 }

src/runtime/virtcontainers/pkg/vcmock/mock_test.go

@@ -17,13 +17,13 @@ import (
 )
 
 const (
+	testSandboxID   = "testSandboxID"
 	testContainerID = "testContainerID"
 )
 
 var (
-	loggerTriggered               = 0
-	factoryTriggered              = 0
-	testSandbox      vc.VCSandbox = &Sandbox{}
+	loggerTriggered  = 0
+	factoryTriggered = 0
 )
 
 func TestVCImplementations(t *testing.T) {
@@ -178,21 +178,21 @@ func TestVCMockCleanupContainer(t *testing.T) {
 	assert.Nil(m.CleanupContainerFunc)
 
 	ctx := context.Background()
-	err := m.CleanupContainer(ctx, testSandbox, testContainerID, false)
+	err := m.CleanupContainer(ctx, testSandboxID, testContainerID, false)
 	assert.Error(err)
 	assert.True(IsMockError(err))
 
-	m.CleanupContainerFunc = func(ctx context.Context, sandbox vc.VCSandbox, containerID string, force bool) error {
+	m.CleanupContainerFunc = func(ctx context.Context, sandboxID, containerID string, force bool) error {
 		return nil
 	}
 
-	err = m.CleanupContainer(ctx, testSandbox, testContainerID, false)
+	err = m.CleanupContainer(ctx, testSandboxID, testContainerID, false)
 	assert.NoError(err)
 
 	// reset
 	m.CleanupContainerFunc = nil
 
-	err = m.CleanupContainer(ctx, testSandbox, testContainerID, false)
+	err = m.CleanupContainer(ctx, testSandboxID, testContainerID, false)
 	assert.Error(err)
 	assert.True(IsMockError(err))
 }
@@ -204,21 +204,21 @@ func TestVCMockForceCleanupContainer(t *testing.T) {
 	assert.Nil(m.CleanupContainerFunc)
 
 	ctx := context.Background()
-	err := m.CleanupContainer(ctx, testSandbox, testContainerID, true)
+	err := m.CleanupContainer(ctx, testSandboxID, testContainerID, true)
 	assert.Error(err)
 	assert.True(IsMockError(err))
 
-	m.CleanupContainerFunc = func(ctx context.Context, sandbox vc.VCSandbox, containerID string, force bool) error {
+	m.CleanupContainerFunc = func(ctx context.Context, sandboxID, containerID string, force bool) error {
 		return nil
 	}
 
-	err = m.CleanupContainer(ctx, testSandbox, testContainerID, true)
+	err = m.CleanupContainer(ctx, testSandboxID, testContainerID, true)
 	assert.NoError(err)
 
 	// reset
 	m.CleanupContainerFunc = nil
 
-	err = m.CleanupContainer(ctx, testSandbox, testContainerID, true)
+	err = m.CleanupContainer(ctx, testSandboxID, testContainerID, true)
 	assert.Error(err)
 	assert.True(IsMockError(err))
 }

src/runtime/virtcontainers/pkg/vcmock/types.go

@@ -88,5 +88,5 @@ type VCMock struct {
 	SetFactoryFunc func(ctx context.Context, factory vc.Factory)
 
 	CreateSandboxFunc    func(ctx context.Context, sandboxConfig vc.SandboxConfig) (vc.VCSandbox, error)
-	CleanupContainerFunc func(ctx context.Context, sandbox vc.VCSandbox, containerID string, force bool) error
+	CleanupContainerFunc func(ctx context.Context, sandboxID, containerID string, force bool) error
 }

src/runtime/virtcontainers/sandbox.go

@@ -38,6 +38,7 @@ import (
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/agent/protocols/grpc"
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/annotations"
 	vccgroups "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/cgroups"
+	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/compatoci"
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/cpuset"
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/rootless"
 	vcTypes "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/types"
@@ -2260,3 +2261,69 @@ func (s *Sandbox) getSandboxCPUSet() (string, string, error) {
 
 	return cpuResult.String(), memResult.String(), nil
 }
+
+// fetchSandbox fetches a sandbox config from a sandbox ID and returns a sandbox.
+func fetchSandbox(ctx context.Context, sandboxID string) (sandbox *Sandbox, err error) {
+	virtLog.Info("fetch sandbox")
+	if sandboxID == "" {
+		return nil, vcTypes.ErrNeedSandboxID
+	}
+
+	var config SandboxConfig
+
+	// load sandbox config fromld store.
+	c, err := loadSandboxConfig(sandboxID)
+	if err != nil {
+		virtLog.WithError(err).Warning("failed to get sandbox config from new store")
+		return nil, err
+	}
+
+	config = *c
+
+	// fetchSandbox is not suppose to create new sandbox VM.
+	sandbox, err = createSandbox(ctx, config, nil)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create sandbox with config %+v: %v", config, err)
+	}
+
+	if sandbox.config.SandboxCgroupOnly {
+		if err := sandbox.createCgroupManager(); err != nil {
+			return nil, err
+		}
+	}
+
+	// This sandbox already exists, we don't need to recreate the containers in the guest.
+	// We only need to fetch the containers from storage and create the container structs.
+	if err := sandbox.fetchContainers(); err != nil {
+		return nil, err
+	}
+
+	return sandbox, nil
+}
+
+// fetchContainers creates new containers structure and
+// adds them to the sandbox. It does not create the containers
+// in the guest. This should only be used when fetching a
+// sandbox that already exists.
+func (s *Sandbox) fetchContainers() error {
+	for i, contConfig := range s.config.Containers {
+		// Add spec from bundle path
+		spec, err := compatoci.GetContainerSpec(contConfig.Annotations)
+		if err != nil {
+			return err
+		}
+		contConfig.CustomSpec = &spec
+		s.config.Containers[i] = contConfig
+
+		c, err := newContainer(s, &s.config.Containers[i])
+		if err != nil {
+			return err
+		}
+
+		if err := s.addContainer(c); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}

src/runtime/virtcontainers/sandbox_test.go

@@ -24,7 +24,9 @@ import (
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/device/manager"
 	exp "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/experimental"
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/persist/fs"
+
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/annotations"
+	vcAnnotations "github.com/kata-containers/kata-containers/src/runtime/virtcontainers/pkg/annotations"
 	"github.com/kata-containers/kata-containers/src/runtime/virtcontainers/types"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
@@ -249,12 +251,55 @@ func testCheckContainerOnDiskState(c *Container, containerState types.ContainerS
 	return nil
 }
 
+// writeContainerConfig write config.json to bundle path
+// and return bundle path.
+// NOTE: don't forget to delete the bundle path
+func writeContainerConfig() (string, error) {
+
+	basicSpec := `
+{
+	"ociVersion": "1.0.0-rc2-dev",
+	"process": {
+		"capabilities": [
+		]
+	}
+}`
+
+	configDir, err := ioutil.TempDir("", "vc-tmp-")
+	if err != nil {
+		return "", err
+	}
+
+	err = os.MkdirAll(configDir, DirMode)
+	if err != nil {
+		return "", err
+	}
+
+	configFilePath := filepath.Join(configDir, "config.json")
+	err = ioutil.WriteFile(configFilePath, []byte(basicSpec), 0644)
+	if err != nil {
+		return "", err
+	}
+
+	return configDir, nil
+}
+
 func TestSandboxSetSandboxAndContainerState(t *testing.T) {
 	contID := "505"
 	contConfig := newTestContainerConfigNoop(contID)
-	hConfig := newHypervisorConfig(nil, nil)
 	assert := assert.New(t)
 
+	configDir, err := writeContainerConfig()
+	if err != nil {
+		os.RemoveAll(configDir)
+	}
+	assert.NoError(err)
+
+	// set bundle path annotation, fetchSandbox need this annotation to get containers
+	contConfig.Annotations[vcAnnotations.BundlePathKey] = configDir
+
+	hConfig := newHypervisorConfig(nil, nil)
+
 	// create a sandbox
 	p, err := testCreateSandbox(t, testSandboxID, MockHypervisor, hConfig, NetworkConfig{}, []ContainerConfig{contConfig}, nil)
 	assert.NoError(err)
@@ -301,11 +346,14 @@ func TestSandboxSetSandboxAndContainerState(t *testing.T) {
 	}
 
 	// force state to be read from disk
-	if err := testCheckSandboxOnDiskState(p, newSandboxState); err != nil {
+	p2, err := fetchSandbox(context.Background(), p.ID())
+	assert.NoError(err)
+
+	if err := testCheckSandboxOnDiskState(p2, newSandboxState); err != nil {
 		t.Error(err)
 	}
 
-	c2, err := p.findContainer(contID)
+	c2, err := p2.findContainer(contID)
 	assert.NoError(err)
 
 	if err := testCheckContainerOnDiskState(c2, newContainerState); err != nil {