Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add rpc call for RevokeBlockPoolPeering #2493

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rewantsoni
Copy link
Member

@rewantsoni rewantsoni commented Mar 5, 2024

Add a new RPC call RevokeBlockPoolPeering to disable one-way mirroring between the block pools.

The rpc call is initiated from the primary cluster which wants to disable mirroring of the pool which was already mirrored, the server receives the request and does the following:

  1. Remove the bootstrap secret ref on the BlockPool and delete the bootstrap secret.
  2. Disable Mirroring on the Block Pool in the request if no bootstrap secret ref is set.
  3. Disable RBD Mirror Daemon if no bootstrap secret is set for any of the block pools in the cluster.

Depends On:

  1. cephblockpool: allow updation of mirroring field from different components #2480
  2. add rpc call for PeerBlockPool #2457

@openshift-ci 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 Mar 5, 2024
Copy link
Contributor

openshift-ci bot commented Mar 5, 2024

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

Copy link
Contributor

openshift-ci bot commented Mar 5, 2024

[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 umangachapagain 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:

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

@rewantsoni
Copy link
Member Author

Testing:
Before

[rewantsoni ] grpcurl -insecure -d '{"secretName":"ocs-storagecluster-cephblockpool-a96546a7-5e86-4d6e-ad","pool":"......=","token":"........"}' localhost:8881 provider.OCSProvider/PeerBlockPool

[rewantsoni ] date; oc get cephblockpool -oyaml; oc get cephrbdmirror
Tue Mar  5 17:02:34 IST 2024
apiVersion: v1
items:
- apiVersion: ceph.rook.io/v1
  kind: CephBlockPool
  metadata:
    name: ocs-storagecluster-cephblockpool
    namespace: openshift-storage
  spec:
    enableRBDStats: true
    erasureCoded:
      codingChunks: 0
      dataChunks: 0
    failureDomain: zone
    mirroring:
      enabled: true
      mode: image
      peers:
        secretNames:
        - ocs-storagecluster-cephblockpool-a96546a7-5e86-4d6e-ad
    quotas: {}
    replicated:
      replicasPerFailureDomain: 1
      size: 3
      targetSizeRatio: 0.49
    statusCheck:
      mirror: {}
  status:
    info:
      rbdMirrorBootstrapPeerSecretName: pool-peer-token-ocs-storagecluster-cephblockpool
    mirroringInfo:
      lastChecked: "2024-03-05T11:32:01Z"
      mode: image
      site_name: 4a8a20b2-760c-4621-820f-2ca8058ee2b7
    mirroringStatus:
      lastChecked: "2024-03-05T11:32:01Z"
      summary:
        daemon_health: ERROR
        health: ERROR
        image_health: OK
        states: {}
    observedGeneration: 12
    phase: Ready
    snapshotScheduleStatus: {}
kind: List
metadata:
  resourceVersion: ""
NAME         PHASE
rbd-mirror   Ready

Request:

[rewantsoni ] grpcurl -insecure -d '{"secretName":"ocs-storagecluster-cephblockpool-a96546a7-5e86-4d6e-ad","pool":"b2NzLXN0b3JhZ2VjbHVzdGVyLWNlcGhibG9ja3Bvb2w="}' localhost:8881 provider.OCSProvider/RevokeBlockPoolPeering
{}

After:

[rewantsoni ] date; oc get cephblockpool -oyaml; oc get cephrbdmirror
Tue Mar  5 17:05:59 IST 2024
apiVersion: v1
items:
- apiVersion: ceph.rook.io/v1
  kind: CephBlockPool
  metadata:
    name: ocs-storagecluster-cephblockpool
    namespace: openshift-storage
  spec:
    enableRBDStats: true
    erasureCoded:
      codingChunks: 0
      dataChunks: 0
    failureDomain: zone
    mirroring:
      peers: {}
    quotas: {}
    replicated:
      replicasPerFailureDomain: 1
      size: 3
      targetSizeRatio: 0.49
    statusCheck:
      mirror: {}
  status:
    info:
      rbdMirrorBootstrapPeerSecretName: pool-peer-token-ocs-storagecluster-cephblockpool
    mirroringInfo:
      lastChanged: "2024-03-05T11:34:02Z"
      lastChecked: "2024-03-05T11:35:02Z"
      mode: image
      site_name: 4a8a20b2-760c-4621-820f-2ca8058ee2b7
    mirroringStatus:
      lastChecked: "2024-03-05T11:35:02Z"
      summary:
        daemon_health: WARNING
        health: WARNING
        image_health: OK
        states: {}
    observedGeneration: 12
    phase: Ready
    snapshotScheduleStatus: {}
kind: List
metadata:
  resourceVersion: ""
No resources found in openshift-storage namespace.

@rewantsoni rewantsoni marked this pull request as ready for review March 6, 2024 06:17
@openshift-ci 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
@rewantsoni
Copy link
Member Author

/test ocs-operator-bundle-e2e-aws

Comment on lines 58 to 59
err := c.client.Delete(ctx, cephRBDMirrorObj)
if err == nil {
return err
}
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just club everything, return c.client.Delete(ctx, cephRBDMirrorObj)

Comment on lines 163 to 166
if cephBlockPool.Spec.Mirroring.Peers != nil && len(cephBlockPool.Spec.Mirroring.Peers.SecretNames) > 0 {
if len(cephBlockPool.Spec.Mirroring.Peers.SecretNames) > 0 {
return true, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there seems to be a redundant check here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, removed it


func (c *cephBlockPoolManager) DisableBlockPoolMirroring(ctx context.Context, blockPoolName string, cephBlockPool *rookCephv1.CephBlockPool) error {

_, err := ctrl.CreateOrUpdate(ctx, c.client, cephBlockPool, func() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rewantsoni could you pls bring this topic in today's team meeting for technical discussion, basically 1. should we use createorupdate on the resources that we didn't create, 2. should be use declarative style while servicing gRPC?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per discussion offline, updated to use Update rather than createOrUpdate for the resources we are not creating

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 14, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 20, 2024
err = s.cephBlockPoolManager.UnSetBootstrapSecretRef(ctx, req.SecretName, cephBlockPool)
// there might be a case where the bootstrap secret was deleted but request failed after this and there was a retry,
// if error is IsNotFound, that means it is safe to proceed as we have deleted the bootstrap secret
if err != nil && !kerrors.IsNotFound(err) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better handle the notfound on delete inside the func itself (same for the rest). Current func is doing two things, maybe state the same in the func name?

return nil, status.Errorf(codes.Internal, "Failed to get if rbd mirror is required: %v,", err)
}

if !isRBDMirrorRequired {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mirroring requires rbd-mirror and so you are deleting rbd-mirror if none of the cbp requires it, why not make the peering also consistent w/ this approach, like, client asks for peering but not explicitly enable mirroring as the latter is a hard requirement for the former?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand what you meant. For one-way mirroring to be set up, we need the following:

  1. Side A (primary) should enable mirroring on their blockpool to have the bootstrap secret generated which will be used by Side B (secondary)
  2. Side B enables the mirroring on their blockpool and set the bootstrap secret ref from side A
  3. Side B deploys the rbd mirror

Signed-off-by: Rewant Soni <resoni@redhat.com>
Signed-off-by: Rewant Soni <resoni@redhat.com>
Signed-off-by: Rewant Soni <resoni@redhat.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2024
Signed-off-by: Rewant Soni <resoni@redhat.com>
Signed-off-by: Rewant Soni <resoni@redhat.com>
Signed-off-by: Rewant Soni <resoni@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants