Repository: dell/csi-baremetal
Pull request #514 (2021-09-03 16:13:25)

[ISSUE-511] Fix restoring the same node after removal
## Purpose
### Issue #511 

Clean node from node-controller cache if node is deleted and csibmnode has drain label

## PR checklist
- [x] Add link to the issue
- [x] Choose PR label
- [ ] New unit tests added
- [ ] Modified code has meaningful comments
- [ ] All TODOs are linked with the issues
- [ ] All comments are resolved

## Testing
```
# Delete
user@user-vm ~/g/s/g/dell> k taint no kind-worker node.dell.com/drain=drain:NoSchedule
node/kind-worker tainted
user@user-vm ~/g/s/g/dell> kdl no kind-worker
node "kind-worker" deleted
user@user-vm ~/g/s/g/dell> kg csibmnode
NAME                                             UUID                                   HOSTNAME             NODE_IP
csibmnode-6a025dc5-ae47-4337-bf25-e216d64dd07b   6a025dc5-ae47-4337-bf25-e216d64dd07b   kind-worker3         172.18.0.6
csibmnode-babd7c71-3b19-48f3-b338-944e6c2da3c0   babd7c71-3b19-48f3-b338-944e6c2da3c0   kind-control-plane   172.18.0.5
csibmnode-ef5b0290-5980-4584-8c78-9cad13ed3bdf   ef5b0290-5980-4584-8c78-9cad13ed3bdf   kind-worker4         172.18.0.2
csibmnode-fe17043c-ffa0-483d-ade0-a2c9747b2562   fe17043c-ffa0-483d-ade0-a2c9747b2562   kind-worker2         172.18.0.3

# Restore
user@user-vm ~/g/s/g/dell> ka ./node.yaml
node/kind-worker created
user@user-vm ~/g/s/g/dell> kg no
NAME                 STATUS     ROLES    AGE    VERSION
kind-control-plane   Ready      master   8d     v1.18.19
kind-worker          NotReady   <none>   3m5s   v1.18.19
kind-worker2         Ready      <none>   8d     v1.18.19
kind-worker3         Ready      <none>   8d     v1.18.19
kind-worker4         Ready      <none>   8d     v1.18.19
user@user-vm ~/g/s/g/dell> sudo docker restart kind-worker
kind-worker
user@user-vm ~/g/s/g/dell> kg csibmnode
NAME                                             UUID                                   HOSTNAME             NODE_IP
csibmnode-6a025dc5-ae47-4337-bf25-e216d64dd07b   6a025dc5-ae47-4337-bf25-e216d64dd07b   kind-worker3         172.18.0.6
csibmnode-babd7c71-3b19-48f3-b338-944e6c2da3c0   babd7c71-3b19-48f3-b338-944e6c2da3c0   kind-control-plane   172.18.0.5
csibmnode-cd5e3cd9-a6d1-489e-891f-317d772af76f   cd5e3cd9-a6d1-489e-891f-317d772af76f   kind-worker          172.18.0.4
csibmnode-ef5b0290-5980-4584-8c78-9cad13ed3bdf   ef5b0290-5980-4584-8c78-9cad13ed3bdf   kind-worker4         172.18.0.2
csibmnode-fe17043c-ffa0-483d-ade0-a2c9747b2562   fe17043c-ffa0-483d-ade0-a2c9747b2562   kind-worker2         172.18.0.3
user@user-vm ~/g/s/g/dell> kg po
NAME                                             READY   STATUS    RESTARTS   AGE
csi-baremetal-controller-799d95d884-c5wjm        4/4     Running   5          12m
csi-baremetal-node-controller-55fcd5df57-vcdq9   1/1     Running   0          34s
csi-baremetal-node-kernel-5.4-fvjxj              4/4     Running   5          4m20s
csi-baremetal-node-kernel-5.4-h55xc              4/4     Running   0          12m
csi-baremetal-node-kernel-5.4-mhcmb              4/4     Running   1          12m
csi-baremetal-node-kernel-5.4-rslln              4/4     Running   0          12m
csi-baremetal-operator-67fd59c6b6-xgkcb          1/1     Running   1          13m
csi-baremetal-se-patcher-xq2w9                   1/1     Running   0          12m
csi-baremetal-se-qlgf8                           1/1     Running   0          12m
```

