Containerd sandbox#460
Conversation
…moved oci.WithImageConfig(pool.imageRef) for one less containerd api call
tylerharter
left a comment
There was a problem hiding this comment.
Haven't read everything yet, still working through it.
| 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).
| } 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") |
| 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?
| 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.
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.
|
|
||
| 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.
| }() | ||
|
|
||
| // 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.
No description provided.