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

feat: configure DWOC from che-operator CR #1494

Merged
merged 4 commits into from
Sep 2, 2022

Conversation

AObuchow
Copy link
Contributor

@AObuchow AObuchow commented Sep 1, 2022

What does this PR do?

This PR adds a new reconciler which is in charge of creating and updating a DevWorkspace-Operator configuration (DWOC) in the che cluster namespace.
The intention is to propagate relevant fields from the che cluster custom resource to the DWOC.

Currently, only the following (storage related) fields are propagated from the che cluster custom resource to the DWOC:

  • checluster.spec.devEnvironments.Storage.pvcStrategy
  • checluster.spec.devEnvironments.Storage.perUserStrategyPvcConfig.ClaimSize
  • checluster.spec.devEnvironments.Storage.perUserStrategyPvcConfig.StorageClass
  • checluster.spec.devEnvironments.Storage.perWorkspaceStrategyPvcConfig.ClaimSize
  • checluster.spec.devEnvironments.Storage.perWorkspaceStrategyPvcConfig.StorageClass

The DWOC fields which can receive values from the che cluster are:

  • workspace.storageClassName
  • workspace.defaultStorageSize.common
  • workspace.defaultStorageSize.perWorkspace

Screenshot/screencast of this PR

n/a

What issues does this PR fix or reference?

Part of eclipse-che/che/issues/21405

How to test this PR?

There are a few automated tests that were added.

For manual testing (on minikube):

  1. Build and push the che-operator image to quay
  2. Deploy che using the custom che-operator image: chectl server:deploy --installer operator -p minikube --che-operator-image=${IMAGE_REGISTRY_HOST}/${IMAGE_REGISTRY_USER_NAME}/che-operator:next
  3. Once deployed, check to ensure the che-owned DWOC has been created:
[aobuchow@fedora ~]$ kubectl describe devworkspaceoperatorconfigs.controller.devfile.io -n eclipse-che
Name:         devworkspace-config
Namespace:    eclipse-che
Labels:       <none>
Annotations:  <none>
API Version:  controller.devfile.io/v1alpha1
Config:
Kind:                    DevWorkspaceOperatorConfig
Metadata:
...
  1. Edit the che cluster's storage-related fields so that they can be propagated to the DWOC: kubectl edit checluster eclipse-che -n eclipse-che
...
  devEnvironments:
    defaultComponents:
    - container:
        image: quay.io/devfile/universal-developer-image:ubi8-38da5c2
        sourceMapping: /projects
      name: universal-developer-image
    defaultEditor: eclipse/che-theia/latest
    defaultNamespace:
      template: <username>-che
    secondsOfInactivityBeforeIdling: 1800
    secondsOfRunBeforeIdling: -1
    storage:
+     perUserStrategyPvcConfig:
+       claimSize: 15Gi
+       storageClass: testName
      pvcStrategy: per-user
  1. Ensure the appropriate changes have been made to the DWOC. In this example, the common storage size should be set to 15Gi and the storage class name should be set to testName.
[aobuchow@fedora ~]$ kubectl describe devworkspaceoperatorconfigs.controller.devfile.io -n eclipse-che
Name:         devworkspace-config
Namespace:    eclipse-che
Labels:       <none>
Annotations:  <none>
API Version:  controller.devfile.io/v1alpha1
Config:
  Workspace:
+   Default Storage Size:
+     Common:            15Gi
+   Storage Class Name:  testName
Kind:                    DevWorkspaceOperatorConfig
Metadata:

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Currently, only the following fields from the checluster custom resource
are propagated to the DevWorkspace-Operator Configuration:

- `checluster.spec.devEnvironments.Storage.pvcStrategy`
- `checluster.spec.devEnvironments.Storage.perUserStrategyPvcConfig.ClaimSize`
- `checluster.spec.devEnvironments.Storage.perUserStrategyPvcConfig.StorageClass`
- `checluster.spec.devEnvironments.Storage.perWorkspaceStrategyPvcConfig.ClaimSize`
- `checluster.spec.devEnvironments.Storage.perWorkspaceStrategyPvcConfig.StorageClass`

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>
@openshift-ci
Copy link

openshift-ci bot commented Sep 1, 2022

Hi @AObuchow. Thanks for your PR.

I'm waiting for a eclipse-che member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #1494 (b95e81c) into main (aa27398) will increase coverage by 0.24%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##             main    #1494      +/-   ##
==========================================
+ Coverage   60.26%   60.51%   +0.24%     
==========================================
  Files          76       77       +1     
  Lines        6262     6308      +46     
==========================================
+ Hits         3774     3817      +43     
- Misses       2125     2128       +3     
  Partials      363      363              
Impacted Files Coverage Δ
controllers/che/checluster_controller.go 0.00% <0.00%> (ø)
pkg/deploy/server/server_configmap.go 71.05% <ø> (+0.79%) ⬆️
...eploy/dev-workspace-config/dev_workspace_config.go 90.19% <90.19%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>
@tolusha tolusha marked this pull request as ready for review September 1, 2022 11:30
@tolusha tolusha changed the title WIP: feat: configure DWOC from che-operator CR feat: configure DWOC from che-operator CR Sep 1, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 1, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AObuchow, tolusha

The full list of commands accepted by this bot can be found 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

workspaceConfig.DefaultStorageSize = &controllerv1alpha1.StorageSizes{}
}

pvcSize := resource.MustParse(pvc.ClaimSize)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tolusha thank you so much for the refactor, things are looking much cleaner now.

The only issue I've found is that, since the pvc.claimSize is a string in the CRD (and not a resource.Quantity), it's possible for resource.MustParse(pvc.ClaimSize) to fail and cause a panic. resource.ParseQuantity() should be used instead.

Want me to submit a fixup for this?

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 submitted the fix up. In the process I had rebased the branch which accidentally dragged some extra commits onto this PR, sorry about that. I can remove those from the PR if desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything looks fine. Merge if needed.

@openshift-ci openshift-ci bot removed the lgtm label Sep 1, 2022
@openshift-ci
Copy link

openshift-ci bot commented Sep 1, 2022

New changes are detected. LGTM label has been removed.

@tolusha
Copy link
Contributor

tolusha commented Sep 2, 2022

/retest

Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@AObuchow
Copy link
Contributor Author

AObuchow commented Sep 2, 2022

@tolusha I squashed the fix up commit, should be good for merge whenever the tests complete.

@tolusha
Copy link
Contributor

tolusha commented Sep 2, 2022

Do we need backport to 7.52.x?

@AObuchow
Copy link
Contributor Author

AObuchow commented Sep 2, 2022

Do we need backport to 7.52.x?

Yes 👍

@openshift-ci
Copy link

openshift-ci bot commented Sep 2, 2022

@AObuchow: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v11-devworkspace-happy-path b95e81c link true /test v11-devworkspace-happy-path

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@tolusha tolusha merged commit 75a31c3 into eclipse-che:main Sep 2, 2022
@che-bot che-bot added this to the 7.54 milestone Sep 2, 2022
tolusha added a commit that referenced this pull request Sep 2, 2022
* feat: configure DWOC from che-operator CR

Currently, only the following fields from the checluster custom resource
are propagated to the DevWorkspace-Operator Configuration:

- `checluster.spec.devEnvironments.Storage.pvcStrategy`
- `checluster.spec.devEnvironments.Storage.perUserStrategyPvcConfig.ClaimSize`
- `checluster.spec.devEnvironments.Storage.perUserStrategyPvcConfig.StorageClass`
- `checluster.spec.devEnvironments.Storage.perWorkspaceStrategyPvcConfig.ClaimSize`
- `checluster.spec.devEnvironments.Storage.perWorkspaceStrategyPvcConfig.StorageClass`

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

* Update CSV and autogenerated apiv2 deepcopy

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>

* Refactoring

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

* make fmt

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Co-authored-by: Anatolii Bazko <abazko@redhat.com>
@tolusha
Copy link
Contributor

tolusha commented Sep 2, 2022

Backported 0a3d57c

@AObuchow
Copy link
Contributor Author

AObuchow commented Sep 2, 2022

@tolusha Thank you for all the help :)

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.

3 participants