Bug 2105123: tuned: disable irqbalance #396

ffromani · 2022-07-20T09:19:48Z

The tuned irqbalance plugin clears the irqbalance banned CPUs list when tuned starts. The list is then managed dynamically by the runtime handlers.

On node restart, the tuned pod can be started AFTER the workload pods (kubelet nor kubernetes offers ordering guarantees when recovering the node state); clearing the banned CPUs list while pods are running and compromising the IRQ isolation guarantees. Same holds true if the NTO pod restarts for whatever reason.

The fix is twofold:

disable the irqbalance plugin, so tuned doesn't clear the irqbalance banned list anymore.
depend on crio to reset the banned CPU list earlier in the boot

in #413 we add e2e tests to verify the aforementioned steps.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2105123
Signed-off-by: Francesco Romani fromani@redhat.com

assets/performanceprofile/scripts/clear-irqbalance-banned-cpus.sh

assets/performanceprofile/tuned/openshift-node-performance

pkg/performanceprofile/controller/performanceprofile/components/assets_scripts_test.go

assets/performanceprofile/scripts/clear-irqbalance-banned-cpus.sh

pkg/performanceprofile/controller/performanceprofile/components/assets_scripts_test.go

ffromani · 2022-07-20T15:16:59Z

thanks @jmencak for the early review. I want to add at least a e2e test to cover the "tuned restart while pods running" scenario and then I'll lift the WIP.

ffromani · 2022-07-21T14:55:47Z

content wise ready. Now testing and checking the e2e test.

ffromani · 2022-07-22T08:13:25Z

/retest

ffromani · 2022-07-22T10:25:35Z

the PR is ready for review. I'm happy enough with the e2e test, but suggestions very welcome.

ffromani · 2022-07-22T10:32:00Z

/cc @yanirq @MarSik

jmencak

Thank you for the PR and the changes so far, Francesco! This looks good to me with the exceptions of some nits, but it might be useful if Yanir/Martin could have a look also from PAO side.

test/e2e/performanceprofile/testdata/render-expected-output/manual_tuned.yaml

pkg/performanceprofile/controller/performanceprofile/components/utils.go

assets/performanceprofile/tuned/openshift-node-performance

ffromani · 2022-07-22T13:07:50Z

/hold
I was made aware of https://github.com/cri-o/cri-o/blob/v1.24.1/internal/runtimehandlerhooks/high_performance_hooks.go#L360 (which I skimmed over before) - so the new unit may be unnecessary (not harmful, but redundant). I'll issue a reduced PR with the e2e test to test if this crio functionality is enough

ffromani · 2022-07-22T15:07:14Z

/retest

ffromani · 2022-08-11T15:49:52Z

proposed crio fix: cri-o/cri-o#6146

since this change needs to be backportable, I'll work on a workaround on the e2e tests.

Testing the irqbalance handling on tuned restart highlighted a crio bug when handling odd-numbered cpu affinity masks. We expect this bug to have little impact on production environments because it's unlikely they will have a number of CPUs multiple by 4 but not multiple by 8, but still we filed cri-o/cri-o#6145 In order to have a backportable change, we fix our utility code to deal with incorrect padding. Signed-off-by: Francesco Romani <fromani@redhat.com>

openshift-ci · 2022-08-11T20:31:54Z

@fromanirh: all tests passed!

Full PR test history. Your PR dashboard.

Instructions 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/test-infra repository. I understand the commands that are listed here.

test/e2e/performanceprofile/functests/1_performance/irqbalance.go

yanirq · 2022-08-15T10:42:37Z

Added some small nit questions but given the discussion above and the crio bug discovered this LTGM.
Feel free to remove hold if it seems fit.
/lgtm

yanirq · 2022-08-15T10:43:26Z

/hold

