Storageclusterpeer #2466

rewantsoni · 2024-02-20T05:37:25Z

add a new CR and controller to peer two block pools with the same name

used the command: operator-sdk create api --group ocs --version v1 --kind StorageClusterPeer --resource --controller to create the CR and controller
added logic for creating and deletion of storageClusterPeer

Depends on (in Order)

openshift-ci · 2024-02-20T05:37:30Z

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

openshift-ci · 2024-02-20T05:37:43Z

[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 assign nb-ohad for approval. For more information see the Kubernetes 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

rewantsoni · 2024-02-29T13:28:26Z

Last 3 commits are up for review

jarrpa · 2024-03-02T15:55:25Z

Leave a note in the PR description saying which PR this is dependent on.

leelavg

reviewing in phases, will do a more thorough one at the earliest/after draft.

leelavg · 2024-03-04T07:45:09Z

controllers/suite_test.go

I don't think this file is required.

leelavg · 2024-03-04T07:45:56Z

PROJECT

I think it'd be better to add the operator-sdk command that you used for generating the files to this commit msg.

leelavg · 2024-03-04T07:47:46Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

 // StorageClusterPeerReconciler reconciles a StorageClusterPeer object
+// nolint:revive


pls do mention what aspect of the linter we are skipping in a new line.

leelavg · 2024-03-04T07:56:59Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

 }

 //+kubebuilder:rbac:groups=ocs.openshift.io,resources=storageclusterpeers,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=ocs.openshift.io,resources=storageclusterpeers/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=ocs.openshift.io,resources=storageclusterpeers/finalizers,verbs=update
+//+kubebuilder:rbac:groups=ceph.rook.io,resources=cephblockpools;,verbs=list;create;update
+//+kubebuilder:rbac:groups=core,resources=secrets,verbs=get


I don't think get alone works, basically underlying infra lists the existing and keeps on updating the cache so you need list perms as well.

During testing it, it worked with get as far as I recall, I will check it once again and update it

Tested the changes, only using get works

ack, maybe only client from runtime manager require both list & get. Pls recheck that there's no existing rbac for list of secrets and resolve the comment.

