Adding multi-cluster support via array of cluster configs based on topology

[dridge93]

Adding multi-cluster support via array of cluster configs based on topology

Describe what this PR does

Adds multi-cluster support to Ceph CSI RBD (#5177)

Related issues

Fixes: #5177

---

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 unrelated
  failure (please report the failure too!)

[dridge93]

Has anyone had a chance to review this PR?

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

[mergify]

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

[akaitux]

@dridge93

I found what looks like a functional bug in the original multi-AZ / multi-cluster topology patch
(933bbff4a9a4747dc593ff7df577dfed95a4884c).

The new helper already returns topology requirements for the clusterTopologyConfigMap path:

func GetClusterTopologiesFromRequest(
    req *csi.CreateVolumeRequest,
) (*[]ClusterTopology, *csi.TopologyRequirement, error)

and inside it:

accessibilityRequirements := req.GetAccessibilityRequirements()
...
return &clusterTopologies, accessibilityRequirements, nil

But in parseVolCreateRequest() that return value is discarded:

rbdVol.TopologyPools, rbdVol.TopologyRequirement, err = util.GetTopologyFromRequest(req)
...
rbdVol.ClusterTopologies, _, err = util.GetClusterTopologiesFromRequest(req)

So when the StorageClass uses only:

parameters:
  clusterTopologyConfigMap: ceph-csi-rbd-cluster-topology

and does not use topologyConstrainedPools, then:

  - rbdVol.ClusterTopologies is populated
  - rbdVol.TopologyRequirement stays nil

Later both CreateVolume() and updateTopologyConstraints() do:

selectedCluster, _, err = util.FindClusterAndTopology(
    rbdVol.ClusterTopologies,
    rbdVol.TopologyRequirement,
)

or:

cluster, topology, err := util.FindClusterAndTopology(
    rbdVol.ClusterTopologies,
    rbdVol.TopologyRequirement,
)

but FindClusterAndTopology() starts with:

if clusterTopologies == nil || accessibilityRequirements == nil {
    return ClusterTopology{}, nil, nil
}

So the cluster selection silently returns empty, and CreateVolume fails with:

no matching cluster found for provided topology requirements

even though the incoming request already contains valid topology like:

"accessibility_requirements": {
  "preferred": [
    {"segments":{"topology.rbd.csi.ceph.com/zone":"az01"}},
    {"segments":{"topology.rbd.csi.ceph.com/zone":"az02"}}
  ]
}

and the live clusterTopologyConfigMap contains matching zones:

{
  "clusterTopology": [
    {
      "clusterID": "...",
      "zones": ["az01"],
      ...
    },
    {
      "clusterID": "...",
      "zones": ["az02"],
      ...
    }
  ]
}

The minimal fix seems to be to preserve the topology requirement returned by GetClusterTopologiesFromRequest() when
rbdVol.TopologyRequirement is still nil.

diff --git a/internal/rbd/controllerserver.go b/internal/rbd/controllerserver.go
index 6ef562373..dd4a3a148 100644
--- a/internal/rbd/controllerserver.go
+++ b/internal/rbd/controllerserver.go
@@ -237,10 +237,14 @@ func (cs *ControllerServer) parseVolCreateRequest(
 	}

 	// store cluster topology information from the request if present
-	rbdVol.ClusterTopologies, _, err = util.GetClusterTopologiesFromRequest(req)
+	var clusterTopologyRequirement *csi.TopologyRequirement
+	rbdVol.ClusterTopologies, clusterTopologyRequirement, err = util.GetClusterTopologiesFromRequest(req)
 	if err != nil {
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
+	if rbdVol.TopologyRequirement == nil {
+		rbdVol.TopologyRequirement = clusterTopologyRequirement
+	}

 	// parse QOS parameters from mutable parameters
 	err = rbdVol.SetQOS(ctx, req.GetMutableParameters())

[akaitux]

@dridge93
Second, there is another gap in the original multi-cluster topology flow: the selected cluster secret is only used on the controller/
provisioning path, but not on the node-stage path.

In CreateVolume(), when req.GetSecrets() is empty, the implementation already falls back to the cluster-specific secret
selected from clusterTopologyConfigMap:

selectedCluster, _, err = util.FindClusterAndTopology(...)
...
rbdVol.ClusterSecretName = selectedCluster.SecretName
...
if len(secrets) == 0 && selectedCluster.SecretName != "" {
    secrets, err = k8s.GetSecret(selectedCluster.SecretName, namespace)
}

and then clusterSecretName is persisted into VolumeContext:

  if rbdVol.ClusterSecretName != "" {
      vol.VolumeContext["clusterSecretName"] = rbdVol.ClusterSecretName
  }

So the controller side already knows which secret belongs to the selected cluster.

However, on the node path, NodeStageVolume() still requires standard CSI node-stage secrets to be present in the request:

  if req.GetSecrets() == nil || len(req.GetSecrets()) == 0 {
      return status.Error(codes.InvalidArgument, "stage secrets cannot be nil or empty")
  }

and then immediately does:

  cr, err := util.NewUserCredentialsWithMigration(req.GetSecrets())

This means that a StorageClass using only:

  parameters:
    clusterTopologyConfigMap: ceph-csi-rbd-cluster-topology

still fails at node stage unless csi.storage.k8s.io/node-stage-secret-* is also set statically.

That is problematic for the multi-cluster mode, because:

• cluster selection is dynamic (az01 / az02 / az03)
• but node-stage-secret-* in the StorageClass is static
• so a single StorageClass cannot correctly represent per-cluster credentials unless the same credentials are valid
  everywhere

The minimal fix is:

1. relax ValidateNodeStageVolumeRequest() so empty req.Secrets is allowed when req.VolumeContext["clusterSecretName"] is
   present
2. in NodeStageVolume(), if req.GetSecrets() is empty, load the secret from clusterSecretName in the driver namespace and use
   it to build credentials

Example fix:

diff --git a/internal/util/validate.go b/internal/util/validate.go
@@
- if req.GetSecrets() == nil || len(req.GetSecrets()) == 0 {
-     return status.Error(codes.InvalidArgument, "stage secrets cannot be nil or empty")
- }
+ if req.GetSecrets() == nil || len(req.GetSecrets()) == 0 {
+     if req.GetVolumeContext()["clusterSecretName"] == "" {
+         return status.Error(codes.InvalidArgument, "stage secrets cannot be nil or empty")
+     }
+ }

diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go
@@
- cr, err := util.NewUserCredentialsWithMigration(req.GetSecrets())
+ secrets := req.GetSecrets()
+ if len(secrets) == 0 {
+     clusterSecretName := req.GetVolumeContext()["clusterSecretName"]
+     if clusterSecretName != "" {
+         namespace, nsErr := util.GetPodNamespace()
+         if nsErr != nil {
+             return nil, status.Error(codes.InvalidArgument, nsErr.Error())
+         }
+         secrets, err = k8s.GetSecret(clusterSecretName, namespace)
+         if err != nil {
+             return nil, status.Error(codes.InvalidArgument, err.Error())
+         }
+     }
+ }
+
+ cr, err := util.NewUserCredentialsWithMigration(secrets)

Without this, the multi-cluster topology flow is only partially implemented:

• controller/provisioning can use per-cluster secrets
• node-stage still requires a static StorageClass secret

[dridge93]

@akaitux I believe I have addressed the feedback you provided. Would you mind verifying if you have the time?

[akaitux]

@dridge93 Unfortunately, it still requires a lot of fixes. The snapshot schema is not fully implemented yet, nor is it for CephFS. Even for RBD, additional patches are needed for resize support.

I’m working on an internal fork based on your changes, but I’m not ready to share it yet.

[dridge93]

@Madhu-1 have you had a chance to review this PR?

[dridge93]

@Madhu-1 can you at least tell me what's preventing the Ceph CSI team from even looking at this? Is there something I can change or fix? I intentionally avoided making more changes than what just would enable multi-clustering to keep changes low. It'd be nice to know if we need to explore other options and not hang on whether or not Ceph CSI will support this feature.

[Madhu-1]

@dridge93 There is a design doc for same feature #5986? usually for new feature it goes via the design doc first rather than a PR with implementation.

[dridge93]

@dridge93 There is a design doc for same feature #5986? usually for new feature it goes via the design doc first rather than a PR with implementation.

That has also gone stale. I'm wanting to know if this is something that is even being considered or if we should be forking and maintaining our own CSI.

[Madhu-1]

@dridge93 There is a design doc for same feature #5986? usually for new feature it goes via the design doc first rather than a PR with implementation.

That has also gone stale. I'm wanting to know if this is something that is even being considered or if we should be forking and maintaining our own CSI.

I will bring this in the csi team meeting. @nixpanic @Rakshith-R JFYI

[Rakshith-R]

@dridge93 There is a design doc for same feature #5986? usually for new feature it goes via the design doc first rather than a PR with implementation.

That has also gone stale. I'm wanting to know if this is something that is even being considered or if we should be forking and maintaining our own CSI.

I will bring this in the csi team meeting. @nixpanic @Rakshith-R JFYI

Yes, design doc is a pre-requisite for such a large change.

If you think that design doc pr has stale
Please feel free to open design doc pr.
(Please consider the secrets part too #5986 (comment))

[akaitux]

I don’t think this will help you much, except perhaps as a PoC, but I’ve published my fork, which is currently running in production and includes fixes for the issues I mentioned above.

I’m a DevOps engineer and built it with the help of AI, but nevertheless, the current code is running in production.

Only RBD functionality has been tested. The ConfigMap configuration syntax has not changed.

https://github.com/akaitux/ceph-csi-multiaz/tree/v3.16.2-multiaz

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

[mergify]

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

[github-actions]

This pull request has been automatically closed due to inactivity. Please re-open if these changes are still required.