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.

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

Make the pods be removed when workspacekit fails. #10085

Merged
merged 3 commits into from
May 25, 2022
Merged

Make the pods be removed when workspacekit fails. #10085

merged 3 commits into from
May 25, 2022

Conversation

utam0k
Copy link
Contributor

@utam0k utam0k commented May 18, 2022

Description

There was a problem with workspace pendings due to finalizer not working properly when iws failed to start (or had a large delay). This PR avoids that by skipping the finalizer. Also, the workspace phase is now consistent with it.

Related Issue(s)

Fixes #9673

How to test

  • workspaces can normally start. You can confirm this preview environment
  • Intentionally break communication between iws and ws-daemon, and when a workspace failed to start, confirm that the workspace phase is properly stopped(NOTE: in this case, the stopping phase is skipped because we don't need to backup.) You can confirm it on this preview environment

Release Notes

ws-manager: Make the pods be removed when workspacekit fails.

Documentation

No

@utam0k utam0k requested a review from a team May 18, 2022 01:27
@github-actions github-actions bot added team: workspace Issue belongs to the Workspace team and removed size/S labels May 18, 2022
Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

Change makes sense.

Could you add a fixture to test this new behaviour: https://github.com/gitpod-io/gitpod/blob/main/components/ws-manager/pkg/manager/monitor_test.go#L20?

components/ws-manager/pkg/manager/monitor.go Outdated Show resolved Hide resolved
components/ws-manager/pkg/manager/monitor.go Outdated Show resolved Hide resolved
@utam0k
Copy link
Contributor Author

utam0k commented May 18, 2022

Change makes sense.

Could you add a fixture to test this new behaviour: https://github.com/gitpod-io/gitpod/blob/main/components/ws-manager/pkg/manager/monitor_test.go#L20?

@csweichel
Thanks for your review. I took looked at the fixtures file but I couldn't find how to generate the golden and status json file. Can I ask you to tell me how to generate these fixtures file?

@sagor999
Copy link
Contributor

sagor999 commented May 23, 2022

@utam0k
To update golden files: run go test ./... -update -force from inside componets/ws-manager folder.
This will regenerate any golden files.
If you need to update what is inside golden files, then you edit corresponding json file and run the command from above to update golden file.

To add a new fixture, you would want to add a new json file that would test this new behaviour (look at existing ones as inspiration), and then run command above to generate golden file from that new json file.

Let me know if you need help with this.

@sagor999 sagor999 self-requested a review May 23, 2022 23:10
@sagor999
Copy link
Contributor

sagor999 commented May 23, 2022

Adding myself as reviewer as Chris is OOO this week.

@utam0k
Copy link
Contributor Author

utam0k commented May 24, 2022

@sagor999 PTAL

@utam0k utam0k requested a review from a team May 25, 2022 04:12
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label May 25, 2022
@utam0k
Copy link
Contributor Author

utam0k commented May 25, 2022

@sagor999 @Furisto PTAL

@roboquat roboquat merged commit 0a68903 into main May 25, 2022
@roboquat roboquat deleted the to/conn-prob branch May 25, 2022 08:26
@roboquat roboquat added the deployed: webapp Meta team change is running in production label May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production release-note size/XXL team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate Pending for long duration while the actual state was pod terminating
5 participants