improve the irqbalance restore flow #6388

jlojosnegros · 2022-11-22T12:21:19Z

What type of PR is this?

/kind cleanup
/kind feature

note: this PR is substitute #6105 PR from @fromanirh due to workload issues

What this PR does / why we need it:

Since #4441 , to support the dynamic irqbalance configuration in turn needed by the high
performance hooks, CRI-O runs the restore code at startup.

In some configuration scenarios it's preferred to have more control over this step. Some configuration wants
to have more control over the mask to be restored, or to disable the restore flow entirely.

For this reason, we add an option to provide a user-supplied restore file. Users wishing to exert more control can provide
their data, or they can disable the flow entirely setting the path to the restore file to empty string ("").

The new option defaults are meant to ensure full backward compatibility.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add configuration option to control the irqbalance configuration restore process for high performance hooks dynamic IRQ pinning.

openshift-ci · 2022-11-22T12:21:35Z

Hi @jlojosnegros. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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

thanks for taking over!

test/irqbalance.bats

ffromani · 2022-11-22T12:25:53Z

/ok-to-test

ffromani · 2022-11-23T14:16:18Z

/retest
let's rule out glitches

ffromani · 2022-11-23T14:20:46Z

possibly relevant: #6392

jlojosnegros · 2023-02-16T07:49:28Z

/assign @haircommander

codecov · 2023-02-20T18:04:58Z

Codecov Report

Merging #6388 (1a488f5) into main (2b11872) will decrease coverage by 0.01%.
The diff coverage is 70.68%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6388      +/-   ##
==========================================
- Coverage   44.55%   44.55%   -0.01%     
==========================================
  Files         128      127       -1     
  Lines       14888    14919      +31     
==========================================
+ Hits         6634     6647      +13     
- Misses       7458     7477      +19     
+ Partials      796      795       -1

haircommander · 2023-02-20T18:08:11Z

pkg/config/config.go

