There was a problem hiding this comment.
Haven't read everything yet, still working through it.
Sorry, something went wrong.
| file, err := os.Open(path) | ||
|
|
||
| if errors.Is(err, os.ErrNotExist) { | ||
| // TODO: use slog |
There was a problem hiding this comment.
Probably simpler to just do it rather than add a comment (or just leave it out of this PR since it is not related).
Sorry, something went wrong.
| } else { | ||
| slog.Info(fmt.Sprintf("run pip install %s from a new Sandbox to %s on host", p.Name, scratchDir)) | ||
| if err := os.Mkdir(scratchDir, 0700); err != nil { | ||
| filesDir := filepath.Join(scratchDir, "files") |
There was a problem hiding this comment.
Why do we need this change?
Sorry, something went wrong.
| github.com/containerd/containerd v1.6.26 | ||
| github.com/fsouza/go-dockerclient v1.10.1 | ||
| github.com/golang-jwt/jwt v3.2.2+incompatible | ||
| github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417 |
There was a problem hiding this comment.
I think containerd is obvious, but could you let me know what each of the other two dependencies provide?
Sorry, something went wrong.
| if errors.Is(err, syscall.EINVAL) { | ||
| fmt.Printf("Sandbox mount root is not mounted. No need to clean up.\n") | ||
| } else { | ||
| } else if errors.Is(err, syscall.ENOENT) { |
There was a problem hiding this comment.
is this related to the other changes? As, does in surface more regularly now? Might be simpler as a separate PR is this is an unrelated issue you stumbled upon.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, this is related. The context is when stopped dirty, stoppedDirtyToStoppedClean func runs and tries to unmount "root-sandboxes", and containerd would ran into no such file error because it doesn't use "root-sandboxes" dir.
Sorry, something went wrong.
|
|
||
| if err := waitForServerPipeReady(c.HostDir()); err != nil { | ||
| c.Destroy("waitForServerPipeReady failed") | ||
| if err := WaitForServerPipeReady(c.HostDir()); err != nil { |
There was a problem hiding this comment.
Is using the pipe the most elegant way to do this? We don't necessarily need to follow what the docker implementation is doing.
Sorry, something went wrong.
| }() | ||
|
|
||
| // Resource limit calculations | ||
| cpuPeriod := uint64(100000) // using Standard CFS period: 100ms (100,000 microseconds) |
There was a problem hiding this comment.
Can you just put the comments before? In a 2-column GitHub diff, it is hard to read this long.
Sorry, something went wrong.
No description provided.