Skip to content

Support DriveMount API in CreateVM. #297

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

Merged
merged 2 commits into from
Oct 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 122 additions & 0 deletions agent/drive_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,20 @@
package main

import (
"context"
"fmt"
"io/ioutil"
"os"
"path/filepath"
"strings"
"time"

"github.com/containerd/containerd/log"
"github.com/containerd/containerd/mount"
"github.com/firecracker-microvm/firecracker-containerd/internal"
drivemount "github.com/firecracker-microvm/firecracker-containerd/proto/service/drivemount/ttrpc"
"github.com/golang/protobuf/ptypes/empty"
"github.com/pkg/errors"
)

const (
Expand All @@ -28,6 +36,14 @@ const (
blockMajorMinor = "dev"
)

var (
bannedSystemDirs = []string{
"/proc",
"/sys",
"/dev",
}
)

type drive struct {
Name string
DriveID string
Expand All @@ -45,6 +61,8 @@ type driveHandler struct {
DrivePath string
}

var _ drivemount.DriveMounterService = &driveHandler{}

func newDriveHandler(blockPath, drivePath string) (*driveHandler, error) {
d := &driveHandler{
drives: map[string]drive{},
Expand Down Expand Up @@ -146,3 +164,107 @@ func isStubDrive(d drive) bool {

return internal.IsStubDrive(f)
}

func (dh driveHandler) MountDrive(ctx context.Context, req *drivemount.MountDriveRequest) (*empty.Empty, error) {
logger := log.G(ctx)
logger.Debugf("%+v", req.String())
logger = logger.WithField("drive_id", req.DriveID)

drive, ok := dh.GetDrive(req.DriveID)
if !ok {
return nil, fmt.Errorf("drive %q could not be found", req.DriveID)
}
logger = logger.WithField("drive_path", drive.Path())

// Do a basic check that we won't be mounting over any important system directories
resolvedDest, err := evalAnySymlinks(req.DestinationPath)
if err != nil {
return nil, errors.Wrapf(err,
"failed to evaluate any symlinks in drive mount destination %q", req.DestinationPath)
}

for _, systemDir := range bannedSystemDirs {
if isOrUnderDir(resolvedDest, systemDir) {
return nil, errors.Errorf(
"drive mount destination %q resolves to path %q under banned system directory %q",
req.DestinationPath, resolvedDest, systemDir,
)
}
}

err = os.MkdirAll(req.DestinationPath, 0700)
if err != nil {
return nil, errors.Wrapf(err, "failed to create drive mount destination %q", req.DestinationPath)
}

// Retry the mount in the case of failure a fixed number of times. This works around a rare issue
// where we get to this mount attempt before the guest OS has realized a drive was patched:
// https://github.com/firecracker-microvm/firecracker-containerd/issues/214
const (
maxRetries = 100
retryDelay = 10 * time.Millisecond
)

for i := 0; i < maxRetries; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which cases drive might not be mountable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can happen when we've patched a stub drive but the OS has not yet realized a change was made, which is rare but did happen in practice occasionally. We needed these retries in the previous implementation too, I just migrated the code as part of this change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think this is useful context and it worth to leave comments explaining the motivation behind retries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good call, added a comment w/ link to original issue

err := mount.All([]mount.Mount{{
Source: drive.Path(),
Type: req.FilesytemType,
Options: req.Options,
}}, req.DestinationPath)
if err == nil {
return &empty.Empty{}, nil
}

if isRetryableMountError(err) {
logger.WithError(err).Warnf("retryable failure mounting drive")
time.Sleep(retryDelay)
continue
}

return nil, errors.Wrapf(err, "non-retryable failure mounting drive from %q to %q",
drive.Path(), req.DestinationPath)
}

return nil, errors.Errorf("exhausted retries mounting drive from %q to %q",
drive.Path(), req.DestinationPath)
}

// evalAnySymlinks is similar to filepath.EvalSymlinks, except it will not return an error if part of the
// provided path does not exist. It will evaluate symlinks present in the path up to a component that doesn't
// exist, at which point it will just append the rest of the provided path to what has been resolved so far.
// We validate earlier that input to this function is an absolute path.
func evalAnySymlinks(path string) (string, error) {
curPath := "/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment here saying that we validate that the curPath must be at /?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not only do we validate it, but we override it if it isn't.

IMO this function should be given a name that more clearly indicates what it does. Converting a relative path to an absolute path doesn't have anything to do with symlinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this function is not to convert a relative path into an absolute path, it's to see if any part of the provided path already exists and, if so, evaluate any symlinks in the part of the path that exists. I needed to add it because the standard EvalSymlinks function in the standard lib will always return an error if any part of the provided path doesn't exist yet.

I.e. if you are given /foo/bar/baz/ and /foo is a symlink to /other, but /other/bar/baz doesn't exist yet, then EvalSymlinks will return an error. evalAnySymlinks, on the other hand, will return /other/bar/baz.

I updated the comment on the function to also state that it's assumed path is an absolute path, which is validation we do earlier. Given this is just a private, one-off util function only used in this package, I'm okay with that, but let me know if there's disagreement.

pathSplit := strings.Split(filepath.Clean(path), "/")
for len(pathSplit) > 0 {
curPath = filepath.Join(curPath, pathSplit[0])
pathSplit = pathSplit[1:]

resolvedPath, err := filepath.EvalSymlinks(curPath)
if os.IsNotExist(err) {
return filepath.Join(append([]string{curPath}, pathSplit...)...), nil
}
if err != nil {
return "", err
}
curPath = resolvedPath
}

return curPath, nil
}

// returns whether the given path is the provided baseDir or is under it
func isOrUnderDir(path, baseDir string) bool {
path = filepath.Clean(path)
baseDir = filepath.Clean(baseDir)

if baseDir == "/" {
return true
}

if path == baseDir {
return true
}

return strings.HasPrefix(path, baseDir+"/")
}
126 changes: 126 additions & 0 deletions agent/drive_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,14 @@
package main

import (
"os"
"path/filepath"
"strconv"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestDiscoverDrives(t *testing.T) {
Expand All @@ -39,3 +44,124 @@ func TestDiscoverDrives(t *testing.T) {
})
}
}

func TestEvalAnySymlinks(t *testing.T) {
existingSymlink := "/proc/self/cwd" // will always exist and be a symlink to our cwd
nonExistentPath := filepath.Join(strconv.Itoa(int(time.Now().UnixNano())), "foo")
testPath := filepath.Join(existingSymlink, nonExistentPath)

resolvedPath, err := evalAnySymlinks(testPath)
require.NoError(t, err, "failed to evaluate symlinks in %q", testPath)

cwd, err := os.Getwd()
require.NoError(t, err, "failed to get current working dir")
assert.Equal(t, filepath.Join(cwd, nonExistentPath), resolvedPath)
}

func TestIsOrUnderDir(t *testing.T) {
type testcase struct {
baseDir string
path string
expectedTrue bool
}

for _, tc := range []testcase{
{
baseDir: "/foo",
path: "/foo/bar",
expectedTrue: true,
},
{
baseDir: "/foo/bar",
path: "/foo/bar/baz",
expectedTrue: true,
},
{
baseDir: "/foo",
path: "/foo",
expectedTrue: true,
},
{
baseDir: "/foo/bar",
path: "/foo/bar",
expectedTrue: true,
},
{
baseDir: "/foo",
path: "/foobar",
expectedTrue: false,
},
{
baseDir: "/foo",
path: "/bar",
expectedTrue: false,
},
{
baseDir: "/foo/bar",
path: "/bar",
expectedTrue: false,
},
{
baseDir: "/foo/bar",
path: "/foo",
expectedTrue: false,
},
{
baseDir: "/foo/bar",
path: "/bar/bar",
expectedTrue: false,
},
{
baseDir: "/foo",
path: "foo",
expectedTrue: false,
},
{
baseDir: "/foo",
path: "bar",
expectedTrue: false,
},
{
baseDir: "/foo",
path: "/foo/../foo",
expectedTrue: true,
},
{
baseDir: "/foo/bar",
path: "/foo/../foo/bar",
expectedTrue: true,
},
{
baseDir: "/foo",
path: "/foo/../bar",
expectedTrue: false,
},
{
baseDir: "/foo",
path: "/foo/..bar",
expectedTrue: true,
},
{
baseDir: "/foo",
path: "/foo/..bar/baz",
expectedTrue: true,
},
{
baseDir: "/",
path: "/",
expectedTrue: true,
},
{
baseDir: "/foo",
path: "/",
expectedTrue: false,
},
{
baseDir: "/",
path: "/foo",
expectedTrue: true,
},
} {
assert.Equalf(t, tc.expectedTrue, isOrUnderDir(tc.path, tc.baseDir), "unexpected output for isOrUnderDir case %+v", tc)
}
}
18 changes: 13 additions & 5 deletions agent/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import (
"github.com/firecracker-microvm/firecracker-containerd/eventbridge"
"github.com/firecracker-microvm/firecracker-containerd/internal/event"
"github.com/firecracker-microvm/firecracker-containerd/internal/vm"

drivemount "github.com/firecracker-microvm/firecracker-containerd/proto/service/drivemount/ttrpc"
)

const (
Expand Down Expand Up @@ -77,19 +79,25 @@ func main() {

log.G(shimCtx).Info("creating task service")

server, err := ttrpc.NewServer()
if err != nil {
log.G(shimCtx).WithError(err).Fatal("failed to create ttrpc server")
}

eventExchange := &event.ExchangeCloser{Exchange: exchange.NewExchange()}
eventbridge.RegisterGetterService(server, eventbridge.NewGetterService(shimCtx, eventExchange))

taskService, err := NewTaskService(shimCtx, shimCancel, eventExchange)
if err != nil {
log.G(shimCtx).WithError(err).Fatal("failed to create task service")
}
taskAPI.RegisterTaskService(server, taskService)

server, err := ttrpc.NewServer()
dh, err := newDriveHandler(blockPath, drivePath)
if err != nil {
log.G(shimCtx).WithError(err).Fatal("failed to create ttrpc server")
log.G(shimCtx).WithError(err).Fatal("failed to create drive handler")
}

taskAPI.RegisterTaskService(server, taskService)
eventbridge.RegisterGetterService(server, eventbridge.NewGetterService(shimCtx, eventExchange))
drivemount.RegisterDriveMounterService(server, dh)

// Run ttrpc over vsock

Expand Down
Loading