codecov[bot]: # [Codecov](https://codecov.io/gh/dell/csi-baremetal/pull/514?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell) Report
> Merging [#514](https://codecov.io/gh/dell/csi-baremetal/pull/514?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell) (00298f4) into [master](https://codecov.io/gh/dell/csi-baremetal/commit/5d8bf8dbba3fd26b9bec1a26941cc9ba57357fd4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell) (5d8bf8d) will **decrease** coverage by `0.14%`.
> The diff coverage is `37.83%`.

> :exclamation: Current head 00298f4 differs from pull request most recent head e45394f. Consider uploading reports for the commit e45394f to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/dell/csi-baremetal/pull/514/graphs/tree.svg?width=650&height=150&src=pr&token=4D8WVNWX5O&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell)](https://codecov.io/gh/dell/csi-baremetal/pull/514?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell)

```diff
@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
- Coverage   60.31%   60.17%   -0.15%     
==========================================
  Files          60       60              
  Lines        5960     5989      +29     
==========================================
+ Hits         3595     3604       +9     
- Misses       2155     2170      +15     
- Partials      210      215       +5     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `60.17% <37.83%> (-0.15%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/dell/csi-baremetal/pull/514?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell) | Coverage Δ | |
|---|---|---|
| [pkg/crcontrollers/operator/controller.go](https://codecov.io/gh/dell/csi-baremetal/pull/514/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell#diff-cGtnL2NyY29udHJvbGxlcnMvb3BlcmF0b3IvY29udHJvbGxlci5nbw==) | `68.60% <37.83%> (-3.90%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/dell/csi-baremetal/pull/514?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/dell/csi-baremetal/pull/514?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell). Last update [5d8bf8d...e45394f](https://codecov.io/gh/dell/csi-baremetal/pull/514?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell).


Patch:
diff --git a/api/v1/drivecrd/drive_types.go b/api/v1/drivecrd/drive_types.go
index b865083..a9121be 100644
--- a/api/v1/drivecrd/drive_types.go
+++ b/api/v1/drivecrd/drive_types.go
@@ -83,9 +83,12 @@ func (in *Drive) Equals(drive *api.Drive) bool {
 }
 
 func (in *Drive) GetDriveDescription() string {
-	return fmt.Sprintf("Drive Details: SN='%s', Node='%s',"+
-		" Type='%s', Model='%s %s',"+
-		" Size='%d', Firmware='%s'",
-		in.Spec.SerialNumber, in.Spec.NodeId, in.Spec.Type,
-		in.Spec.VID, in.Spec.PID, in.Spec.Size, in.Spec.Firmware)
+	spec := in.Spec
+	description := fmt.Sprintf("Drive Details: SN='%s', Model='%s %s', Type='%s', Size='%d', Node='%s'",
+		spec.SerialNumber, spec.VID, spec.PID, spec.Type, spec.Size, spec.NodeId)
+	// add firmware info only if detected
+	if spec.Firmware != "" {
+		description += fmt.Sprintf(", Firmware='%s'", spec.Firmware)
+	}
+	return description
 }
diff --git a/docs/node-removal.md b/docs/node-removal.md
index 5ce88dd..6180880 100644
--- a/docs/node-removal.md
+++ b/docs/node-removal.md
@@ -8,24 +8,59 @@ Need to determine actions for CSI for node removing process
 
 ## Proposal
 
+### Automated
+
+#### User actions
+
+- CSI - components on CSI side including CSI Operator
+- Operator - application controller (or other service, which watch resources)
+
+Removal procedure:
+1. Taint a node with:
+- `node.dell.com/drain=drain:NoSchedule`
+2. Wait until Operator replaced PVs and destroyed pods
+3. Delete a node from kubernetes cluster
+4. CSI deletes all corresponding CR resources automatically (including Csibmnode)
+
+After removal the node can't be restored as in [replacement](https://github.com/dell/csi-baremetal/blob/master/docs/proposals/node-replacement.md)!
+
+#### Implementation
+
+Set label:
+1. CSI Operator must watch Nodes on taint changing events
+2. Label the Csibmnode with `node.dell.com/drain=drain` if the Node has taint `node.dell.com/drain=drain:NoSchedule`
+
+Unset label:
+1. CSI Operator must watch Nodes on taint changing events
+2. Check, if the Csibmnode has `node.dell.com/drain=drain` label and the Node with the same ID has no `node.dell.com/drain=drain:NoSchedule` taint
+3. Delete `node.dell.com/drain=drain` label
+
+Removal:
+1. CSI Operator must watch Nodes on deleting events
+2. Start removal procedure, if if the Csibmnode has `node.dell.com/drain=drain` label and the Node with the same ID has been deleted from kubernetes cluster
+3. Delete drives, ACs
+4. Patch and delete Volumes, Lvgs, Csibmnode
+
+### Manual
+
 If user aims to delete node from cluster, he/she must perform following steps for CSI.  
 
-### Node is healthy
+#### Node is healthy
 
-#### After node drain
+After node drain:
 1) Determine removing Node UUID 
 2) Delete PVCs used by Pods, which were run on deleted node
 
