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

Jump to bottom

Added storageClassName in the PVCs for Minio and MariaDB #464

Merged
merged 2 commits into from
Dec 12, 2023
Merged

Added storageClassName in the PVCs for Minio and MariaDB #464

merged 2 commits into from
Dec 12, 2023

Conversation

VaniHaripriya
Copy link
Contributor

@VaniHaripriya VaniHaripriya commented Nov 16, 2023 •
edited
Loading

Resolves #192

Description of your changes:

  1. Added StorageClassName field under minio and mariaDB sections in the CRD and in minio & mariaDB structs.

  2. Added StorageClassName in the PVC template file for minio and mariaDB.

Testing instructions

  • Deploy DSPO and create a dspa instance using the dspa.yaml file in this path- config/samples/custom-configs.
  • Check the storageClassName assigned with default value by looking at the pvc created for minio and mariaDB.
  • dspa.yaml file can be edited with a different storageClassName to see if its getting assigned.

Checklist

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-464

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-464

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-464

gmfrasca
gmfrasca reviewed Nov 16, 2023
View reviewed changes
config/internal/mariadb/pvc.yaml.tmpl Outdated
@@ -9,6 +9,7 @@ metadata:
spec:
accessModes:
- ReadWriteOnce
storageClassName: {{.MariaDB.StorageClassName}}
Copy link
Member

Choose a reason for hiding this comment

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

Have we tested to ensure if storageClassName was set to "" instead of an actual storage class name, as shown in custom config dspa.yaml below, this still works? It feels like it shouldn't (and therefore we should have logic to only add the item if an explicit value is provided) but perhaps kube/openshift has magic for it

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually seeing an error when I don't specify storageclass:

2023-12-07T19:38:59Z	ERROR	Reconciler error	{"controller": "datasciencepipelinesapplication", "controllerGroup": "datasciencepipelinesapplications.opendatahub.io", "controllerKind": "DataSciencePipelinesApplication", "DataSciencePipelinesApplication": {"name":"sample","namespace":"dspa"}, "namespace": "dspa", "name": "sample", "reconcileID": "46523bbf-59c5-4bf8-9819-1c1c0f9a2768", "error": "PersistentVolumeClaim \"mariadb-sample\" is invalid: spec: Forbidden: spec is immutable after creation except resources.requests for bound claims\n  core.PersistentVolumeClaimSpec{\n  \t... // 2 identical fields\n  \tResources:        {Requests: {s\"storage\": {i: {...}, s: \"10Gi\", Format: \"BinarySI\"}}},\n  \tVolumeName:       \"pvc-1bee7a8f-94b0-48f3-b036-c9f87536f73c\",\n- \tStorageClassName: &\"standard-csi\",\n+ \tStorageClassName: nil,\n  \tVolumeMode:       &\"Filesystem\",\n  \tDataSource:       nil,\n  \tDataSourceRef:    nil,\n  }\n"}
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
	/opt/app-root/src/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.13.0/pkg/internal/controller/controller.go:326
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
	/opt/app-root/src/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.13.0/pkg/internal/controller/controller.go:273
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
	/opt/app-root/src/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.13.0/pkg/internal/controller/controller.go:234

@VaniHaripriya can you take a look at the operator logs when you try to deploy a simple dspa without any storageclassname field set?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to what @gmfrasca and @HumairAK mentioned, when the storageClassName is set to "", this doesn't work.

gmfrasca
gmfrasca reviewed Nov 16, 2023
View reviewed changes
Copy link
Member

@gmfrasca gmfrasca left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 16, 2023
gmfrasca
gmfrasca reviewed Nov 16, 2023
View reviewed changes
api/v1alpha1/dspipeline_types.go Outdated
@@ -194,6 +194,9 @@ type MariaDB struct {
// Customize the size of the PVC created for the default MariaDB instance. Default: 10Gi
// +kubebuilder:default:="10Gi"
PVCSize resource.Quantity `json:"pvcSize,omitempty"`
// volumeMode Filesystem storageClass to use for PVC creation
Copy link
Member

@gmfrasca gmfrasca Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this is trivial but the description provided here very slightly differs from what is committed in the CRD below. (volumeMode vs Volume Mode). They are functionally the same but a side effect of this is that make manifests will update the CRD yaml, causing a git changeset, which may confuse some.

So with that said, could you reconcile this please (either change this to "Volume Mode" or update config/crd/bases/datasciencepipelinesapplications.opendatahub.io_datasciencepipelinesapplications.yaml and run make manifests? to ensure they match up?

@openshift-ci openshift-ci bot removed the lgtm label Nov 17, 2023
@VaniHaripriya
Added storageClassName in the PVCs for Minio and MariaDB
05f7cd7 
Removed unwanted change

Fixed precommit issue

Update description of StorageClassName
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-464

gmfrasca
gmfrasca reviewed Nov 27, 2023
View reviewed changes
Copy link
Member

@gmfrasca gmfrasca left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 27, 2023
@openshift-ci openshift-ci bot removed the lgtm label Dec 11, 2023
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-464

@amadhusu
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 12, 2023
DharmitD
DharmitD approved these changes Dec 12, 2023
View reviewed changes
Copy link
Member

@DharmitD DharmitD left a comment

Choose a reason for hiding this comment

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

/Approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 12, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amadhusu, DharmitD

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

@openshift-merge-bot openshift-merge-bot bot merged commit 28dbeb8 into opendatahub-io:main Dec 12, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HumairAK HumairAK HumairAK left review comments

@gmfrasca gmfrasca gmfrasca left review comments

@amadhusu amadhusu amadhusu approved these changes

@DharmitD DharmitD DharmitD approved these changes

Assignees

@gmfrasca gmfrasca

@amadhusu amadhusu

@DharmitD DharmitD

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mariaDB and minio pods not running without cluster default storageclass set in their PVCs
6 participants
@VaniHaripriya @dsp-developers @amadhusu @HumairAK @gmfrasca @DharmitD