firecracker icon indicating copy to clipboard operation
firecracker copied to clipboard

Test `Failed to resize fdtable`

Open JonathanWoollett-Light opened this issue 2 years ago • 6 comments

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

JonathanWoollett-Light avatar Oct 02 '23 08:10 JonathanWoollett-Light

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.

roypat avatar Oct 04 '23 08:10 roypat

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.

JonathanWoollett-Light avatar Oct 04 '23 10:10 JonathanWoollett-Light

Agree with @roypat, this sounds more like a unit test where we can mock the result of a syscall.

pb8o avatar Oct 05 '23 07:10 pb8o

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 avatar Apr 16 '24 23:04 Ecazares15

@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.

JonathanWoollett-Light avatar Apr 17 '24 10:04 JonathanWoollett-Light

@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.

JonathanWoollett-Light avatar Apr 18 '24 17:04 JonathanWoollett-Light