Add sandboxed e2e tests for host resources. - #2959
Conversation
7b7ea00 to
6d09f82
Compare
GoogleCuttlefishTesterBot
removed
the
kokoro:force-run
| func ensurePath() { | ||
| parts := strings.Split(os.Getenv("PATH"), ":") | ||
| have := map[string]bool{} | ||
| for _, p := range parts { | ||
| have[p] = true | ||
| } | ||
| for _, e := range []string{"/usr/sbin", "/usr/bin", "/sbin", "/bin"} { | ||
| if !have[e] { | ||
| parts = append(parts, e) | ||
| } | ||
| } | ||
| os.Setenv("PATH", strings.Join(parts, ":")) | ||
| } |
There was a problem hiding this comment.
Can the callers to these executables use absolute paths rather than setting up PATH? Are the locations of these executables unpredictable?
There was a problem hiding this comment.
The file is called network_ns.go, which gives me the impression that it is creating the network namespace, but if I understand correctly it is doing the initial setup inside the namespace. Maybe inside_network_ns.go to be less ambiguous?
| } | ||
|
|
||
| func NewSandbox(t *testing.T) *Sandbox { | ||
| t.Helper() |
There was a problem hiding this comment.
Using Helper() in these files seems like it will make failures harder to understand.
When printing file and line information, that function will be skipped.
| s.pid = cmd.Process.Pid | ||
|
|
||
| if err := s.waitReady(); err != nil { | ||
| s.Close() |
There was a problem hiding this comment.
If t.Cleanup(s.Close) is moved before this block, would all these error handlers need to call s.Close() manually?
There was a problem hiding this comment.
The file is called filesystem_ns.go, which gives me the impression that it is creating the filesystem namespace, but if I understand correctly it is doing the initial setup inside the namespace. Maybe inside_filesystem_ns.go to be less ambiguous?
There was a problem hiding this comment.
makes sense, though i think the best prefix here would be init or something similar
| cmd.Stdout = &outBuf | ||
| cmd.Stderr = &errBuf | ||
| log.Printf("[sandbox] running: %s", strings.Join(args, " ")) | ||
| err := cmd.Run() |
There was a problem hiding this comment.
Should this use the context from the test environment so that the commands are halted if the test exits?
| keeper *exec.Cmd | ||
| pid int | ||
| tempdir string | ||
| initScript string |
There was a problem hiding this comment.
If I understand correctly, this refers to the /etc/init.d/cuttlefish-host-resources script. This isn't a great separation of concerns: the Sandbox type is aware both of namespacing concerns and is involved in managing the host-resources script directly. It would be easier to consider the Sandbox type purely as managing namespaces, and it should expose enough operations that the test can use it to start operations on the init script inside.
| cmd := exec.Command("unshare", "--user", "--map-root-user", "--net", "--mount", "sleep", "infinity") | ||
| if err := cmd.Start(); err != nil { | ||
| t.Fatalf("cannot create rootless user+net namespace: %v", err) | ||
| } | ||
| s.keeper = cmd | ||
| s.pid = cmd.Process.Pid |
There was a problem hiding this comment.
Rather than using unshare, is it possible to use SysProcAttr with flags like CLONE_NEWNET?
It doesn't look like there is a convenient alternative for nsenter though. A kind of awkward way to avoid nsenter would be to have bash as the root process of the namespace and keep file descriptors open to run commands inside by writing to the shell's stdin and reading results from its stdout, but that seems more convoluted.
There was a problem hiding this comment.
it appears that that is mostly what unshare does, plus a bunch of setup:
https://github.com/util-linux/util-linux/blob/master/sys-utils/unshare.c#L939
There was a problem hiding this comment.
The functions in this file that are member functions of Sandbox feel like they could just as easily be free functions that accept a Sandbox, as they don't interact with any of its private members.
http://go/UnhealthyIdioms#violation-of-single-responsibility-principle
It doesn't make much difference in go semantics since visibility is package-level so even non-member functions can access private struct members, but as a reader it draws clearer distinctions about the meaning of the Sandbox type.
There was a problem hiding this comment.
This file defines some parse* functions that internally define more complex structures with json annotations and return simpler types with easily accessible public fields.
What do you think instead of defining the public types around the json schema, keeping the fields private, and then defining accessor methods to produce the values the callers need to access? It would make it easier to connect the code to the command output.
The downside is that it makes it harder to describe the ideal state in test cases like TestStaticResourcesInit which also rely on being able to construct the same types. Maybe a hybrid of the two approaches is to try to extract the json schema types out of the parse* methods and implement conversions between the two types of structures.
There's also UnmarshalerFrom and with jsontext.Decoder but that looks like a bigger hassle because of how low-level it is.
There was a problem hiding this comment.
my thinking was actually to let this file fall on the messier side so the test files are super clear, but let me see if i can straighten things out in here. i'll try a few things
3 participants
Add a way to run tests for resource allocation. This will be adapted for cvdalloc soon.
Currently, I don't run the code in the new directory in kokoro. I plan to try and do so if possible w/ the cvdalloc stuff, but the main use of this right now is for local testing.