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

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

Closed

Conversation

AObuchow
Copy link
Collaborator

What does this PR do?

When calculating the per-workspace PVC size, container components in a devworkspace are now treated as if they were volume components with no size specified.

This change was made to address an edge case in the per-workspace PVC size calculation where a devworkspace that has no volume components, but has a container component with mountSources enabled will request a PVC size of 0.

When mountSources is used, the devworkspace requires storage to store the project sources. However, it's ambiguous as to how much storage is required for the project sources. Thus, the best we can do is to treat container components with mountSources enabled as if they were volume components with an unspecified size.

I also documented the original 3 rules that were followed when calculating the per-workspace PVC size (mentioned in this PR) and added a 4th rule regarding the mountSources edge case.

What issues does this PR fix or reference?

Fix #1239

Is it tested? How?

  1. Install DWO built from this PR
  2. Apply the following DevWorkspace, which used to fail in the original issue:
kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-per-workspace
spec:
  started: true
  routingClass: 'basic'
  template:
    attributes:
        controller.devfile.io/storage-type: per-workspace
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"
  1. Ensure the devworkspace starts up successfully.
  2. Ensure the devworkspace's PVC size is the default 5Gi

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
    • v8-devworkspace-operator-e2e: DevWorkspace e2e test
    • v8-che-happy-path: Happy path for verification integration with Che

Copy link

openshift-ci bot commented Apr 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AObuchow

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

@AObuchow AObuchow changed the title chore: Document per-workspace PVC size calculation function fix: account for mountSources when calculating per-workspace PVC size Apr 17, 2024
Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>
Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>
Fix devfile#1239

This commit fixes an edge case where a devworkspace that has no volume components,
but has a container component with mountSources enabled will request a PVC size of 0
when using the per-workspace storage strategy.

When mountSources are used, the devworkspace requires storage to store the project sources.
However, it is unknown how much storage is actually required for the project sources.
Thus, we treat container components with mountSources enabled as if they were volume components
with an unspecified size.

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>
@AObuchow AObuchow force-pushed the mount-sources-pvc-size-calculation branch from 2416989 to 3eb15f5 Compare April 17, 2024 21:21
@@ -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

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.

//
// 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)

@@ -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

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.

@AObuchow
Copy link
Collaborator Author

AObuchow commented Dec 6, 2024

Closing this PR as it hasn't been worked on in months (low priority), however this comment is the solution I wanted to adopt to resolve the issue.

@AObuchow AObuchow closed this Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

per-workspace storage PVC size calculation is incorrect when no volumes defined & mountSources used
2 participants