From 7a6f2059700508adf260619f740bf99b8f1dbe79 Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Sun, 22 Jul 2018 11:23:38 +0800 Subject: [PATCH] virtcontainers: keep qmp connection when possible For each time a sandbox structure is created, we ensure s.Release() is called. Then we can keep the qmp connection as long as Sandbox pointer is alive. All VC interfaces are still stateless as s.Release() is called before each API returns. OTOH, for VCSandbox APIs, FetchSandbox() must be paired with s.Release, the same as before. Fixes: #500 Signed-off-by: Peng Tao --- virtcontainers/api.go | 35 +++++++++++++++++++++----- virtcontainers/api_test.go | 2 ++ virtcontainers/hypervisor.go | 1 + virtcontainers/mock_hypervisor.go | 3 +++ virtcontainers/mock_hypervisor_test.go | 6 +++++ virtcontainers/qemu.go | 11 +++----- virtcontainers/sandbox.go | 4 +++ 7 files changed, 49 insertions(+), 13 deletions(-) diff --git a/virtcontainers/api.go b/virtcontainers/api.go index abaebb50ed..8c75b095dc 100644 --- a/virtcontainers/api.go +++ b/virtcontainers/api.go @@ -35,7 +35,12 @@ func SetLogger(logger logrus.FieldLogger) { // CreateSandbox is the virtcontainers sandbox creation entry point. // CreateSandbox creates a sandbox and its containers. It does not start them. func CreateSandbox(sandboxConfig SandboxConfig, factory Factory) (VCSandbox, error) { - return createSandboxFromConfig(sandboxConfig, factory) + s, err := createSandboxFromConfig(sandboxConfig, factory) + if err == nil { + s.Release() + } + + return s, err } func createSandboxFromConfig(sandboxConfig SandboxConfig, factory Factory) (*Sandbox, error) { @@ -86,6 +91,7 @@ func DeleteSandbox(sandboxID string) (VCSandbox, error) { if err != nil { return nil, err } + defer s.Release() // Delete it. if err := s.Delete(); err != nil { @@ -97,7 +103,8 @@ func DeleteSandbox(sandboxID string) (VCSandbox, error) { // FetchSandbox is the virtcontainers sandbox fetching entry point. // FetchSandbox will find out and connect to an existing sandbox and -// return the sandbox structure. +// return the sandbox structure. The caller is responsible of calling +// VCSandbox.Release() after done with it. func FetchSandbox(sandboxID string) (VCSandbox, error) { if sandboxID == "" { return nil, errNeedSandboxID @@ -110,21 +117,22 @@ func FetchSandbox(sandboxID string) (VCSandbox, error) { defer unlockSandbox(lockFile) // Fetch the sandbox from storage and create it. - sandbox, err := fetchSandbox(sandboxID) + s, err := fetchSandbox(sandboxID) if err != nil { return nil, err } // If the proxy is KataBuiltInProxyType type, it needs to restart the proxy to watch the // guest console if it hadn't been watched. - if isProxyBuiltIn(sandbox.config.ProxyType) { - err = sandbox.startProxy() + if isProxyBuiltIn(s.config.ProxyType) { + err = s.startProxy() if err != nil { + s.Release() return nil, err } } - return sandbox, nil + return s, nil } // StartSandbox is the virtcontainers sandbox starting entry point. @@ -147,6 +155,7 @@ func StartSandbox(sandboxID string) (VCSandbox, error) { if err != nil { return nil, err } + defer s.Release() return startSandbox(s) } @@ -184,6 +193,7 @@ func StopSandbox(sandboxID string) (VCSandbox, error) { if err != nil { return nil, err } + defer s.Release() // Stop it. err = s.stop() @@ -211,6 +221,7 @@ func RunSandbox(sandboxConfig SandboxConfig, factory Factory) (VCSandbox, error) if err != nil { return nil, err } + defer s.Release() lockFile, err := rwLockSandbox(s.id) if err != nil { @@ -269,6 +280,7 @@ func StatusSandbox(sandboxID string) (SandboxStatus, error) { unlockSandbox(lockFile) return SandboxStatus{}, err } + defer s.Release() // We need to potentially wait for a separate container.stop() routine // that needs to be terminated before we return from this function. @@ -320,6 +332,7 @@ func CreateContainer(sandboxID string, containerConfig ContainerConfig) (VCSandb if err != nil { return nil, nil, err } + defer s.Release() c, err := s.CreateContainer(containerConfig) if err != nil { @@ -351,6 +364,7 @@ func DeleteContainer(sandboxID, containerID string) (VCContainer, error) { if err != nil { return nil, err } + defer s.Release() return s.DeleteContainer(containerID) } @@ -376,6 +390,7 @@ func StartContainer(sandboxID, containerID string) (VCContainer, error) { if err != nil { return nil, err } + defer s.Release() c, err := s.StartContainer(containerID) if err != nil { @@ -406,6 +421,7 @@ func StopContainer(sandboxID, containerID string) (VCContainer, error) { if err != nil { return nil, err } + defer s.Release() // Fetch the container. c, err := s.findContainer(containerID) @@ -443,6 +459,7 @@ func EnterContainer(sandboxID, containerID string, cmd Cmd) (VCSandbox, VCContai if err != nil { return nil, nil, nil, err } + defer s.Release() c, process, err := s.EnterContainer(containerID, cmd) if err != nil { @@ -473,6 +490,7 @@ func StatusContainer(sandboxID, containerID string) (ContainerStatus, error) { unlockSandbox(lockFile) return ContainerStatus{}, err } + defer s.Release() // We need to potentially wait for a separate container.stop() routine // that needs to be terminated before we return from this function. @@ -559,6 +577,7 @@ func KillContainer(sandboxID, containerID string, signal syscall.Signal, all boo if err != nil { return err } + defer s.Release() // Fetch the container. c, err := s.findContainer(containerID) @@ -608,6 +627,7 @@ func ProcessListContainer(sandboxID, containerID string, options ProcessListOpti if err != nil { return nil, err } + defer s.Release() // Fetch the container. c, err := s.findContainer(containerID) @@ -639,6 +659,7 @@ func UpdateContainer(sandboxID, containerID string, resources specs.LinuxResourc if err != nil { return err } + defer s.Release() return s.UpdateContainer(containerID, resources) } @@ -664,6 +685,7 @@ func StatsContainer(sandboxID, containerID string) (ContainerStats, error) { if err != nil { return ContainerStats{}, err } + defer s.Release() return s.StatsContainer(containerID) } @@ -687,6 +709,7 @@ func togglePauseContainer(sandboxID, containerID string, pause bool) error { if err != nil { return err } + defer s.Release() // Fetch the container. c, err := s.findContainer(containerID) diff --git a/virtcontainers/api_test.go b/virtcontainers/api_test.go index c7a5ebe8d3..40c1a92377 100644 --- a/virtcontainers/api_test.go +++ b/virtcontainers/api_test.go @@ -1811,6 +1811,7 @@ func TestStatusContainerStateReady(t *testing.T) { if err != nil { t.Fatal(err) } + defer p2.Release() expectedStatus := ContainerStatus{ ID: contID, @@ -1884,6 +1885,7 @@ func TestStatusContainerStateRunning(t *testing.T) { if err != nil { t.Fatal(err) } + defer p2.Release() expectedStatus := ContainerStatus{ ID: contID, diff --git a/virtcontainers/hypervisor.go b/virtcontainers/hypervisor.go index 81b27c5090..1e8a04e12e 100644 --- a/virtcontainers/hypervisor.go +++ b/virtcontainers/hypervisor.go @@ -547,5 +547,6 @@ type hypervisor interface { hotplugAddDevice(devInfo interface{}, devType deviceType) (interface{}, error) hotplugRemoveDevice(devInfo interface{}, devType deviceType) (interface{}, error) getSandboxConsole(sandboxID string) (string, error) + disconnect() capabilities() capabilities } diff --git a/virtcontainers/mock_hypervisor.go b/virtcontainers/mock_hypervisor.go index 38f9a44d7a..85e599ebc3 100644 --- a/virtcontainers/mock_hypervisor.go +++ b/virtcontainers/mock_hypervisor.go @@ -73,3 +73,6 @@ func (m *mockHypervisor) hotplugRemoveDevice(devInfo interface{}, devType device func (m *mockHypervisor) getSandboxConsole(sandboxID string) (string, error) { return "", nil } + +func (m *mockHypervisor) disconnect() { +} diff --git a/virtcontainers/mock_hypervisor_test.go b/virtcontainers/mock_hypervisor_test.go index 194939dc75..ccde7c2ab5 100644 --- a/virtcontainers/mock_hypervisor_test.go +++ b/virtcontainers/mock_hypervisor_test.go @@ -104,3 +104,9 @@ func TestMockHypervisorSaveSandbox(t *testing.T) { t.Fatal(err) } } + +func TestMockHypervisorDisconnect(t *testing.T) { + var m *mockHypervisor + + m.disconnect() +} diff --git a/virtcontainers/qemu.go b/virtcontainers/qemu.go index 1664da5f1e..1d84fa88d7 100644 --- a/virtcontainers/qemu.go +++ b/virtcontainers/qemu.go @@ -560,7 +560,6 @@ func (q *qemu) stopSandbox() error { if err != nil { return err } - defer q.qmpShutdown() err = q.qmpMonitorCh.qmp.ExecuteQuit(q.qmpMonitorCh.ctx) if err != nil { @@ -581,7 +580,6 @@ func (q *qemu) togglePauseSandbox(pause bool) error { if err != nil { return err } - defer q.qmpShutdown() if pause { err = q.qmpMonitorCh.qmp.ExecuteStop(q.qmpMonitorCh.ctx) @@ -663,7 +661,6 @@ func (q *qemu) hotplugBlockDevice(drive *deviceDrivers.Drive, op operation) erro if err != nil { return err } - defer q.qmpShutdown() devID := "virtio-" + drive.ID @@ -725,7 +722,6 @@ func (q *qemu) hotplugVFIODevice(device deviceDrivers.VFIODevice, op operation) if err != nil { return err } - defer q.qmpShutdown() devID := "vfio-" + device.DeviceInfo.ID @@ -800,7 +796,6 @@ func (q *qemu) hotplugCPUs(vcpus uint32, op operation) (uint32, error) { if err != nil { return 0, err } - defer q.qmpShutdown() if op == addDevice { return q.hotplugAddCPUs(vcpus) @@ -928,7 +923,6 @@ func (q *qemu) hotplugAddMemory(memDev *memoryDevice) error { if err != nil { return err } - defer q.qmpShutdown() err = q.qmpMonitorCh.qmp.ExecHotplugMemory(q.qmpMonitorCh.ctx, "memory-backend-ram", "mem"+strconv.Itoa(memDev.slot), "", memDev.sizeMB) if err != nil { @@ -989,7 +983,6 @@ func (q *qemu) saveSandbox() error { if err != nil { return err } - defer q.qmpShutdown() // BootToBeTemplate sets the VM to be a template that other VMs can clone from. We would want to // bypass shared memory when saving the VM to a local file through migration exec. @@ -1015,6 +1008,10 @@ func (q *qemu) saveSandbox() error { return nil } +func (q *qemu) disconnect() { + q.qmpShutdown() +} + // genericAppendBridges appends to devices the given bridges func genericAppendBridges(devices []govmmQemu.Device, bridges []Bridge, machineType string) []govmmQemu.Device { bus := defaultPCBridgeBus diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index 316dbd72cc..b175f2ef20 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -541,10 +541,12 @@ func (s *Sandbox) GetContainer(containerID string) VCContainer { // Release closes the agent connection and removes sandbox from internal list. func (s *Sandbox) Release() error { + s.Logger().Info("release sandbox") globalSandboxList.removeSandbox(s.id) if s.monitor != nil { s.monitor.stop() } + s.hypervisor.disconnect() return s.agent.disconnect() } @@ -808,6 +810,7 @@ func (s *Sandbox) storeSandbox() error { // fetchSandbox fetches a sandbox config from a sandbox ID and returns a sandbox. func fetchSandbox(sandboxID string) (sandbox *Sandbox, err error) { + virtLog.WithField("sandbox-id", sandboxID).Info("fetch sandbox") if sandboxID == "" { return nil, errNeedSandboxID } @@ -1388,6 +1391,7 @@ func togglePauseSandbox(sandboxID string, pause bool) (*Sandbox, error) { if err != nil { return nil, err } + defer s.Release() if pause { err = s.Pause()