remove storageClassClaim and add new storageClassRequest CR #1913

subhamkrai · 2023-01-10T08:44:37Z

Signed-off-by: subhamkrai srai@redhat.com

openshift-ci · 2023-01-10T08:44:41Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

subhamkrai · 2023-01-12T03:41:39Z

/test all

subhamkrai · 2023-01-12T05:38:09Z

/test all

subhamkrai · 2023-01-12T08:53:38Z

/test all

subhamkrai · 2023-01-18T11:52:57Z

/test all

Madhu-1

@subhamkrai Thanks for the required changes. @nb-ohad, this also needs some migration changes as storageclassclaim needs to be replaced with storageClassRequest in existing clusters.

Madhu-1 · 2023-04-03T10:35:50Z

api/v1/storagecluster_types.go

@@ -244,26 +244,6 @@ type ExternalStorageClusterSpec struct {
 	//+kubebuilder:validation:Enum=ocs;rhcs
 	// StorageProviderKind Identify the type of storage provider cluster this consumer cluster is going to connect to.
 	StorageProviderKind ExternalStorageKind `json:"storageProviderKind,omitempty"`


We dont need storageProviderKind also anymore?

lets keep it with rhcs mode also it might help in the future to add new functionalities

there is no reference to this, so I removed it. Also, in case required in the future, we can add it later.

lets keep it for now as am not sure if someone is using this field other than managed service

@Madhu-1 The default was always RHCS, so no RHCS deployment was using it. And the only other value is ODF which now becomes irrelevant. I think a quick search on the codebase will assure you that this is not even looked at. If I am mistaken here we can keep it

Yes you are correct, it's not used anywhere to be on the safer side we are not removing the Field from the Spec to avoid the foreseen problem, we can also remove it later if it's not used by others after some confirmation.

Madhu-1 · 2023-04-03T10:37:56Z

api/v1alpha1/storageclassrequest_types.go

+
+const (
+	StorageClassRequestFinalizer  = "StorageClassRequest.ocs.openshift.io"
+	StorageClassRequestAnnotation = "ocs.openshift.io.fulfillstoragesclassclaim"


The annotation value need to be changed to math storageClassRequest

Madhu-1 · 2023-04-03T10:39:09Z

config/manifests/bases/ocs-operator.clusterserviceversion.yaml

@@ -0,0 +1,62 @@
+apiVersion: operators.coreos.com/v1alpha1


Why we got the new CSV file here?

my branch is older, so before fusion addition, it was like this. So, I removed the file now. Thanks

Madhu-1 · 2023-04-03T10:43:12Z

services/provider/client/client.go

@@ -54,15 +54,14 @@ func (cc *OCSProviderClient) Close() {

 // OnboardConsumer to validate the consumer and create StorageConsumer
 // resource on the StorageProvider cluster
-func (cc *OCSProviderClient) OnboardConsumer(ctx context.Context, ticket, name, capacity string) (*pb.OnboardConsumerResponse, error) {
+func (cc *OCSProviderClient) OnboardConsumer(ctx context.Context, ticket, name string) (*pb.OnboardConsumerResponse, error) {


we dont need any change in this file as its a GRPC client

Just remove the capacity nothing else

Madhu-1 · 2023-04-03T10:44:10Z

services/provider/proto/provider.proto

@@ -26,10 +26,10 @@ service OCSProvider {
  rpc AcknowledgeOnboarding(AcknowledgeOnboardingRequest)
  returns (AcknowledgeOnboardingResponse){}

-  // FulfillStorageClassClaim RPC call to create the StorageclassClaim CR on
+  // StorageClassRequest RPC call to create the StorageclassClaim CR on


No change is required in proto file as well

Is this supposed to change terminology? We are still fulfilling a storageclassclaim (not request) so I am not sure why is this change here.

Madhu-1 · 2023-04-03T10:44:50Z

services/provider/server/server.go

@@ -76,14 +76,14 @@ func NewOCSProviderServer(ctx context.Context, namespace string) (*OCSProviderSe

 	storageClassClaimManager, err := newStorageClassClaimManager(ctx, client, namespace)


newStorageClassClaimManager need to be changed to newStorageClassRequestManager

Madhu-1 · 2023-04-03T10:45:27Z

services/provider/server/server.go

 // provider cluster.
-func (s *OCSProviderServer) FulfillStorageClassClaim(ctx context.Context, req *pb.FulfillStorageClassClaimRequest) (*pb.FulfillStorageClassClaimResponse, error) {
+func (s *OCSProviderServer) StorageClassRequest(ctx context.Context, req *pb.StorageClassRequestRequest) (*pb.StorageClassRequestResponse, error) {


the RPC calls should remain the same only the CRD create/Delete/Get API calls need to be changed

Is this supposed to change terminology? We are still fulfilling a storageclassclaim (not request) so I am not sure why is this change here.

Madhu-1 · 2023-04-03T12:09:57Z

api/v1alpha1/storageclassrequest_types.go

+	metav1.TypeMeta   `json:",inline"`
+	metav1.ObjectMeta `json:"metadata,omitempty"`
+
+	Spec   StorageClassRequestSpec   `json:"spec,omitempty"`


not related to this PR but Spec is a required field not a optional to do omitempty

Madhu-1 · 2023-04-03T12:11:39Z

config/rbac/role.yaml

+- apiGroups:
+  - ocs.openshift.io
+  resources:
+  - storageclassrequests
+  verbs:
+  - get
+  - list


Why did this gets added new?

should be good now

Madhu-1 · 2023-04-03T12:13:07Z

controllers/storageclassrequest/storageclassrequest_controller.go

+	var result reconcile.Result
+	var reconcileError error
+
+	result, reconcileError = r.reconcileProviderPhases()


call this as reconcilePhases()?

Madhu-1 · 2023-04-03T12:15:28Z

main.go

-	if err = (&controllers.StorageConsumerReconciler{
-		Client: mgr.GetClient(),
-		Log:    ctrl.Log.WithName("controllers").WithName("StorageConsumer"),
-		Scheme: mgr.GetScheme(),
-	}).SetupWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create controller", "controller", "StorageConsumer")
-		os.Exit(1)
-	}


Why storageConsumer is removed?

Madhu-1 · 2023-04-03T12:16:06Z

services/provider/server/server.go

@@ -74,16 +74,16 @@ func NewOCSProviderServer(ctx context.Context, namespace string) (*OCSProviderSe
 		return nil, fmt.Errorf("failed to create new OCSConumer instance. %v", err)
 	}

-	storageClassClaimManager, err := newStorageClassClaimManager(ctx, client, namespace)
+	storageClassRequestManager, err := newStorageClassClaimManager(ctx, client, namespace)


still its calling newStorageClassClaimManager

rewantsoni · 2023-04-12T05:52:51Z

Tested migration of the provider and consumer with the build shared by Subham and it was a success. @Madhu-1 @subhamkrai

Madhu-1

LGTM

Madhu-1 · 2023-04-12T06:41:15Z

services/provider/server/server.go

@@ -33,16 +33,16 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/klog/v2"
+	"k8s.io/klog"


this is not addressed yet.

Madhu-1 · 2023-04-12T06:42:13Z

Tested migration of the provider and consumer with the build shared by Subham and it was a success. @Madhu-1 @subhamkrai

Thank you @rewantsoni for testing it.

subhamkrai · 2023-04-12T10:23:30Z

Tested migration of the provider and consumer with the build shared by Subham and it was a success. @Madhu-1 @subhamkrai

thanks @rewantsoni for testing

controllers/storagecluster/uninstall_reconciler.go

iamniting · 2023-04-12T11:18:16Z

catalog/ocs-bundle.yaml

@@ -98,11 +98,6 @@ properties:
    group: ocs.openshift.io
    kind: OCSInitialization
    version: v1
- type: olm.gvk


@subhamkrai Can you drop this change also?

reverted. Thanks!

from 4.13 we are moving from storageClassClaim to storageClassRequest CR. Also, we don't need consumer mode code in ocs-operator now, new operator will have consumer. Signed-off-by: subhamkrai <srai@redhat.com>

Signed-off-by: subhamkrai <srai@redhat.com>

openshift-ci · 2023-04-13T05:43:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, Madhu-1, subhamkrai

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~~ [iamniting]

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

iamniting · 2023-04-13T07:03:10Z

/cherry-pick release-4.13

openshift-cherrypick-robot · 2023-04-13T07:03:49Z

@iamniting: #1913 failed to apply on top of branch "release-4.13":

Applying: remove storageClassClaim and add new cr
Using index info to reconstruct a base tree...
M	controllers/storageclassclaim/storageclassclaim_controller.go
M	controllers/storagecluster/external_resources.go
Falling back to patching base and 3-way merge...
Removing services/provider/server/storageclaim_test.go
Removing services/provider/server/storageclaim.go
Removing services/consumer/main.go
Removing deploy/ocs-operator/manifests/storageclassclaim.crd.yaml
Removing deploy/ics-operator/manifests/storageclassclaim.crd.yaml
Auto-merging controllers/storagecluster/external_resources.go
Removing controllers/storagecluster/external_ocs.go
CONFLICT (modify/delete): controllers/storageclassclaim/storageclassclaim_controller.go deleted in remove storageClassClaim and add new cr and modified in HEAD. Version HEAD of controllers/storageclassclaim/storageclassclaim_controller.go left in tree.
Removing config/rbac/storageclassclaim_viewer_role.yaml
Removing config/rbac/storageclassclaim_editor_role.yaml
Removing config/crd/bases/ocs.openshift.io_storageclassclaims.yaml
Removing api/v1alpha1/storageclassclaim_types.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 remove storageClassClaim and add new cr
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.13

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.

iamniting · 2023-04-13T07:05:30Z

@subhamkrai Can you pls create a manual backport?

subhamkrai · 2023-04-13T07:20:50Z

@subhamkrai Can you pls create a manual backport?

here #2007

Madhu-1 · 2023-04-21T11:02:36Z

services/provider/server/storageclassrequest.go

+			Namespace: s.namespace,
+			Labels: map[string]string{
+				controllers.ConsumerUUIDLabel: consumerUUID,
+				storageClassRequestName:       storageClassRequestName,


@subhamkrai This should be storageClassClaimNameLabel we are adding name for both the label key and label value.

Let's prioritize it and get it in both 4.13 and 4.12

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2023

subhamkrai force-pushed the remove-storage-class-claim branch from 3527e83 to f39ce66 Compare January 12, 2023 03:40

subhamkrai force-pushed the remove-storage-class-claim branch from f39ce66 to a0acbb8 Compare January 12, 2023 03:59

subhamkrai force-pushed the remove-storage-class-claim branch from a0acbb8 to e25bf76 Compare January 12, 2023 08:51

subhamkrai force-pushed the remove-storage-class-claim branch from e25bf76 to f80144c Compare January 18, 2023 11:01

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2023

subhamkrai force-pushed the remove-storage-class-claim branch from f80144c to 7a66d7b Compare January 24, 2023 11:25

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2023

subhamkrai force-pushed the remove-storage-class-claim branch from 7a66d7b to 67c933d Compare March 30, 2023 14:43

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2023

subhamkrai force-pushed the remove-storage-class-claim branch 7 times, most recently from 546e2c4 to efc8f0a Compare April 3, 2023 10:13

subhamkrai marked this pull request as ready for review April 3, 2023 10:28

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2023

Madhu-1 reviewed Apr 3, 2023

View reviewed changes

subhamkrai force-pushed the remove-storage-class-claim branch from efc8f0a to 770dada Compare April 3, 2023 11:02

subhamkrai requested a review from Madhu-1 April 3, 2023 11:09

Madhu-1 reviewed Apr 3, 2023

View reviewed changes

Madhu-1 requested a review from nb-ohad April 3, 2023 12:26

Madhu-1 requested a review from iamniting April 12, 2023 06:36

Madhu-1 approved these changes Apr 12, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2023

subhamkrai force-pushed the remove-storage-class-claim branch from 6c07044 to bf0c1c8 Compare April 12, 2023 10:19

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2023

subhamkrai force-pushed the remove-storage-class-claim branch from bf0c1c8 to 6708e0e Compare April 12, 2023 10:22

subhamkrai force-pushed the remove-storage-class-claim branch from 6708e0e to 97ccb44 Compare April 12, 2023 10:48

iamniting requested changes Apr 12, 2023

View reviewed changes

controllers/storagecluster/uninstall_reconciler.go Show resolved Hide resolved

iamniting requested changes Apr 12, 2023

View reviewed changes

subhamkrai added 2 commits April 12, 2023 17:13

remove storageClassClaim and add new cr

55e5c08

from 4.13 we are moving from storageClassClaim to storageClassRequest CR. Also, we don't need consumer mode code in ocs-operator now, new operator will have consumer. Signed-off-by: subhamkrai <srai@redhat.com>

these are auto generated files

1429d7e

Signed-off-by: subhamkrai <srai@redhat.com>

subhamkrai force-pushed the remove-storage-class-claim branch from 97ccb44 to 1429d7e Compare April 12, 2023 11:44

iamniting approved these changes Apr 13, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2023

openshift-merge-robot merged commit ee09e5d into red-hat-storage:main Apr 13, 2023
17 checks passed

subhamkrai deleted the remove-storage-class-claim branch April 13, 2023 07:09

iamniting changed the title ~~Bug 2184769: remove storageClassClaim and add new storageClassRequest CR~~ remove storageClassClaim and add new storageClassRequest CR Apr 13, 2023

iamniting removed bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. labels Apr 13, 2023

iamniting mentioned this pull request Apr 13, 2023

Bug 2184769:[release-4.13] remove storageClassClaim and add new storageClassRequest CR #2007

Merged

Madhu-1 reviewed Apr 21, 2023

View reviewed changes

		@@ -76,14 +76,14 @@ func NewOCSProviderServer(ctx context.Context, namespace string) (*OCSProviderSe

		storageClassClaimManager, err := newStorageClassClaimManager(ctx, client, namespace)

remove storageClassClaim and add new storageClassRequest CR #1913

remove storageClassClaim and add new storageClassRequest CR #1913

Conversation

subhamkrai commented Jan 10, 2023

openshift-ci bot commented Jan 10, 2023

subhamkrai commented Jan 12, 2023

subhamkrai commented Jan 12, 2023

subhamkrai commented Jan 12, 2023

subhamkrai commented Jan 18, 2023

Madhu-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rewantsoni commented Apr 12, 2023

Madhu-1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Apr 12, 2023

subhamkrai commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 13, 2023

iamniting commented Apr 13, 2023

openshift-cherrypick-robot commented Apr 13, 2023

iamniting commented Apr 13, 2023

subhamkrai commented Apr 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment