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

Jump to bottom

[ws-daemon] Improve cache error handling #8366

Merged
merged 1 commit into from
Feb 23, 2022
Merged

[ws-daemon] Improve cache error handling #8366

merged 1 commit into from
Feb 23, 2022

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Feb 21, 2022

Description

A non-existing file is not an error (in these cases)

Check logging events https://cloudlogging.app.goo.gl/FPPVKf8QeobE9PYq6

Release Notes

NONE

@aledbf aledbf requested a review from a team February 21, 2022 23:59
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label Feb 22, 2022
@aledbf
Copy link
Member Author

aledbf commented Feb 22, 2022

@utam0k can you review this change, please? (you added the tests).

The current behavior returns an error, but that pollutes the logs. Is this change acceptable?

@aledbf
Copy link
Member Author

aledbf commented Feb 22, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-aledbf-cache.2

@utam0k utam0k mentioned this pull request Feb 22, 2022
Closed
@utam0k
Copy link
Contributor

utam0k commented Feb 22, 2022

@utam0k can you review this change, please? (you added the tests).

The current behavior returns an error, but that pollutes the logs. Is this change acceptable?

Thanks for your PR. I think we can investigate this a little more carefully. This is because this update is performed when the pod is updated, so it's unnatural that these file doesn't exist. I think we can accept this PR as it stands, but I think we need to create an issue to resolve or research this problem.

@utam0k
Copy link
Contributor

utam0k commented Feb 22, 2022

@aledbf I've created the issue. Can I ask you to leave the TODO or something comment in error handling codes.?
#8368

csweichel
csweichel previously approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@csweichel csweichel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the hasty approval - I had not seen the prior discussion, but just gone through the code.

@csweichel
Copy link
Contributor

/hold

@aledbf
Copy link
Member Author

aledbf commented Feb 22, 2022

@aledbf I've created the issue. Can I ask you to leave the TODO or something comment in error handling codes.? #8368

done

csweichel
csweichel reviewed Feb 22, 2022
View reviewed changes
components/ws-daemon/pkg/daemon/cache_reclaim.go
Comment on lines +121 to +122
// TODO(toru): find out why the file does not exists
if errors.Is(err, fs.ErrNotExist) {
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.

@codecov
Copy link

codecov bot commented Feb 22, 2022 •
edited
Loading

Codecov Report

Merging #8366 (aad8697) into main (3f05d6f) will increase coverage by 2.10%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8366      +/-   ##
==========================================
+ Coverage   12.31%   14.42%   +2.10%     
==========================================
  Files          20       49      +29     
  Lines        1161     4701    +3540     
==========================================
+ Hits          143      678     +535     
- Misses       1014     3963    +2949     
- Partials        4       60      +56
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)
components-installer-raw-app 4.58% <ø> (?)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
components-ws-daemon-app 20.27% <50.00%> (?)
components-ws-daemon-lib 20.27% <50.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/ws-daemon/pkg/daemon/cache_reclaim.go 26.66% <50.00%> (ø)
components/local-app/pkg/auth/pkce.go
components/local-app/pkg/auth/auth.go
components/installer/pkg/common/storage.go 0.00% <0.00%> (ø)
components/ws-daemon/pkg/quota/size.go 87.30% <0.00%> (ø)
components/installer/pkg/common/render.go 0.00% <0.00%> (ø)
components/ws-daemon/pkg/cpulimit/cpulimit.go 68.83% <0.00%> (ø)
...s/installer/pkg/components/ws-manager/tlssecret.go 0.00% <0.00%> (ø)
components/ws-daemon/pkg/content/config.go 33.33% <0.00%> (ø)
components/ws-daemon/pkg/content/hooks.go 0.00% <0.00%> (ø)
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f05d6f...aad8697. Read the comment docs.

utam0k
utam0k approved these changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

LGTM

@utam0k
Copy link
Contributor

utam0k commented Feb 22, 2022

/lgtm

@aledbf aledbf requested review from csweichel and Furisto February 22, 2022 15:52
@roboquat roboquat merged commit a87a1c8 into main Feb 23, 2022
@roboquat roboquat deleted the aledbf/cache branch February 23, 2022 10:51
@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utam0k utam0k utam0k approved these changes

@Furisto Furisto Furisto approved these changes

@csweichel csweichel csweichel approved these changes

Assignees
No one assigned
Labels
deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note-none size/S team: workspace Issue belongs to the Workspace team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aledbf @utam0k @csweichel @Furisto @roboquat