leelavg · 2024-03-04T07:57:36Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+	// Apply status changes to the StorageClassRequest
+	statusError := r.Client.Status().Update(r.ctx, r.storageClusterPeer)
+	if statusError != nil {
+		r.Log.Info("Failed to update StorageClassRequest status.")


storageClassRequest, a typo?

Yes, I will fix it

controllers/storageclusterpeer/storageclusterpeer_controller.go

leelavg · 2024-03-04T08:01:31Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+
+func (r *StorageClusterPeerReconciler) reconcileCephBlockPool() error {
+
+	for _, cephBlockPool := range r.cephBlockPoolList.Items {


so mirroring on block pools is all or none?

Yes, we decided that this will be the initial design, if required we might add annotations to whitelist/blacklist the blockpools

leelavg · 2024-03-04T08:11:06Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+}
+
+func (r *StorageClusterPeerReconciler) newExternalClient() (*providerClient.OCSProviderClient, error) {
+	ocsClient, err := providerClient.NewProviderClient(r.ctx, r.storageClusterPeer.Spec.OCSAPIServerURI, time.Second*10)


why do we need to construct a grpc client on every reconcile?

We need to APIServerURI, which will be different for different clusters

rewantsoni · 2024-03-06T06:17:02Z

Testing:

Cluster 1(primary):
[rewantsoni ] date; oc get clusterversion -oyaml | grep clusterID; oc get cephblockpool ocs-storagecluster-cephblockpool -oyaml | yq '.spec .mirroring'; oc get storageclusterpeer
Wed Mar  6 11:38:12 IST 2024
    clusterID: 4293c939-42b2-4565-b22f-81f53df19844
{}
No resources found in openshift-storage namespace.

Cluster 2(secondary):
[rewantsoni ] date; oc get clusterversion -oyaml | grep clusterID; oc get cephblockpool ocs-storagecluster-cephblockpool -oyaml | yq '.spec.mirroring'; oc get cephrbdmirror 
Wed Mar  6 11:40:37 IST 2024
    clusterID: a96546a7-5e86-4d6e-aded-b6664c3d8f3e
{}
No resources found in openshift-storage namespace.

After applying StorageClusterPeer:

Cluster 1(primary):
[rewantsoni ] date; oc get clusterversion -oyaml | grep clusterID; oc get cephblockpool ocs-storagecluster-cephblockpool -oyaml | yq '.spec .mirroring'; oc get storageclusterpeer
Wed Mar  6 11:41:48 IST 2024
    clusterID: 4293c939-42b2-4565-b22f-81f53df19844
enabled: true
mode: image
NAME                        AGE   PHASE
storageclusterpeer-sample   18s   Ready

Cluster 2(secondary):
[rewantsoni ] date; oc get clusterversion -oyaml | grep clusterID; oc get cephblockpool ocs-storagecluster-cephblockpool -oyaml | yq '.spec.mirroring'; oc get cephrbdmirror; oc get secret bootstrap-secret-e01e021da3ee4ed84ed95581cb7ae967
Wed Mar  6 11:43:16 IST 2024
    clusterID: a96546a7-5e86-4d6e-aded-b6664c3d8f3e
enabled: true
mode: image
peers:
  secretNames:
    - bootstrap-secret-e01e021da3ee4ed84ed95581cb7ae967
NAME         PHASE
rbd-mirror   Ready
NAME                                                TYPE     DATA   AGE
bootstrap-secret-e01e021da3ee4ed84ed95581cb7ae967   Opaque   2      99s

After deleting the StorageClusterpeer:

Cluster 1(primary):
[rewantsoni ] date; oc get clusterversion -oyaml | grep clusterID; oc get cephblockpool ocs-storagecluster-cephblockpool -oyaml | yq '.spec .mirroring'; oc get storageclusterpeer
Wed Mar  6 11:44:35 IST 2024
    clusterID: 4293c939-42b2-4565-b22f-81f53df19844
{}
No resources found in openshift-storage namespace.

Cluster 2(secondary):
[rewantsoni ] date; oc get clusterversion -oyaml | grep clusterID; oc get cephblockpool ocs-storagecluster-cephblockpool -oyaml | yq '.spec.mirroring'; oc get cephrbdmirror; oc get secret bootstrap-secret-e01e021da3ee4ed84ed95581cb7ae967
Wed Mar  6 11:45:27 IST 2024
    clusterID: a96546a7-5e86-4d6e-aded-b6664c3d8f3e
peers: {}
No resources found in openshift-storage namespace.
Error from server (NotFound): secrets "bootstrap-secret-e01e021da3ee4ed84ed95581cb7ae967" not found

rewantsoni · 2024-03-06T08:28:59Z

/test ocs-operator-bundle-e2e-aws

leelavg · 2024-03-06T12:06:22Z

config/samples/ocs_v1_storageclusterpeer.yaml

+  labels:
+  name: storageclusterpeer-sample
+spec:
+  ocsApiServerUri: 10.0.0.0/31659


intentionally mentioned / inplace of :?

No, updated it. thanks for pointing it out

leelavg · 2024-03-06T12:10:27Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

 }

 // SetupWithManager sets up the controller with the Manager.
 func (r *StorageClusterPeerReconciler) SetupWithManager(mgr ctrl.Manager) error {
+
+	enqueueStorageClusterPeerRequest := handler.EnqueueRequestsFromMapFunc(
+		func(context context.Context, obj client.Object) []reconcile.Request {


can use ctx inplace of context

leelavg · 2024-03-06T12:12:10Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+		func(context context.Context, obj client.Object) []reconcile.Request {
+			// Get the StorageClusterPeer objects
+			scpList := &ocsv1.StorageClusterPeerList{}
+			err := r.Client.List(context, scpList, &client.ListOptions{Namespace: obj.GetNamespace()})


so you'll have multiple scp in the same namespace? more like, if you have two blockpools and first has two peers, second has only one peer, should you enqueue all the peer CRs (all 3) or can enqueue peers corresponding to block pool only?

let me know if my understanding is incorrect.

A single storageClusterPeer will peer all the block pools created, if we have two storageClusterPeer, it will peer all the block pools present to both peer clusters

leelavg · 2024-03-06T12:17:56Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+
+		//remove finalizer
+		r.Log.Info("removing finalizer from the StorageClusterPeer resource")
+		r.storageClusterPeer.SetFinalizers(slices.Filter([]string{}, r.storageClusterPeer.GetFinalizers(), func(s string) bool {


where is this SetFinalizers defined? why can't we use controllerutil.SetFinalizer and conditionally update the resource?

SetFinalizers is a pre-defined function, it is defined as:

func (meta *ObjectMeta) SetFinalizers(finalizers []string) { meta.Finalizers = finalizers }

Updated to use helper function

leelavg · 2024-03-06T12:18:24Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+	slices.Contains(r.storageClusterPeer.GetFinalizers(), ocsv1.StorageClusterPeerFinalizer)
+	if !slices.Contains(r.storageClusterPeer.GetFinalizers(), ocsv1.StorageClusterPeerFinalizer) {


redundant op?

Yes, updated it

leelavg · 2024-03-06T12:18:44Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+	// add finalizer
+	slices.Contains(r.storageClusterPeer.GetFinalizers(), ocsv1.StorageClusterPeerFinalizer)
+	if !slices.Contains(r.storageClusterPeer.GetFinalizers(), ocsv1.StorageClusterPeerFinalizer) {
+		r.Log.V(-1).Info("finalizer missing on the StorageClusterPeer resource, adding...")
+		r.storageClusterPeer.SetFinalizers(append(r.storageClusterPeer.GetFinalizers(), ocsv1.StorageClusterPeerFinalizer))
+		if err := r.update(r.storageClusterPeer); err != nil {
+			return reconcile.Result{}, fmt.Errorf("failed to update StorageClusterPeer with finalizer: %v", err)
+		}
+	}


pls use helper from controllerutil.

leelavg · 2024-03-06T12:24:19Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+		_, err := ctrl.CreateOrUpdate(r.ctx, r.Client, &cephBlockPool, func() error {
+			cephBlockPool.Spec.Mirroring.Enabled = true
+			cephBlockPool.Spec.Mirroring.Mode = "image"
+			return nil
+		})


is there any reason in enabling mirroring daemon via gRPC but not locally?

The mirror daemon is required on the secondary Cluster not on primary cluster

leelavg · 2024-03-06T12:25:58Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+
+	r.storageClusterPeer.Status.Phase = ocsv1.StorageClusterPeerConfiguring
+
+	if err := r.reconcileCephBlockPool(); err != nil {


maybe reconcileOwnBlockPool sounds better as other one is being called reconcilePeerBlockPool 🤔?

how about reconcileLocalBlockPool?

leelavg · 2024-03-06T12:27:13Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+			return err
+		}
+
+		_, err := ocsClient.PeerBlockPool(r.ctx, generateSecretName(cephBlockPool.Name, clusterID), secret.Data["pool"], secret.Data["token"])


in existing rdr how is the secret name generated?

@vbnrh @umangachapagain could you help me in answering this question?

leelavg · 2024-03-06T12:28:49Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+
+func (r *StorageClusterPeerReconciler) reconcilePeerBlockPool(ocsClient *providerClient.OCSProviderClient) error {
+
+	clusterID := util.GetClusterID(r.ctx, r.Client, &r.Log)


if clusterID is required for each reconcile, why not store it in the struct itself as it's same throughout the lifetime of cluster?

on another note, I did come across this util func, seems a bit misleading, it'd have been better if it returns err on failure not sure why func dependents are ok with empty val.

Yes, even I am not sure why we are okay with returning empty val,
@iamniting @malayparida2000 could you elaborate on why we are returning empty val?

This func is called at one place earlier while constructing configmap. I am not able to recall why we were returning an empty value. As you are already checking if the value is empty and returning the reconcile that should be good.

leelavg · 2024-04-29T07:33:32Z

controllers/storageclusterpeer/storageclusterpeer_controller.go

+
+		//remove finalizer
+		r.Log.Info("removing finalizer from the StorageClusterPeer resource")
+		controllerutil.RemoveFinalizer(r.storageClusterPeer, ocsv1.StorageClusterPeerFinalizer)


incorrect I guess, you are removing finalizer from in-memory object but not on api server, need to update here.

Signed-off-by: Rewant Soni <resoni@redhat.com>

ran the command: operator-sdk create api --group ocs --version v1 --kind StorageClusterPeer --resource --controller Signed-off-by: Rewant Soni <resoni@redhat.com>

Signed-off-by: Rewant Soni <resoni@redhat.com>

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 20, 2024

rewantsoni force-pushed the storageclusterpeer branch from ba6d227 to 059074b Compare February 20, 2024 10:35

rewantsoni force-pushed the storageclusterpeer branch 5 times, most recently from d8b406f to fc34c02 Compare February 29, 2024 10:57

leelavg reviewed Mar 4, 2024

View reviewed changes

rewantsoni force-pushed the storageclusterpeer branch 2 times, most recently from d5cac87 to 0847bb6 Compare March 5, 2024 13:40

rewantsoni marked this pull request as ready for review March 6, 2024 06:17

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 Mar 6, 2024

leelavg reviewed Mar 6, 2024

View reviewed changes

rewantsoni force-pushed the storageclusterpeer branch 2 times, most recently from 5f71e7e to c08bb37 Compare March 6, 2024 14:02

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2024

rewantsoni force-pushed the storageclusterpeer branch from c08bb37 to c078f1c Compare March 18, 2024 11:16

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 18, 2024

rewantsoni force-pushed the storageclusterpeer branch from c078f1c to 4302e62 Compare March 26, 2024 05:57

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

rewantsoni force-pushed the storageclusterpeer branch from 4302e62 to 247aeb1 Compare March 26, 2024 06:09

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024

rewantsoni force-pushed the storageclusterpeer branch from 247aeb1 to ab281a3 Compare April 17, 2024 09:19

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 17, 2024

leelavg reviewed Apr 29, 2024

View reviewed changes

rewantsoni force-pushed the storageclusterpeer branch 2 times, most recently from 0393c64 to 283faaa Compare April 29, 2024 09:22

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 29, 2024

rewantsoni added 9 commits May 9, 2024 14:45

add rpc call for peerBlockPool

a6b7e55

Signed-off-by: Rewant Soni <resoni@redhat.com>

add client implementation for the peerblockpool rpc call

5634a8f

Signed-off-by: Rewant Soni <resoni@redhat.com>

add server implementation for peerblockpool rpc call

a5e4604

Signed-off-by: Rewant Soni <resoni@redhat.com>

add rpc call for revokeBlockPoolPeering

3d913dd

Signed-off-by: Rewant Soni <resoni@redhat.com>

add client implementation for revokeBlockPoolPeering rpc call

24fb2fb

Signed-off-by: Rewant Soni <resoni@redhat.com>

add server implementation for revokeBlockPoolPeering rpc call

321868a

Signed-off-by: Rewant Soni <resoni@redhat.com>

Add new CR and controller for storageClusterPeer

f20f598

ran the command: operator-sdk create api --group ocs --version v1 --kind StorageClusterPeer --resource --controller Signed-off-by: Rewant Soni <resoni@redhat.com>

add generated changes for storageClusterPeer

4d4f47e

Signed-off-by: Rewant Soni <resoni@redhat.com>

storageclusterpeer: setup the reconciler

e9b03f3

Signed-off-by: Rewant Soni <resoni@redhat.com>

rewantsoni force-pushed the storageclusterpeer branch from 283faaa to e9b03f3 Compare May 9, 2024 09:18

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2024

		// StorageClusterPeerReconciler reconciles a StorageClusterPeer object
		// nolint:revive


		func (r *StorageClusterPeerReconciler) reconcileCephBlockPool() error {

		for _, cephBlockPool := range r.cephBlockPoolList.Items {

		slices.Contains(r.storageClusterPeer.GetFinalizers(), ocsv1.StorageClusterPeerFinalizer)
		if !slices.Contains(r.storageClusterPeer.GetFinalizers(), ocsv1.StorageClusterPeerFinalizer) {


		r.storageClusterPeer.Status.Phase = ocsv1.StorageClusterPeerConfiguring

		if err := r.reconcileCephBlockPool(); err != nil {


		func (r StorageClusterPeerReconciler) reconcilePeerBlockPool(ocsClient providerClient.OCSProviderClient) error {

		clusterID := util.GetClusterID(r.ctx, r.Client, &r.Log)

Storageclusterpeer #2466

Are you sure you want to change the base?

Storageclusterpeer #2466

Conversation

rewantsoni commented Feb 20, 2024 • edited

openshift-ci bot commented Feb 20, 2024

openshift-ci bot commented Feb 20, 2024

rewantsoni commented Feb 29, 2024 • edited

jarrpa commented Mar 2, 2024

leelavg 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

rewantsoni commented Mar 6, 2024

rewantsoni commented Mar 6, 2024

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 Mar 6, 2024 • edited

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 Feb 20, 2024 •

edited

rewantsoni commented Feb 29, 2024 •

edited

rewantsoni Mar 6, 2024 •

edited