virtcontainers: revert CleanupContainer from PR 1079

In PR 1079, CleanupContainer's parameter of sandboxID is changed to VCSandbox, but at cleanup,
there is no VCSandbox is constructed, we should load it from disk by loadSandboxConfig() in
persist.go. This commit reverts parts of #1079

Fixes: #1119

Signed-off-by: bin liu <bin@hyper.sh>

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{}
 )
 
 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 {