Skip to content

OCPBUGS-92836: UPSTREAM: <carry>: upkeep cpu partitioning admission webhook #2713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
eggfoobar wants to merge 1 commit into
base: master
Choose a base branch
from
+15 −46
Open

OCPBUGS-92836: UPSTREAM: <carry>: upkeep cpu partitioning admission webhook #2713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 2 additions & 24 deletions openshift-kube-apiserver/admission/autoscaling/managementcpusoverride/admission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apiserver/pkg/admission"
"k8s.io/apiserver/pkg/admission/initializer"
Expand Down Expand Up @@ -187,16 +186,6 @@ func (a *managementCPUsOverride) Admit(ctx context.Context, attr admission.Attri
return admission.NewForbidden(attr, fmt.Errorf("%s node or namespace or infra config cache not synchronized", PluginName))
}

nodes, err := a.nodeLister.List(labels.Everything())
if err != nil {
return admission.NewForbidden(attr, err) // can happen due to informer latency
}

// we still need to have nodes under the cluster to decide if the management resource enabled or not
if len(nodes) == 0 {
return admission.NewForbidden(attr, fmt.Errorf("%s the cluster does not have any nodes", PluginName))
}

clusterInfra, err := a.infraConfigLister.Get(infraClusterName)
if err != nil {
return admission.NewForbidden(attr, err) // can happen due to informer latency
Expand All @@ -215,7 +204,7 @@ func (a *managementCPUsOverride) Admit(ctx context.Context, attr admission.Attri
}

// Check if we are in CPU Partitioning mode for AllNodes
if !isCPUPartitioning(clusterInfra.Status, nodes, workloadType) {
if !isCPUPartitioning(clusterInfra.Status) {
return nil
}

Expand Down Expand Up @@ -284,18 +273,7 @@ func (a *managementCPUsOverride) Admit(ctx context.Context, attr admission.Attri
return nil
}

func isCPUPartitioning(infraStatus configv1.InfrastructureStatus, nodes []*corev1.Node, workloadType string) bool {
// If status is not for CPU partitioning and we're single node we also check nodes to support upgrade event
// TODO: This should not be needed after 4.13 as all clusters after should have this feature on at install time, or updated by migration in NTO.
if infraStatus.CPUPartitioning != configv1.CPUPartitioningAllNodes && infraStatus.ControlPlaneTopology == configv1.SingleReplicaTopologyMode {
managedResource := fmt.Sprintf("%s.%s", workloadType, containerWorkloadResourceSuffix)
for _, node := range nodes {
// We only expect a single node to exist, so we return on first hit
if _, ok := node.Status.Allocatable[corev1.ResourceName(managedResource)]; ok {
return true
}
}
}
func isCPUPartitioning(infraStatus configv1.InfrastructureStatus) bool {
return infraStatus.CPUPartitioning == configv1.CPUPartitioningAllNodes
}

Expand Down
35 changes: 13 additions & 22 deletions openshift-kube-apiserver/admission/autoscaling/managementcpusoverride/admission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func TestAdmit(t *testing.T) {
pod: testManagedPodWithWorkloadAnnotation("500m", "250m", "500Mi", "250Mi", "non-existent"),
expectedCpuRequest: resource.MustParse("250m"),
namespace: testManagedNamespace(),
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
expectedError: fmt.Errorf("the pod namespace %q does not allow the workload type non-existent", "managed-namespace"),
},
Expand All @@ -96,7 +96,7 @@ func TestAdmit(t *testing.T) {
pod: testPod("500m", "250m", "500Mi", "250Mi"),
expectedCpuRequest: resource.MustParse("250m"),
namespace: testManagedNamespace(),
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
},
{
name: "should return admission error when the pod has more than one workload annotation",
Expand All @@ -112,7 +112,7 @@ func TestAdmit(t *testing.T) {
),
expectedCpuRequest: resource.MustParse("250m"),
namespace: testManagedNamespace(),
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
expectedError: fmt.Errorf("the pod can not have more than one workload annotations"),
},
Expand All @@ -129,7 +129,7 @@ func TestAdmit(t *testing.T) {
),
expectedCpuRequest: resource.MustParse("250m"),
namespace: testManagedNamespace(),
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
expectedError: fmt.Errorf("the workload annotation key should have format %s<workload_type>", podWorkloadTargetAnnotationPrefix),
},
Expand All @@ -146,7 +146,7 @@ func TestAdmit(t *testing.T) {
),
expectedCpuRequest: resource.MustParse("250m"),
namespace: testManagedNamespace(),
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
expectedError: fmt.Errorf(`failed to get workload annotation effect: failed to parse "{" annotation value: unexpected end of JSON input`),
},
Expand All @@ -163,7 +163,7 @@ func TestAdmit(t *testing.T) {
),
expectedCpuRequest: resource.MustParse("250m"),
namespace: testManagedNamespace(),
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
expectedError: fmt.Errorf(`failed to get workload annotation effect: the workload annotation value map["test":"test"] does not have "effect" key`),
infra: testClusterSNOInfra(),
},
Expand All @@ -183,7 +183,7 @@ func TestAdmit(t *testing.T) {
workloadAdmissionWarning: "skipping pod CPUs requests modifications because the namespace namespace is not annotated with workload.openshift.io/allowed to allow workload partitioning",
},
namespace: testNamespace(),
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
},
{
Expand All @@ -196,7 +196,7 @@ func TestAdmit(t *testing.T) {
fmt.Sprintf("%s%s", containerResourcesAnnotationPrefix, "initTest"): fmt.Sprintf(`{"%s":256}`, containerResourcesAnnotationValueKeyCPUShares),
fmt.Sprintf("%s%s", podWorkloadTargetAnnotationPrefix, workloadTypeManagement): fmt.Sprintf(`{"%s":"%s"}`, podWorkloadAnnotationEffect, workloadEffectPreferredDuringScheduling),
},
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
},
{
Expand All @@ -209,7 +209,7 @@ func TestAdmit(t *testing.T) {
fmt.Sprintf("%s%s", containerResourcesAnnotationPrefix, "initTest"): fmt.Sprintf(`{"%s": 2}`, containerResourcesAnnotationValueKeyCPUShares),
fmt.Sprintf("%s%s", podWorkloadTargetAnnotationPrefix, workloadTypeManagement): fmt.Sprintf(`{"%s":"%s"}`, podWorkloadAnnotationEffect, workloadEffectPreferredDuringScheduling),
},
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
},
{
Expand All @@ -221,7 +221,7 @@ func TestAdmit(t *testing.T) {
fmt.Sprintf("%s%s", podWorkloadTargetAnnotationPrefix, workloadTypeManagement): fmt.Sprintf(`{"%s":"%s"}`, podWorkloadAnnotationEffect, workloadEffectPreferredDuringScheduling),
kubetypes.ConfigSourceAnnotationKey: kubetypes.FileSource,
},
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
},
{
Expand All @@ -232,7 +232,7 @@ func TestAdmit(t *testing.T) {
expectedAnnotations: map[string]string{
workloadAdmissionWarning: "skip pod CPUs requests modifications because it has guaranteed QoS class",
},
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
},
{
Expand All @@ -245,7 +245,7 @@ func TestAdmit(t *testing.T) {
fmt.Sprintf("%s%s", containerResourcesAnnotationPrefix, "initTest"): fmt.Sprintf(`{"%s":256,"cpulimit":500}`, containerResourcesAnnotationValueKeyCPUShares),
fmt.Sprintf("%s%s", podWorkloadTargetAnnotationPrefix, workloadTypeManagement): fmt.Sprintf(`{"%s":"%s"}`, podWorkloadAnnotationEffect, workloadEffectPreferredDuringScheduling),
},
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
},
{
Expand All @@ -256,7 +256,7 @@ func TestAdmit(t *testing.T) {
expectedAnnotations: map[string]string{
workloadAdmissionWarning: fmt.Sprintf("skip pod CPUs requests modifications because it will change the pod QoS class from %s to %s", corev1.PodQOSBurstable, corev1.PodQOSBestEffort),
},
nodes: []*corev1.Node{testNodeWithManagementResource()},
nodes: []*corev1.Node{testNode()},
infra: testClusterSNOInfra(),
},
{
Expand All @@ -267,15 +267,6 @@ func TestAdmit(t *testing.T) {
nodes: []*corev1.Node{testNode()},
infra: testClusterInfraWithoutWorkloadPartitioning(),
},
{
name: "should return admission error when the cluster does not have any nodes",
pod: testManagedPod("500m", "250m", "500Mi", "250Mi"),
expectedCpuRequest: resource.MustParse("250m"),
namespace: testManagedNamespace(),
nodes: []*corev1.Node{},
infra: testClusterSNOInfra(),
expectedError: fmt.Errorf("the cluster does not have any nodes"),
},
}

for _, test := range tests {
Expand Down