fix(localpv-hostpath):pv deletion caused panic, if node not found #1662
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After the PV is created and node affinity is set based on kubernetes.io/hostname label, either: - hostname label changed on the node or - the node is deleted from the cluster. This PR fixes the panic, by logging the details of the node hostname that no longer exists in the cluster. The code doesn't force delete the PV, considering that the node might intermittently be out of the cluster. If the user decides that node is not going to come back, then the user can go ahead with force delete of pv using `kubectl delete pv`. An alternate case is that node hostname has really changed. In this case, user will have to perform a manual migration of the localpv-hostpath PV to the new node using the following steps: - Scale down the deployment or sts using the PVC. - Save the PVC and PV yamls files. - Delete the PVC and PV - Modify the saved PV yaml will the node hostname and apply Note: when re-applying the yamls, the uuid of pv and pvc objects will change, so the metadata around self-uuid,etc needs to be cleared. - Modify the saved PVC yaml for stale references and re-apply. - Update the PV yaml with the uuid of the newly created PVC - Scale up the deployment. Signed-off-by: kmova <kiran.mova@mayadata.io>
vishnuitta
approved these changes
Apr 10, 2020
akhilerm
reviewed
Apr 10, 2020
| @@ -134,8 +134,12 @@ func (p *Provisioner) GetNodeObjectFromHostName(hostName string) (*v1.Node, erro | |||
| Limit: 1, | |||
| } | |||
| nodeList, err := p.kubeClient.CoreV1().Nodes().List(listOptions) | |||
| if err != nil { | |||
| return nil, errors.Errorf("Unable to get the Node with the NodeHostName") | |||
| if err != nil || nodeList.Items == nil || len(nodeList.Items) == 0 { | |||
There was a problem hiding this comment.
Do we need both the checks, for Items not nil and len(Items)
There was a problem hiding this comment.
@AmitKumarDas -- what is the idiomatic go way for above checks?
There was a problem hiding this comment.
Len check only for arrays is good
akhilerm
approved these changes
Apr 10, 2020
Signed-off-by: kmova <kiran.mova@mayadata.io>
…path.go Signed-off-by: kmova <kiran.mova@mayadata.io>
AmitKumarDas
approved these changes
Apr 10, 2020
vishnuitta
pushed a commit
that referenced
this pull request
Apr 10, 2020
…1662) (#1664) After the PV is created and node affinity is set based on kubernetes.io/hostname label, either: - hostname label changed on the node or - the node is deleted from the cluster. This PR fixes the panic, by logging the details of the node hostname that no longer exists in the cluster. The code doesn't force delete the PV, considering that the node might intermittently be out of the cluster. If the user decides that node is not going to come back, then the user can go ahead with force delete of pv using `kubectl delete pv`. An alternate case is that node hostname has really changed. In this case, user will have to perform a manual migration of the localpv-hostpath PV to the new node using the following steps: - Scale down the deployment or sts using the PVC. - Save the PVC and PV yamls files. - Delete the PVC and PV - Modify the saved PV yaml will the node hostname and apply Note: when re-applying the yamls, the uuid of pv and pvc objects will change, so the metadata around self-uuid,etc needs to be cleared. - Modify the saved PVC yaml for stale references and re-apply. - Update the PV yaml with the uuid of the newly created PVC - Scale up the deployment. Signed-off-by: kmova <kiran.mova@mayadata.io> (cherry picked from commit 728a003)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref: openebs/openebs#2993
What this PR does / why we need it:
After the PV is created and node affinity is set
based on kubernetes.io/hostname label, either:
This PR fixes the panic, by logging the details
of the node hostname that no longer exists
in the cluster.
The code doesn't force delete the PV, considering that
the node might intermittently be out of the cluster.
If the user decides that node is not going to come back,
then the user can go ahead with force delete of pv using
kubectl delete pv.An alternate case is that node hostname has really changed.
In this case, the user will have to perform a manual migration
of the localpv-hostpath PV to the new node using the
following steps:
Note: when re-applying the yamls, the uuid of pv and pvc objects will change, so the metadata around self-UUID,etc needs to be cleared.
Signed-off-by: kmova kiran.mova@mayadata.io
Special notes for your reviewer:
Tested by renaming the hostname after PV was created. The deletion will not panic, but display the following error message and continue to re-concile delete:
Manually running the
kubectl delete pvclears the PV from the system. If the node was re-named, then the PV folder will continue to exist on the node and that also needs to be manualy cleared.Checklist:
documentationtagbreaking-changestag (no breaking changes)requires-upgradetag (no upgrade changes)