Labels volumesnapshot class with storageid #240

vbnrh · 2024-11-21T11:26:46Z

This commit labels all rbd and cephfs storageclasses and associated volumesnapshotclasses with storageid.

Also adds storageid label to the volumereplication class being created

The implementation is moved to work for both MDR and RDR scenarios

This commit labels all rbd and cephfs storageclasses and associated volumesnapshotclasses with storageid. Also adds storageid label to the volumereplication class being created The implementation is moved to work for both MDR and RDR scenarios Signed-off-by: vbadrina <[email protected]>

vbnrh · 2024-11-21T13:05:46Z

/test integration-test

vbnrh · 2024-11-21T15:43:22Z

/test integration-test

ShyamsundarR

Other than queries raised this PR is functionally fine.

I have other concerns though which are more dated (and possibly can be fixed/updated in the current cycle as Ramen starts leveraging the IDs more ).

labelStorageClasses is currently labeling all SCs that match the provisioner name with the same fsid
- So if a user created a new replica-2 pool or a new SC for some other pool type, this code would label it with the same fsid as the storageID
- We should be selective while labeling the SCs, currently only the default SC should be labelled and other SCs should not be labelled at all based on the fact that we only mirror enable the default CephBlockPool with MCO
labelVolumeSnapshotClasses has the same concerns as above
We should specialize the fsid to be more narrow than the overall Ceph fsid and should add the poolID into the mix
- For example, if a user creates a non-default replica-2 pool, then we would need to differentiate between the 2 pools storageID. For such purposes, we would need to consider the pool ID in addition to the fsid when labeling this as the storageID (and similarly when crafting the replicationID as well)

As it currently stands, this will work in the narrow use-case where a user has the default SCs (and hence pools) in the internal mode of operation. It would be good to address some of the ID generation and mapping the same into the classes as a subsequent PR for the current release cycle, such that as we consider enhancements to support mirror enabling user created pools, we can distinguish their storage/replication IDs sufficiently well.d (and possibly can be fixed/updated in the current cycle as Ramen starts leveraging the IDs more ).

cc: @umangachapagain

ShyamsundarR · 2024-11-21T21:27:38Z

controllers/drpolicy_controller.go

@@ -188,6 +189,7 @@ func (r *DRPolicyReconciler) createOrUpdateManifestWorkForVRC(ctx context.Contex
 		labels := make(map[string]string)
 		labels[fmt.Sprintf(RamenLabelTemplate, ReplicationIDKey)] = replicationId
 		labels[fmt.Sprintf(RamenLabelTemplate, "maintenancemodes")] = "Failover"
+		labels[fmt.Sprintf(RamenLabelTemplate, StorageIDKey)] = clusterFSIDs[pr.ClusterName]


On upgrade the reconciler will ensure this is added? Just checking, the code is using the appropriate CreateOrUpdate etc. but asking anyway.

Yes, i tested it.

ShyamsundarR · 2024-11-21T22:12:28Z

addons/agent_mirrorpeer_controller.go

-			return ctrl.Result{}, fmt.Errorf("an unknown error occurred while fetching the cluster fsids, retrying again: %v", err)
+	clusterFSIDs := make(map[string]string)
+	logger.Info("Fetching clusterFSIDs")
+	err = r.fetchClusterFSIDs(ctx, &mirrorPeer, clusterFSIDs)


If it is of type hasStorageClientRef then will the call the fetchClusterFSIDs work or always error out? As it was called within the if prior to this case, wondering if we should retain it as such.

Yes, it will work. If hasStorageClientRef is true , then these agents would be created on provider clusters. It should always be able to fetch ceph fsids.

vbnrh · 2024-11-25T12:13:58Z

Other than queries raised this PR is functionally fine.

I have other concerns though which are more dated (and possibly can be fixed/updated in the current cycle as Ramen starts leveraging the IDs more ).
* `labelStorageClasses` is currently labeling all SCs that match the provisioner name with the same fsid
  
  * So if a user created a new replica-2 pool or a new SC for some other pool type, this code would label it with the same fsid as the storageID
  * We should be selective while labeling the SCs, currently only the default SC should be labelled and other SCs should not be labelled at all based on the fact that we only mirror enable the default CephBlockPool with MCO

* labelVolumeSnapshotClasses has the same concerns as above

* We should specialize the fsid to be more narrow than the overall Ceph fsid and should add the poolID into the mix
  
  * For example, if a user creates a non-default replica-2 pool, then we would need to differentiate between the 2 pools storageID. For such purposes, we would need to consider the pool ID in addition to the fsid when labeling this as the storageID (and similarly when crafting the replicationID as well)
As it currently stands, this will work in the narrow use-case where a user has the default SCs (and hence pools) in the internal mode of operation. It would be good to address some of the ID generation and mapping the same into the classes as a subsequent PR for the current release cycle, such that as we consider enhancements to support mirror enabling user created pools, we can distinguish their storage/replication IDs sufficiently well.d (and possibly can be fixed/updated in the current cycle as Ramen starts leveraging the IDs more ).

cc: @umangachapagain

If the ID generation

Other than queries raised this PR is functionally fine.

I have other concerns though which are more dated (and possibly can be fixed/updated in the current cycle as Ramen starts leveraging the IDs more ).
* `labelStorageClasses` is currently labeling all SCs that match the provisioner name with the same fsid
  
  * So if a user created a new replica-2 pool or a new SC for some other pool type, this code would label it with the same fsid as the storageID
  * We should be selective while labeling the SCs, currently only the default SC should be labelled and other SCs should not be labelled at all based on the fact that we only mirror enable the default CephBlockPool with MCO

* labelVolumeSnapshotClasses has the same concerns as above

* We should specialize the fsid to be more narrow than the overall Ceph fsid and should add the poolID into the mix
  
  * For example, if a user creates a non-default replica-2 pool, then we would need to differentiate between the 2 pools storageID. For such purposes, we would need to consider the pool ID in addition to the fsid when labeling this as the storageID (and similarly when crafting the replicationID as well)
As it currently stands, this will work in the narrow use-case where a user has the default SCs (and hence pools) in the internal mode of operation. It would be good to address some of the ID generation and mapping the same into the classes as a subsequent PR for the current release cycle, such that as we consider enhancements to support mirror enabling user created pools, we can distinguish their storage/replication IDs sufficiently well.d (and possibly can be fixed/updated in the current cycle as Ramen starts leveraging the IDs more ).

cc: @umangachapagain

Is the ask only to make the StorageID more unique ? (CephFSID + PoolID)

We may need to sync it onto the hub in simliar way to cephfsid when we would apply StorageID onto the volumereplicationclasses as well. We will have to add it onto the design. I will send a subsequent PR for it. Will that work ?

ShyamsundarR · 2024-11-25T22:11:36Z

As it currently stands, this will work in the narrow use-case where a user has the default SCs (and hence pools) in the internal mode of operation. It would be good to address some of the ID generation and mapping the same into the classes as a subsequent PR for the current release cycle, such that as we consider enhancements to support mirror enabling user created pools, we can distinguish their storage/replication IDs sufficiently well.d (and possibly can be fixed/updated in the current cycle as Ramen starts leveraging the IDs more ).
cc: @umangachapagain

Is the ask only to make the StorageID more unique ? (CephFSID + PoolID)

We may need to sync it onto the hub in simliar way to cephfsid when we would apply StorageID onto the volumereplicationclasses as well. We will have to add it onto the design. I will send a subsequent PR for it. Will that work ?

Ack! I suggest we complete this soon. Get this PR in, such that current DR works, but avoid upgrade issues by tackling the issue of ID uniqueness soon after within the same release.

The intention for internal mode would be (as it does not leverage RADOS namespaces (yet?)):

StorageID = Ceph fsid + Pool ID (MDS pool of CephFS or RBD pool ID as the case maybe)
ReplicationID = StorageID1+StorageID2 (I think this is the case at present, but just making sure of the same)

umangachapagain · 2024-11-27T05:08:36Z

We will continue this discussion and send a follow up patch. Merging this PR.

openshift-ci · 2024-11-27T05:08:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: umangachapagain, vbnrh

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~~ [umangachapagain,vbnrh]

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

vbnrh · 2024-12-04T11:08:37Z

/cherry-pick release-4.18

openshift-cherrypick-robot · 2024-12-04T11:09:16Z

@vbnrh: new pull request created: #243

In response to this:

/cherry-pick release-4.18

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.

openshift-ci bot added the approved label Nov 21, 2024

ShyamsundarR reviewed Nov 21, 2024

View reviewed changes

umangachapagain approved these changes Nov 27, 2024

View reviewed changes

openshift-ci bot assigned umangachapagain Nov 27, 2024

openshift-ci bot added the lgtm label Nov 27, 2024

openshift-merge-bot bot merged commit 1aeb02f into red-hat-storage:main Nov 27, 2024
11 checks passed

openshift-cherrypick-robot mentioned this pull request Dec 4, 2024

DFBUGS-911: [release-4.18] Labels volumesnapshot class with storageid #243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Labels volumesnapshot class with storageid #240

Labels volumesnapshot class with storageid #240

vbnrh commented Nov 21, 2024

vbnrh commented Nov 21, 2024

vbnrh commented Nov 21, 2024

ShyamsundarR left a comment

ShyamsundarR Nov 21, 2024

vbnrh Nov 25, 2024

ShyamsundarR Nov 21, 2024

vbnrh Nov 25, 2024

vbnrh commented Nov 25, 2024

ShyamsundarR commented Nov 25, 2024

umangachapagain commented Nov 27, 2024

openshift-ci bot commented Nov 27, 2024

vbnrh commented Dec 4, 2024

openshift-cherrypick-robot commented Dec 4, 2024

Labels volumesnapshot class with storageid #240

Labels volumesnapshot class with storageid #240

Conversation

vbnrh commented Nov 21, 2024

vbnrh commented Nov 21, 2024

vbnrh commented Nov 21, 2024

ShyamsundarR left a comment

Choose a reason for hiding this comment

ShyamsundarR Nov 21, 2024

Choose a reason for hiding this comment

vbnrh Nov 25, 2024

Choose a reason for hiding this comment

ShyamsundarR Nov 21, 2024

Choose a reason for hiding this comment

vbnrh Nov 25, 2024

Choose a reason for hiding this comment

vbnrh commented Nov 25, 2024

ShyamsundarR commented Nov 25, 2024

umangachapagain commented Nov 27, 2024

openshift-ci bot commented Nov 27, 2024

vbnrh commented Dec 4, 2024

openshift-cherrypick-robot commented Dec 4, 2024