hotplug: fix cpu hotplug counter

The cpu hotplug calculation should take into account the initial vCPUs
counts otherwise we end up hotplugging more vCPUs than actual need.

Fixes: #695

Signed-off-by: Peng Tao <bergwolf@gmail.com>

virtcontainers/container.go

@@ -1201,6 +1201,11 @@ func (c *Container) addResources() error {
 
 	// Container is being created, try to add the number of vCPUs specified
 	vCPUs := c.config.Resources.VCPUs
+	vCPUs, err := c.sandbox.adjustVCPUCount(vCPUs, true)
+	if err != nil {
+		return err
+	}
+
 	if vCPUs != 0 {
 		virtLog.Debugf("hot adding %d vCPUs", vCPUs)
 		data, err := c.sandbox.hypervisor.hotplugAddDevice(vCPUs, cpuDev)
@@ -1213,6 +1218,10 @@ func (c *Container) addResources() error {
 			return fmt.Errorf("Could not get the number of vCPUs added, got %+v", data)
 		}
 
+		if err = c.sandbox.saveSandboxState(); err != nil {
+			return err
+		}
+
 		// A different number of vCPUs was added, we have to update
 		// the resources in order to don't remove vCPUs used by other containers.
 		if vcpusAdded != vCPUs {
@@ -1244,11 +1253,20 @@ func (c *Container) removeResources() error {
 	}
 
 	vCPUs := config.Resources.VCPUs
+	vCPUs, err = c.sandbox.adjustVCPUCount(vCPUs, false)
+	if err != nil {
+		return err
+	}
+
 	if vCPUs != 0 {
 		virtLog.Debugf("hot removing %d vCPUs", vCPUs)
 		if _, err := c.sandbox.hypervisor.hotplugRemoveDevice(vCPUs, cpuDev); err != nil {
 			return err
 		}
+
+		if err = c.sandbox.saveSandboxState(); err != nil {
+			return err
+		}
 	}
 
 	return nil
@@ -1274,6 +1292,14 @@ func (c *Container) updateResources(oldResources, newResources ContainerResource
 	if oldVCPUs < newVCPUs {
 		// hot add vCPUs
 		vCPUs = newVCPUs - oldVCPUs
+		vCPUs, err := c.sandbox.adjustVCPUCount(vCPUs, true)
+		if err != nil {
+			return err
+		}
+		if vCPUs == 0 {
+			return nil
+		}
+
 		virtLog.Debugf("hot adding %d vCPUs", vCPUs)
 		data, err := c.sandbox.hypervisor.hotplugAddDevice(vCPUs, cpuDev)
 		if err != nil {
@@ -1291,6 +1317,14 @@ func (c *Container) updateResources(oldResources, newResources ContainerResource
 	} else {
 		// hot remove vCPUs
 		vCPUs = oldVCPUs - newVCPUs
+		vCPUs, err := c.sandbox.adjustVCPUCount(vCPUs, false)
+		if err != nil {
+			return err
+		}
+		if vCPUs == 0 {
+			return nil
+		}
+
 		virtLog.Debugf("hot removing %d vCPUs", vCPUs)
 		data, err := c.sandbox.hypervisor.hotplugRemoveDevice(vCPUs, cpuDev)
 		if err != nil {
@@ -1304,6 +1338,10 @@ func (c *Container) updateResources(oldResources, newResources ContainerResource
 		newResources.VCPUs = oldVCPUs - vcpusRemoved
 	}
 
+	if err := c.sandbox.saveSandboxState(); err != nil {
+		return err
+	}
+
 	// Set and save container's config
 	c.config.Resources = newResources
 	return c.storeContainer()

virtcontainers/container_test.go

@@ -6,6 +6,7 @@
 package virtcontainers
 
 import (
+	"context"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -286,10 +287,13 @@ func TestContainerAddResources(t *testing.T) {
 
 	c := &Container{
 		sandbox: &Sandbox{
-			storage: &filesystem{},
+			storage: &noopResourceStorage{},
 		},
 	}
-	err := c.addResources()
+	err := c.sandbox.storage.createAllResources(context.Background(), c.sandbox)
+	assert.Nil(err)
+
+	err = c.addResources()
 	assert.Nil(err)
 
 	c.config = &ContainerConfig{Annotations: make(map[string]string)}
@@ -310,7 +314,7 @@ func TestContainerAddResources(t *testing.T) {
 			vCPUs: vCPUs,
 		},
 		agent:   &noopAgent{},
-		storage: &filesystem{},
+		storage: &noopResourceStorage{},
 	}
 	err = c.addResources()
 	assert.Nil(err)
@@ -321,7 +325,8 @@ func TestContainerRemoveResources(t *testing.T) {
 
 	c := &Container{
 		sandbox: &Sandbox{
-			storage: &filesystem{},
+			storage: &noopResourceStorage{},
+			config:  &SandboxConfig{},
 		},
 	}
 
@@ -346,7 +351,8 @@ func TestContainerRemoveResources(t *testing.T) {
 		hypervisor: &mockHypervisor{
 			vCPUs: vCPUs,
 		},
-		storage: &filesystem{},
+		storage: &noopResourceStorage{},
+		config:  &SandboxConfig{},
 	}
 
 	err = c.removeResources()

virtcontainers/noop_resource_storage.go

@@ -6,12 +6,14 @@
 package virtcontainers
 
 import (
+	"context"
+
 	"github.com/kata-containers/runtime/virtcontainers/device/api"
 )
 
 type noopResourceStorage struct{}
 
-func (n *noopResourceStorage) createAllResources(sandbox *Sandbox) error {
+func (n *noopResourceStorage) createAllResources(ctx context.Context, sandbox *Sandbox) error {
 	return nil
 }
 

virtcontainers/noop_resource_storage_test.go

@@ -6,6 +6,7 @@
 package virtcontainers
 
 import (
+	"context"
 	"testing"
 
 	"github.com/kata-containers/runtime/virtcontainers/device/api"
@@ -15,7 +16,7 @@ import (
 func TestNoopCreateAllResources(t *testing.T) {
 	n := &noopResourceStorage{}
 
-	err := n.createAllResources(nil)
+	err := n.createAllResources(context.Background(), nil)
 	assert.Nil(t, err)
 }
 

virtcontainers/sandbox.go

@@ -60,15 +60,19 @@ type State struct {
 	// File system of the rootfs incase it is block device
 	Fstype string `json:"fstype"`
 
-	// Bool to indicate if the drive for a container was hotplugged.
-	HotpluggedDrive bool `json:"hotpluggedDrive"`
-
 	// PCI slot at which the block device backing the container rootfs is attached.
 	RootfsPCIAddr string `json:"rootfsPCIAddr"`
 
 	// Pid is the process id of the sandbox container which is the first
 	// container to be started.
 	Pid int `json:"pid"`
+
+	// Free static vCPUs that can be assigned to individual containers
+	FreeStaticCPU uint32 `json:"freeStaticCpu,omitempty"`
+
+	// Bool to indicate if the drive for a container was hotplugged.
+	// This is moved to bottom of the struct to pass maligned check.
+	HotpluggedDrive bool `json:"hotpluggedDrive"`
 }
 
 // valid checks that the sandbox state is valid.
@@ -763,6 +767,9 @@ func createSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Fac
 		return nil, err
 	}
 
+	// set initial static cpu count
+	s.state.FreeStaticCPU = sandboxConfig.HypervisorConfig.DefaultVCPUs
+
 	// Set sandbox state
 	if err := s.setSandboxState(StateReady); err != nil {
 		return nil, err
@@ -1201,6 +1208,52 @@ func (s *Sandbox) CreateContainer(contConfig ContainerConfig) (VCContainer, erro
 	return c, nil
 }
 
+// Adjust sandbox remaining static vcpu counter
+// When credit is true, hand out avaiable static vCPUs. Otherwise take it back.
+func (s *Sandbox) adjustVCPUCount(num uint32, credit bool) (uint32, error) {
+	if credit {
+		// No initial vCPUs so no adjustment necessary.
+		if s.state.FreeStaticCPU == 0 {
+			return num, nil
+		}
+
+		if num <= s.state.FreeStaticCPU {
+			s.state.FreeStaticCPU -= num
+			num = 0
+		} else {
+			num -= s.state.FreeStaticCPU
+			s.state.FreeStaticCPU = 0
+		}
+	} else {
+		max := s.config.HypervisorConfig.DefaultVCPUs
+		// Static vCPUs full
+		if s.state.FreeStaticCPU == max {
+			return num, nil
+		}
+
+		if s.state.FreeStaticCPU+num <= max {
+			s.state.FreeStaticCPU += num
+			num = 0
+		} else {
+			num = max - s.state.FreeStaticCPU
+			s.state.FreeStaticCPU = max
+		}
+	}
+
+	if num == 0 {
+		if err := s.saveSandboxState(); err != nil {
+			return 0, err
+		}
+	}
+
+	return num, nil
+}
+
+func (s *Sandbox) saveSandboxState() error {
+	// update static vcpu counter
+	return s.storage.storeSandboxResource(s.id, stateFileType, s.state)
+}
+
 // StartContainer starts a container in the sandbox
 func (s *Sandbox) StartContainer(containerID string) (VCContainer, error) {
 	// Fetch the container.

virtcontainers/sandbox_test.go

@@ -1722,3 +1722,29 @@ func TestGetNetNs(t *testing.T) {
 	netNs = s.GetNetNs()
 	assert.Equal(t, netNs, expected)
 }
+
+func TestAdjustVCPUCount(t *testing.T) {
+	assert := assert.New(t)
+	s := Sandbox{
+		storage: &noopResourceStorage{},
+		config:  &SandboxConfig{},
+	}
+
+	s.state.FreeStaticCPU = 10
+
+	num, err := s.adjustVCPUCount(1, true)
+	assert.Nil(err)
+	assert.Equal(num, 0)
+
+	num, err = s.adjustVCPUCount(1, false)
+	assert.Nil(err)
+	assert.Equal(num, 0)
+
+	num, err = s.adjustVCPUCount(11, true)
+	assert.Nil(err)
+	assert.Equal(num, 1)
+
+	num, err = s.adjustVCPUCount(11, false)
+	assert.Nil(err)
+	assert.Equal(num, 1)
+}