Skip to content

Make TakeSnapshot more reliable #6144

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

Merged
merged 6 commits into from
Nov 4, 2021
Merged

Make TakeSnapshot more reliable #6144

merged 6 commits into from
Nov 4, 2021

Conversation

geropl
Copy link
Member

@geropl geropl commented Oct 11, 2021

Description

This fixes #5862 by following this idea: #5862 (comment)

In detail

  • we add a state: 'pending' | 'error' | 'available'' field to Snapshot (+ message and availableTime)
  • we add a method content-service.WorkspaceSnapshotExists
  • we make sure ws-daemon.TakeSnapshot returns early
  • we add an API method server.waitForSnapshot
  • we add a SnapshotService to server which is responsible for making sure all
  • we change the semantics fo server.takeSnapshot to:
    • return as early as we have the upload file URL from ws-daemon, stored that in the DB (with state: 'pending') and return the snapshotId
    • we make sure SnapshotService monitors this DB entry and regularly checks if it's done or timed out, resulting in an update to the DB
    • clients have to take that snapshotId and have to call server.waitForSnapshot to wait for the snapshot to become available (or error in case sth went south). This internally waits for the same Promise SnapshotService is already driving

Regarding deployment

This change has breaking changes to IDE and workspace. To untangle and make the deployment as easy as possible:

  • once reviewed (and tested) split this PR into two: IDE and workspace + meta
  • server and workspace are fully backwards-compatible to each other; server API is backwards compatible as well. Thus we have to make sure meta is deployed before IDE, the rest does not matter.

Related Issue(s)

Fixes #5862

IDE companion PR: gitpod-io/openvscode-server#169

How to test

Release Notes

make "snapshots" more reliable

Documentation

  • /werft with-clean-slate-deployment

@geropl
Copy link
Member Author

geropl commented Oct 11, 2021

/werft run

👍 started the job as gitpod-build-gpl-5862-snapshot.5

@geropl geropl force-pushed the gpl/5862-snapshot branch 2 times, most recently from 0f0db41 to ccff46b Compare October 11, 2021 14:03
@roboquat roboquat added size/XS and removed size/S labels Oct 11, 2021
@geropl geropl force-pushed the gpl/5862-snapshot branch from ccff46b to 61c9cef Compare October 12, 2021 07:12
@roboquat roboquat added size/S and removed size/XS labels Oct 12, 2021
@geropl geropl force-pushed the gpl/5862-snapshot branch 2 times, most recently from a6fa036 to d04812d Compare October 12, 2021 09:36
@roboquat roboquat added team: IDE team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team size/XXL and removed size/S labels Oct 18, 2021
@geropl geropl force-pushed the gpl/5862-snapshot branch 3 times, most recently from 61ca4b1 to c19e486 Compare October 20, 2021 11:50
@codecov
Copy link

codecov bot commented Oct 20, 2021

Codecov Report

Merging #6144 (3534f69) into main (0514edc) will increase coverage by 14.43%.
The diff coverage is 0.00%.

❗ Current head 3534f69 differs from pull request most recent head 9ed6299. Consider uploading reports for the commit 9ed6299 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6144       +/-   ##
===========================================
+ Coverage   19.04%   33.48%   +14.43%     
===========================================
  Files           2       23       +21     
  Lines         168     4680     +4512     
===========================================
+ Hits           32     1567     +1535     
- Misses        134     2988     +2854     
- Partials        2      125      +123     
Flag Coverage Δ
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-manager-app 39.55% <0.00%> (?)
installer-app 7.45% <ø> (?)

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

Impacted Files Coverage Δ
components/ws-manager/pkg/manager/manager_ee.go 0.00% <0.00%> (ø)
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go
installer/pkg/components/ws-manager/tlssecret.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/metrics.go 11.11% <0.00%> (ø)
installer/pkg/components/ws-manager/rolebinding.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/probe.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/imagespec.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/status.go 73.77% <0.00%> (ø)
...staller/pkg/components/ws-manager/networkpolicy.go 0.00% <0.00%> (ø)
... and 16 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 0514edc...9ed6299. Read the comment docs.

@geropl geropl force-pushed the gpl/5862-snapshot branch 4 times, most recently from 4f87159 to 0e47cfc Compare October 21, 2021 07:30
@geropl geropl force-pushed the gpl/5862-snapshot branch from 13514a5 to 0491ef5 Compare November 3, 2021 13:35
@roboquat roboquat removed the lgtm label Nov 3, 2021
@geropl
Copy link
Member Author

geropl commented Nov 3, 2021

/werft run

👍 started the job as gitpod-build-gpl-5862-snapshot.45

another try---

@geropl geropl force-pushed the gpl/5862-snapshot branch from 0491ef5 to ef7cdd5 Compare November 3, 2021 14:20
@geropl
Copy link
Member Author

geropl commented Nov 3, 2021

/werft run

👍 started the job as gitpod-build-gpl-5862-snapshot.47

...and again

@geropl
Copy link
Member Author

geropl commented Nov 3, 2021

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-gpl-5862-snapshot.48

The best things come in threes. 🤞

@geropl geropl force-pushed the gpl/5862-snapshot branch from 8f4fe1c to 9ed6299 Compare November 3, 2021 15:37
@geropl geropl requested review from csweichel and removed request for akosyakov November 3, 2021 15:54
@geropl
Copy link
Member Author

geropl commented Nov 3, 2021

/unhold

Ready to merge now

@JanKoehnlein
Copy link
Contributor

/lgtm

@roboquat roboquat added the lgtm label Nov 4, 2021
@roboquat
Copy link
Contributor

roboquat commented Nov 4, 2021

LGTM label has been added.

Git tree hash: e74e73cd5ea3964fe6b81a871af7253bdf9429f6

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Tried this for a workspace larger than 3.5GB which used to fail and worked as expected! 🔮

Thanks @geropl! 🏁

@csweichel
Copy link
Contributor

did not test, but API and service changes lgtm

/lgtm

@roboquat
Copy link
Contributor

roboquat commented Nov 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csweichel, JanKoehnlein

Associated issue: #5862

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 61ea972 into main Nov 4, 2021
@roboquat roboquat deleted the gpl/5862-snapshot branch November 4, 2021 11:18
@shaal
Copy link
Contributor

shaal commented Nov 4, 2021

I can't wait for this feature to be working again 🎉
When do you expect this to be available on gitpod.io ?

@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 4, 2021

Thanks, @shaal! The fix should probably land in production within a week.

FYI, we're trying to get better at deploying to production sooner and more often.
We're aiming at deploying at least twice a week. 💡

@geropl
Copy link
Member Author

geropl commented Nov 8, 2021

@gtsiolis Just to clarify: This PR is just introducing the server/workspace changes. IDE change will land later, I expect next week.

@gtsiolis
Copy link
Contributor

gtsiolis commented Nov 8, 2021

Thanks for clarifying, @geropl!

Is there an open issue for the IDE changes?

Cc @shaal

@geropl
Copy link
Member Author

geropl commented Nov 8, 2021

It's the same issue: #5862

Draft PR is here: gitpod-io/openvscode-server#169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved groundwork: in review 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.

Cannot generate a workspace snapshot
7 participants