csi: only create CSI config configmap in CSI reconciler #14089

BlaineEXE · 2024-04-18T17:14:07Z

There have been some issues with non-CSI Rook controllers that are creating the CSI config configmap (rook-ceph-csi-config). This causes problems with the K8s OwnerReference. The primary CSI reconciler (controller) creates the configmap with the correct owner reference, which is supposed to be the operator deployment.

Other instances were creating the configmap with owner references set to the CephCluster. This is a minor bug, but it can result in this configmap being deleted along with the first CephCluster that initially created it.

To fix this issue, remove all instances of CreateCsiConfigMap() except the single usage which the CSI reconcile uses to initially create the configmap. Other controllers that might have attempted to create this configmap previously will return an error indicating that it is waiting for the configmap to be created.

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

BlaineEXE · 2024-04-18T17:21:55Z

pkg/operator/ceph/csi/controller.go

+	err = CreateCsiConfigMap(r.opManagerContext, r.opConfig.OperatorNamespace, r.context.Clientset, ownerInfo)
+	if err != nil {
+		return opcontroller.ImmediateRetryResult, errors.Wrap(err, "failed creating csi config map")
+	}


This is now the only usage of CreateCsiConfigMap(). It is called with owner info being the operator deployment, as it is supposed to be.

Because other instances were called with owner info being some CephCluster, they were all removed.

This code was moved up from its location below so that it would run before reconcileSaveCSIDriverOptions() is called for multus clusters.

BlaineEXE · 2024-04-18T17:22:40Z

pkg/operator/ceph/csi/cluster_config.go

-			if err != nil {
-				return errors.Wrap(err, "failed creating csi config map")
-			}
+			return errors.Wrap(err, "waiting for CSI config map to be created")


Any use of SaveClusterConfig() will now wait until the main CSI reconcile routine creates the configmap here: https://github.com/rook/rook/pull/14089/files#r1571122479

travisn · 2024-04-18T17:52:45Z

pkg/operator/ceph/cluster/cluster_external.go

@@ -108,12 +108,6 @@ func (c *ClusterController) configureExternalCephCluster(cluster *cluster) error
 		}
 	}

-	// Create CSI config map
-	err = csi.CreateCsiConfigMap(c.OpManagerCtx, c.namespacedName.Namespace, c.context.Clientset, cluster.ownerInfo)


Will the external cluster reconcile be failed at some point if the configmap doesn't exist?

I looked into this, and I don't believe so.

pkg/operator/ceph/csi/controller.go line 192 "gates" creation of the csi config map. The only metric that affects creation is whether or not any CephClusters exist. It doesn't matter if the clusters are internal or external. By my logic, if an external cluster exists, the CSI controller will create the configmap (unless there is a create error), and so it is safe for the external reconcile to wait until that happens.

It might be good for you to double check my logic just to be sure.

I see now that SaveClusterConfig() will be called below on line 133, but given your changes in that method, it will fail the reconcile and retry. Makes sense to me.

There have been some issues with non-CSI Rook controllers that are creating the CSI config configmap (`rook-ceph-csi-config`). This causes problems with the K8s OwnerReference. The primary CSI reconciler (controller) creates the configmap with the correct owner reference, which is supposed to be the operator deployment. Other instances were creating the configmap with owner references set to the CephCluster. This is a minor bug, but it can result in this configmap being deleted along with the first CephCluster that initially created it. To fix this issue, remove all instances of `CreateCsiConfigMap()` except the single usage which the CSI reconcile uses to initially create the configmap. Other controllers that might have attempted to create this configmap previously will return an error indicating that it is waiting for the configmap to be created. Signed-off-by: Blaine Gardner <blaine.gardner@ibm.com>

In the primary CSI reconcile, ensure the CSI config map (`rook-ceph-csi-config`) has the correct owner info. This corrects any pre-existing config maps that might have incorrect owner info, which has observed to include references to CephClusters. The config map should only have a single reference, and it should refer to the operator deployment. If any existing Rook clusters have a CSI config map which has a CephCluster as an owner, this change will ensure that the config map is not deleted when the CephCluster is deleted. This is especially important for any environments with multiple CephClusters installed. Signed-off-by: Blaine Gardner <blaine.gardner@ibm.com>

travisn · 2024-04-18T19:41:09Z

pkg/operator/ceph/cluster/cluster_external.go

@@ -108,12 +108,6 @@ func (c *ClusterController) configureExternalCephCluster(cluster *cluster) error
 		}
 	}

-	// Create CSI config map
-	err = csi.CreateCsiConfigMap(c.OpManagerCtx, c.namespacedName.Namespace, c.context.Clientset, cluster.ownerInfo)


