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

fix: account for mountSources when calculating per-workspace PVC size #1253

Closed
wants to merge 3 commits into from
Closed

fix: account for mountSources when calculating per-workspace PVC size #1253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ddb177
chore: Document per-workspace PVC size calculation function
AObuchow Apr 17, 2024
b497c14
chore: add test for mountSources PVC size calculation
AObuchow Apr 5, 2024
3eb15f5
fix: Use at least minimum PVC size when mountSources used
AObuchow Apr 17, 2024
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
19 changes: 19 additions & 0 deletions pkg/provision/storage/perWorkspaceStorage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,16 @@ func (p *PerWorkspaceStorageProvisioner) rewriteContainerVolumeMounts(workspaceI
return nil
}

// Calculates the per-workspace PVC size required for the given devworkspace based on the following rules:
//
// 1. If all volumes in the devworkspace specify their size, the computed PVC size will be used.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dkwon17 I made a mistake here (and repeated this mistake while on our call):
If all volumes in the devworkspace specify their size, the computed PVC size will used if it is greater than the default PVC size (see this line and this test case).

Will modify this function's documentation accordingly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My above statement was wrong... my mistake.

If all volume sizes are defined, and the computed PVC size is smaller than the default PVC size, the computed PVC size will be used.

This creates a conflict with my work-around for mountSources from this PR: the only way you could get a PVC that is smaller than the default PVC size is to define all volumes and disable mountSources. IMO, this is not acceptable.

I don't know yet if there's an elegant solution to the conflict between using mountSources and wanting a PVC that is smaller than the default. My current ideas are to:

  • Specifically handle the case where no volumes are defined, but mountSources are used (like in the issue reproducer) so that the default PVC size is used. IMO, this is a bit too specific.
  • Handle the case where the computed PVC size is 0, and use the default PVC size instead. This seems to be a bit more of a general solution, but would also affect cases where a user erroneously sets all their volumes size to 0.
    • Currently, if a user explicitly requests 0 Gi of storage (by setting their volumes size to 0), their workspace will fail to startup due to requesting a PVC size of 0. Their workspace failing to startup suggests something is wrong with their devfile, but it's a bit cryptic, since the error message would be something like: Error provisioning storage: Failed to sync storage-workspace8723f4cb2dfa4922 PVC to cluster: PersistentVolumeClaim "storage-workspace8723f4cb2dfa4922" is invalid: spec.resources[storage]: Invalid value: "0": must be greater than zero
    • Is it better UX to just use the default PVC size in this case and let their workspace startup? The user would not necessarily notice something's wrong with their devfile... which is bad. But, if someone is using the non-ephemeral storage strategy, then it's safe to assume they do want persistent storage.
    • I guess a compromise here would be to add a warning status to the workspace about an invalid PVC size request, and to use the default PVC size instead.
  • Maybe the best solution is to leverage the link between the (potentially implicit) projects volume and the mountSources functionality: if mountSources are being used, the user either has a projects volume defined, or an implicit projects volume will be provisioned for them.
    • When encountering a container component with mountSources enabled while calculating the PVC size, we should search for the projects volume. If the user defined one, then it will be accounted for in the PVC size calculation and we can continue. Otherwise, there is an implicit projects volume being used with an undefined size, meaning not all volume sizes are defined.
    • The implication of this solution: to get a PVC size that is smaller than the default PVC size when mountSources are used, the user must define all volume sizes and must define a projects volume with a size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dkwon17 I'm gonna make an update to this PR, most likely based on my last suggested approach.

Copy link
Collaborator Author

@AObuchow AObuchow Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe the best solution is to leverage the link between the (potentially implicit) projects volume and the mountSources functionality: if mountSources are being used, the user either has a projects volume defined, or an implicit projects volume will be provisioned for them.

When encountering a container component with mountSources enabled while calculating the PVC size, we should search for the projects volume. If the user defined one, then it will be accounted for in the PVC size calculation and we can continue. Otherwise, there is an implicit projects volume being used with an undefined size, meaning not all volume sizes are defined.
The implication of this solution: to get a PVC size that is smaller than the default PVC size when mountSources are used, the user must define all volume sizes and must define a projects volume with a size.

I believe this is the proper solution for this issue.

//
// 2. If all volumes in the devworkspace either specify their size or are ephemeral, the computed PVC size will be used.
//
// 3. If at least one volume in the devworkspace specifies its size, and the computed PVC size is greater
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Little formatting mistake here (extra space should be removed)

// than the default per-workspace PVC size, the computed PVC size will be used.
//
// 4. Container components with mountSources enabled in the devworkspace are treated as if they were volumes with an unspecified size.
func getPVCSize(workspace *common.DevWorkspaceWithConfig, namespacedConfig *nsconfig.NamespacedConfig) (*resource.Quantity, error) {
defaultPVCSize := *workspace.Config.Workspace.DefaultStorageSize.PerWorkspace

Expand All @@ -189,6 +199,15 @@ func getPVCSize(workspace *common.DevWorkspaceWithConfig, namespacedConfig *nsco
}
requiredPVCSize.Add(volumeSize)
}
if component.Container != nil {
if component.Container.MountSources != nil && *component.Container.MountSources {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use

devworkspace-operator/pkg/library/container/mountSources.go

Line 34 in 9325f55

func HasMountSources(devfileContainer *dw.ContainerComponent) bool {
?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch ! Yes we should.

// Edge case: If the project source code is being mounted to a container component
// then an undefined amount of persistent storage is required.
// We treat this case as if there was a volume component with no size defined.
allVolumeSizesDefined = false
continue
}
}
}

// Use the calculated PVC size if it's greater than default PVC size
Expand Down
39 changes: 39 additions & 0 deletions ...ision/storage/testdata/perWorkspace-storage/use-default-size-when-mount-sources-used.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
name: "Uses default PVC size when no volumes define their size but mountSources used"

input:
devworkspaceId: "test-workspaceid"
podAdditions:
containers:
- name: testing-container-1
image: testing-image
volumeMounts:
- name: "projects"
mountPath: "/projects-mountpath"
initContainers:
- name: testing-initContainer-1
image: testing-image

workspace:
components:
- name: testing-container-1
container:
image: testing-image-1
mountSources: true

output:
podAdditions:
containers:
- name: testing-container-1
image: testing-image
volumeMounts:
- name: storage-test-workspaceid
subPath: "projects"
mountPath: "/projects-mountpath"
initContainers:
- name: testing-initContainer-1
image: testing-image

volumes:
- name: storage-test-workspaceid
persistentVolumeClaim:
claimName: storage-test-workspaceid
Loading