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

Add support for ephemeral volumes #281

Merged
merged 4 commits into from
Mar 1, 2021
Merged

Conversation

amisevsk
Copy link
Collaborator

What does this PR do?

Adds support for volume components with ephemeral: true, which are provisioned as emptyDir volumes in the workspace pod.

This PR updates the logic for determining whether a container needs storage. The new logic is:

  • If any non-ephemeral volumes are defined, we need storage
  • If any container component has mountSources: true, we need storage unless there's an explicit projects volume with ephemeral: true
    this new check does not check that any container has mountSources as before, since that is covered by the first case, or an error (a container mounting a volume that is not defined)

I had to update the relevant test to support this change, and also added tests to cover the ephemeral volume use cases.

I've also added a sample that showcases some of the ways to use ephemeral volumes:

  • We can override the default projects volume to be ephemeral
  • We can override volumes in plugins to be ephemeral (we make the theia plugins volume ephemeral)

What issues does this PR fix or reference?

Closes devfile/api#310

Is it tested? How?

  1. Deploy controller, make sure CRDs are updated to include support for ephemeral volumes (I made that mistake...) (see: Update devfile/api dependency and message with info about failures #270)
  2. Deploy plugin templates (make install_plugin_templates)
    • Note some of the templates have been updated to e.g. use an ephemeral volume for remote runtime
  3. Deploy sample that uses emptyDir
    kubectl apply -f samples/with-k8s-ref/emptyDir-theia.yaml
  4. Verify:
    • All volumes in workspace pod are emptyDir (`kubectl get po -o yaml | yq -y '.spec.volumes')
    • All volumeMounts in containers refer to these emptyDir containers
    • No finalizer is set on the workspace
    • The workspace runs normally

I had a strange issue testing locally that I'm currently blaming minikube for: when I initially apply the sample, the volumes in the devworkspace are simply

      - name: projects
        volume: {}

but applying twice correctly shows the ephemeral field. No idea why that's happening and praying it doesn't reproduce elsewhere.

@amisevsk
Copy link
Collaborator Author

@sleshchenko I might be seeing your issue from #269 (review) -- using che-theia:next I'm stuck at the loading spinner, but switching to 7.26.0 I have no issues.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Tested and the described scenario works as expected, apart from fact that I needed to patch theia to be 7.26.0 which is know issue.

@amisevsk
Copy link
Collaborator Author

/test v5-devworkspaces-operator-e2e

Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

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

Tested and working as expected 👍

@amisevsk
Copy link
Collaborator Author

From Serhii:

DevWorkspace with

spec:
  started: true
  template:
    components:
    - name: theia-next
      plugin:
        kubernetes:
          name: theia-next
          namespace: devworkspace-plugins
    - name: machine-exec
      plugin:
        kubernetes:
          name: machine-exec
          namespace: devworkspace-plugins

has PVC mounted but no finalizer set; looking into it.

amisevsk and others added 4 commits February 25, 2021 23:45
Add handling of volume components with ephemeral: true, which are
propagated as emptyDir volumes without changing their name.

Signed-off-by: Angel Misevski <[email protected]>
* Fix existing samples to use an ephemeral volume for the remote runtime
injector
* Add sample that uses emptyDir volumes for plugins and projects

Signed-off-by: Angel Misevski <[email protected]>
Update pkg/library/storage/testdata/error-unparseable-ephemeral-size.yaml

Co-authored-by: Serhii Leshchenko <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
Fix the check for whether a finalizer is necessary to use the flattened
workspace. Otherwise, we may miss some components that need storage (and
so need to be finalized) if they are defined as plugins.

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk
Copy link
Collaborator Author

/test v5-devworkspaces-operator-e2e

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Still LGTM, additional thanks for handling the reported bug.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, JPinkney, sleshchenko

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:
  • OWNERS [JPinkney,amisevsk,sleshchenko]

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

@amisevsk amisevsk merged commit d76f112 into devfile:main Mar 1, 2021
@amisevsk amisevsk deleted the ephemeral-volumes branch February 8, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extending Volume components to support more features
4 participants