I see now that SaveClusterConfig() will be called below on line 133, but given your changes in that method, it will fail the reconcile and retry. Makes sense to me.

travisn · 2024-04-18T19:48:28Z

pkg/operator/ceph/csi/cluster_config.go

 	}

 	logger.Infof("successfully created csi config map %q", configMap.Name)
 	return nil
 }

+// check the owner references on the csi config map, and fix incorrect references if needed
+func updateCsiConfigMapOwnerRefs(ctx context.Context, namespace string, clientset kubernetes.Interface, expectedOwnerInfo *k8sutil.OwnerInfo) error {


SaveClusterConfig() already has a code path that updates the configmap. Would that code path naturally update the ownerref at the same time also ensuring the configmap contents are updated? Or perhaps there is a small update to that code path? Then we don't need this new update method.

That's a fair question. SaveClusterConfig() doesn't do anything with the owner ref during update -- either before or after these changes. It could be modified to update the owner ref fairly easily, but we should still add a couple unit tests to be certain that it was updating things.

Using SaveClusterConfig() would require less overall Rook code, but I think it would result in overall more k8s API calls. But neither pro/con is a dramatic change compared to the alternative.

After looking into it, it seems to me like it's slightly preferable to keep this PR as-is just to avoid the developer time needed to rework it.

Agreed, let's keep it as is.

travisn · 2024-04-18T20:10:18Z

pkg/operator/ceph/csi/cluster_config.go

 	}

 	logger.Infof("successfully created csi config map %q", configMap.Name)
 	return nil
 }