@@ -124,6 +124,8 @@ const (
 const (
 	// DefaultIrqBalanceConfigFile default irqbalance service configuration file path
 	DefaultIrqBalanceConfigFile = "/etc/sysconfig/irqbalance"
+	// DefaultIrqBalanceConfigRestoreFile contains the banned cpu mask configuration to restore. Name due to backward compatibility.
+	DefaultIrqBalanceConfigRestoreFile = "/etc/sysconfig/orig_irq_banned_cpus"


I would expect this config option would mean we remove the hard-coded one in internal/runtimehandlerhooks/high_performance_hooks.go

also, given the name of the file, I wouldn't expect it to be a restore file. Is this file's name/format documented anywhere?

I think this PR provides an alternative param for the user to have more control over the restore process while keeping the already working way to do it, so it will keep the exact same behaviour if the user do not give the new option.
At least that is how I have understood the change but I could be wrong.

That's true. We keep the filename originally added back in time for full backward compatibility.

docs/crio.8.md

Use `internal/log`, not logrus directly in the rest of the high performance hooks code. To enable this, change the functions signatures to accept context.Context, which is a good idea in general. Signed-off-by: Francesco Romani <fromani@redhat.com>

Use ExpectWithOffset in the common hjelper to get a more immediate understanding of where a test failed. Signed-off-by: Francesco Romani <fromani@redhat.com>

Add logs in the irqbalance restore flow to improve the debuggability. This code runs once at startup, so the extra weight in the logs should be negligible. Signed-off-by: Francesco Romani <fromani@redhat.com>

jlojosnegros · 2023-02-22T11:14:50Z

rebase and minor typo ...

ffromani · 2023-02-22T11:17:57Z

/lgtm

haircommander · 2023-02-22T19:21:57Z

/override ci/prow/e2e-gcp-ovn
/override ci/prow/e2e-aws-ovn
LGTM

@saschagrunert PTAL for final approval

openshift-ci · 2023-02-22T19:25:56Z

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/e2e-aws-ovn, ci/prow/e2e-gcp-ovn

In response to this:

/override ci/prow/e2e-gcp-ovn
/override ci/prow/e2e-aws-ovn
LGTM

@saschagrunert PTAL for final approval

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.

Since cri-o#4441 , to support the dynamic irqbalance configuration in turn needed by the high performance hooks, CRI-O runs the restore code at startup. In some configuration scenarios it's preferred to have more control over this step. Some configuration wants to have more control over the mask to be restored, or to disable the restore flow entirely. For this reason, we add an option to provide a user-supplied restore file. Users wishing to exert more control can provide their data, or they can disable the flow entirely setting the path to the restore file "disable". The new option defaults are meant to ensure full backward compatibility. Signed-off-by: Francesco Romani <fromani@redhat.com>

Add integration tests for the irqbalance CPU ban list save/restore feature. Signed-off-by: Francesco Romani <fromani@redhat.com>

irqbalance tests relay on some file contents to work. If tests are run in parallel test will not find the expected content in the files and will fail. Signed-off-by: Jose Luis Ojosnegros Manchón <jojosneg@redhat.com>

jlojosnegros · 2023-02-23T08:56:40Z

Minor changes to solve doc and completion validations failures.

openshift-ci · 2023-02-23T10:39:52Z

@jlojosnegros: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-rhel-integration	`7ebbbe0`	link	true	`/test ci-rhel-integration`

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.

ffromani · 2023-02-23T11:06:47Z

/lgtm

haircommander · 2023-02-27T21:46:44Z

/override ci/prow/e2e-aws-ovn
/override ci/prow/e2e-gcp-ovn
/approve

openshift-ci · 2023-02-27T21:46:52Z

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/e2e-aws-ovn, ci/prow/e2e-gcp-ovn

In response to this:

/override ci/prow/e2e-aws-ovn
/override ci/prow/e2e-gcp-ovn
/approve

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.

openshift-ci · 2023-02-27T21:46:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, jlojosnegros

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~~ [haircommander]

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

jlojosnegros requested a review from mrunalp as a code owner November 22, 2022 12:21

openshift-ci bot requested review from QiWang19 and wgahnagl November 22, 2022 12:21

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 22, 2022

ffromani mentioned this pull request Nov 22, 2022

WIP: improve the irqbalance restore flow #6105

Closed

ffromani reviewed Nov 22, 2022

View reviewed changes

test/irqbalance.bats Outdated Show resolved Hide resolved

test/irqbalance.bats Outdated Show resolved Hide resolved

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 22, 2022

jlojosnegros force-pushed the irqbalance-restore branch from 42cb308 to b0e712a Compare November 22, 2022 15:06

jlojosnegros force-pushed the irqbalance-restore branch from b0e712a to e830f4f Compare November 24, 2022 07:45

openshift-ci bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 24, 2022

jlojosnegros force-pushed the irqbalance-restore branch from 94a1b0d to 7518ace Compare November 24, 2022 15:46

openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Nov 24, 2022

jlojosnegros force-pushed the irqbalance-restore branch 5 times, most recently from d722aa1 to ab817fd Compare November 28, 2022 09:43

openshift-ci bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 30, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2023

openshift-ci bot assigned haircommander Feb 16, 2023

haircommander reviewed Feb 20, 2023

View reviewed changes

TomSweeneyRedHat reviewed Feb 21, 2023

View reviewed changes

docs/crio.8.md Outdated Show resolved Hide resolved

ffromani added 3 commits February 22, 2023 12:13

highperfhooks: test: use ExpectWithOffset

3774df8

Use ExpectWithOffset in the common hjelper to get a more immediate understanding of where a test failed. Signed-off-by: Francesco Romani <fromani@redhat.com>

highperfhooks: add logs in the irqbalance restore

b267ed7

Add logs in the irqbalance restore flow to improve the debuggability. This code runs once at startup, so the extra weight in the logs should be negligible. Signed-off-by: Francesco Romani <fromani@redhat.com>

jlojosnegros force-pushed the irqbalance-restore branch from 1cb0f1a to d2a6860 Compare February 22, 2023 11:14

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2023

ffromani and others added 3 commits February 23, 2023 09:55

test: add integration test for irqbalance

b74d76c

Add integration tests for the irqbalance CPU ban list save/restore feature. Signed-off-by: Francesco Romani <fromani@redhat.com>

test: Avoid parallel execution

1a488f5

irqbalance tests relay on some file contents to work. If tests are run in parallel test will not find the expected content in the files and will fail. Signed-off-by: Jose Luis Ojosnegros Manchón <jojosneg@redhat.com>

jlojosnegros force-pushed the irqbalance-restore branch from d2a6860 to 1a488f5 Compare February 23, 2023 08:55

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2023

openshift-merge-robot merged commit a3efa73 into cri-o:main Feb 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve the irqbalance restore flow #6388

improve the irqbalance restore flow #6388

jlojosnegros commented Nov 22, 2022 •

edited

Loading

openshift-ci bot commented Nov 22, 2022

ffromani left a comment

ffromani commented Nov 22, 2022

ffromani commented Nov 23, 2022

ffromani commented Nov 23, 2022

jlojosnegros commented Feb 16, 2023

codecov bot commented Feb 20, 2023 •

edited

Loading

haircommander Feb 20, 2023

haircommander Feb 20, 2023

jlojosnegros Feb 22, 2023

ffromani Feb 22, 2023

jlojosnegros commented Feb 22, 2023

ffromani commented Feb 22, 2023

haircommander commented Feb 22, 2023

openshift-ci bot commented Feb 22, 2023

jlojosnegros commented Feb 23, 2023

openshift-ci bot commented Feb 23, 2023 •

edited

Loading

ffromani commented Feb 23, 2023

haircommander commented Feb 27, 2023

openshift-ci bot commented Feb 27, 2023

openshift-ci bot commented Feb 27, 2023

improve the irqbalance restore flow #6388

improve the irqbalance restore flow #6388

Conversation

jlojosnegros commented Nov 22, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

openshift-ci bot commented Nov 22, 2022

ffromani left a comment

Choose a reason for hiding this comment

ffromani commented Nov 22, 2022

ffromani commented Nov 23, 2022

ffromani commented Nov 23, 2022

jlojosnegros commented Feb 16, 2023

codecov bot commented Feb 20, 2023 • edited Loading

Codecov Report

haircommander Feb 20, 2023

Choose a reason for hiding this comment

haircommander Feb 20, 2023

Choose a reason for hiding this comment

jlojosnegros Feb 22, 2023

Choose a reason for hiding this comment

ffromani Feb 22, 2023

Choose a reason for hiding this comment

jlojosnegros commented Feb 22, 2023

ffromani commented Feb 22, 2023

haircommander commented Feb 22, 2023

openshift-ci bot commented Feb 22, 2023

jlojosnegros commented Feb 23, 2023

openshift-ci bot commented Feb 23, 2023 • edited Loading

ffromani commented Feb 23, 2023

haircommander commented Feb 27, 2023

openshift-ci bot commented Feb 27, 2023

openshift-ci bot commented Feb 27, 2023

jlojosnegros commented Nov 22, 2022 •

edited

Loading

codecov bot commented Feb 20, 2023 •

edited

Loading

openshift-ci bot commented Feb 23, 2023 •

edited

Loading