Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ws-daemon] Improve cache error handling #8366

Merged
merged 1 commit into from
Feb 23, 2022
Merged

[ws-daemon] Improve cache error handling #8366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions components/ws-daemon/pkg/daemon/cache_reclaim.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ package daemon
import (
"bufio"
"context"
"errors"
"io/fs"
"io/ioutil"
"math"
"os"
Expand Down Expand Up @@ -116,6 +118,11 @@ func readLimit(memCgroupPath string) (uint64, error) {
fn := filepath.Join(string(memCgroupPath), "memory.limit_in_bytes")
fc, err := os.ReadFile(fn)
if err != nil {
// TODO(toru): find out why the file does not exists
if errors.Is(err, fs.ErrNotExist) {
Comment on lines +121 to +122
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely the file doesn't exist anymore because our "dispatch" mechanism takes too long to cancel the context. I.e. the container is already removed before the dispatch mechanism shuts down this mechanism.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have todos in the code? If we have them we should at least create an issue and reference them in the comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue has already been created.
#8368

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw it 👍 , but was asking more generally if we should have todos in the code.

Copy link
Contributor

@utam0k utam0k Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comment 🙏 It seems quite unnatural to ignore only this error. So I'd like to write something about the reason.

return 0, nil
}

return 0, xerrors.Errorf("cannot read memory.limit_in_bytes: %v", err)
}

Expand All @@ -134,6 +141,11 @@ func readLimit(memCgroupPath string) (uint64, error) {
func readCache(memCgroupPath string) (uint64, error) {
f, err := os.Open(filepath.Join(string(memCgroupPath), "memory.stat"))
if err != nil {
// TODO(toru): find out why the file does not exists
if errors.Is(err, fs.ErrNotExist) {
return 0, nil
}

return 0, xerrors.Errorf("cannot read memory.stat: %w", err)
}
defer f.Close()
Expand Down
4 changes: 2 additions & 2 deletions components/ws-daemon/pkg/daemon/cache_reclaim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ func TestReadLimitBadValue(t *testing.T) {
t.Fatal(err)
}
_, err = readCache(tempdir)
if err == nil {
t.Fatal("expected failure")
if err != nil {
t.Fatalf("unexpected error: is '%v' but expected no error", err)
}
}
}
Expand Down