Support DriveMount API in CreateVM. #297

sipsma · 2019-10-17T22:34:57Z

This implements the proposal found in docs/drive-mounts-proposal.md, with the
exception for supporting RateLimiters and IsReadOnly in the DriveMount objects,
due to the need for further refactoring of the internal stub drive code (will
be followed up in #296).

Signed-off-by: Erik Sipsma [email protected]

This change will require a fair bit of non-trivial rebasing with the jailer PR, so I'd like to let that PR get merged first and then rebase this one on top if at all possible.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

kzys · 2019-10-17T23:08:17Z

agent/drive_handler.go

+	}
+
+	for _, systemDir := range bannedSystemDirs {
+		if strings.HasPrefix(filepath.Clean(resolvedDest), filepath.Clean(systemDir)) {


You don't have to call filepath.Clean() on systemDir, while it is harmless

Wouldn't it match a path that technically contain systemDir such as /system?

Good point on it matching /system. Will fix

kzys · 2019-10-17T23:17:38Z

agent/drive_handler.go

+	logger := log.G(ctx).WithField("MountDriveRequest", req.String())
+	logger.Debug()
+
+	driveID := strings.TrimSpace(req.DriveID)


Can't we just use req.DriveID as is?

Yeah I think so, this was just copied from the previous implementation, but unless I'm missing something TrimSpace shouldn't be needed, will update

xibz · 2019-10-17T23:05:44Z

agent/drive_handler.go

+// 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.
+func evalAnySymlinks(path string) (string, error) {
+	curPath := "/"


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

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.

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.

xibz · 2019-10-17T23:17:05Z

agent/service.go

+		if err != nil {
+			cleanupErr := ts.doCleanup(taskID, execID)
+			if cleanupErr != nil {
+				logger.WithError(err).Error("failed to cleanup task")


Do we not want to log the cleanup error as well?

We do, this was an oversight, will fix

xibz · 2019-10-17T23:19:54Z

agent/service.go

+		if err != nil {
+			cleanupErr := ts.doCleanup(taskID, execID)
+			if cleanupErr != nil {
+				logger.WithError(err).Error("failed to cleanup task")


Same comment here regarding not logging cleanupErr

xibz · 2019-10-17T23:24:19Z

internal/fsutil.go

+		}
+
+		// see man 5 fstab for the format of /proc/mounts
+		var source string


would prefer

var ( source string dest string ... )

xibz · 2019-10-17T23:28:33Z

runtime/service.go

+		}
+
+		_, err = s.driveMountClient.MountDrive(requestCtx, &drivemount.MountDriveRequest{
+			DriveID:         *driveID,


Should use firecracker.StringValue here to prevent panics

agent/drive_handler.go

agent/service.go

mxpv · 2019-10-17T23:30:45Z

agent/drive_handler.go

+		retryDelay = 10 * time.Millisecond
+	)
+
+	for i := 0; i < maxRetries; i++ {


In which cases drive might not be mountable?

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.

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

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

xibz · 2019-10-17T23:44:49Z

runtime/service.go


+func (s *service) mountDrives(requestCtx context.Context, driveMounts []*proto.FirecrackerDriveMount) error {
+	for _, driveMount := range driveMounts {
+		driveID, err := s.stubDriveHandler.PatchStubDrive(requestCtx, s.machine, driveMount.HostPath)


PatchStubDrive should return a string as opposed to *string.

mxpv

LGTM

mxpv · 2019-10-18T22:04:58Z

agent/drive_handler.go

+		retryDelay = 10 * time.Millisecond
+	)
+
+	for i := 0; i < maxRetries; i++ {


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

kzys

LGTM

sipsma · 2019-10-21T18:05:23Z

Have the approvals now, but will be waiting until the Jailer PR is merged and do a rebase on top of that before merging this one. Any more comments in the meantime are welcome though of course.

sipsma · 2019-10-24T22:37:36Z

Rebased on the Jailer PR. It ended up being straightforward to resolve conflicts, nothing substantive had to change in the drive mount logic or the jailer logic.

sipsma · 2019-10-25T18:55:40Z

I just realized that because the jailer config is not enabled by default there currently aren't integ-tests that use both drive mounts and the jailer, I'm gonna update with that before merging too. Looking at the jailer code again I think this will require some updates to the "ExposeDeviceToJail" method in order to handle exposing flat files in addition to exposing block devices.

This implements the proposal found in docs/drive-mounts-proposal.md, with the exception for supporting RateLimiters and IsReadOnly in the DriveMount objects, due to the need for further refactoring of the internal stub drive code (will be followed up in firecracker-microvm#296). Signed-off-by: Erik Sipsma <[email protected]>

The primary change required is for the Jailer to support exposing regular files to the jail in addition to the pre-existing support for block devices. Signed-off-by: Erik Sipsma <[email protected]>

@xibz

Addressed @xibz's comments

sipsma requested review from kzys, nmeyerhans, samuelkarp, mxpv, xibz and IRCody October 17, 2019 22:34

kzys reviewed Oct 17, 2019

View reviewed changes

xibz previously requested changes Oct 17, 2019

View reviewed changes

mxpv reviewed Oct 17, 2019

View reviewed changes

xibz reviewed Oct 17, 2019

View reviewed changes

sipsma force-pushed the drivemount branch from ab326d5 to f2312e5 Compare October 18, 2019 01:40

mxpv approved these changes Oct 18, 2019

View reviewed changes

kzys approved these changes Oct 21, 2019

View reviewed changes

sipsma force-pushed the drivemount branch from e6c4ef0 to f375d41 Compare October 21, 2019 17:44

sipsma force-pushed the drivemount branch 3 times, most recently from 807b42b to 0f7ac63 Compare October 24, 2019 22:18

sipsma requested review from mxpv, xibz and kzys October 24, 2019 22:37

kzys approved these changes Oct 25, 2019

View reviewed changes

sipsma added 2 commits October 25, 2019 21:37

Integrate drive mounts with jailer.

9f0eb50

The primary change required is for the Jailer to support exposing regular files to the jail in addition to the pre-existing support for block devices. Signed-off-by: Erik Sipsma <[email protected]>

sipsma force-pushed the drivemount branch from 0f7ac63 to 9f0eb50 Compare October 25, 2019 21:51

nmeyerhans approved these changes Oct 25, 2019

View reviewed changes

sipsma merged commit 38742ee into firecracker-microvm:master Oct 25, 2019

sipsma deleted the drivemount branch October 25, 2019 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support DriveMount API in CreateVM. #297

Support DriveMount API in CreateVM. #297

sipsma commented Oct 17, 2019

kzys Oct 17, 2019

sipsma Oct 17, 2019

kzys Oct 17, 2019

sipsma Oct 17, 2019

xibz Oct 17, 2019

nmeyerhans Oct 21, 2019

sipsma Oct 21, 2019

xibz Oct 17, 2019

sipsma Oct 17, 2019

xibz Oct 17, 2019

xibz Oct 17, 2019

xibz Oct 17, 2019

mxpv Oct 17, 2019

sipsma Oct 17, 2019

mxpv Oct 18, 2019

sipsma Oct 21, 2019

xibz Oct 17, 2019 •

edited

Loading

sipsma Oct 18, 2019

mxpv left a comment

mxpv Oct 18, 2019

kzys left a comment

sipsma commented Oct 21, 2019

sipsma commented Oct 24, 2019

sipsma commented Oct 25, 2019

Support DriveMount API in CreateVM. #297

Support DriveMount API in CreateVM. #297

Conversation

sipsma commented Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xibz Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxpv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzys left a comment

Choose a reason for hiding this comment

sipsma commented Oct 21, 2019

sipsma commented Oct 24, 2019

sipsma commented Oct 25, 2019

xibz Oct 17, 2019 •

edited

Loading