csi: update network fence CR name #13615

riya-singhal31 · 2024-01-24T10:50:39Z

this commit updates the name of network fence CR to node name + driver name, which will allow the creation of multiple network fence per drivers on the same node.

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.

subhamkrai

please update the unit test to have both pv in single node and verify both NF are created.

pkg/operator/ceph/cluster/watcher.go

travisn · 2024-01-24T17:44:42Z

pkg/operator/ceph/cluster/watcher.go

@@ -516,7 +521,7 @@ func (c *clientCluster) createNetworkFence(ctx context.Context, pv corev1.Persis

 	networkFence := &addonsv1alpha1.NetworkFence{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      node.Name,
+			Name:      node.Name + driver,


How about also add a - to the name? It will be more standard K8s convention and also more readable.

Suggested change

Name: node.Name + driver,

Name: node.Name + "-" + driver,

pkg/operator/ceph/cluster/watcher.go

travisn · 2024-01-24T17:45:52Z

pkg/operator/ceph/cluster/watcher.go

+	}
+
+	err = wait.PollUntilContextTimeout(ctx, 2*time.Second, 60*time.Second, true, func(ctx context.Context) (bool, error) {
+		err = c.client.Get(ctx, types.NamespacedName{Name: node.Name + "rbd", Namespace: cluster.Namespace}, networkFence)


Suggested change

err = c.client.Get(ctx, types.NamespacedName{Name: node.Name + "rbd", Namespace: cluster.Namespace}, networkFence)

err = c.client.Get(ctx, types.NamespacedName{Name: node.Name + "-rbd", Namespace: cluster.Namespace}, networkFence)

travisn · 2024-01-24T17:46:20Z

pkg/operator/ceph/cluster/watcher.go

@@ -565,7 +615,7 @@ func (c *clientCluster) unfenceAndDeleteNetworkFence(ctx context.Context, node c
 	}

 	err = wait.PollUntilContextTimeout(ctx, 2*time.Second, 60*time.Second, true, func(ctx context.Context) (bool, error) {
-		err = c.client.Get(ctx, types.NamespacedName{Name: node.Name, Namespace: cluster.Namespace}, networkFence)
+		err = c.client.Get(ctx, types.NamespacedName{Name: node.Name + "cephFS", Namespace: cluster.Namespace}, networkFence)


Let's just keep it lower case, IMO the K8s resource naming is more consistent that way

Suggested change

err = c.client.Get(ctx, types.NamespacedName{Name: node.Name + "cephFS", Namespace: cluster.Namespace}, networkFence)

err = c.client.Get(ctx, types.NamespacedName{Name: node.Name + "-cephfs", Namespace: cluster.Namespace}, networkFence)

travisn · 2024-01-24T17:48:27Z

pkg/operator/ceph/cluster/watcher.go

+	return nil
+}
+
+func (c *clientCluster) unfenceAndDeleteCephFSNetworkFence(ctx context.Context, node corev1.Node, cluster *cephv1.CephCluster) error {


Can we have a common helper method for unfencing? If the only difference between unfenceAndDeleteCephFSNetworkFence() and unfenceAndDeleteRbdNetworkFence() is the name of the CR and the log messages, then we can make the name a parameters and keep a single method.

Yes, made the changes.

riya-singhal31 · 2024-01-24T19:20:28Z

Thanks for the review Travis and Subham,
I've updated the PR for the same, PTAL
cc: @travisn , @subhamkrai

riya-singhal31 · 2024-01-24T19:24:58Z

please update the unit test to have both pv in single node and verify both NF are created.

Thanks, Updated, and working as expected.

travisn · 2024-01-24T20:50:55Z

pkg/operator/ceph/cluster/watcher.go

@@ -575,7 +580,7 @@ func (c *clientCluster) unfenceAndDeleteNetworkFence(ctx context.Context, node c
 			return false, err
 		}

-		logger.Infof("successfully unfenced network fence cr %q, proceeding with deletion", networkFence.Name)
+		logger.Infof("successfully unfenced %q  network fence cr %q, proceeding with deletion", driver, networkFence.Name)


Suggested change

logger.Infof("successfully unfenced %q network fence cr %q, proceeding with deletion", driver, networkFence.Name)

logger.Infof("successfully unfenced %q network fence cr %q, proceeding with deletion", driver, networkFence.Name)

travisn · 2024-01-24T20:52:50Z

pkg/operator/ceph/cluster/watcher.go

@@ -516,7 +521,7 @@ func (c *clientCluster) createNetworkFence(ctx context.Context, pv corev1.Persis

 	networkFence := &addonsv1alpha1.NetworkFence{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      node.Name,
+			Name:      node.Name + "-" + driver,


Since this name generation is used in several places, how about a helper method to implement it? Something like:

Suggested change

Name: node.Name + "-" + driver,

Name: fenceResourceName(node.Name, driver),

Thanks, Updated!

subhamkrai

few suggestions

pkg/operator/ceph/cluster/watcher.go

pkg/operator/ceph/cluster/watcher_test.go

Madhu-1

Secret:
    Name:       rook-csi-rbd-provisioner
    Namespace:  openshift-storage
Status:
  Message:  fencing operation successful
  Result:   Succeeded
Events:
  Type     Reason                    Age   From                          Message
  ----     ------                    ----  ----                          -------
  Warning  OwnerRefInvalidNamespace  89s   garbage-collector-controller  ownerRef [ceph.rook.io/v1/CephCluster, namespace: , name: ocs-storagecluster-cephcluster, uid: db2eb584-6ebb-4865-8902-a17241e49ff2] does not exist in namespace ""
(venv) [jopinto@jopinto ceph-csi]$

what about the warning mentioned in the BZ, can we check that as well or else we might get i to garbage collection problem.

use SetControllerReference from the k8sutil/resources.go file which check for valid Reference.

Madhu-1 · 2024-01-25T12:18:38Z

pkg/operator/ceph/cluster/watcher.go

@@ -343,7 +348,7 @@ func listRWOCephFSPV(listPVs *corev1.PersistentVolumeList, cluster *cephv1.CephC
 				continue
 			}

-			if pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" || pv.Spec.CSI.VolumeAttributes["pool"] == "" {
+			if pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" || pv.Spec.CSI.VolumeAttributes["fsName"] == "" {


we dont need to check fsName for a static volume if staticVolume check is enough, i don't see when fsName can be set to empty.

So we can simplify to this:

Suggested change

if pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" || pv.Spec.CSI.VolumeAttributes["fsName"] == "" {

if pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" {

Madhu-1 · 2024-01-25T12:19:16Z

pkg/operator/ceph/cluster/watcher.go

@@ -500,7 +505,11 @@ func concatenateWatcherIp(address string) string {
 	return watcherIP
 }

-func (c *clientCluster) createNetworkFence(ctx context.Context, pv corev1.PersistentVolume, node *corev1.Node, cluster *cephv1.CephCluster, cidr []string) error {
+func fenceResourceName(nodeName string, driver string) string {


Suggested change

func fenceResourceName(nodeName string, driver string) string {

func fenceResourceName(nodeName , driver string) string {

parth-gr · 2024-01-25T13:17:49Z

pkg/operator/ceph/cluster/watcher.go

@@ -185,9 +185,14 @@ func (c *clientCluster) handleNodeFailure(ctx context.Context, cluster *cephv1.C
 		return nil
 	}

-	err = c.unfenceAndDeleteNetworkFence(ctx, *node, cluster)
+	err = c.unfenceAndDeleteNetworkFence(ctx, *node, cluster, "rbd")


why hardcoding can we define a variable?

Sure, updated, Thanks.

parth-gr · 2024-01-25T13:18:23Z

pkg/operator/ceph/cluster/watcher.go

@@ -500,7 +505,11 @@ func concatenateWatcherIp(address string) string {
 	return watcherIP
 }

-func (c *clientCluster) createNetworkFence(ctx context.Context, pv corev1.PersistentVolume, node *corev1.Node, cluster *cephv1.CephCluster, cidr []string) error {
+func fenceResourceName(nodeName string, driver string) string {
+	return nodeName + "-" + driver


use fmt.stringf

travisn · 2024-01-25T16:02:50Z

pkg/operator/ceph/cluster/watcher.go

@@ -343,7 +348,7 @@ func listRWOCephFSPV(listPVs *corev1.PersistentVolumeList, cluster *cephv1.CephC
 				continue
 			}

-			if pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" || pv.Spec.CSI.VolumeAttributes["pool"] == "" {
+			if pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" || pv.Spec.CSI.VolumeAttributes["fsName"] == "" {


So we can simplify to this:

Suggested change

if pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" || pv.Spec.CSI.VolumeAttributes["fsName"] == "" {

if pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" {

pkg/operator/ceph/cluster/watcher.go

riya-singhal31 · 2024-01-25T18:37:10Z

Secret:
    Name:       rook-csi-rbd-provisioner
    Namespace:  openshift-storage
Status:
  Message:  fencing operation successful
  Result:   Succeeded
Events:
  Type     Reason                    Age   From                          Message
  ----     ------                    ----  ----                          -------
  Warning  OwnerRefInvalidNamespace  89s   garbage-collector-controller  ownerRef [ceph.rook.io/v1/CephCluster, namespace: , name: ocs-storagecluster-cephcluster, uid: db2eb584-6ebb-4865-8902-a17241e49ff2] does not exist in namespace ""
(venv) [jopinto@jopinto ceph-csi]$

what about the warning mentioned in the BZ, can we check that as well or else we might get i to garbage collection problem.

use SetControllerReference from the k8sutil/resources.go file which check for valid Reference.

Thanks @Madhu-1 , for letting us know about this.
Created a tracker -> #13626

this commit updates the name of network fence CR to node name + driver name, which will allow the creation of multiple network fence per drivers on the same node, and modifies the unit test and deletion procedure for the same. Signed-off-by: Riya Singhal <rsinghal@redhat.com>

travisn

The PR looks good, could you also confirm in local testing? The unit tests don't catch everything.

subhamkrai · 2024-02-01T08:10:45Z

@riya-singhal31 please do the local testing, thanks

riya-singhal31 · 2024-02-05T13:23:13Z

Hi! Working as expected,

These are the volumes & pods for cephFS and rbd each driver.

riyasinghal@rsinghal-mac rook % kubectl get pvc,pods
NAME                               STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS      AGE
persistentvolumeclaim/cephfs-pvc   Bound    pvc-7f8aa823-7ec7-4950-917e-80136e00c0b2   1Gi        RWO            rook-cephfs       36m
persistentvolumeclaim/rbd-pvc      Bound    pvc-e840b0a9-f1d7-4338-8ac0-c69dd6e471b1   1Gi        RWO            rook-ceph-block   60m

NAME                     READY   STATUS    RESTARTS   AGE
pod/csicephfs-demo-pod   1/1     Running   0          61s
pod/csirbd-demo-pod      1/1     Running   0          92s

Scheduled on same node, to test the functionality for this PR,

riyasinghal@rsinghal-mac rook % kubectl describe pod/csirbd-demo-pod 
Name:             csirbd-demo-pod
Namespace:        default
Priority:         0
Service Account:  default
Node:             ip-10-0-113-254.us-west-2.compute.internal/[10.0.113.254](http://10.0.113.254/)
Start Time:       Mon, 05 Feb 2024 18:38:27 +0530

riyasinghal@rsinghal-mac rook % kubectl describe pod csicephfs-demo-pod
Name:             csicephfs-demo-pod
Namespace:        default
Priority:         0
Service Account:  default
Node:             ip-10-0-113-254.us-west-2.compute.internal/[10.0.113.254](http://10.0.113.254/)
Start Time:       Mon, 05 Feb 2024 18:38:58 +0530

Since there are 2 drivers, so we expect 2 different Network fences to be created after applying taint to the node for each driver,

riyasinghal@rsinghal-mac rook % kubectl get [networkfences.csiaddons.openshift.io](http://networkfences.csiaddons.openshift.io/)                                  
NAME                                                DRIVER                          CIDRS               FENCESTATE   AGE    RESULT
ip-10-0-113-254.us-west-2.compute.internal-cephfs   [rook-ceph.cephfs.csi.ceph.com](http://rook-ceph.cephfs.csi.ceph.com/)   ["[100.64.0.5/32](http://100.64.0.5/32)"]   Fenced       6m6s   Succeeded
ip-10-0-113-254.us-west-2.compute.internal-rbd      [rook-ceph.rbd.csi.ceph.com](http://rook-ceph.rbd.csi.ceph.com/)      ["[100.64.0.5/32](http://100.64.0.5/32)"]   Fenced       6m7s   Succeeded

The above step was the main requirement for the concern of bug and this PR.

Thanks,
@travisn , @subhamkrai

subhamkrai

I guess ready to merge once @Madhu-1 approves.

feedback addressed

csi: update network fence CR name (backport #13615)

riya-singhal31 changed the title ~~csi: update network fence CR name~~ [WIP]csi: update network fence CR name Jan 24, 2024

riya-singhal31 force-pushed the node-name branch from 13d15f0 to c4d6128 Compare January 24, 2024 13:00

subhamkrai requested changes Jan 24, 2024

View reviewed changes

riya-singhal31 force-pushed the node-name branch from 88d27a3 to cacc0be Compare January 24, 2024 13:20

travisn requested changes Jan 24, 2024

View reviewed changes

pkg/operator/ceph/cluster/watcher.go Outdated Show resolved Hide resolved

riya-singhal31 force-pushed the node-name branch 5 times, most recently from 400ea02 to 115aca5 Compare January 24, 2024 17:39

travisn requested changes Jan 24, 2024

View reviewed changes

riya-singhal31 force-pushed the node-name branch 10 times, most recently from 803d301 to 40d8262 Compare January 24, 2024 19:19

riya-singhal31 changed the title ~~[WIP]csi: update network fence CR name~~ csi: update network fence CR name Jan 24, 2024

riya-singhal31 requested review from travisn and subhamkrai January 24, 2024 19:23

travisn requested changes Jan 24, 2024

View reviewed changes

riya-singhal31 force-pushed the node-name branch from 40d8262 to 2cd8b1b Compare January 25, 2024 07:05

subhamkrai requested changes Jan 25, 2024

View reviewed changes

pkg/operator/ceph/cluster/watcher.go Show resolved Hide resolved

pkg/operator/ceph/cluster/watcher.go Show resolved Hide resolved

pkg/operator/ceph/cluster/watcher_test.go Show resolved Hide resolved

riya-singhal31 force-pushed the node-name branch from 2cd8b1b to f687e21 Compare January 25, 2024 09:22

riya-singhal31 force-pushed the node-name branch 2 times, most recently from 1ba54f7 to 6f9db56 Compare January 25, 2024 10:57

Madhu-1 reviewed Jan 25, 2024

View reviewed changes

parth-gr previously requested changes Jan 25, 2024

View reviewed changes

travisn requested changes Jan 25, 2024

View reviewed changes

riya-singhal31 force-pushed the node-name branch 2 times, most recently from 9b4da07 to c07f4ad Compare January 25, 2024 18:33

riya-singhal31 mentioned this pull request Jan 25, 2024

Bug: OwnerRefInvalidNamespace in Networkfence CR #13626

Closed

riya-singhal31 requested review from Madhu-1, parth-gr, subhamkrai and travisn January 25, 2024 18:40

riya-singhal31 force-pushed the node-name branch from c07f4ad to c2e3cf3 Compare January 25, 2024 18:48

travisn approved these changes Jan 25, 2024

View reviewed changes

subhamkrai approved these changes Feb 5, 2024

View reviewed changes

Madhu-1 approved these changes Feb 5, 2024

View reviewed changes

travisn merged commit 73427c9 into rook:master Feb 5, 2024
49 of 50 checks passed

travisn added the backport-release-1.13 label Feb 5, 2024

mergify bot mentioned this pull request Feb 5, 2024

csi: update network fence CR name (backport #13615) #13688

Merged

mergify bot added a commit that referenced this pull request Feb 6, 2024

Merge pull request #13688 from rook/mergify/bp/release-1.13/pr-13615

9221e59

csi: update network fence CR name (backport #13615)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

csi: update network fence CR name #13615

csi: update network fence CR name #13615

riya-singhal31 commented Jan 24, 2024 •

edited

subhamkrai left a comment

travisn Jan 24, 2024

travisn Jan 24, 2024

travisn Jan 24, 2024

travisn Jan 24, 2024

riya-singhal31 Jan 24, 2024

riya-singhal31 commented Jan 24, 2024

riya-singhal31 commented Jan 24, 2024

travisn Jan 24, 2024

travisn Jan 24, 2024

riya-singhal31 Jan 25, 2024

subhamkrai left a comment

Madhu-1 left a comment

Madhu-1 Jan 25, 2024

travisn Jan 25, 2024

Madhu-1 Jan 25, 2024

parth-gr Jan 25, 2024

riya-singhal31 Jan 25, 2024

parth-gr Jan 25, 2024

travisn Jan 25, 2024

riya-singhal31 commented Jan 25, 2024

travisn left a comment

subhamkrai commented Feb 1, 2024

riya-singhal31 commented Feb 5, 2024 •

edited

subhamkrai left a comment

	err = c.client.Get(ctx, types.NamespacedName{Name: node.Name + "rbd", Namespace: cluster.Namespace}, networkFence)
	err = c.client.Get(ctx, types.NamespacedName{Name: node.Name + "-rbd", Namespace: cluster.Namespace}, networkFence)

	logger.Infof("successfully unfenced %q network fence cr %q, proceeding with deletion", driver, networkFence.Name)
	logger.Infof("successfully unfenced %q network fence cr %q, proceeding with deletion", driver, networkFence.Name)

	Name: node.Name + "-" + driver,
	Name: fenceResourceName(node.Name, driver),

	if pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" \|\| pv.Spec.CSI.VolumeAttributes["fsName"] == "" {
	if pv.Spec.CSI.VolumeAttributes["staticVolume"] == "true" {

	func fenceResourceName(nodeName string, driver string) string {
	func fenceResourceName(nodeName , driver string) string {

csi: update network fence CR name #13615

csi: update network fence CR name #13615

Conversation

riya-singhal31 commented Jan 24, 2024 • edited

subhamkrai left a 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

riya-singhal31 commented Jan 24, 2024

riya-singhal31 commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhamkrai left a comment

Choose a reason for hiding this comment

Madhu-1 left a 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

riya-singhal31 commented Jan 25, 2024

travisn left a comment

Choose a reason for hiding this comment

subhamkrai commented Feb 1, 2024

riya-singhal31 commented Feb 5, 2024 • edited

subhamkrai left a comment

Choose a reason for hiding this comment

riya-singhal31 commented Jan 24, 2024 •

edited

riya-singhal31 commented Feb 5, 2024 •

edited