Skip to content

Conversation

@giuseppe
Copy link
Member

change the device used for block I/O limits (read/write BPS and IOPS).

Since /dev/zero is not a block device, it's unsuitable for testing cgroup controls designed for block devices.

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 30, 2025
@giuseppe giuseppe force-pushed the test-do-not-set-limits-on-dev-zero branch 5 times, most recently from f4fd259 to cfc811a Compare April 30, 2025 12:35
@packit-as-a-service
Copy link

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

@giuseppe giuseppe force-pushed the test-do-not-set-limits-on-dev-zero branch from cfc811a to fd918cb Compare April 30, 2025 13:12
@giuseppe giuseppe changed the title [WIP][DO NOT REVIEW] citest: Use block devices for I/O limit tests test: use block devices for I/O limit Apr 30, 2025
@giuseppe giuseppe changed the title test: use block devices for I/O limit test: use block devices for I/O limit tests Apr 30, 2025
@giuseppe giuseppe marked this pull request as ready for review April 30, 2025 13:13
@openshift-ci 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 Apr 30, 2025
@giuseppe
Copy link
Member Author

@Luap99 PTAL

@giuseppe giuseppe force-pushed the test-do-not-set-limits-on-dev-zero branch from fd918cb to 3ec6c51 Compare April 30, 2025 14:00
@Luap99 Luap99 added the 5.5 label Apr 30, 2025
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, a small hint for module loading would be nice. These tests are run in many external CI systems so it would be helpful for them to know how to enable this test without having to search for this specific kernel device.

}
}

func SkipIfNotExist(path string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a reason field, it would be nice to pass a hint, i.e. run modprobe null_blk nr_devices=1 so external consumers know directly how to make the test work.

skip_if_rootless "cannot use this flag in rootless mode"

if test \! -e /dev/nullb0; then
skip "/dev/nullb0 not present"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here include the modprobe null_blk nr_devices=1 info please

Cgroup block I/O limits cannot be applied to character devices.

Ignore character devices in the inspect output.

Update the API tests to use the null block device `/dev/nullb0` (if
available) instead of `/dev/zero` for testing I/O limits.

Signed-off-by: Giuseppe Scrivano <[email protected]>
The tests were incorrectly using `/dev/zero`.  These options are
intended to set I/O limits on specific block devices.

The test already sets up a loopback device, so reuse it.

Signed-off-by: Giuseppe Scrivano <[email protected]>
The tests for device I/O limits were using `/dev/zero`,
which is not a block device suitable for these cgroup
controls.

Update the tests to use `/dev/nullb0` if it exists.

Signed-off-by: Giuseppe Scrivano <[email protected]>
Load the `null_blk` kernel module to have the `/dev/nullb0` device
available for the tests.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the test-do-not-set-limits-on-dev-zero branch from 3ec6c51 to 80065f7 Compare April 30, 2025 14:21
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

/cherry-pick v5.5

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 30, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@Luap99
Copy link
Member

Luap99 commented Apr 30, 2025

@siretart @ricardobranco777 As I know you run these tests downstream just a heads up that you may have to run modprobe null_blk nr_devices=1 on your test systems to keep coverage of this test otherwise it will get skipped.

@ricardobranco777
Copy link
Contributor

Since /dev/zero is not a block device, it's unsuitable for testing cgroup controls designed for block devices.

Thanks for the heads-up!

@giuseppe
Copy link
Member Author

@mheon PTAL

@mheon
Copy link
Member

mheon commented Apr 30, 2025

/lgtm

Too late to make RC2 unfortunately, but it should be in 5.5 final

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 8f22a0c into containers:main Apr 30, 2025
83 checks passed
@Luap99
Copy link
Member

Luap99 commented May 2, 2025

/cherry-pick v5.5

@openshift-cherrypick-robot
Copy link
Collaborator

@Luap99: new pull request created: #26049

Details

In response to this:

/cherry-pick v5.5

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-sigs/prow repository.

@Luap99
Copy link
Member

Luap99 commented May 2, 2025

@lsm5 @jnovy We will also need to run modprobe null_blk nr_devices=1 in the fedora/RHEL gating envs before the tests.

@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 1, 2025
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Aug 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

5.5 approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants