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

[1.2.x] Allow configuring the storage class name for PVCs used by both DB and app #392

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented Jul 1, 2024

Description

This PR adds 2 new fields to the CRD:

  • spec.application.storageClassName
  • spec.database.storageClassName

They allow configuring the storage classes to be used by the PVCs created by the Operator, i.e., the ephemeral PVC created for the dynamic-plugins-root volume, as well as the PVC used by the local database StatefulSet.

#388 intended to provide a more generic way to handle this (along with supporting customizing other properties like labels, annotations, resources, sidecar containers, ...), but it currently does not apply to the local database.

Given that https://issues.redhat.com/browse/RHIDP-2705 is a current a blocker for the upcoming 1.2.2 release, this PR is more focused, covering only the storage class use case with the current v1alpha1 API version.

#388 will need to be updated later for 1.3.

Preview in the OCP console
Screenshot from 2024-07-01 12-56-19

Which issue(s) does this PR fix or relate to

PR acceptance criteria

  • Tests
  • Documentation
  • If the bundle manifests have been updated, make sure to review the rhdh-operator.csv.yaml file accordingly

How to test changes / Special notes to the reviewer

(Tested on a ClusterBot cluster, but feel free to use any storage class on your particular cluster)

cat <<EOF | kubectl apply -f -
apiVersion: rhdh.redhat.com/v1alpha1
kind: Backstage
metadata:
  name: developer-hub
spec:
  application:
    storageClassName: "gp2-csi"
  database:
    storageClassName: "gp3-csi"
EOF

Then run kubectl get pvc and check the storage classes used by the PVCs, e.g.:

$ kubectl get pvc
NAME                                                               STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS   AGE
backstage-backstage-sample-5dbd785b5c-6h7qt-dynamic-plugins-root   Bound    pvc-06f259dc-6c02-4fbd-a6d1-4a78d40ca9ac   2Gi        RWO            gp2-csi        4m43s
data-backstage-psql-backstage-sample-0                             Bound    pvc-2e44619b-cf1c-4d62-bc4c-fea423537648   1Gi        RWO            gp3-csi        4m43s

Copy link

openshift-ci bot commented Jul 1, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rm3l. For more information see the Kubernetes Code Review Process.

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

rm3l added 3 commits July 5, 2024 11:03
…ds allowing to set storage class names for Application and Database PVCs

This is a quick fix for 1.2, until https://issues.redhat.com/browse/RHIDP-2232 addresses CR customization in a more generic way
… select a storage class from a list populated from the cluster)

As depicted in [1], this will display a list of
 storage classes from which the user can select a storage class.
This can be useful to reduce errors when using the Operator.

References
- Operator SDK documentation [2]
- OLM reference [3]

[1] https://www.redhat.com/en/blog/openshift-4-2-declarative-dynamic-ui-for-your-operator
[2] https://sdk.operatorframework.io/docs/building-operators/golang/references/markers/
[3] https://github.com/openshift/console/blob/master/frontend/packages/operator-lifecycle-manager/src/components/descriptors/reference/reference.md
@rm3l rm3l force-pushed the RHIDP-2705--allow-configuring-storageclass-configuration-for-pvc-created-by-operator branch from 0a54885 to 6c45a3a Compare July 5, 2024 09:09
Copy link
Member

@gazarenkov gazarenkov left a comment

Choose a reason for hiding this comment

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

I do not think new feature, especially related to API change can be a part of patch version (see https://semver.org/).

Instead, to make deployment configuration feature to be available sooner, I would propose to make a 0.3 (1.3) operator release, including #388, ASAP.

@rm3l
Copy link
Member Author

rm3l commented Jul 8, 2024

I do not think new feature, especially related to API change can be a part of patch version (see semver.org).

Instead, to make deployment configuration feature to be available sooner, I would propose to make a 0.3 (1.3) operator release, including #388, ASAP.

Sure, it can be seen as a feature semantically-speaking, but in my understanding, https://issues.redhat.com/browse/RHIDP-2705 has been marked as a blocker for 1.2.2, upon customer request. So it cannot wait for 1.3, I guess.

#388 covers too many things right now; so to minimize the risks of introducing breaking changes to 1.2.2, it was concluded that a dedicated PR (addressing only the storage class request) would be created for 1.2.2, hence this PR.
This PR only targets 1.2.x, so only for the upcoming 1.2.2 release, which will leave time to address the review comments on #388 for 1.3
Hope that clarifies.

@gazarenkov
Copy link
Member

gazarenkov commented Jul 9, 2024

I do not think new feature, especially related to API change can be a part of patch version (see semver.org).
Instead, to make deployment configuration feature to be available sooner, I would propose to make a 0.3 (1.3) operator release, including #388, ASAP.

Sure, it can be seen as a feature semantically-speaking, but in my understanding, https://issues.redhat.com/browse/RHIDP-2705 has been marked as a blocker for 1.2.2, upon customer request. So it cannot wait for 1.3, I guess.

#388 covers too many things right now; so to minimize the risks of introducing breaking changes to 1.2.2, it was concluded that a dedicated PR (addressing only the storage class request) would be created for 1.2.2, hence this PR. This PR only targets 1.2.x, so only for the upcoming 1.2.2 release, which will leave time to address the review comments on #388 for 1.3 Hope that clarifies.

No, it does not, sorry :)

  • Deployment configuration and v1alpha2 version #388 was urgently introduced as a reaction on exactly the same challenge, it is here for about 3 weeks already. It does NOT cover many things. Majority of files with code changes are related to renaming the alias (as I explained earlier) which one time operation (will not be needed until we come to Major version 2, which is highly unlikely) and is NOT risky at all.
  • Perfect quality is not guaranteed in any case but this PR contains unit and integration tests for the new functionality and I made a couple of commits as a reaction on review (thanks for that) BUT we can not be blocked with new things just because we are afraid of bugs.
  • We can not introduce new functionality and especially API changes in the bug-fix version. If we are blocked for making 1.3 release earlier than September than we have a real issue IMO, let's discuss and try to solve it ASAP.
  • Also, as a side comment, it is possible to configure storageClass using (a) default configuration (b) raw configuration so it can not be considered as a blocker for using in any case

@rm3l
Copy link
Member Author

rm3l commented Jul 11, 2024

Customer confirmed this is not needed in 1.2.x

/close

@openshift-ci openshift-ci bot closed this Jul 11, 2024
Copy link

openshift-ci bot commented Jul 11, 2024

@rm3l: Closed this PR.

In response to this:

Customer confirmed this is not needed in 1.2.x

/close

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-sigs/prow repository.

@rm3l rm3l deleted the RHIDP-2705--allow-configuring-storageclass-configuration-for-pvc-created-by-operator branch July 11, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants