From 5dc5f1de06b19560a3250e7fe8313311b8b7ebd1 Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Thu, 18 Jul 2019 13:12:30 +0200 Subject: [PATCH 1/2] Update the cpumanager to error out if an invalid policy is given Previously, the cpumanager would simply fall back to the None() policy if an invalid policy was specified. This patch updates this to return an error when an invalid policy is passed, forcing the kubelet to fail fast when this occurs. These semantics should be preferable because an invalid policy likely indicates operator error in setting the policy flag on the kubelet correctly (e.g. misspelling 'static' as 'statiic'). In this case it is better to fail fast so the operator can detect this and correct the mistake, than to mask the error and essentially disable the cpumanager unexpectedly. --- pkg/kubelet/cm/cpumanager/cpu_manager.go | 3 +-- pkg/kubelet/cm/cpumanager/cpu_manager_test.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/kubelet/cm/cpumanager/cpu_manager.go b/pkg/kubelet/cm/cpumanager/cpu_manager.go index 84f239157d0..f3503d6ba8c 100644 --- a/pkg/kubelet/cm/cpumanager/cpu_manager.go +++ b/pkg/kubelet/cm/cpumanager/cpu_manager.go @@ -132,8 +132,7 @@ func NewManager(cpuPolicyName string, reconcilePeriod time.Duration, machineInfo policy = NewStaticPolicy(topo, numReservedCPUs) default: - klog.Errorf("[cpumanager] Unknown policy \"%s\", falling back to default policy \"%s\"", cpuPolicyName, PolicyNone) - policy = NewNonePolicy() + return nil, fmt.Errorf("unknown policy: \"%s\"", cpuPolicyName) } stateImpl, err := state.NewCheckpointState(stateFileDirectory, cpuManagerStateFileName, policy.Name()) diff --git a/pkg/kubelet/cm/cpumanager/cpu_manager_test.go b/pkg/kubelet/cm/cpumanager/cpu_manager_test.go index 5d08b84cbf7..1bb68dcb772 100644 --- a/pkg/kubelet/cm/cpumanager/cpu_manager_test.go +++ b/pkg/kubelet/cm/cpumanager/cpu_manager_test.go @@ -232,7 +232,7 @@ func TestCPUManagerGenerate(t *testing.T) { description: "invalid policy name", cpuPolicyName: "invalid", nodeAllocatableReservation: nil, - expectedPolicy: "none", + expectedError: fmt.Errorf("unknown policy: \"invalid\""), }, { description: "static policy", From 4ee5d5409e9d69d19de79418c2b8dffd5589b99c Mon Sep 17 00:00:00 2001 From: Kevin Klues Date: Thu, 18 Jul 2019 13:18:35 +0200 Subject: [PATCH 2/2] Update the topologymanager to error out if an invalid policy is given Previously, the topologymanager would simply fall back to the None() policy if an invalid policy was specified. This patch updates this to return an error when an invalid policy is passed, forcing the kubelet to fail fast when this occurs. These semantics should be preferable because an invalid policy likely indicates operator error in setting the policy flag on the kubelet correctly (e.g. misspelling 'strict' as 'striict'). In this case it is better to fail fast so the operator can detect this and correct the mistake, than to mask the error and essentially disable the topologymanager unexpectedly. --- .../cm/topologymanager/topology_manager.go | 9 ++--- .../topologymanager/topology_manager_test.go | 36 +++++++++++++------ 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/pkg/kubelet/cm/topologymanager/topology_manager.go b/pkg/kubelet/cm/topologymanager/topology_manager.go index 03e9d80db01..6af26b6cedd 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager.go @@ -17,6 +17,8 @@ limitations under the License. package topologymanager import ( + "fmt" + "k8s.io/api/core/v1" "k8s.io/klog" "k8s.io/kubernetes/pkg/kubelet/cm/topologymanager/socketmask" @@ -71,7 +73,7 @@ type TopologyHint struct { var _ Manager = &manager{} //NewManager creates a new TopologyManager based on provided policy -func NewManager(topologyPolicyName string) Manager { +func NewManager(topologyPolicyName string) (Manager, error) { klog.Infof("[topologymanager] Creating topology manager with %s policy", topologyPolicyName) var policy Policy @@ -87,8 +89,7 @@ func NewManager(topologyPolicyName string) Manager { policy = NewStrictPolicy() default: - klog.Errorf("[topologymanager] Unknown policy %s, using default policy %s", topologyPolicyName, PolicyNone) - policy = NewNonePolicy() + return nil, fmt.Errorf("unknown policy: \"%s\"", topologyPolicyName) } var hp []HintProvider @@ -101,7 +102,7 @@ func NewManager(topologyPolicyName string) Manager { policy: policy, } - return manager + return manager, nil } func (m *manager) GetAffinity(podUID string, containerName string) TopologyHint { diff --git a/pkg/kubelet/cm/topologymanager/topology_manager_test.go b/pkg/kubelet/cm/topologymanager/topology_manager_test.go index c14a1a17cb1..48c38f6209e 100644 --- a/pkg/kubelet/cm/topologymanager/topology_manager_test.go +++ b/pkg/kubelet/cm/topologymanager/topology_manager_test.go @@ -17,7 +17,9 @@ limitations under the License. package topologymanager import ( + "fmt" "reflect" + "strings" "testing" "k8s.io/api/core/v1" @@ -39,28 +41,40 @@ func NewTestSocketMaskFull() socketmask.SocketMask { func TestNewManager(t *testing.T) { tcases := []struct { - name string - policyType string + description string + policyName string + expectedPolicy string + expectedError error }{ { - name: "Policy is set preferred", - policyType: "preferred", + description: "Policy is set preferred", + policyName: "preferred", + expectedPolicy: "preferred", }, { - name: "Policy is set to strict", - policyType: "strict", + description: "Policy is set to strict", + policyName: "strict", + expectedPolicy: "strict", }, { - name: "Policy is set to unknown", - policyType: "unknown", + description: "Policy is set to unknown", + policyName: "unknown", + expectedError: fmt.Errorf("unknown policy: \"unknown\""), }, } for _, tc := range tcases { - mngr := NewManager(tc.policyType) + mngr, err := NewManager(tc.policyName) - if _, ok := mngr.(Manager); !ok { - t.Errorf("result is not Manager type") + if tc.expectedError != nil { + if !strings.Contains(err.Error(), tc.expectedError.Error()) { + t.Errorf("Unexpected error message. Have: %s wants %s", err.Error(), tc.expectedError.Error()) + } + } else { + rawMgr := mngr.(*manager) + if rawMgr.policy.Name() != tc.expectedPolicy { + t.Errorf("Unexpected policy name. Have: %q wants %q", rawMgr.policy.Name(), tc.expectedPolicy) + } } } }