From f04024a48b01fe036e1f121b7fffb2db3463125e Mon Sep 17 00:00:00 2001 From: Peng Tao Date: Tue, 4 Sep 2018 15:37:22 +0800 Subject: [PATCH] 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 --- virtcontainers/container.go | 38 +++++++++++++ virtcontainers/container_test.go | 16 ++++-- virtcontainers/noop_resource_storage.go | 4 +- virtcontainers/noop_resource_storage_test.go | 3 +- virtcontainers/sandbox.go | 59 +++++++++++++++++++- virtcontainers/sandbox_test.go | 26 +++++++++ 6 files changed, 136 insertions(+), 10 deletions(-) diff --git a/virtcontainers/container.go b/virtcontainers/container.go index e14d78d37b..3135b0e87a 100644 --- a/virtcontainers/container.go +++ b/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() diff --git a/virtcontainers/container_test.go b/virtcontainers/container_test.go index 68bfd699e1..a41ee327b9 100644 --- a/virtcontainers/container_test.go +++ b/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() diff --git a/virtcontainers/noop_resource_storage.go b/virtcontainers/noop_resource_storage.go index 2c1b931e0c..b67f56c832 100644 --- a/virtcontainers/noop_resource_storage.go +++ b/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 } diff --git a/virtcontainers/noop_resource_storage_test.go b/virtcontainers/noop_resource_storage_test.go index 1e4a696bf6..b5260719bd 100644 --- a/virtcontainers/noop_resource_storage_test.go +++ b/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) } diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index d6fb118108..6dea15e3dd 100644 --- a/virtcontainers/sandbox.go +++ b/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 available 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. diff --git a/virtcontainers/sandbox_test.go b/virtcontainers/sandbox_test.go index ce7d67f276..846404fb51 100644 --- a/virtcontainers/sandbox_test.go +++ b/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) +}