-### Node is unhealthy
+#### Node is unhealthy
 
-#### Before node deletion: 
+Before node deletion:
 1) Determine removing Node UUID 
 
-#### After node deletion: 
+After node deletion:
 1) Delete PVCs used by Pods, which were run on deleted node
 
-### Common actions
-#### After node deletion: 
+#### User actions
+After node deletion:
 1) Patch according Volumes custom resources with empty finalizer  
 ```
 kubectl get volume | grep <node_id> | awk '{print $1}' | xargs kubectl patch volume --type merge -p '{"metadata":{"finalizers":null}}'
@@ -69,4 +104,7 @@ There is no problem with compatibility
 ID | Name | Descriptions | Status | Comments
 ---| -----| -------------| ------ | --------
 ISSUE-1 | Should we mark drives as OFFLINE instead of removing  |  | Open  |   
-ISSUE-2 | Should we marked volumes as INOPERATIVE instead of removing?  |  | Open  |   
\ No newline at end of file
+ISSUE-2 | Should we marked volumes as INOPERATIVE instead of removing?  |  | Open  |
+ISSUE-3 | Should we do node removal procedure in node-controller? | Node-controller reconciles Csibmnodes and nodes already | Open |
+ISSUE-4 | Should we delete related PVs and PVCs? |  | Open  |
+ISSUE-5 | We don't provide any procedure to remove a node without deleting it from cluster |  | Open |
diff --git a/docs/proposals/automated-node-removal.md b/docs/proposals/automated-node-removal.md
deleted file mode 100644
index 718981f..0000000
--- a/docs/proposals/automated-node-removal.md
+++ /dev/null
@@ -1,71 +0,0 @@
-# Proposal: Automation of node removal procedure
-
-Last updated: 02.07.2021
-
-## Abstract
-
-Implementation of the option to delete CSI CRs from a node, which is going to be removed. 
-
-## Background
-
-User manual actions for node removal are described in [doc](https://github.com/dell/csi-baremetal/blob/master/docs/node-removal.md).
-The goal of this proposal is automating most of the steps of that procedure.
-
-## Proposal
-
-#### User API
-
-CSI - components on CSI side including CSI Operator
-
-Operator - application controller (or other service, which watch resources)
-
-Removal procedure:
-1. Taint a node with:
-- `node.dell.com/drain=drain:NoSchedule`
-2. Wait until Operator replaced PVs and destroyed pods
-3. Delete a node from kubernetes cluster
-4. CSI deletes all corresponding CR resources automatically (including Csibmnode)
-
-After removal the node can't be restored ad in [replacement](https://github.com/dell/csi-baremetal/blob/master/docs/proposals/node-replacement.md)! 
-
-
-## Implementation
-
-Set label:
-1. CSI Operator must watch Nodes on taint changing events
-2. Label the Csibmnode with `node.dell.com/drain=drain` if the Node has taint `node.dell.com/drain=drain:NoSchedule`
-
-Unset label:
-1. CSI Operator must watch Nodes on taint changing events
-2. Check, if the Csibmnode has `node.dell.com/drain=drain` label and the Node with the same ID has no `node.dell.com/drain=drain:NoSchedule` taint
-3. Delete `node.dell.com/drain=drain` label
-
-Removal:
-1. CSI Operator must watch Nodes on deleting events
-2. Start removal procedure, if if the Csibmnode has `node.dell.com/drain=drain` label and the Node with the same ID has been deleted from kubernetes cluster
-3. Delete drives, ACs
-4. Patch and delete Volumes, Lvgs, Csibmnode 
-
-## Rationale
-
-#### Alternative approach 1
-
-Perform CSI CRs deletion in CSI Node-controller. Trigger will be deleting the corresponding `csibmnode` CR.
-
-#### Alternative approach 2
-
-Delete CRs (Drive, LVG) in `Reconcile` method in appropriate controller if node has no `uuid label` or has `node removal` label/annotation
-
-Problem - if node was removed from cluster before and csi-baremetal-node pod is unavailable, CSI resources will be stacked as currently
-
-## Compatibility
-
-This proposal is valid for CSI deployed with CSI Operator
-
-## Open issues (same as [here](https://github.com/dell/csi-baremetal/blob/master/docs/node-removal.md))
-
-ID | Name | Descriptions | Status | Comments
----| -----| -------------| ------ | --------
-ISSUE-1 | Should we do node removal procedure in node-controller? | Node-controller reconciles Csibmnodes and nodes already | Open | 
-ISSUE-2 | Should we delete related PVs and PVCs? |  | Open  | 
-ISSUE-3 | We don't provide any procedure to remove a node without deleting it from cluster |  | Open | 
diff --git a/pkg/crcontrollers/node/controller.go b/pkg/crcontrollers/node/controller.go
index d561746..4e9f5f8 100644
--- a/pkg/crcontrollers/node/controller.go
+++ b/pkg/crcontrollers/node/controller.go
@@ -76,6 +76,8 @@ type label struct {
 type nodesMapping struct {
 	k8sToBMNode map[string]string // k8s node name to Node CR name
 	bmToK8sNode map[string]string // Node CR name to k8s node name
+
+	cacheMu sync.RWMutex
 }
 
 func (nc *nodesMapping) getK8sNodeName(bmNodeName string) (string, bool) {
@@ -89,8 +91,17 @@ func (nc *nodesMapping) getCSIBMNodeName(k8sNodeName string) (string, bool) {
 }
 
 func (nc *nodesMapping) put(k8sNodeName, bmNodeName string) {
+	nc.cacheMu.Lock()
 	nc.k8sToBMNode[k8sNodeName] = bmNodeName
 	nc.bmToK8sNode[bmNodeName] = k8sNodeName
+	nc.cacheMu.Unlock()
+}
+
+func (nc *nodesMapping) remove(k8sNodeName, bmNodeName string) {
+	nc.cacheMu.Lock()
+	delete(nc.bmToK8sNode, bmNodeName)
+	delete(nc.k8sToBMNode, k8sNodeName)
+	nc.cacheMu.Unlock()
 }
 
 // NewController returns instance of Controller
@@ -346,15 +357,22 @@ func (bmc *Controller) reconcileForCSIBMNode(bmNode *nodecrd.Node) (ctrl.Result,
 		k8sNode          = &coreV1.Node{}
 		k8sNodes         []coreV1.Node
 		k8sNodeFromCache bool
+		k8sNodeNotFound  = false
+		k8sNodeName      string
 	)
 
 	// get corresponding k8s node name from cache
-	if k8sNodeName, k8sNodeFromCache := bmc.cache.getK8sNodeName(bmNode.Name); k8sNodeFromCache {
-		if err := bmc.k8sClient.ReadCR(context.Background(), k8sNodeName, "", k8sNode); err != nil {
+	if k8sNodeName, k8sNodeFromCache = bmc.cache.getK8sNodeName(bmNode.Name); k8sNodeFromCache {
+		err := bmc.k8sClient.ReadCR(context.Background(), k8sNodeName, "", k8sNode)
+		if err != nil && !k8sError.IsNotFound(err) {
 			ll.Errorf("Unable to read k8s node %s: %v", k8sNodeName, err)
 			return ctrl.Result{Requeue: true}, err
 		}
-		k8sNodes = []coreV1.Node{*k8sNode}
+		if k8sError.IsNotFound(err) {
+			k8sNodeNotFound = true
+		} else {
+			k8sNodes = []coreV1.Node{*k8sNode}
+		}
 	}
 
 	if !k8sNodeFromCache {
@@ -382,11 +400,16 @@ func (bmc *Controller) reconcileForCSIBMNode(bmNode *nodecrd.Node) (ctrl.Result,
 	}
 
 	if !bmNode.GetDeletionTimestamp().IsZero() {
-		bmc.disableForNode(k8sNode.Name)
-		if err := bmc.removeLabelsAndAnnotation(k8sNode); err != nil {
-			ll.Errorf("Unable to remove annotations or labels from node %s: %v", k8sNode.Name, err)
-			bmc.enableForNode(k8sNode.Name)
-			return ctrl.Result{Requeue: true}, err
+		if k8sNodeNotFound {
+			ll.Infof("Node %s should be removed, clean it from cache", k8sNodeName)
+			bmc.cleanNodeFromCache(bmNode.Name, k8sNodeName)
+		} else {
+			bmc.disableForNode(k8sNode.Name)
+			if err := bmc.removeLabelsAndAnnotation(k8sNode); err != nil {
+				ll.Errorf("Unable to remove annotations or labels from node %s: %v", k8sNode.Name, err)
+				bmc.enableForNode(k8sNode.Name)
+				return ctrl.Result{Requeue: true}, err
+			}
 		}
 
 		ll.Infof("Annotations and labels from node %s was removed. Removing finalizer from %s.", k8sNode.Name, bmNode.Name)
@@ -398,6 +421,11 @@ func (bmc *Controller) reconcileForCSIBMNode(bmNode *nodecrd.Node) (ctrl.Result,
 		return ctrl.Result{}, err
 	}
 
+	if k8sNodeNotFound {
+		ll.Errorf("K8sNode %s is not found", k8sNodeName)
+		return ctrl.Result{Requeue: true}, errors.New("k8sNode is not found")
+	}
+
 	if len(matchedNodes) == 1 {
 		bmc.cache.put(k8sNode.Name, bmNode.Name)
 		return bmc.updateNodeLabelsAndAnnotation(k8sNode, bmNode.Spec.UUID)
@@ -462,18 +490,8 @@ func (bmc *Controller) removeLabelsAndAnnotation(k8sNode *coreV1.Node) error {
 		}
 	}
 
-	// check labels
-	labels := k8sNode.GetLabels()
-	// external csi-provisioner label
-	// TODO https://github.com/dell/csi-baremetal/issues/319 Rework after operator implementation
-	if _, ok := labels[common.NodeIDTopologyLabelKey]; ok {
-		delete(labels, common.NodeIDTopologyLabelKey)
-		toUpdate = true
-	}
-
 	if toUpdate {
 		k8sNode.Annotations = annotations
-		k8sNode.Labels = labels
 		return bmc.k8sClient.UpdateCR(context.Background(), k8sNode)
 	}
 
@@ -512,3 +530,11 @@ func (bmc *Controller) constructNodeID(k8sNode *coreV1.Node) string {
 
 	return uuid.New().String()
 }
+
+func (bmc *Controller) cleanNodeFromCache(bmNodeCRName, k8sNodeName string) {
+	bmc.enabledMu.Lock()
+	delete(bmc.enabledForNode, k8sNodeName)
+	bmc.enabledMu.Unlock()
+
+	bmc.cache.remove(k8sNodeName, bmNodeCRName)
+}
diff --git a/pkg/node/volumemgr.go b/pkg/node/volumemgr.go
index 89dfe70..c2f9349 100644
--- a/pkg/node/volumemgr.go
+++ b/pkg/node/volumemgr.go
@@ -369,6 +369,10 @@ func (m *VolumeManager) updateVolumeAndDriveUsageStatus(ctx context.Context, vol
 		m.addVolumeStatusAnnotation(drive, volume.Name, apiV1.VolumeUsageReleased)
 	}
 	if drive != nil {
+		if driveStatus == apiV1.DriveUsageFailed {
+			eventMsg := fmt.Sprintf("Failed to release volume(s), %s", drive.GetDriveDescription())
+			m.recorder.Eventf(drive, eventing.DriveRemovalFailed, eventMsg)
+		}
 		drive.Spec.Usage = driveStatus
 		if err := m.k8sClient.UpdateCR(ctx, drive); err != nil {
 			ll.Errorf("Unable to change drive %s usage status to %s, error: %v.",


Test patch:
diff --git a/pkg/crcontrollers/node/controller_test.go b/pkg/crcontrollers/node/controller_test.go
index e8a44cb..885af0c 100644
--- a/pkg/crcontrollers/node/controller_test.go
+++ b/pkg/crcontrollers/node/controller_test.go
@@ -427,6 +427,34 @@ func Test_reconcileForCSIBMNode(t *testing.T) {
 		_, ok = nodeObj.GetAnnotations()[common.DeafultNodeIDAnnotationKey]
 		assert.False(t, ok)
 	})
+
+	t.Run("Delete bmnode related with removed k8snode", func(t *testing.T) {
+		var (
+			c           = setup(t)
+			k8sNodeName = "k8s-node"
+			bmNode      = testCSIBMNode1.DeepCopy()
+		)
+
+		c.cache.put(k8sNodeName, bmNode.Name)
+		bmNode.DeletionTimestamp = &metaV1.Time{Time: time.Now()}
+		bmNode.Finalizers = []string{"csibm-finalizer"}
+
+		createObjects(t, c.k8sClient, bmNode)
+		res, err := c.reconcileForCSIBMNode(bmNode)
+		assert.Nil(t, err)
+		assert.Equal(t, ctrl.Result{}, res)
+
+		bmNodesList := &nodecrd.NodeList{}
+		assert.Nil(t, c.k8sClient.ReadList(testCtx, bmNodesList))
+		assert.Equal(t, 1, len(bmNodesList.Items))
+
+		bmNode = &bmNodesList.Items[0]
+		assert.Nil(t, bmNode.GetFinalizers())
+		_, ok := c.cache.bmToK8sNode[bmNode.Name]
+		assert.False(t, ok)
+		_, ok = c.cache.k8sToBMNode[k8sNodeName]
+		assert.False(t, ok)
+	})
 }
 
 func Test_checkAnnotationAndLabels(t *testing.T) {