Test `Failed to resize fdtable`
We test some logs but not all. We do not exactly match all log outputs from all tests.
A specific gap in this testing can be seen in https://github.com/firecracker-microvm/firecracker/pull/4047#discussion_r1342430678, this gap should be tested such that if it is changed accidentally in the future a test will fail.
This log:
if let Err(err) = resize_fdtable() {
match err {
// These errors are non-critical: In the worst case we have worse snapshot restore
// performance.
ResizeFdTableError::GetRlimit | ResizeFdTableError::Dup2(_) => {
vmm::logger::debug!("Failed to resize fdtable: {err}")
}
// This error means that we now have a random file descriptor lying around, abort to be
// cautious.
ResizeFdTableError::Close(_) => return Err(MainError::ResizeFdtable(err)),
}
}
See https://github.com/firecracker-microvm/firecracker/blob/main/src/firecracker/src/main.rs#L116
I don't really think we need to test every single log message (or even just this one specifically, because why this one specifically?). I also feel like this one in particular will be hard to test because I'm not even sure how to setup a test case that causes the getrlimit or dup2 syscall to fail.
I don't really think we need to test every single log message
I don't disagree. Its not really worth the effort as anything beyond an on-boarding task. But it still brings a very small amount of value, so as a task a contributor might take when learning Firecracker it could make sense.
because why this one specifically?
Got to start somewhere.
I also feel like this one in particular will be hard to test because I'm not even sure how to setup a test case that causes the getrlimit or dup2 syscall to fail.
All the more reason to test it.
Agree with @roypat, this sounds more like a unit test where we can mock the result of a syscall.
Hello!
We are students from the University of Texas at Austin taking a virtualization course (cs360v) looking for opportunities to contribute to an open source project for class credit.
Could I be assigned to this?
@Ecazares15 Hi, it would be great if you could contribute to this issue. Feel free to just post a PR next time you don't need to ask.
@Ecazares15 I would hold off on working on this right now, we are re-evaluating if we want this change, I've marked the issue as parked for now.