+// check the owner references on the csi config map, and fix incorrect references if needed
+func updateCsiConfigMapOwnerRefs(ctx context.Context, namespace string, clientset kubernetes.Interface, expectedOwnerInfo *k8sutil.OwnerInfo) error {


Agreed, let's keep it as is.

csi: only create CSI config configmap in CSI reconciler (backport #14089)

leelavg · 2024-04-23T06:51:52Z

@Madhu-1 as per @parth-gr this PR might have introduced a regression in provider mode where CSI is deployed by client-op and probably doesn't deploy this CM which rook expects to exist. Could you pls check and suggest next steps? As the intention of this PR seems to be CSI should create CM then ocs-client should do it?

Madhu-1 · 2024-04-23T07:11:00Z

rook/pkg/operator/ceph/csi/controller.go

Lines 203 to 218 in 4b9ada6

    
           ownerRef, err := k8sutil.GetDeploymentOwnerReference(r.opManagerContext, r.context.Clientset, os.Getenv(k8sutil.PodNameEnvVar), r.opConfig.OperatorNamespace) 
        
           if err != nil { 
        
           	logger.Warningf("could not find deployment owner reference to assign to csi drivers. %v", err) 
        
           } 
        
           if ownerRef != nil { 
        
           	blockOwnerDeletion := false 
        
           	ownerRef.BlockOwnerDeletion = &blockOwnerDeletion 
        
           } 
        
           ownerInfo := k8sutil.NewOwnerInfoWithOwnerRef(ownerRef, r.opConfig.OperatorNamespace) 
        
           // create an empty config map. config map will be filled with data 
        
           // later when clusters have mons 
        
           err = CreateCsiConfigMap(r.opManagerContext, r.opConfig.OperatorNamespace, r.context.Clientset, ownerInfo) 
        
           if err != nil { 
        
           	return opcontroller.ImmediateRetryResult, errors.Wrap(err, "failed creating csi config map") 
        
           }

need to be moved before

rook/pkg/operator/ceph/csi/controller.go

Lines 160 to 166 in 4b9ada6

    
           disableCSI, err := strconv.ParseBool(k8sutil.GetValue(r.opConfig.Parameters, "ROOK_CSI_DISABLE_DRIVER", "false")) 
        
           if err != nil { 
        
           	return reconcile.Result{}, errors.Wrap(err, "unable to parse value for 'ROOK_CSI_DISABLE_DRIVER") 
        
           } else if disableCSI { 
        
           	logger.Info("ceph csi driver is disabled") 
        
           	return reconcile.Result{}, nil 
        
           }

so that configmap gets created and other controllers will not get into the problem. @BlaineEXE @travisn we have missed a case where csi driver is disabled and configmap never gets created.

parth-gr · 2024-04-23T07:11:07Z

@Madhu-1 as per @parth-gr this PR might have introduced a regression in provider mode where CSI is deployed by client-op and probably doesn't deploy this CM which rook expects to exist. Could you pls check and suggest next steps? As the intention of this PR seems to be CSI should create CM then ocs-client should do it?

The workaround currently is to create the csi-configmap with the mon-endpoint JSON data,
And later rook will update the csi-configmap with mon endpoints

rook/pkg/operator/ceph/cluster/mon/mon.go

Line 1115 in 4b9ada6

    
           if err := csi.SaveClusterConfig(c.context.Clientset, c.Namespace, c.ClusterInfo, csiConfigEntry); err != nil {

To propose the fix, we can think of making the two config maps independent of each other,
This can also solve this problem #12595 (comment).

And for the client operator to work well when CSI is not deployed we can make sure rook always create an empty config map with the desired structure for a quick fix.

Let's discuss this in today's huddle.

parth-gr · 2024-04-23T07:24:12Z

@Madhu-1 can we store the values to mon endpoint configmap, instead of csi configmap first

rook/pkg/operator/ceph/cluster/mon/mon.go

Line 1115 in 4b9ada6

    
           if err := csi.SaveClusterConfig(c.context.Clientset, c.Namespace, c.ClusterInfo, csiConfigEntry); err != nil {

And later csi configmap can fetch these details if needed.

This will make csi configmap more independent as well as it will only store values of the specific cluster tenants.

And mon endpoint configmap will be the source of truth.

PS: And this can be also a proposal for updating the csi-configmap only through csi-controller,
Every controller should have their specific configmap and csi controller should watch for them.
#13978

travisn · 2024-04-23T21:22:26Z

Created #14123 to continue the discussion...

BlaineEXE added the backport-release-1.14 label Apr 18, 2024

BlaineEXE requested review from travisn and Madhu-1 April 18, 2024 17:14

BlaineEXE force-pushed the csi-config-map-limit-creators branch from 8010a51 to 3f1eeda Compare April 18, 2024 17:15

BlaineEXE marked this pull request as ready for review April 18, 2024 17:15

BlaineEXE force-pushed the csi-config-map-limit-creators branch from 3f1eeda to 03ddb7e Compare April 18, 2024 17:18

BlaineEXE commented Apr 18, 2024

View reviewed changes

BlaineEXE marked this pull request as draft April 18, 2024 17:40

travisn reviewed Apr 18, 2024

View reviewed changes

BlaineEXE added 2 commits April 18, 2024 12:22

BlaineEXE force-pushed the csi-config-map-limit-creators branch from 03ddb7e to 605b963 Compare April 18, 2024 18:54

BlaineEXE marked this pull request as ready for review April 18, 2024 18:59

travisn reviewed Apr 18, 2024

View reviewed changes

travisn approved these changes Apr 18, 2024

View reviewed changes

BlaineEXE merged commit 54f219d into rook:master Apr 18, 2024
52 of 53 checks passed

BlaineEXE deleted the csi-config-map-limit-creators branch April 18, 2024 20:17

mergify bot mentioned this pull request Apr 18, 2024

csi: only create CSI config configmap in CSI reconciler (backport #14089) #14090

Merged

6 tasks

travisn added a commit that referenced this pull request Apr 18, 2024

Merge pull request #14090 from rook/mergify/bp/release-1.14/pr-14089

297ba70

csi: only create CSI config configmap in CSI reconciler (backport #14089)

parth-gr mentioned this pull request Apr 19, 2024

csi configmap should be updated by single controllers #13978

Open

travisn mentioned this pull request Apr 23, 2024

Creating a cluster never succeeds if CSI driver is disabled #14123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

csi: only create CSI config configmap in CSI reconciler #14089

csi: only create CSI config configmap in CSI reconciler #14089

BlaineEXE commented Apr 18, 2024

BlaineEXE Apr 18, 2024 •

edited

BlaineEXE Apr 18, 2024

travisn Apr 18, 2024

BlaineEXE Apr 18, 2024

travisn Apr 18, 2024

travisn Apr 18, 2024

travisn Apr 18, 2024

BlaineEXE Apr 18, 2024

travisn Apr 18, 2024

travisn Apr 18, 2024

leelavg commented Apr 23, 2024

Madhu-1 commented Apr 23, 2024

parth-gr commented Apr 23, 2024 •

edited

parth-gr commented Apr 23, 2024 •

edited

travisn commented Apr 23, 2024

csi: only create CSI config configmap in CSI reconciler #14089

csi: only create CSI config configmap in CSI reconciler #14089

Conversation

BlaineEXE commented Apr 18, 2024

BlaineEXE Apr 18, 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

leelavg commented Apr 23, 2024

Madhu-1 commented Apr 23, 2024

parth-gr commented Apr 23, 2024 • edited

parth-gr commented Apr 23, 2024 • edited

travisn commented Apr 23, 2024

BlaineEXE Apr 18, 2024 •

edited

parth-gr commented Apr 23, 2024 •

edited

parth-gr commented Apr 23, 2024 •

edited