Add Cgroup V1 block IO integration test #537
Conversation
|
The CI is likely to fail for the same reason as #528 (comment) |
|
Oh yes, I tested youki with that change and it passes the tests as well. I'm waiting for that PR to be merged so it will pass in the CI itself, and the I can ask for reviews. Thanks 👍 |
Codecov Report
@@ Coverage Diff @@
## main #537 +/- ##
=======================================
Coverage 60.52% 60.52%
=======================================
Files 98 98
Lines 12626 12626
=======================================
Hits 7642 7642
Misses 4984 4984 |
| let weight_string = fs::read_to_string(format!("{}/blkio.weight", path)) | ||
| .context("error in reading block io weight")?; | ||
| device.weight = weight_string | ||
| .parse() | ||
| .context("error in parsing block io weight")?; |
There was a problem hiding this comment.
| let weight_string = fs::read_to_string(format!("{}/blkio.weight", path)) | |
| .context("error in reading block io weight")?; | |
| device.weight = weight_string | |
| .parse() | |
| .context("error in parsing block io weight")?; | |
| let weight_path = Path::new(path).join("blkio.weight"); | |
| let weight_string = fs::read_to_string(weight_path) | |
| .with_context(|| format!("error in reading block io weight from {:?}", weight_path)?; | |
| device.weight = weight_string | |
| .parse() | |
| .with_context(|| format!("error in parsing block io weight {:?}", weight_string))?; |
There are a few more like this. You should include the full cgroup path that it attempted to read to make debugging easier.
| let leaf_weight_string = fs::read_to_string(format!("{}/blkio.leaf_weight", path)) | ||
| .context("error in reading block io leaf weight")?; | ||
| device.leaf_weight = leaf_weight_string | ||
| .parse() | ||
| .context("error in parsing block io weight")?; |
| for line in device_weight_string.lines() { | ||
| let (device_data, weight) = line | ||
| .split_once(" ") | ||
| .context("invalid weight device format")?; |
There was a problem hiding this comment.
| .context("invalid weight device format")?; | |
| with_context(|| format!("invalid weight device format: {}", line)?; |
Again there are a few more instances of this. The general idea is that if you are parsing something and it fails you should include the value it tried to parse so that it is included in the output and you do not need to go hunting after the value on which it failed.
| let (device_data, rate) = line | ||
| .split_once(" ") | ||
| .context("invalid weight device format")?; | ||
| let (major_str, minor_str) = device_data | ||
| .split_once(":") | ||
| .context("invalid major-minor number format")?; |
There was a problem hiding this comment.
This snippet seems to be used a few times. Could be worth creating a method for this.
| // weight ------ | ||
| if supports_weight() { | ||
| if spec_block_io.weight().is_none() { | ||
| return Err(anyhow::anyhow!("spec block io weight is none")); |
There was a problem hiding this comment.
| return Err(anyhow::anyhow!("spec block io weight is none")); | |
| bail!("spec block io weight is none"); |
A few more instances of this.
|
Hey, sorry for the mistakes 😅 😅 I have fixed that, as well as improved error reporting as you have suggested. One of the issue that it caused is that we need to clone the path, as fs::read_to_string takes the path by value. I'm not sure why read_to_string needs to take it by value, as I checked rust src, and it eventually calls as_ref on it, which needs only by ref, not by value... 🤔 Anyways, please take a look now, thanks :) |
Can you point to a specific example? PathBuf implements AsRef so you should not need to clone it. |
YJDoc2
force-pushed
the
cgroups_blkio
branch
from
1a1891f to
c40a306
Compare
Yes you're right, I have been using it incorrectly 😓 I have removed the clone calls, so PTAL. |
Thank you for taking care of my pr. I still have a few things to do on that PR of mine, so you can merge this one first! |
This adds Cgroups V1 block io integration test