Skip to content

[pvc] fix prebuilds when using pvc #12746

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
Merged
merged 1 commit into from
Sep 12, 2022
Merged

[pvc] fix prebuilds when using pvc #12746

merged 1 commit into from
Sep 12, 2022

Conversation

sagor999
Copy link
Contributor

@sagor999 sagor999 commented Sep 7, 2022 •
edited by jenting
Loading

Description

First, kudos to @jenting for catching this issue. ❤️
What was happening is actually when using pvc and prebuilds, it would not know that workspace was open from prebuild, rendering prebuild useless (as it would do all the work that prebuild already done).
This PR fixes this.

Related Issue(s)

Fixes #12464

How to test

Open regular workspace
Open PVC regular workspace
Open regular prebuild
Open PVC prebuild

Release Notes

none

Documentation

Werft options:

  • /werft with-preview

@sagor999
Copy link
Contributor Author

sagor999 commented Sep 7, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft build

👎 unknown command: build
Use /werft help to list the available commands

@sagor999
Copy link
Contributor Author

sagor999 commented Sep 7, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-pavel-12464.3
(with .werft/ from main)

sagor999
sagor999 commented Sep 9, 2022
View reviewed changes
components/content-service/pkg/layer/provider.go
// version of this function for persistent volume claim feature
// we cannot use /workspace folder as when mounting /workspace folder through PVC
// it will mask anything that was in container layer, hence we are using /.workspace instead here
func contentDescriptorToLayerPVC(cdesc []byte) (*Layer, error) {
layers := []fileInLayer{
{&tar.Header{Typeflag: tar.TypeDir, Name: "/.workspace", Uid: initializer.GitpodUID, Gid: initializer.GitpodGID, Mode: 0755}, nil},
{&tar.Header{Typeflag: tar.TypeDir, Name: "/.workspace/.gitpod", Uid: initializer.GitpodUID, Gid: initializer.GitpodGID, Mode: 0755}, nil},
{&tar.Header{Typeflag: tar.TypeReg, Name: "/.workspace/.gitpod/pvc", Uid: 0, Gid: 0, Mode: 0775, Size: int64(len(pvcEnabledFile))}, []byte(pvcEnabledFile)},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this allows us to reliably check in supervisor if workspace is using pvc or not, instead of relying on content.json being present, which is not the case everytime (restoring from prebuild will not generate any content.json for example).

components/content-service/pkg/layer/provider.go
return layerFromContent(
fileInLayer{&tar.Header{Typeflag: tar.TypeDir, Name: "/.workspace", Uid: initializer.GitpodUID, Gid: initializer.GitpodGID, Mode: 0755}, nil},
fileInLayer{&tar.Header{Typeflag: tar.TypeDir, Name: "/.workspace/.gitpod", Uid: initializer.GitpodUID, Gid: initializer.GitpodGID, Mode: 0755}, nil},
fileInLayer{&tar.Header{Typeflag: tar.TypeReg, Name: "/.workspace/.gitpod/ready", Uid: initializer.GitpodUID, Gid: initializer.GitpodGID, Mode: 0755, Size: int64(len(ctnt))}, []byte(ctnt)},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this allows us to place ready file.
why we cannot use /workspace folder? because when using pvc that folder is masked by pvc.
also, on top of that, when restoring from prebuild for example, prebuild volume snapshot will contain ready file in there from the time when prebuild was generated.
so this allows us to completely separate pvc and non pvc approach.

components/supervisor/pkg/supervisor/supervisor.go
fn = "/.workspace/.gitpod/content.json"
log.Info("Detected content.json in /.workspace folder, assuming PVC feature enabled")
fnReady = "/.workspace/.gitpod/ready"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to wait for ready file that comes from PVC layer, we should not use ready file that comes from /workspace since that file will be backed up into volume snapshot when it is created from pvc.
so this makes a clear separation between pvc and non-pvc code path.

@sagor999 sagor999 changed the title wip [pvc] fix prebuilds when using pvc Sep 9, 2022
@sagor999 sagor999 marked this pull request as ready for review September 9, 2022 01:36
@sagor999 sagor999 requested review from a team September 9, 2022 01:36
@github-actions github-actions bot added team: IDE team: workspace Issue belongs to the Workspace team labels Sep 9, 2022
@sagor999
Copy link
Contributor Author

sagor999 commented Sep 9, 2022 •
edited
Loading

@kylos101 due to necessary refactor in supervisor, once IDE rolls it out, PVC feature will become broken in prod until we roll out new workspace cluster. So we can still start dog fooding PVC at workspace team level, but will need to stop once IDE deploys this change, and we will need to deploy new cluster.
no longer the case. disregard. placed workaround to ensure old code path will work as well.

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-12464.14 because the annotations in the pull request description changed
(with .werft/ from main)

jenting
jenting reviewed Sep 9, 2022
View reviewed changes
Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

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

I have verified it, it works like a charm.

components/content-service/pkg/layer/provider.go Outdated
@@ -287,8 +295,12 @@ func (s *Provider) GetContentLayerPVC(ctx context.Context, owner, workspaceID st
if err != nil {
return nil, nil, err
}
layerReady, err := workspaceReadyLayerPVC(csapi.WorkspaceInitFromOther)
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I don't know what is the correct WorkspaceInitSource for the snapshot.

Copy link
Contributor

Choose a reason for hiding this comment

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

I test the snapshot without the PVC feature flag. The content within read file is {"source":"from-backup"}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it matters that much if it is from backup or from other, but for consistency, updated to match. ❤️

@sagor999 sagor999 requested a review from jenting September 11, 2022 17:48
@jenting
Copy link
Contributor

jenting commented Sep 12, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-preview

👍 started the job as gitpod-build-pavel-12464.17
(with .werft/ from main)

@jenting
Copy link
Contributor

jenting commented Sep 12, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-preview with-clean-slate-deployment

👍 started the job as gitpod-build-pavel-12464.18
(with .werft/ from main)

@jenting
Copy link
Contributor

jenting commented Sep 12, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-pavel-12464.19
(with .werft/ from main)

@jenting
Copy link
Contributor

jenting commented Sep 12, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-pavel-12464.20
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-pavel-12464.21 because the annotations in the pull request description changed
(with .werft/ from main)

@jenting
Copy link
Contributor

jenting commented Sep 12, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-pavel-12464.22
(with .werft/ from main)

@jenting
Copy link
Contributor

jenting commented Sep 12, 2022 •
edited
Loading

The werft finally happy.

akosyakov
akosyakov approved these changes Sep 12, 2022
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

lgtm, i have not tried, rely on @pavel review

it seems that changes to supervisor are backward compatible

/hold

unhold when you comfortable to merge

@sagor999
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit ef92c82 into main Sep 12, 2022
@roboquat roboquat deleted the pavel/12464 branch September 12, 2022 18:12
@roboquat roboquat added the deployed: IDE IDE change is running in production label Sep 13, 2022
@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akosyakov akosyakov akosyakov approved these changes

@jenting jenting jenting approved these changes

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

Successfully merging this pull request may close these issues.

[PVC] Open workspace from prebuild PVC, no message This task ran as a workspace prebuild on CLI
4 participants
@sagor999 @jenting @akosyakov @roboquat