Add initial OEP for lso cleanup #700
Conversation
gnufied
force-pushed
the
cleanup-lso-symlink
branch
from
6c967b9
to
5314740
Compare
|
|
||
| ### Goals | ||
|
|
||
| * Do not allow deletion of `LocalVolumeSet` object if it has bound PVCs. |
There was a problem hiding this comment.
Seeing it written like this: why do we allow deletion of LocalVolumes then? Please add a note that LocalVolumes can't be deleted already.
| a. If there are no bound PVs that are created by this LSO CR. | ||
| b. None of the existing PVs(that match LSO CR) should have reclaim policy of `Retain.` | ||
| c. If there are unbound PVs with reclaimPolicy of `Delete` then finalizer should not be removed until all such PVs are deleted. |
There was a problem hiding this comment.
I admit that the conditions here (and below) are hard to read and put together.
In a. it allows deletion when there are unbound PVs and in b. and c. it does not allow to delete those.
In addition, why should ReclaimPolicy matter? Unound PV was not used, so there is no data to retain on them.
Perhaps it would be better to use PV phase:
- LSO removes the finalizer on LV/LVS only when all PVs associated with it are deleted.
- LSO automatically deletes Pending and Available PVS for LV/LVS with a deletion timestamp, as we expect there is no data on them. See below how LSO deletes them avoiding potential races with PV controller.
- Released PVs with
Deletepolicy are deleted according to the policy. - Bound PVs or Released PVs with
Retainpolicy actively block LSO from removing the finalizer. An event is sent to tell users which PV blocks the deletion.- For Bound PVs, user should delete the corresponding PVC to get the PV Released.
- For Released PVs with reclaim policy "Retain", user should back up any important data from the volume and delete the PV.
There was a problem hiding this comment.
I reworded this section.
| c. If there are unbound PVs with reclaimPolicy of `Delete` then finalizer should not be removed until all such PVs are deleted. | ||
|
|
||
| 2. If `LocalVolume` or `LocalVolumeSet` object can not be deleted because of #1.a or #1.b - then appropriate event should be logged and user should be informed. LSO is not going to perform any cleanup of bound PVs or PVs with `Retain` reclaim policy. | ||
| 3. Before we can remove finalizer from LSO managed CRs, we should ensure deletion of created unbound PVs. To do that: |
There was a problem hiding this comment.
This should be probably more separated from the points above, maybe a sub-chapter / paragraph how to safely remove a PV without races?
There was a problem hiding this comment.
divided this into sections.
|
|
||
| ## Drawbacks | ||
|
|
||
| One drawback is - if user wants to keep one or more PVs around (in case they have data on it) and still delete the `LocalVolume` or `LocalVolumeSet` object - this design will not allow such flow. Also this change would result in data being wiped off from previously created PVs, which could surprise some users. |
There was a problem hiding this comment.
User can use Retain policy on PV and force-remove the LocalVolume.
gnufied
force-pushed
the
cleanup-lso-symlink
branch
from
a59c340
to
e72ae54
Compare
gnufied
force-pushed
the
cleanup-lso-symlink
branch
from
e72ae54
to
bf0c0ca
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsafrane The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
cc @openshift/sig-storage