rbd: implement GetReplicationDestinationInfo for volumes and groups - #6366
Open
rewantsoni wants to merge 7 commits into
Open
rbd: implement GetReplicationDestinationInfo for volumes and groups#6366rewantsoni wants to merge 7 commits into
rewantsoni wants to merge 7 commits into
Conversation
6 tasks
rewantsoni
force-pushed
the
implement-replication
branch
5 times, most recently
from
July 1, 2026 08:02
cef70f8 to
6180c2c
Compare
rewantsoni
marked this pull request as ready for review
July 16, 2026 08:38
Contributor
Author
|
Testing: |
iPraveenParihar
requested changes
Jul 27, 2026
| } | ||
| srcVolumeID, err := vol.GetID(ctx) | ||
| if err != nil { | ||
| return "", status.Error(codes.Internal, err.Error()) |
Contributor
There was a problem hiding this comment.
This would double wrap grpc status errors. getDestinationVolumeID should return plain fmt.Errorf errors. Only the RPC handlers should apply status.Error.
| } | ||
|
|
||
| // Decompose the source volume ID to get pool ID | ||
| var sourceID util.CSIIdentifier |
Contributor
There was a problem hiding this comment.
This helper has code duplication with getDestinationIDFromCSIID.
Can we extract the params here and delegates to getDestinationIDFromCSIID?
| // Parse the remote pool ID from string to int64 | ||
| parsedPoolID, err := strconv.ParseInt(remotePoolDetails.PoolID, 10, 64) | ||
| if err != nil { | ||
| return "", err |
Contributor
There was a problem hiding this comment.
More info could be added?
Suggested change
| return "", err | |
| return "", fmt.Errorf("invalid poolID %q for pool %q in cluster %q: %w", remotePoolDetails.PoolID, poolName, clusterID, err) |
| req *replication.GetReplicationDestinationInfoRequest, | ||
| ) (*replication.GetReplicationDestinationInfoResponse, error) { | ||
| // Validate request | ||
| if req.GetReplicationSource() == nil { |
Contributor
There was a problem hiding this comment.
why not validate for empty secrets too?
| } | ||
| } | ||
|
|
||
| // getVolumeReplicationDestinationInfo handles volume replication destination info. |
Contributor
There was a problem hiding this comment.
Suggested change
| // getVolumeReplicationDestinationInfo handles volume replication destination info. | |
| // getVolumeReplicationDestinationInfo extracts the RBD volume information | |
| // from the volumeID and maps it to the destination cluster and pool using | |
| // the replicationDestination configuration from the CSI ConfigMap. |
| // Decompose the source volume ID to get pool ID | ||
| var sourceID util.CSIIdentifier | ||
| if err := sourceID.DecomposeCSIID(srcVolumeID); err != nil { | ||
| return "", fmt.Errorf("failed to decompose source volume ID: %w", err) |
Contributor
There was a problem hiding this comment.
Suggested change
| return "", fmt.Errorf("failed to decompose source volume ID: %w", err) | |
| return "", fmt.Errorf("failed to decompose source volume ID %q: %w", srcVolumeID, err) |
| return "", fmt.Errorf("failed to decompose source volume ID: %w", err) | ||
| } | ||
|
|
||
| remoteClusterID := destInfo.RemoteClusterID |
Contributor
There was a problem hiding this comment.
If its a required field, then probably more this before doing DecomposeCSIID?
|
|
||
| remoteClusterID := destInfo.RemoteClusterID | ||
| if remoteClusterID == "" { | ||
| return "", errors.New("remote cluster ID is empty in replication destination config") |
Contributor
There was a problem hiding this comment.
doesn't say which source cluster's config is broken
Suggested change
| return "", errors.New("remote cluster ID is empty in replication destination config") | |
| return "", fmt.Errorf("remoteClusterID is empty in replicationDestination for cluster %q", clusterID) |
|
|
||
| // If no destination configured, return the same ID | ||
| if destInfo == nil { | ||
| log.DebugLog(ctx, "no replication destination configured for cluster %s, assuming same ID", clusterID) |
Contributor
There was a problem hiding this comment.
may be WarningLog?
Suggested change
| log.DebugLog(ctx, "no replication destination configured for cluster %s, assuming same ID", clusterID) | |
| log.WarningLog(ctx, "replicationDestination not configured for cluster %s, returning source volume ID unchanged", clusterID) |
Contributor
Author
There was a problem hiding this comment.
I think we should keep it as debug, adding a ClientProfileReplication might not be required and it's not a warn message it's a choice of the user to create it
| return "", fmt.Errorf("failed to compose destination volume ID: %w", err) | ||
| } | ||
|
|
||
| log.UsefulLog(ctx, "mapped source volume %s to destination volume %s", srcVolumeID, destVolumeID) |
Contributor
There was a problem hiding this comment.
Suggested change
| log.UsefulLog(ctx, "mapped source volume %s to destination volume %s", srcVolumeID, destVolumeID) | |
| log.UsefulLog(ctx, "mapped source ID %s to destination ID %s", srcVolumeID, destVolumeID) |
rewantsoni
force-pushed
the
implement-replication
branch
4 times, most recently
from
July 28, 2026 11:21
4451914 to
2ffd5f3
Compare
rewantsoni
force-pushed
the
implement-replication
branch
from
July 28, 2026 11:29
2ffd5f3 to
79d9e7a
Compare
Member
|
Maybe rename this PR to something like this? rbd: implement GetReplicationDestinationInfo for volumes and groups |
rewantsoni
changed the title
rewantsoni
force-pushed
the
implement-replication
branch
from
July 28, 2026 16:57
79d9e7a to
486d005
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support in the RBD CSI-Addons replication service for the new GetReplicationDestinationInfo RPC, enabling DR orchestrators to map source volume/volume-group CSI IDs to their corresponding destination IDs across mirrored clusters (including optional pool ID remapping via ConfigMap).
Changes:
- Vendor-bump
github.com/csi-addons/specand regenerate protobuf/grpc stubs to include the new RPC + capability enum. - Implement
GetReplicationDestinationInfoin the RBD CSI-Addons replication server for both volumes and volume groups, includingreplicationDestinationConfigMap schema support. - Extend cluster mapping utilities/tests and add unit tests for destination-ID mapping and basic RPC request validation.
Reviewed changes
Copilot reviewed 10 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/csi-addons/rbd/replication.go | Implements the new RPC and ID mapping helper for volumes and volume groups. |
| internal/csi-addons/rbd/replication_destination_test.go | Adds unit tests for destination ID mapping logic and request validation. |
| internal/csi-addons/rbd/identity.go | Advertises the new CSI-Addons replication capability. |
| internal/util/csiconfig.go | Adds accessor for replication destination config from CSI config. |
| internal/util/cluster_mapping.go | Adds helper to resolve mapped cluster IDs (cluster-mapping.json). |
| internal/util/cluster_mapping_test.go | Adds unit tests for mapped cluster ID resolution. |
| api/deploy/kubernetes/csi-config-map.go | Extends CSI config schema with replication destination configuration. |
| vendor/github.com/ceph/ceph-csi/api/deploy/kubernetes/csi-config-map.go | Vendored copy of the CSI config schema update. |
| PendingReleaseNotes.md | Documents the new RPC and configuration behavior. |
| docs/design/proposals/replication-destination-info.md | Updates design proposal with capability advertisement details. |
| go.mod / go.sum | Updates dependency version for csi-addons spec. |
| vendor/modules.txt | Updates vendored module versions. |
| vendor/github.com/csi-addons/spec/lib/go/replication/replication.pb.go | Vendored protobuf changes for new messages/types. |
| vendor/github.com/csi-addons/spec/lib/go/replication/replication_grpc.pb.go | Vendored grpc service changes for new RPC. |
| vendor/github.com/csi-addons/spec/lib/go/identity/identity.pb.go | Vendored capability enum update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1246
to
+1250
| // Decompose the source ID | ||
| var sourceID util.CSIIdentifier | ||
| if err := sourceID.DecomposeCSIID(srcID); err != nil { | ||
| return "", fmt.Errorf("failed to decompose source volume ID %q: %w", srcID, err) | ||
| } |
Comment on lines
+1067
to
+1072
| rbdVol, err := mgr.GetVolumeByID(ctx, volumeID) | ||
| if err != nil { | ||
| log.ErrorLog(ctx, "failed to get volume with id %q: %v", volumeID, err) | ||
|
|
||
| return nil, getGRPCError(err) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
We shouldn't destroy the volume
Comment on lines
+1203
to
+1207
| destVolID, err := getDestinationIDFromCSIID(ctx, sourceVolID, localClusterID, poolName, util.CsiConfigFile) | ||
| if err != nil { | ||
| log.ErrorLog(ctx, "failed to get destination ID for volume %q: %v", sourceVolID, err) | ||
|
|
||
| return nil, status.Error(codes.Internal, err.Error()) |
| return "", fmt.Errorf("failed to compose destination ID: %w", err) | ||
| } | ||
|
|
||
| log.UsefulLog(ctx, "mapped source ID %s to destination ID %s", srcID, destCSIID) |
Contributor
Author
There was a problem hiding this comment.
I think it would be good to have the new mapped ID in UseFulLog so that we can backtrack in case of any errors
Comment on lines
+7
to
+17
| 1. Added `GetReplicationDestinationInfo` RPC to map source volume/volume | ||
| group IDs to destination IDs across mirrored clusters. This enables DR | ||
| orchestrators to discover the correct destination volume IDs when pools | ||
| have different IDs across clusters. The RPC supports: | ||
| - Volume replication: Maps source volume ID to destination volume ID | ||
| - Volume group replication: Maps source group ID and all member volume | ||
| IDs to their destination IDs | ||
| - Pool name-based mapping via `replicationDestination` ConfigMap | ||
| configuration | ||
| - Backward compatibility with existing cluster-mapping.json via | ||
| ClientProfileMapping integration |
rewantsoni
force-pushed
the
implement-replication
branch
2 times, most recently
from
July 30, 2026 04:45
738fdaa to
bd1bb11
Compare
Update the replication destination info proposal to document that the GET_REPLICATION_DESTINATION_INFO capability must be advertised via GetCapabilities RPC. This allows DR orchestrators to discover whether the driver supports the GetReplicationDestinationInfo feature. Signed-off-by: Rewant Soni <resoni@redhat.com>
Add ReplicationDestinationInfo, RemoteRBDDetails, and RemotePoolDetails structures to the ClusterInfo ConfigMap schema. This enables CSI driver to map source volume/group IDs to destination volume/group IDs when pool IDs differ across mirrored clusters. Signed-off-by: Rewant Soni <resoni@redhat.com>
Update csi-addons/spec from b5a7205f6a79 to d4a373713b9a to include GetReplicationDestinationInfo RPC definitions for volume and volume group replication. Signed-off-by: Rewant Soni <resoni@redhat.com>
rewantsoni
force-pushed
the
implement-replication
branch
from
July 30, 2026 04:47
bd1bb11 to
af236c2
Compare
Add GetReplicationDestinationInfo RPC implementation for both volume and volume group replication. This RPC maps source volume/group IDs to destination IDs by using the replicationDestination configuration from the ConfigMap. Signed-off-by: Rewant Soni <resoni@redhat.com>
Add GET_REPLICATION_DESTINATION_INFO to the list of advertised capabilities in GetCapabilities RPC. The capability is advertised on controller servers and applies to both volume and volume group replication, as both are handled by the same Replication Controller service. Signed-off-by: Rewant Soni <resoni@redhat.com>
Add unit tests for GetReplicationDestinationInfo RPC Signed-off-by: Rewant Soni <resoni@redhat.com>
Document the new GetReplicationDestinationInfo RPC feature in the pending release notes for v3.18. This RPC enables DR orchestrators to map source volume/volume group IDs to destination IDs across mirrored clusters when pools have different IDs, supporting both single volumes and volume groups with pool-based mapping configuration. Signed-off-by: Rewant Soni <resoni@redhat.com>
rewantsoni
force-pushed
the
implement-replication
branch
from
July 30, 2026 05:01
af236c2 to
26e6c47
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe what this PR does
Implement the change for the GetReplicationDestinationInfo RPC as per the design proposal in #6316
Is there anything that requires special attention
Provide any external context for the change, if any.
Checklist:
Show available bot commands
These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:
/retest ci/centos/<job-name>: retest the<job-name>after unrelatedfailure (please report the failure too!)