-
Notifications
You must be signed in to change notification settings - Fork 118
perfprof: enable exec-cpu-affinity by default (annotation)
#1426
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shajmakh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/hold |
|
/cc @bartwensley |
7d801be to
05cf460
Compare
05cf460 to
b7824a4
Compare
|
/unhold |
...formanceprofile/controller/performanceprofile/components/machineconfig/machineconfig_test.go
Outdated
Show resolved
Hide resolved
bartwensley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me (once the tests are passing).
test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
Outdated
Show resolved
Hide resolved
test/e2e/performanceprofile/functests/1_performance/cpu_management.go
Outdated
Show resolved
Hide resolved
43944ca to
57c2a0e
Compare
|
@bartwensley @Tal-or thanks for the review comments, I addressed them and extended e2e test coverage and filed a PR here for more review flexibility: |
57c2a0e to
f2b9f2f
Compare
pkg/performanceprofile/controller/performanceprofile/components/machineconfig/machineconfig.go
Outdated
Show resolved
Hide resolved
f2b9f2f to
a9ea0bd
Compare
|
/hold |
a9ea0bd to
7c2dd69
Compare
|
filed an ocp bug on that: OCPNODE-3891 |
|
/retest |
exec-cpu-affinity by defaultexec-cpu-affinity by default (annotation)
6b0d39f to
b27abcc
Compare
For High-performance configuration, cri-o started supporting exec-cpu-affinity feature and when configured to `first` it provides the ability for exec process to be pinned to the first CPU from the shared-CPUs IF set or to the first one from the islolated set. (see cri-o/cri-o@4dd7fb9) In performance profile, we want to enable this high-performance feature by default, and disbale it (legacy) it provides an annotation option. The annotation is there just as a backup in case of bugs getting reported by the consequences of this feature enablement, and should be removed in 2 releases. Run `./hack/render-sync.sh` to update the (no-cluster) e2e tests expected outputs. Signed-off-by: Shereen Haj <shajmakh@redhat.com>
u/s CI stops the run after 2 failures of the same test, which prevents a full run of the tests. The fail-fast option is better be removed in test runs that do not involve node reboots, that way the CI would reflect a full run of the suite which would help us save reruns to see the next failing test. In other words, removing it will reflect all of the failing tests in one run. remove this flag initially for suites of PP that are known to not have reboots. Signed-off-by: Shereen Haj <shajmakh@redhat.com>
|
/unhold |
|
/hold |
|
@shajmakh: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
For High-performance configuration, cri-o started supporting exec-cpu-affinity feature and when configured to
firstit provides the ability for exec process to be pinned to the first CPU from the shared-CPUs IF set or to the first one from the islolated set.(see cri-o/cri-o@4dd7fb9)
In performance profile, we want to enable this high-performance feature by default, and disbale it (legacy) it provides an annotation option.
The annotation is there just as a backup in case of bugs getting reported by the consequences of this feature enablement, and should be removed in 2 releases.