Read affinity args are not added to (CephFS/RBD) NodePlugin

[iPraveenParihar]

Describe the bug

I see a way to enable read affinity and pass crush location labels in CephConnection CR which I did

$ k get cephconnections.csi.ceph.io ceph-cluster -ojsonpath='{.spec}' | jq
{
  "monitors": [
    "10.98.80.66:6789"
  ],
  "readAffinity": {
    "crushLocationLabels": [
      "kubernetes.io/hostname",
      "topology.kubernetes.io/region",
      "topology.kubernetes.io/zone",
      "topology.rook.io/chassis",
      "topology.rook.io/rack",
      "topology.rook.io/row",
      "topology.rook.io/pdu",
      "topology.rook.io/pod",
      "topology.rook.io/room",
      "topology.rook.io/datacenter"
    ]
  }
}

But still the read affinity args not added to (cephfs/rbd) nodeplugin.
Looks like, we missed adding those?

ceph-csi-operator/internal/controller/driver_controller.go

Lines 996 to 1042 in 02aac5e

	AllowPrivilegeEscalation: ptr.To(true),
	},
	Args: utils.DeleteZeroValues(
	[]string{
	utils.LogVerbosityContainerArg(logVerbosity),
	utils.TypeContainerArg(string(r.driverType)),
	utils.NodeServerContainerArg,
	utils.NodeIdContainerArg,
	utils.DriverNameContainerArg(r.driver.Name),
	utils.EndpointContainerArg,
	utils.PidlimitContainerArg,
	utils.If(forceKernelClient, utils.ForceCephKernelClientContainerArg, ""),
	utils.If(
	ptr.Deref(r.driver.Spec.DeployCsiAddons, false),
	utils.CsiAddonsEndpointContainerArg,
	"",
	),
	utils.If(
	r.isRdbDriver(),
	utils.StagingPathContainerArg(kubeletDirPath),
	"",
	),
	utils.If(
	r.isCephFsDriver(),
	utils.KernelMountOptionsContainerArg(r.driver.Spec.KernelMountOptions),
	"",
	),
	utils.If(
	r.isCephFsDriver(),
	utils.FuseMountOptionsContainerArg(r.driver.Spec.FuseMountOptions),
	"",
	),
	utils.If(
	topology,
	utils.DomainLabelsContainerArg(domainLabels),
	"",
	),
	utils.If(logRotationEnabled, utils.LogToStdErrContainerArg, ""),
	utils.If(logRotationEnabled, utils.AlsoLogToStdErrContainerArg, ""),
	utils.If(
	logRotationEnabled,
	utils.LogFileContainerArg(fmt.Sprintf("csi-%splugin", r.driverType)),
	"",
	),
	},
	),
	Env: []corev1.EnvVar{

ceph-csi reference - https://github.com/ceph/ceph-csi/blob/8ddb615df28f5b9dabfb92fad12565206813a04c/deploy/rbd/kubernetes/csi-rbdplugin.yaml#L50-L57

[Madhu-1]

Yes it looks like, please feel free to assign it if you are planning to work on it.

[iPraveenParihar]

Thought about it 🤔, I checked the ceph-csi implementation of readAffinity and the nodePlugin arg --crush-location-labels (--enable-read-affinity=true, defaults false) are consider only when the readAffinity: true and crushLocationLabels is empty in ceph-csi-config ConfigMap.

initially, the readAffinity was provided from nodeplugin args but later we moved it to ceph-csi-config CM. But nodeplugin args stayed for backward compatibility. I think in operator it actually doesn't require to add the readAffinity args in nodeplugin. Users moving to operator deployment should take care of setting readAffinity as required.

@Madhu-1, do you have different thoughts on this?

[Madhu-1]

Thought about it 🤔, I checked the ceph-csi implementation of readAffinity and the nodePlugin arg --crush-location-labels (--enable-read-affinity=true, defaults false) are consider only when the readAffinity: true and crushLocationLabels is empty in ceph-csi-config ConfigMap.

initially, the readAffinity was provided from nodeplugin args but later we moved it to ceph-csi-config CM. But nodeplugin args stayed for backward compatibility. I think in operator it actually doesn't require to add the readAffinity args in nodeplugin. Users moving to operator deployment should take care of setting readAffinity as required.

@Madhu-1, do you have different thoughts on this?

Now i remember that was the reason we considered not to add it to the NodePlugin but rather kept it at configmap, if the feature is not working as expected we can add it to the Cli argument, please test out the functionality and see if its working as expected or not.

[github-actions]

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