firecracker-microvm / firecracker

Secure and fast microVMs for serverless computing.
http://firecracker-microvm.io
Apache License 2.0
25.36k stars 1.76k forks source link

Test `Failed to resize fdtable` #4143

Open JonathanWoollett-Light opened 1 year ago

JonathanWoollett-Light commented 1 year ago

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

roypat commented 12 months ago

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.

JonathanWoollett-Light commented 12 months ago

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.

pb8o commented 12 months ago

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

Ecazares15 commented 5 months ago

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?

JonathanWoollett-Light commented 5 months ago

@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 commented 5 months ago

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