Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test Failed to resize fdtable #4143

Open
JonathanWoollett-Light opened this issue Oct 2, 2023 · 6 comments
Open

Test Failed to resize fdtable #4143

JonathanWoollett-Light opened this issue Oct 2, 2023 · 6 comments
Labels
Status: Parked Indicates that an issues or pull request will be revisited later

Comments

@JonathanWoollett-Light
Copy link
Contributor

JonathanWoollett-Light commented Oct 2, 2023

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 #4047 (comment), 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 JonathanWoollett-Light added the Good first issue Indicates a good issue for first-time contributors label Oct 2, 2023
@roypat
Copy link
Contributor

roypat commented Oct 4, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

pb8o commented Oct 5, 2023

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

@Ecazares15
Copy link
Contributor

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
Copy link
Contributor Author

JonathanWoollett-Light commented Apr 17, 2024

@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
Copy link
Contributor Author

@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 JonathanWoollett-Light added Status: Parked Indicates that an issues or pull request will be revisited later and removed Good first issue Indicates a good issue for first-time contributors labels Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Parked Indicates that an issues or pull request will be revisited later
Projects
None yet
Development

No branches or pull requests

4 participants