Merge pull request #55841 from ConnorDoyle/cpuman-file-state-for-none-policy

Automatic merge from submit-queue (batch tested with PRs 55841, 55948, 55945). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. CPU Manager: file state for all policies **What this PR does / why we need it**: Before this change, the new file-backed state was only enabled for the static CPU manager policy. This patch enables persistent state for all policies. This PR fixes #55736 and the potential CPU resource leak described in that issue. **Release note**: ```release-note NONE ``` /kind bug /sig node /assign @balajismaniam

Merge pull request #55841 from ConnorDoyle/cpuman-file-state-for-none-policy
869b5ab1 · Kubernetes Submit Queue · GitHub · c60b35bc · c95ee342 · 869b5ab1
Unverified Commit 869b5ab1 authored Nov 18, 2017 by Kubernetes Submit Queue Committed by GitHub Nov 18, 2017
Hide whitespace changes
Inline Side-by-side

Showing with 31 additions and 6 deletions

cpu_manager.go pkg/kubelet/cm/cpumanager/cpu_manager.go +5 -5

state_file_test.go pkg/kubelet/cm/cpumanager/state/state_file_test.go +26 -1

No files found.
--- a/pkg/kubelet/cm/cpumanager/cpu_manager.go
+++ b/pkg/kubelet/cm/cpumanager/cpu_manager.go
@@ -106,13 +106,11 @@ func NewManager(
 	stateFileDirecory string,
 ) (Manager, error) {
 	var policy Policy
-	var stateHandle state.State

 	switch policyName(cpuPolicyName) {

 	case PolicyNone:
 		policy = NewNonePolicy()
-		stateHandle = state.NewMemoryState()

 	case PolicyStatic:
 		topo, err := topology.Discover(machineInfo)
@@ -141,18 +139,20 @@ func NewManager(
 		reservedCPUsFloat := float64(reservedCPUs.MilliValue()) / 1000
 		numReservedCPUs := int(math.Ceil(reservedCPUsFloat))
 		policy = NewStaticPolicy(topo, numReservedCPUs)
-		stateHandle = state.NewFileState(path.Join(stateFileDirecory, CPUManagerStateFileName), policy.Name())

 	default:
 		glog.Errorf("[cpumanager] Unknown policy \"%s\", falling back to default policy \"%s\"", cpuPolicyName, PolicyNone)
 		policy = NewNonePolicy()
-		stateHandle = state.NewMemoryState()
 	}

+	stateImpl := state.NewFileState(
+		path.Join(stateFileDirecory, CPUManagerStateFileName),
+		policy.Name())
+
 	manager := &manager{
 		policy:                     policy,
 		reconcilePeriod:            reconcilePeriod,
-		state:                      stateHandle,
+		state:                      stateImpl,
 		machineInfo:                machineInfo,
 		nodeAllocatableReservation: nodeAllocatableReservation,
 	}

--- a/pkg/kubelet/cm/cpumanager/state/state_file_test.go
+++ b/pkg/kubelet/cm/cpumanager/state/state_file_test.go
@@ -75,12 +75,14 @@ func TestFileStateTryRestore(t *testing.T) {
 	testCases := []struct {
 		description      string
 		stateFileContent string
+		policyName       string
 		expErr           string
 		expectedState    *stateMemory
 	}{
 		{
 			"Invalid JSON - empty file",
 			"\n",
+			"none",
 			"state file: could not unmarshal, corrupted state file",
 			&stateMemory{
 				assignments:   ContainerCPUAssignments{},
@@ -90,6 +92,7 @@ func TestFileStateTryRestore(t *testing.T) {
 		{
 			"Invalid JSON - invalid content",
 			"{",
+			"none",
 			"state file: could not unmarshal, corrupted state file",
 			&stateMemory{
 				assignments:   ContainerCPUAssignments{},
@@ -99,6 +102,7 @@ func TestFileStateTryRestore(t *testing.T) {
 		{
 			"Try restore defaultCPUSet only",
 			`{"policyName": "none", "defaultCpuSet": "4-6"}`,
+			"none",
 			"",
 			&stateMemory{
 				assignments:   ContainerCPUAssignments{},
@@ -108,6 +112,7 @@ func TestFileStateTryRestore(t *testing.T) {
 		{
 			"Try restore defaultCPUSet only - invalid name",
 			`{"policyName": "none", "defaultCpuSet" "4-6"}`,
+			"none",
 			"",
 			&stateMemory{
 				assignments:   ContainerCPUAssignments{},
@@ -123,6 +128,7 @@ func TestFileStateTryRestore(t *testing.T) {
 					"container2": "1-3"
 				}
 			}`,
+			"none",
 			"",
 			&stateMemory{
 				assignments: ContainerCPUAssignments{
@@ -133,8 +139,23 @@ func TestFileStateTryRestore(t *testing.T) {
 			},
 		},
 		{
+			"Try restore invalid policy name",
+			`{
+				"policyName": "A",
+				"defaultCpuSet": "0-7",
+				"entries": {}
+			}`,
+			"B",
+			"policy configured \"B\" != policy from state file \"A\"",
+			&stateMemory{
+				assignments:   ContainerCPUAssignments{},
+				defaultCPUSet: cpuset.NewCPUSet(),
+			},
+		},
+		{
 			"Try restore invalid assignments",
 			`{"entries": }`,
+			"none",
 			"state file: could not unmarshal, corrupted state file",
 			&stateMemory{
 				assignments:   ContainerCPUAssignments{},
@@ -151,6 +172,7 @@ func TestFileStateTryRestore(t *testing.T) {
 					"container2": "1-3"
 				}
 			}`,
+			"none",
 			"",
 			&stateMemory{
 				assignments: ContainerCPUAssignments{
@@ -166,6 +188,7 @@ func TestFileStateTryRestore(t *testing.T) {
 				"policyName": "none",
 				"defaultCpuSet": "2-sd"
 			}`,
+			"none",
 			"state file: could not parse state file",
 			&stateMemory{
 				assignments:   ContainerCPUAssignments{},
@@ -182,6 +205,7 @@ func TestFileStateTryRestore(t *testing.T) {
 					"container2": "1-3"
 				}
 			}`,
+			"none",
 			"state file: could not parse state file",
 			&stateMemory{
 				assignments:   ContainerCPUAssignments{},
@@ -191,6 +215,7 @@ func TestFileStateTryRestore(t *testing.T) {
 		{
 			"TryRestoreState creates empty state file",
 			"",
+			"none",
 			"",
 			&stateMemory{
 				assignments:   ContainerCPUAssignments{},
@@ -214,7 +239,7 @@ func TestFileStateTryRestore(t *testing.T) {
 			defer os.Remove(sfilePath.Name())

 			logData, fileState := stderrCapture(t, func() State {
-				return NewFileState(sfilePath.Name(), "none")
+				return NewFileState(sfilePath.Name(), tc.policyName)
 			})

 			if tc.expErr != "" {