openshift-ci · 2022-08-17T12:50:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bartwensley, fromanirh, MarSik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [MarSik,fromanirh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

MarSik · 2022-08-17T12:51:14Z

/bugzilla refresh

openshift-ci · 2022-08-17T12:51:21Z

@MarSik: This pull request references Bugzilla bug 2105123, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.12.0) matches configured target release for branch (4.12.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gsr-shanks

In response to this:

/bugzilla refresh

Instructions 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/test-infra repository.

ffromani · 2022-08-17T13:04:40Z

/hold cancel
agreed offline to move forward

openshift-ci · 2022-08-17T13:08:54Z

@fromanirh: All pull requests linked via external trackers have merged:

openshift/cluster-node-tuning-operator#396

Bugzilla bug 2105123 has been moved to the MODIFIED state.

In response to this:

Bug 2105123: tuned: disable irqbalance

Instructions 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/test-infra repository.

MarSik · 2022-08-18T14:23:26Z

/cherry-pick release-4.11

openshift-cherrypick-robot · 2022-08-18T14:24:07Z

@MarSik: new pull request created: #435

In response to this:

/cherry-pick release-4.11

Instructions 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/test-infra repository.

* test: perfprof: utils: make sure to unquote cpus Make sure to unquote the cpumask output to prevent false negatives Signed-off-by: Francesco Romani <fromani@redhat.com> * perfprof: utils: robustness fixes Add testcases and log enhancement emerged during the local testing. Signed-off-by: Francesco Romani <fromani@redhat.com> * perfprof: tuned: disable the irqbalance plugin The tuned irqbalance plugin clears the irqbalance banned CPUs list when tuned starts. The list is then managed dynamically by the runtime handlers. On node restart, the tuned pod can be started AFTER the workload pods (kubelet nor kubernetes offers ordering guarantees when recovering the node state); clearing the banned CPUs list while pods are running and compromising the IRQ isolation guarantees. Same holds true if the NTO pod restarts for whatever reason. To prevent this disruption, we disable the irqbalance plugin entirely. Another component in the stack must now clear the irqbalance cpu ban list once per node reboot. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2105123 Signed-off-by: Francesco Romani <fromani@redhat.com> * e2e: perfprof: add tests for cpu ban list handling Add a test to verify that a tuned restart will not clear the irqbalance cpu ban list, which is the key reason why we disabled the irqbalance tuned plugin earlier. Note there is no guarantee that any component in the stack will reset the irqbalance copu ban list once. crio inconditionally does a restore depending on a snapshot taken the first time the server runs, which is likely but not guaranteed to be correct. There's no way to declare or check the content of the value crio would reset. Signed-off-by: Francesco Romani <fromani@redhat.com> * perfprof: functests: utils: fix cpu mask padding Testing the irqbalance handling on tuned restart highlighted a crio bug when handling odd-numbered cpu affinity masks. We expect this bug to have little impact on production environments because it's unlikely they will have a number of CPUs multiple by 4 but not multiple by 8, but still we filed cri-o/cri-o#6145 In order to have a backportable change, we fix our utility code to deal with incorrect padding. Signed-off-by: Francesco Romani <fromani@redhat.com> Signed-off-by: Francesco Romani <fromani@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 20, 2022

openshift-ci bot requested review from jmencak and kpouget July 20, 2022 09:21

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2022

ffromani force-pushed the stable-banned-cpus branch from d4d5210 to 37845e7 Compare July 20, 2022 11:10

jmencak reviewed Jul 20, 2022

View reviewed changes

ffromani force-pushed the stable-banned-cpus branch from 37845e7 to 536df06 Compare July 20, 2022 15:16

ffromani force-pushed the stable-banned-cpus branch 6 times, most recently from 70b850e to e0252d7 Compare July 21, 2022 14:00

ffromani force-pushed the stable-banned-cpus branch from e0252d7 to 5583284 Compare July 21, 2022 16:22

ffromani changed the title ~~WIP: tuned: disable irqbalance, handle ban cpus earlier~~ tuned: disable irqbalance, handle ban cpus earlier Jul 22, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2022

openshift-ci bot requested review from MarSik and yanirq July 22, 2022 10:32

jmencak reviewed Jul 22, 2022

View reviewed changes

ffromani mentioned this pull request Jul 22, 2022

highperfhooks: avoid unbound growth of irqbalance cri-o/cri-o#6087

Merged

ffromani force-pushed the stable-banned-cpus branch from 5583284 to c31e736 Compare July 22, 2022 12:32

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2022

ffromani mentioned this pull request Jul 22, 2022

WIP: tuned: disable irqbalance, handle ban cpus earlier - using crio #400

Closed

ffromani force-pushed the stable-banned-cpus branch from 4821953 to c1e9c2e Compare August 11, 2022 16:06

ffromani force-pushed the stable-banned-cpus branch from c1e9c2e to eae8b1e Compare August 11, 2022 17:50

yanirq reviewed Aug 15, 2022

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Show resolved Hide resolved

yanirq reviewed Aug 15, 2022

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/irqbalance.go Show resolved Hide resolved

openshift-ci bot assigned yanirq Aug 15, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2022

MarSik approved these changes Aug 17, 2022

View reviewed changes

openshift-ci bot assigned MarSik Aug 17, 2022

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Aug 17, 2022

openshift-ci bot requested a review from gsr-shanks August 17, 2022 12:51

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2022

openshift-merge-robot merged commit c5cf0bd into openshift:master Aug 17, 2022

ffromani deleted the stable-banned-cpus branch August 17, 2022 13:08

openshift-cherrypick-robot mentioned this pull request Aug 18, 2022

[release-4.11] OCPBUGS-263: tuned: disable irqbalance #435

Merged

marioferh mentioned this pull request Aug 24, 2022

irqbalance: add unit to clear the cpu ban list #413 #444

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2105123: tuned: disable irqbalance #396

Bug 2105123: tuned: disable irqbalance #396

ffromani commented Jul 20, 2022 •

edited

Loading

ffromani commented Jul 20, 2022

ffromani commented Jul 21, 2022

ffromani commented Jul 22, 2022

ffromani commented Jul 22, 2022

ffromani commented Jul 22, 2022

jmencak left a comment

ffromani commented Jul 22, 2022

ffromani commented Jul 22, 2022

ffromani commented Aug 11, 2022

openshift-ci bot commented Aug 11, 2022

yanirq commented Aug 15, 2022

yanirq commented Aug 15, 2022

openshift-ci bot commented Aug 17, 2022

MarSik commented Aug 17, 2022

openshift-ci bot commented Aug 17, 2022

ffromani commented Aug 17, 2022

openshift-ci bot commented Aug 17, 2022

MarSik commented Aug 18, 2022

openshift-cherrypick-robot commented Aug 18, 2022

Bug 2105123: tuned: disable irqbalance #396

Bug 2105123: tuned: disable irqbalance #396

Conversation

ffromani commented Jul 20, 2022 • edited Loading

ffromani commented Jul 20, 2022

ffromani commented Jul 21, 2022

ffromani commented Jul 22, 2022

ffromani commented Jul 22, 2022

ffromani commented Jul 22, 2022

jmencak left a comment

Choose a reason for hiding this comment

ffromani commented Jul 22, 2022

ffromani commented Jul 22, 2022

ffromani commented Aug 11, 2022

openshift-ci bot commented Aug 11, 2022

yanirq commented Aug 15, 2022

yanirq commented Aug 15, 2022

openshift-ci bot commented Aug 17, 2022

MarSik commented Aug 17, 2022

openshift-ci bot commented Aug 17, 2022

ffromani commented Aug 17, 2022

openshift-ci bot commented Aug 17, 2022

MarSik commented Aug 18, 2022

openshift-cherrypick-robot commented Aug 18, 2022

ffromani commented Jul 20, 2022 •

edited

Loading