Support rate-limiters and read-only for drive mounts. #332
Conversation
sipsma
requested review from
kzys,
nmeyerhans,
samuelkarp,
mxpv,
xibz and
IRCody
proto/types.proto
Outdated
| // (Optional) If set to true, IsReadOnly results in the backing file for the | ||
| // drive being opened as read-only by the Firecracker VMM on the host, preventing | ||
| // any writes to the image from within the guest. | ||
| bool IsReadOnly = 6; |
There was a problem hiding this comment.
Can we negate the flag to IsWritable? Mostly because FirecrackerRootDrive has that.
There was a problem hiding this comment.
Good call, updated. As a result I needed to add a little more functionality though because Linux will default a mount to "rw" if not otherwise specified. So this meant that if a DriveMount was read-only by default then the user would get an error unless they explicitly set "ro" in their mount options, which kind of sucks. So I updated the implementation to check that the IsWritable field matches what was specified in the mount options or, if there was no ro/rw option specified, explicitly sets it on behalf of the user.
|
|
||
| return paths, nil | ||
| func stubPathToDriveID(stubPath string) string { |
There was a problem hiding this comment.
Can you explain which characters are problematic in a comment?
There was a problem hiding this comment.
Base64 will remove "/" but may contain "-" in the output. We could use Base32 instead.
There was a problem hiding this comment.
Good catch! I didn't know that. Switched to Base32 and also switched to using just the base name of the stub file (which is unique per shim/vm anyways) so the length of the drive ids doesn't get too outrageous.
There was a problem hiding this comment.
Can we add a test that has an invalid stubPath just to ensure that this doesn't regress and is being used properly?
There was a problem hiding this comment.
Can we add a test that has an invalid stubPath just to ensure that this doesn't regress and is being used properly?
I'm not sure I follow how we could create an invalid stubPath as the implementation of our code now shouldn't have any invalid values for a stubPath. Would the test create a drive id with an invalid character and then provide that directly to the firecracker API? That wouldn't really be a test of our code.
The only thing I can think of here would be to do some kind of general fuzz testing on our api to try to find inputs that create errors, but that seems like a much, much larger effort outside of scope here.
There was a problem hiding this comment.
Would the test create a drive id with an invalid character and then provide that directly to the firecracker API? That wouldn't really be a test of our code.
Yea, I was thinking this, just to ensure that it is encoding it properly. You should then be able to read the stub drive at the given base32 path
There was a problem hiding this comment.
Ohh you just mean this function in isolation, I see, I was thinking about some way to get an invalid stub path passed to this function. Yeah I will add a test that does a regex check on the output of this function for some paths with invalid chars.
There was a problem hiding this comment.
Updated with that test
sipsma
force-pushed
the
drivemount-rl-ro
branch
2 times, most recently
from
02cd13e to
2182d33
Compare
| type StubDriveHandler struct { | ||
| freeDrives []*stubDrive | ||
| // map of id -> stub drive being used by that task | ||
| usedDrives map[string]*stubDrive |
There was a problem hiding this comment.
What is the purpose of this field? I don't really see it used anywhere other than being set
There was a problem hiding this comment.
I'm just laying some groundwork for the implementation of #243 to build off of, which I am personally okay with since it's not complicating anything at the moment. If there's disagreement with including it even though it technically doesn't need to exist yet, then I can remove for now, let me know.
There was a problem hiding this comment.
Ah, okay. I was just making sure it was there intentionally. I am okay with leaving it.
|
|
||
| return paths, nil | ||
| func stubPathToDriveID(stubPath string) string { |
There was a problem hiding this comment.
Can we add a test that has an invalid stubPath just to ensure that this doesn't regress and is being used properly?
sipsma
force-pushed
the
drivemount-rl-ro
branch
from
2182d33 to
ea5fa1d
Compare
Signed-off-by: Erik Sipsma <[email protected]>
sipsma
force-pushed
the
drivemount-rl-ro
branch
from
ea5fa1d to
76b3fbe
Compare
Memory ballooning support on firecracker-go-sdk. Add wrappers for the Firecracker endpoints "/ballloon", "/balloon/statistics". Signed-off-by: Royce Zhao <[email protected]>
includes firecracker-microvm#289, firecracker-microvm#317, firecracker-microvm#328, firecracker-microvm#329, firecracker-microvm#330, firecracker-microvm#332 Signed-off-by: Bora M. Alper <[email protected]>
Signed-off-by: Erik Sipsma [email protected]
Resolves #296
There is an unrelated failing integ test that seems like it will be fixed by #331
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.