fix: mark the topolvm storageclass as default #210
Conversation
openshift-ci
bot
added
the
approved
controllers/topolvm_storageclass.go
Outdated
| sc := []*storagev1.StorageClass{} | ||
| allowVolumeExpansion := true | ||
| volumeBindingMode := storagev1.VolumeBindingWaitForFirstConsumer | ||
| // defaultStorageClass := false |
There was a problem hiding this comment.
| // defaultStorageClass := false | |
| // defaultStorageClass := false |
controllers/topolvm_storageclass.go
Outdated
| r.Log.Error(err, "failed to list storage classes. Not setting any storageclass as the default") | ||
| } else { | ||
| if len(scList.Items) == 0 { | ||
| //defaultStorageClass = true |
There was a problem hiding this comment.
| //defaultStorageClass = true | |
| // defaultStorageClass = true |
controllers/topolvm_storageclass.go
Outdated
| //defaultStorageClass = true | ||
| } else { | ||
| for _, sc := range scList.Items { | ||
| v := sc.Annotations["storageclass.kubernetes.io/is-default-class"] |
There was a problem hiding this comment.
could move storageclass.kubernetes.io/is-default-class out as a variable as its used in more than one place.
controllers/topolvm_storageclass.go
Outdated
|
|
||
| // Mark the first lvmo storage class default if no other storageclasses exist on the cluster | ||
| scList := &storagev1.StorageClassList{} | ||
| err := r.Client.List(context.TODO(), scList) |
There was a problem hiding this comment.
| err := r.Client.List(context.TODO(), scList) | |
| err := r.Client.List(ctx, scList) |
|
Making lvmo storage class as default seems like an important detail that user should know about. Are there plans to update the docs to reflect this? |
| scList := &storagev1.StorageClassList{} | ||
| err := r.Client.List(context.TODO(), scList) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
This if else block can be removed and only for loop can be used.
There was a problem hiding this comment.
updated to set a new var in the if block.
controllers/topolvm_storageclass.go
Outdated
| @@ -106,6 +129,11 @@ func getTopolvmStorageClasses(lvmCluster *lvmv1alpha1.LVMCluster) []*storagev1.S | |||
| "csi.storage.k8s.io/fstype": TopolvmFilesystemType, | |||
| }, | |||
| } | |||
| // reconcile will pick up any existing LVMO storage classes as well | |||
| if defaultStorageClassName == "" || defaultStorageClassName == storageClass.Name { | |||
There was a problem hiding this comment.
won't defaultStorageClassName == "" condition be enough even if the reconciler picks up existing lvmo storage classes ?
There was a problem hiding this comment.
This is to handle a scenario where multiple VGs may be supported.
controllers/topolvm_storageclass.go
Outdated
| if len(scList.Items) != 0 { | ||
| for _, sc := range scList.Items { |
There was a problem hiding this comment.
| if len(scList.Items) != 0 { | |
| for _, sc := range scList.Items { | |
| for _, sc := range scList.Items { |
| for _, deviceClass := range lvmCluster.Spec.Storage.DeviceClasses { | ||
| scName := "odf-lvm-" + deviceClass.Name |
There was a problem hiding this comment.
Should we define a new variable called scPrefix := "odf-lvm-" and use it everywhere in place of this string?
There was a problem hiding this comment.
That would need to be done in a separate PR.
This commit marks the topolvm storageClass as default if there are no other storageClasses that are present or is no other class is marked default. Signed-off-by: N Balachandran <nibalach@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nbalacha, sp98 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 |
|
/cherry-pick release-4.11 |
|
@nbalacha: new pull request created: #214 In response to this:
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. |
Operations require a default storage class to function properly, since we are installing those, we can also mark them default: - Set HPP storage class as default - LVM Operator takes care of creating and setting its storage class to default: openshift/lvm-operator#210 So no action required. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
Operations require a default storage class to function properly, since we are installing those, we can also mark them default: - Set HPP storage class as default - LVM Operator takes care of creating and setting its storage class to default: openshift/lvm-operator#210 So no action required. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
Operations require a default storage class to function properly, since we are installing those, we can also mark them default: - Set HPP storage class as default - LVM Operator takes care of creating and setting its storage class to default: openshift/lvm-operator#210 So no action required. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
This commit marks the topolvm storageClass as the default
if there are no other storageClasses that are present or
is no other class is marked default.
Signed-off-by: N Balachandran nibalach@redhat.com