From fdbf7d3222645f7dd8f5da3f04fd1326cf861a07 Mon Sep 17 00:00:00 2001 From: bin liu Date: Mon, 16 Nov 2020 17:40:18 +0800 Subject: [PATCH] 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 --- src/runtime/containerd-shim-v2/service.go | 6 +- src/runtime/containerd-shim-v2/utils.go | 4 +- src/runtime/virtcontainers/api.go | 19 +-- src/runtime/virtcontainers/api_test.go | 2 +- src/runtime/virtcontainers/implementation.go | 4 +- src/runtime/virtcontainers/interfaces.go | 2 +- src/runtime/virtcontainers/persist.go | 116 ++++++++++++++++++ src/runtime/virtcontainers/pkg/vcmock/mock.go | 6 +- .../virtcontainers/pkg/vcmock/mock_test.go | 22 ++-- .../virtcontainers/pkg/vcmock/types.go | 2 +- src/runtime/virtcontainers/sandbox.go | 67 ++++++++++ src/runtime/virtcontainers/sandbox_test.go | 54 +++++++- 12 files changed, 269 insertions(+), 35 deletions(-) diff --git a/src/runtime/containerd-shim-v2/service.go b/src/runtime/containerd-shim-v2/service.go index 2903824779..795f27e4b5 100644 --- a/src/runtime/containerd-shim-v2/service.go +++ b/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 } diff --git a/src/runtime/containerd-shim-v2/utils.go b/src/runtime/containerd-shim-v2/utils.go index 3172483500..05e883c5d4 100644 --- a/src/runtime/containerd-shim-v2/utils.go +++ b/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 diff --git a/src/runtime/virtcontainers/api.go b/src/runtime/virtcontainers/api.go index 5c3f402108..951ee8a9b2 100644 --- a/src/runtime/virtcontainers/api.go +++ b/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) diff --git a/src/runtime/virtcontainers/api_test.go b/src/runtime/virtcontainers/api_test.go index f0447b3ffb..927e450c8d 100644 --- a/src/runtime/virtcontainers/api_test.go +++ b/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) diff --git a/src/runtime/virtcontainers/implementation.go b/src/runtime/virtcontainers/implementation.go index 186b59e87a..177797ebd2 100644 --- a/src/runtime/virtcontainers/implementation.go +++ b/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) } diff --git a/src/runtime/virtcontainers/interfaces.go b/src/runtime/virtcontainers/interfaces.go index 8aeed1498f..365c329db0 100644 --- a/src/runtime/virtcontainers/interfaces.go +++ b/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 diff --git a/src/runtime/virtcontainers/persist.go b/src/runtime/virtcontainers/persist.go index a842df4404..78c200bc99 100644 --- a/src/runtime/virtcontainers/persist.go +++ b/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 +} diff --git a/src/runtime/virtcontainers/pkg/vcmock/mock.go b/src/runtime/virtcontainers/pkg/vcmock/mock.go index 0dd2b579f6..3b18151669 100644 --- a/src/runtime/virtcontainers/pkg/vcmock/mock.go +++ b/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) } diff --git a/src/runtime/virtcontainers/pkg/vcmock/mock_test.go b/src/runtime/virtcontainers/pkg/vcmock/mock_test.go index 79c543b3cd..9043b168da 100644 --- a/src/runtime/virtcontainers/pkg/vcmock/mock_test.go +++ b/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)) } diff --git a/src/runtime/virtcontainers/pkg/vcmock/types.go b/src/runtime/virtcontainers/pkg/vcmock/types.go index de7f034421..8c6cd4f512 100644 --- a/src/runtime/virtcontainers/pkg/vcmock/types.go +++ b/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 } diff --git a/src/runtime/virtcontainers/sandbox.go b/src/runtime/virtcontainers/sandbox.go index 118ece60e0..e838d28174 100644 --- a/src/runtime/virtcontainers/sandbox.go +++ b/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 +} diff --git a/src/runtime/virtcontainers/sandbox_test.go b/src/runtime/virtcontainers/sandbox_test.go index 24458a2117..b16b2cf718 100644 --- a/src/runtime/virtcontainers/sandbox_test.go +++ b/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 {