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

StopVMM doesn't wait cleanup to be done #428

Closed
Joffref opened this issue Jul 29, 2022 · 0 comments · Fixed by #429
Closed

StopVMM doesn't wait cleanup to be done #428

Joffref opened this issue Jul 29, 2022 · 0 comments · Fixed by #429

Comments

@Joffref
Copy link
Contributor

Joffref commented Jul 29, 2022

What is the bug ?

StopVMM doesn't wait cleanup to be done. That means, if your main thread closes directly after calling StopVMM method, cleanup will not be done (e.g delete CNI).
As a SDK user I except StopVMM ensure cleanup.

How to reproduce

The following code reproduces this behavior.

package main

import (
    "context"
    "fmt"
    log "github.com/sirupsen/logrus"
    "os"
    "github.com/firecracker-microvm/firecracker-go-sdk"
    "github.com/firecracker-microvm/firecracker-go-sdk/client/models"
)

func main() {
    vmConfig := firecracker.Config{
        SocketPath:      "/tmp/firecracker.sock",
        LogPath:         "/tmp/test_log",
        LogLevel:        "Debug",
        KernelImagePath: "/home/ubuntu/hello-vmlinux.bin",
        Drives:          firecracker.NewDrivesBuilder("/home/ubuntu/hello-rootfs.ext4").Build(),
        NetworkInterfaces: firecracker.NetworkInterfaces{
            {
                CNIConfiguration: &firecracker.CNIConfiguration{
                    NetworkName: "default",
                    IfName:      "test",
                },
            },
        },
        MachineCfg: models.MachineConfiguration{
            MemSizeMib: firecracker.Int64(252),
            Smt:        firecracker.Bool(true),
            VcpuCount:  firecracker.Int64(1),
        },
    }
    // stdout will be directed to this file
    stdoutPath := "stdout.log"
    stdout, err := os.OpenFile(stdoutPath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0644)
    if err != nil {
        panic(fmt.Errorf("failed to create stdout file: %v", err))
    }

    // stderr will be directed to this file
    stderrPath := "stderr.log"
    stderr, err := os.OpenFile(stderrPath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0644)
    if err != nil {
        panic(fmt.Errorf("failed to create stderr file: %v", err))
    }

    ctx := context.Background()
    // build our custom command that contains our two files to
    // write to during process execution
    cmd := firecracker.VMCommandBuilder{}.
        WithBin("/home/ubuntu/firecracker").
        WithSocketPath("/tmp/firecracker.sock").
        WithStdout(stdout).
        WithStderr(stderr).
        Build(ctx)
    lg := log.New()
    log.SetOutput(os.Stdout)
    lg.SetLevel(log.DebugLevel)
    m, err := firecracker.NewMachine(ctx, vmConfig, firecracker.WithProcessRunner(cmd), firecracker.WithLogger(lg.WithField("driver", "firecracker")))
    if err != nil {
        panic(fmt.Errorf("failed to create new machine: %v", err))
    }
    if err := m.Start(ctx); err != nil {
        return
    }
    defer os.Remove(vmConfig.SocketPath)

	  if err := m.StopVMM(); err != nil {
		  panic(err)
	  }
} 

Where does it came from ?

Under the hood cleaning up is handle by a goroutine that doesn't propagate its state to the stopVMM method, so stopVMM doesn't wait on this goroutine to finish.
stopVMM should wait on cleaning to finish, so the goroutine must notify stopVMM that VM has been torn down properly.

protochron added a commit to protochron/firecracker-task-driver that referenced this issue Aug 10, 2022
This is panicking because the veth name doesn't match the name in the
container. It also leaks the network namespace, and isn't necessary once
firecracker-microvm/firecracker-go-sdk#428 is
in.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant