deprecate the allowRemoteStorageConsumer spec from storageCluster #3019

rewantsoni · 2025-02-06T11:27:44Z

The PR does the following:

Deprecate the spec allowRemoteStorageConsumer from storageCluster API
Remove all references of allowRemoteStorageConsumer and the annotation ocs.openshift.io/deployment-mode
Stop reconciling the mirroring field in the cephblockpool controller (this was done as mirroring for internal mode was controlled via the ocs.openshift.io/deployment-mode annotation)
Stop creating/managing the default svg as that will be handled with provider-client communication
Always set DisableCSIDriver to true in ocs-operator-config
Fix the tests
Deploy ocs-client op along with the ocs-op in the ci

openshift-ci · 2025-02-06T11:27:48Z

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

rewantsoni · 2025-02-06T13:01:52Z

/hold for testing

api/v1/storagecluster_types.go

leelavg · 2025-02-10T05:05:23Z

controllers/storagecluster/cephblockpools.go

were these removed code paths in this file guarding against X from happening or ensuring that X happens? if it's the latter where is the replacement?

as an example

if it's not provider mode do X

if it's provider mode do X

The code paths that are removed in this profile were ensuring that mirroring of cephblockpool is established for DR. The replacement for that Mirroring controller, that handles mirroring of blockpools as well as radosnamespace.
There would need to be adjustments in MCO op as well such that they follow the new method of setting up RDR

There would need to be adjustments in MCO

Ack, this what I was looking for, thanks.

leelavg · 2025-02-10T05:11:00Z

controllers/storagecluster/cephfilesystem.go

better not to move this code, let the implicit rns & default svg continue to be created in this reconcile itself as the same code will anyways need to run even at storageconsumer controller if moved after a check.

in consumer controller we just perform the check to create or not to create an svg. let's try to move the pieces as less as possible until it goes against final design/outcome, like in storage classes it's a must but not for svg.

Why, shouldnt we need to have storageconsumer as the source of truth for the SVG and RNS? we should not be creating it in multiple classes

leelavg · 2025-02-10T06:11:05Z

controllers/storagecluster/noobaa_system_reconciler.go

-	if sc.Spec.AllowRemoteStorageConsumers {
-		util.AddAnnotation(nb, "MulticloudObjectGatewayProviderMode", "true")
-	}
+	util.AddAnnotation(nb, "MulticloudObjectGatewayProviderMode", "true")


does any change to this needs a touch in Noobaa? Let's note such instances directly on corresponding Jira for posterity.

can you get more info on this? is removing this check makes MCG behave differently than internal mode, which we don't want.

Adding this annotation sets the "RESTRICT_RESOURCE_DELETION" env variable in noobaa. This ensures that the bucket can be deleted only by the owner of the bucket and not by any other noobaa account.
cc: @ezio-auditore

How would that affect users who deletes buckets using S3? or using the new s3 bowser experience?

leelavg · 2025-02-10T06:14:16Z

controllers/storagecluster/storageclasses.go

-		if r.AvailableCrds[VirtualMachineCrdName] {
-			ret = append(ret, newCephBlockPoolVirtualizationStorageClassConfiguration(initData))
-		}
+	//TODO: will be removed when we start handling kubevirt class through provider-client communication


as mentioned above, pls add these notes to corresponding Jira as well, preferably as a single comment (after PR merge)?

ack, I will add these to jira

rewantsoni · 2025-02-18T05:42:55Z

/test ocs-operator-bundle-e2e-aws

deprecate allowremotestorageconsumer flag from the storagecluster spec Signed-off-by: Rewant Soni <[email protected]>

Signed-off-by: Rewant Soni <[email protected]>

rewantsoni · 2025-02-28T09:42:28Z

/test ocs-operator-bundle-e2e-aws

catalog/index.yaml

Madhu-1 · 2025-02-28T12:51:59Z

controllers/storagecluster/cephblockpools.go

@@ -18,29 +18,6 @@ import (

 type ocsCephBlockPools struct{}

-// ensures that peer cluster secret exists and adds it to CephBlockPool
-func (o *ocsCephBlockPools) addPeerSecretsToCephBlockPool(r *StorageClusterReconciler, storageCluster *ocsv1.StorageCluster, poolName, poolNamespace string) *cephv1.MirroringPeerSpec {


This commit is having 3 different changes? can we move specific changes to a commit (with details) so that it helps in reviewing and captures the history on why its changed

Madhu-1 · 2025-02-28T12:53:16Z

controllers/storagecluster/cephfilesystem.go

Why, shouldnt we need to have storageconsumer as the source of truth for the SVG and RNS? we should not be creating it in multiple classes

Madhu-1 · 2025-02-28T12:54:38Z

controllers/storagecluster/volumegroupsnapshotterclasses.go

@@ -56,15 +56,19 @@ func newCephFilesystemGroupSnapshotClassConfiguration(instance *ocsv1.StorageClu
 	return GroupSnapshotClassConfiguration{
 		groupSnapshotClass: newVolumeGroupSnapshotClass(instance, cephfsGroupSnapshotter),
 		reconcileStrategy:  ReconcileStrategy(instance.Spec.ManagedResources.CephFilesystems.ReconcileStrategy),
-		disable:            instance.Spec.AllowRemoteStorageConsumers,
+		//TODO: This resource will be managed by the provider-client flow, replacing it with same field used to disable


open a jira task for the specific TODO so that we dont miss it

There is a jira already present and Malay is working on it: #3037

Madhu-1 · 2025-02-28T12:55:17Z

pkg/deploy-manager/subscription.go

@@ -186,6 +186,20 @@ func (t *DeployManager) generateClusterObjects(ocsCatalogImage string, subscript
 	}
 	ocsSubscription.SetGroupVersionKind(schema.GroupVersionKind{Group: v1alpha1.SchemeGroupVersion.Group, Kind: "Subscription", Version: v1alpha1.SchemeGroupVersion.Version})

+	ocsClientSubscription := v1alpha1.Subscription{


can you please add a comment for this change?

controllers/storagecluster/cephfilesystem_test.go

nb-ohad · 2025-03-02T12:43:39Z

controllers/storagecluster/noobaa_system_reconciler.go

-	if sc.Spec.AllowRemoteStorageConsumers {
-		util.AddAnnotation(nb, "MulticloudObjectGatewayProviderMode", "true")
-	}
+	util.AddAnnotation(nb, "MulticloudObjectGatewayProviderMode", "true")


How would that affect users who deletes buckets using S3? or using the new s3 bowser experience?

openshift-ci · 2025-03-03T05:41:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rewantsoni
Once this PR has been reviewed and has the lgtm label, please ask for approval from nb-ohad. For more information see the 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:

OWNERS

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

Signed-off-by: Rewant Soni <[email protected]>

Remove the deployment mode annotation ref from cephblockpool reconcile since mirroring will be handled by the mirroring controller Signed-off-by: Rewant Soni <[email protected]>

Signed-off-by: Rewant Soni <[email protected]>

rewantsoni · 2025-03-03T08:35:34Z

/test ocs-operator-bundle-e2e-aws

Signed-off-by: Rewant Soni <[email protected]>

openshift-ci · 2025-03-03T15:04:11Z

@rewantsoni: 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/ocs-operator-bundle-e2e-aws	`2581fae`	link	true	`/test ocs-operator-bundle-e2e-aws`

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-sigs/prow repository. I understand the commands that are listed here.

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 Feb 6, 2025

rewantsoni force-pushed the allowremotestorageconsuemrs branch 3 times, most recently from 2f49263 to 01d4434 Compare February 6, 2025 12:46

rewantsoni marked this pull request as ready for review February 6, 2025 13:01

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 Feb 6, 2025

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 6, 2025

rewantsoni changed the title ~~deprecate and remove the allowRemoteStorageConsumer spec from storageCluster~~ deprecate the allowRemoteStorageConsumer spec from storageCluster Feb 6, 2025

leelavg reviewed Feb 10, 2025

View reviewed changes

rewantsoni force-pushed the allowremotestorageconsuemrs branch from 01d4434 to e1d602d Compare February 10, 2025 13:04

rewantsoni mentioned this pull request Feb 27, 2025

remove the init container that decides the allowing of reconciliation of manager red-hat-storage/ocs-client-operator#301

Open

rewantsoni added 2 commits February 27, 2025 17:40

api: deprecate allowremotestorageconsumer in sc spec

fa34083

deprecate allowremotestorageconsumer flag from the storagecluster spec Signed-off-by: Rewant Soni <[email protected]>

add generated changes

b4cfa1f

Signed-off-by: Rewant Soni <[email protected]>

rewantsoni force-pushed the allowremotestorageconsuemrs branch 3 times, most recently from 000a473 to 6a5c83d Compare February 27, 2025 15:41

Madhu-1 reviewed Feb 28, 2025

View reviewed changes

nb-ohad requested changes Mar 2, 2025

View reviewed changes

openshift-ci bot assigned nb-ohad Mar 2, 2025

rewantsoni force-pushed the allowremotestorageconsuemrs branch from 6a5c83d to e151088 Compare March 3, 2025 05:41

rewantsoni added 2 commits March 3, 2025 11:13

controllers: remove allowremotestorageconsumer ref

b865955

Signed-off-by: Rewant Soni <[email protected]>

controllers: remove the ocs.openshift.io/deployment-mode annotation ref

2417530

Remove the deployment mode annotation ref from cephblockpool reconcile since mirroring will be handled by the mirroring controller Signed-off-by: Rewant Soni <[email protected]>

rewantsoni force-pushed the allowremotestorageconsuemrs branch from e151088 to 6f8d16f Compare March 3, 2025 05:43

fix the tests

e67b581

Signed-off-by: Rewant Soni <[email protected]>

rewantsoni force-pushed the allowremotestorageconsuemrs branch from 6f8d16f to 551fa90 Compare March 3, 2025 05:45

rewantsoni force-pushed the allowremotestorageconsuemrs branch from 551fa90 to b908cc2 Compare March 3, 2025 09:46

bundle: deploy ocs-client with ocs-op

2581fae

Signed-off-by: Rewant Soni <[email protected]>

rewantsoni force-pushed the allowremotestorageconsuemrs branch from b908cc2 to 2581fae Compare March 3, 2025 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate the allowRemoteStorageConsumer spec from storageCluster #3019

deprecate the allowRemoteStorageConsumer spec from storageCluster #3019

rewantsoni commented Feb 6, 2025 •

edited

Loading

openshift-ci bot commented Feb 6, 2025

rewantsoni commented Feb 6, 2025

leelavg Feb 10, 2025

rewantsoni Feb 10, 2025

leelavg Feb 10, 2025

leelavg Feb 10, 2025

Madhu-1 Feb 28, 2025

leelavg Feb 10, 2025

rewantsoni Feb 10, 2025

nb-ohad Mar 2, 2025

leelavg Feb 10, 2025

rewantsoni Feb 10, 2025

rewantsoni commented Feb 18, 2025

rewantsoni commented Feb 28, 2025

Madhu-1 Feb 28, 2025

Madhu-1 Feb 28, 2025

Madhu-1 Feb 28, 2025

rewantsoni Mar 3, 2025

Madhu-1 Feb 28, 2025

nb-ohad Mar 2, 2025

openshift-ci bot commented Mar 3, 2025

rewantsoni commented Mar 3, 2025

openshift-ci bot commented Mar 3, 2025

deprecate the allowRemoteStorageConsumer spec from storageCluster #3019

Are you sure you want to change the base?

deprecate the allowRemoteStorageConsumer spec from storageCluster #3019

Conversation

rewantsoni commented Feb 6, 2025 • edited Loading

openshift-ci bot commented Feb 6, 2025

rewantsoni commented Feb 6, 2025

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 Feb 18, 2025

rewantsoni commented Feb 28, 2025

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

openshift-ci bot commented Mar 3, 2025

rewantsoni commented Mar 3, 2025

openshift-ci bot commented Mar 3, 2025

rewantsoni commented Feb 6, 2025 •

edited

Loading