Skip to content

Commit 33b83c7

Browse files
MarSikMarSik
authored
CNF-16638: Reduce code duplication of RPS enabled checks (#1301)
Centralize the RPS enabled logic into just one function and add a way to force RPS off. This is prep-work for disabling RPS globally for all use cases.
1 parent b82b74f commit 33b83c7

File tree

13 files changed

+551
-21
lines changed

13 files changed

+551
-21
lines changed

‎hack/render-sync.sh

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,9 @@ function rendersync() {
3939
}
4040

4141
rendersync manual-cluster/performance base/performance default
42+
rendersync manual-cluster/performance-norps base/performance default/pp-norps
4243
rendersync bootstrap-cluster/performance pinned-cluster/default bootstrap/no-mcp
4344
rendersync bootstrap-cluster/performance pinned-cluster/default bootstrap-cluster/extra-mcp bootstrap/extra-mcp
44-
rendersync --owner-ref none -- base/performance manual-cluster/performance no-ref
45-
rendersync --owner-ref none -- base/performance manual-cluster/cpuFrequency default/cpuFrequency
45+
rendersync --owner-ref none -- base/performance manual-cluster/performance no-ref
46+
rendersync --owner-ref none -- base/performance manual-cluster/cpuFrequency default/cpuFrequency
4647
rendersync --owner-ref none -- base/performance manual-cluster/arm default/arm

‎pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -182,8 +182,7 @@ func getIgnitionConfig(profile *performancev2.PerformanceProfile, opts *componen
182182
}
183183

184184
// add script files under the node /usr/local/bin directory
185-
if profileutil.IsRpsEnabled(profile) || profile.Spec.WorkloadHints == nil ||
186-
profile.Spec.WorkloadHints.RealTime == nil || *profile.Spec.WorkloadHints.RealTime {
185+
if profileutil.IsRpsEnabled(profile) {
187186
scripts = []string{hugepagesAllocation, setRPSMask, setCPUsOffline, clearIRQBalanceBannedCPUs}
188187
} else {
189188
// realtime is explicitly disabled by workload hint
@@ -209,8 +208,7 @@ func getIgnitionConfig(profile *performancev2.PerformanceProfile, opts *componen
209208
addContent(ignitionConfig, crioConfigSnippetContent, crioConfSnippetDst, &crioConfdRuntimesMode)
210209

211210
// do not add RPS handling when realtime is explicitly disabled by workload hint
212-
if profileutil.IsRpsEnabled(profile) || profile.Spec.WorkloadHints == nil ||
213-
profile.Spec.WorkloadHints.RealTime == nil || *profile.Spec.WorkloadHints.RealTime {
211+
if profileutil.IsRpsEnabled(profile) {
214212
// configure default rps mask applied to all network devices
215213
sysctlConfContent, err := renderSysctlConf(profile, filepath.Join("configs", defaultRPSMaskConfig))
216214
if err != nil {

‎pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,30 @@ var _ = Describe("Machine Config", func() {
9494
})
9595
})
9696

97+
Context("machine config creation with disabled RPS", func() {
98+
It("should create machine config with no trace of RPS", func() {
99+
profile := testutils.NewPerformanceProfile("test")
100+
profile.Annotations = map[string]string{}
101+
profile.Annotations[performancev2.PerformanceProfileEnableRpsAnnotation] = "false"
102+
103+
mc, err := New(profile, &components.MachineConfigOptions{})
104+
Expect(err).ToNot(HaveOccurred())
105+
106+
result := igntypes.Config{}
107+
108+
err = json.Unmarshal(mc.Spec.Config.Raw, &result)
109+
Expect(err).ToNot(HaveOccurred())
110+
111+
for _, f := range result.Storage.Files {
112+
Expect(f.Node.Path).To(Not(ContainSubstring("rps")), "rps configuration %s should not be present", f.Node.Path)
113+
}
114+
115+
for _, f := range result.Systemd.Units {
116+
Expect(f.Name).To(Not(ContainSubstring("rps")), "rps systemd unit %s should not be present", f.Name)
117+
}
118+
})
119+
})
120+
97121
Context("with hugepages with specified NUMA node and offlinedCPUs", func() {
98122
var manifest string
99123

‎pkg/performanceprofile/controller/performanceprofile/components/profile/profile.go

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,15 +71,19 @@ func IsPhysicalRpsEnabled(profile *performancev2.PerformanceProfile) bool {
7171

7272
// IsRpsEnabled checks if all RPS should be applied
7373
func IsRpsEnabled(profile *performancev2.PerformanceProfile) bool {
74-
if profile.Annotations == nil {
75-
return false
76-
}
77-
isRpsEnabled, ok := profile.Annotations[performancev2.PerformanceProfileEnableRpsAnnotation]
78-
if ok && isRpsEnabled == "true" {
79-
return true
74+
if profile.Annotations != nil {
75+
// First check overrides
76+
isRpsEnabled, ok := profile.Annotations[performancev2.PerformanceProfileEnableRpsAnnotation]
77+
if ok && isRpsEnabled == "true" {
78+
return true
79+
} else if ok && isRpsEnabled == "false" {
80+
return false
81+
}
8082
}
8183

82-
return false
84+
// The default behavior enables RPS for real time workloads
85+
return profile.Spec.WorkloadHints == nil ||
86+
profile.Spec.WorkloadHints.RealTime == nil || *profile.Spec.WorkloadHints.RealTime
8387
}
8488

8589
func IsMixedCPUsEnabled(profile *performancev2.PerformanceProfile) bool {

‎test/e2e/performanceprofile/cluster-setup/manual-cluster/performance-norps/kustomization.yaml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
apiVersion: kustomize.config.k8s.io/v1beta1
2+
kind: Kustomization
3+
4+
bases:
5+
- ../../base/performance
6+
7+
resources:
8+
- performance_profile.yaml

‎test/e2e/performanceprofile/cluster-setup/manual-cluster/performance-norps/performance_profile.yaml

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
apiVersion: performance.openshift.io/v2
2+
kind: PerformanceProfile
3+
metadata:
4+
name: manual
5+
annotations:
6+
performance.openshift.io/enable-rps: "false"
7+
spec:
8+
cpu:
9+
isolated: "1"
10+
reserved: "0"
11+
offlined: "2,3"
12+
hugepages:
13+
defaultHugepagesSize: "1G"
14+
pages:
15+
- size: "1G"
16+
count: 1
17+
node: 0
18+
- size: "2M"
19+
count: 128
20+
realTimeKernel:
21+
enabled: true
22+
numa:
23+
topologyPolicy: "single-numa-node"
24+
nodeSelector:
25+
node-role.kubernetes.io/worker-cnf: ""
26+
workloadHints:
27+
highPowerConsumption: false
28+
realTime: true
29+
perPodPowerManagement: false

‎test/e2e/performanceprofile/functests-render-command/1_render_command/render_test.go

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,14 @@ const (
1919
noRefExpectedDir = "no-ref"
2020
cpuFrequencyExpectedDir = defaultExpectedDir + "/" + "cpuFrequency"
2121
armExpectedDir = defaultExpectedDir + "/" + "arm"
22+
ppNoRPSExpectedDir = defaultExpectedDir + "/" + "pp-norps"
2223
)
2324

2425
var (
2526
assetsOutDir string
27+
assetsInDir string
2628
assetsInDirs, assetsCpuFrequencyInDirs, assetsARMInDirs []string
27-
ppDir string
29+
ppDir, ppDirNoRPS string
2830
ppCpuFrequencyDir string
2931
armDir string
3032
testDataPath string
@@ -38,10 +40,11 @@ var _ = Describe("render command e2e test", func() {
3840

3941
BeforeEach(func() {
4042
assetsOutDir = createTempAssetsDir()
41-
assetsInDir := filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "base", "performance")
43+
assetsInDir = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "base", "performance")
4244
bootstrapPPDir = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "bootstrap-cluster", "performance")
4345
extraMCPDir = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "bootstrap-cluster", "extra-mcp")
4446
ppDir = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "manual-cluster", "performance")
47+
ppDirNoRPS = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "manual-cluster", "performance-norps")
4548
ppCpuFrequencyDir = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "manual-cluster", "cpuFrequency")
4649
armDir = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "manual-cluster", "arm")
4750
defaultPinnedDir = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "pinned-cluster", "default")
@@ -143,6 +146,19 @@ var _ = Describe("render command e2e test", func() {
143146
cmd := exec.Command(cmdline[0], cmdline[1:]...)
144147
runAndCompare(cmd, armExpectedDir)
145148
})
149+
150+
It("should not render RPS when disabled", func() {
151+
cmdline := []string{
152+
filepath.Join(binPath, "cluster-node-tuning-operator"),
153+
"render",
154+
"--asset-input-dir", assetsInDir + "," + ppDirNoRPS,
155+
"--asset-output-dir", assetsOutDir,
156+
}
157+
fmt.Fprintf(GinkgoWriter, "running: %v\n", cmdline)
158+
159+
cmd := exec.Command(cmdline[0], cmdline[1:]...)
160+
runAndCompare(cmd, ppNoRPSExpectedDir)
161+
})
146162
})
147163

148164
Context("With pinned cluster resources", func() {

‎test/e2e/performanceprofile/functests/1_performance/performance.go

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -375,8 +375,7 @@ var _ = Describe("[rfe_id:27368][performance]", Ordered, func() {
375375
}
376376
})
377377
It("[test_id: 59572] Check RPS Mask is applied to at least one single rx queue on all veth interface", func() {
378-
if profile.Spec.WorkloadHints != nil && profile.Spec.WorkloadHints.RealTime != nil &&
379-
!*profile.Spec.WorkloadHints.RealTime && !profileutil.IsRpsEnabled(profile) {
378+
if !profileutil.IsRpsEnabled(profile) {
380379
Skip("realTime Workload Hints is not enabled")
381380
}
382381
count := 0
@@ -415,8 +414,7 @@ var _ = Describe("[rfe_id:27368][performance]", Ordered, func() {
415414
}
416415
})
417416
It("[test_id:55012] Should have the correct RPS configuration", func() {
418-
if profile.Spec.WorkloadHints != nil && profile.Spec.WorkloadHints.RealTime != nil &&
419-
!*profile.Spec.WorkloadHints.RealTime && !profileutil.IsRpsEnabled(profile) {
417+
if !profileutil.IsRpsEnabled(profile) {
420418
Skip("realTime Workload Hints is not enabled")
421419
}
422420

@@ -491,8 +489,7 @@ var _ = Describe("[rfe_id:27368][performance]", Ordered, func() {
491489
}
492490
})
493491
It("[test_id:54190] Should not have RPS configuration set when realtime workload hint is explicitly set", func() {
494-
if profile.Spec.WorkloadHints != nil && profile.Spec.WorkloadHints.RealTime != nil &&
495-
!*profile.Spec.WorkloadHints.RealTime && !profileutil.IsRpsEnabled(profile) {
492+
if !profileutil.IsRpsEnabled(profile) {
496493
for _, node := range workerRTNodes {
497494
// Verify the systemd RPS services were not created
498495
cmd := []string{"ls", "/rootfs/etc/systemd/system/[email protected]"}

‎test/e2e/performanceprofile/testdata/render-expected-output/default/pp-norps/manual_kubeletconfig.yaml

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
apiVersion: machineconfiguration.openshift.io/v1
2+
kind: KubeletConfig
3+
metadata:
4+
creationTimestamp: null
5+
labels:
6+
performance.openshift.io/weak-owner-reference-name: manual
7+
name: performance-manual
8+
spec:
9+
kubeletConfig:
10+
apiVersion: kubelet.config.k8s.io/v1beta1
11+
authentication:
12+
anonymous: {}
13+
webhook:
14+
cacheTTL: 0s
15+
x509: {}
16+
authorization:
17+
webhook:
18+
cacheAuthorizedTTL: 0s
19+
cacheUnauthorizedTTL: 0s
20+
containerRuntimeEndpoint: ""
21+
cpuManagerPolicy: static
22+
cpuManagerPolicyOptions:
23+
full-pcpus-only: "true"
24+
cpuManagerReconcilePeriod: 5s
25+
evictionHard:
26+
imagefs.available: 15%
27+
memory.available: 100Mi
28+
nodefs.available: 10%
29+
nodefs.inodesFree: 5%
30+
evictionPressureTransitionPeriod: 0s
31+
fileCheckFrequency: 0s
32+
httpCheckFrequency: 0s
33+
imageMaximumGCAge: 0s
34+
imageMinimumGCAge: 0s
35+
kind: KubeletConfiguration
36+
kubeReserved:
37+
memory: 500Mi
38+
logging:
39+
flushFrequency: 0
40+
options:
41+
json:
42+
infoBufferSize: "0"
43+
text:
44+
infoBufferSize: "0"
45+
verbosity: 0
46+
memoryManagerPolicy: Static
47+
memorySwap: {}
48+
nodeStatusReportFrequency: 0s
49+
nodeStatusUpdateFrequency: 0s
50+
reservedMemory:
51+
- limits:
52+
memory: 1100Mi
53+
numaNode: 0
54+
reservedSystemCPUs: "0"
55+
runtimeRequestTimeout: 0s
56+
shutdownGracePeriod: 0s
57+
shutdownGracePeriodCriticalPods: 0s
58+
streamingConnectionIdleTimeout: 0s
59+
syncFrequency: 0s
60+
systemReserved:
61+
memory: 500Mi
62+
topologyManagerPolicy: single-numa-node
63+
volumeStatsAggPeriod: 0s
64+
machineConfigPoolSelector:
65+
matchLabels:
66+
machineconfiguration.openshift.io/role: worker-cnf
67+
status:
68+
conditions: null

0 commit comments

Comments
 (0)