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 <bergwolf@gmail.com>

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) {
+	if isProxyBuiltIn(s.config.ProxyType) {
-		err = sandbox.startProxy()
+		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)

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,

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
 }

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

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()
+}

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

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()