Conversation
go/worker/sandbox/cgroups/cgroup.go
Outdated
| } | ||
|
|
||
| func (cg *CgroupImpl) TryReadIntKVFromFile(file *os.File, key string, buf []byte) (int64, error) { | ||
| bytesRead, err := file.ReadAt(buf, 0) |
There was a problem hiding this comment.
Why pass the buf in? Also, don't we want to make sure we get the whole thing? Perhaps this would be better?
file.Seek(0, io.SeekStart)
data, err := io.ReadAll(file)
There was a problem hiding this comment.
Initially I was using file.Read with file.Seek, but that was taking too long and causing issues with destroying cgroups, so I switched to this approach. It was probably because I was still doing TryIntReadKV and doing an extra read on the open file, though.
go/worker/sandbox/cgroups/cgroup.go
Outdated
| return 0, err | ||
| } | ||
| body := string(raw) | ||
| func (cg *CgroupImpl) ScanIntKV(body string, key string) (int64, error) { |
There was a problem hiding this comment.
I don't think we will use this much elsewhere, so I would include this logic in the second part of TryReadIntKVFromFile.
| return cg.ScanIntKV(body, key) | ||
| } | ||
|
|
||
| func (cg *CgroupImpl) TryReadIntKV(resource string, key string) (int64, error) { |
There was a problem hiding this comment.
I think the cleanest implementation of this function would be just opening the file, and passing the file to TryReadIntKVFromFile.
|
|
||
| defer func(start time.Time) { | ||
| elapsed := time.Since(start) | ||
| if elapsed >= 250*time.Millisecond { |
There was a problem hiding this comment.
To make sure we don't busy wait in the future, should we also count calls to Poll and give a warning if it is more than, say, 5?
|
|
||
| resourcePath := cg.ResourcePath("cgroup.events") | ||
|
|
||
| eventFile, err := os.Open(resourcePath) |
There was a problem hiding this comment.
It took us a long time to figure out the behavior of poll and the file descriptor for cgroup.events. It is not documented well in the manpages, so we really need to comment on this carefully so somebody doesn't break it in the future and need to rediscover how things work.
go/worker/sandbox/cgroups/cgroup.go
Outdated
| func (cg *CgroupImpl) TryReadIntKVFromFile(file *os.File, key string, buf []byte) (int64, error) { | ||
| bytesRead, err := file.ReadAt(buf, 0) | ||
| if err != nil && err != io.EOF { | ||
| return 0, err |
There was a problem hiding this comment.
Better to wrap err with "%w" like you do elsewhere. Otherwise, as this gets passed back a few functions, the error will just say something about not being able to read, but nobody will know what we were trying to read.
There was a problem hiding this comment.
Got it! Should I make the same change to TryReadIntKV? Right now it handles errors with reading like this:
raw, err := ioutil.ReadFile(cg.ResourcePath(resource))
if err != nil {
return 0, err
}
2 participants
No description provided.