feat: support unclear stale pvc #167

njuptlzf · 2023-10-09T03:49:39Z

Pull Request template

Why is this PR required? What issue does it fix?:
fix: #166

What this PR does?:
Provide a switch to turn off the function of clearing stale pvc to avoid being cleared by mistake after restoring stale pvc.

Does this PR require any upgrade changes?:
If you need to turn off the clear stale pvc function, you need to provide environment variables (enabled by default)

        # The NFSGarbageCollectionEnable environment variable is the switch for the garbage collector.(default true)
        - name: OPENEBS_IO_NFS_SERVER_GARBAGE_COLLECTION_ENABLED
          value: "false"

If the changes in this PR are manually verified, list down the scenarios covered::

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

Fixes stale nfs-pvc will be automatically deleted, causing backup recovery to fail. #166
PR Title follows the convention of <type>(<scope>): <subject>
Has the change log section been updated?
Commit has unit tests
Commit has integration tests
(Optional) Does this PR change require updating NFS-Provisioner Chart? If yes, mention the Helm Chart PR #
(Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
(Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

PLEASE REMOVE BELOW INFORMATION BEFORE SUBMITTING

The PR title message must follow convention:
<type>(<scope>): <subject>.

Where:
Most common types are:
* feat - for new features, not a new feature for build script
* fix - for bug fixes or improvements, not a fix for build script
* chore - changes not related to production code
* docs - changes related to documentation
* style - formatting, missing semi colons, linting fix etc; no significant production code changes
* test - adding missing tests, refactoring tests; no production code change
* refactor - refactoring production code, eg. renaming a variable or function name, there should not be any significant production code changes
* cherry-pick - if PR is merged in master branch and raised to release branch(like v0.4.x)

IMPORTANT: Please review the CONTRIBUTING.md file for detailed contributing guidelines.

Signed-off-by: Kasakaze <njuptlzf@163.com>

njuptlzf · 2023-10-09T04:16:06Z

1. My backup method

kubectl exec -n velero $(kubectl get po -n velero -l component=velero -oname | head -n 1) -it -- /velero backup create --include-namespaces=openebs --selector=nfs.openebs.io /nfs-pvc-name=nfs-pvc bk-pv-1

2. Create Disaster

# kubectl delete ns nginx-example

3. My way to restore

Restart nfs-provisioner pod to turn off stale pvc automatic cleaning

         - name: OPENEBS_IO_NFS_SERVER_GARBAGE_COLLECTION_ENABLED
           value: "false"

Restore the rwo volume first

kubectl exec -n velero $(kubectl get po -n velero -l component=velero -oname | head -n 1) -it -- /velero restore create --restore-volumes=true --from-backup bk-pv-1

With this step, I might even take a nap... : )
3. Deploy the app and create a new rwx volume (it will automatically create a new nfs-pvc pod, svc)

kubectl apply -f with-rxw-pvc.yaml

Modify the nfs service of openebs and change the selector to the old nfs-pvc name to use the old rwo volume
Restart the pod that mounts the nfs rwx volume to remount the rwo volume with data
Restart nfs-provisioner pod to enable automatic cleanup of stale pvc

         - name: OPENEBS_IO_NFS_SERVER_GARBAGE_COLLECTION_ENABLED
           value: "true"

njuptlzf · 2023-10-09T04:20:20Z

@niladrih ptal. : )

niladrih · 2023-11-08T06:17:38Z

Hi @njuptlzf, I can see a CI check failing, could you take a look at this?

lizhifengones · 2023-11-12T07:41:15Z

Hi @njuptlzf, I can see a CI check failing, could you take a look at this?

I think it may be that the my golang version (the golang version I use go.mod) is inconsistent with the ci golang version(Still can't find it by looking at the ci log), causing go fmt to fail. But I have executed it myself and there is no problem. I will continue to output the same content with ci and correct it.

ryshoooo · 2023-11-20T09:25:02Z

provisioner/env.go

@@ -58,6 +58,9 @@ const (
 	// NFSBackendPvcTimeout defines env name to store BackendPvcBoundTimeout value
 	NFSBackendPvcTimeout menv.ENVKey = "OPENEBS_IO_NFS_SERVER_BACKEND_PVC_TIMEOUT"

+	// NFSCleanUpStalePvcEnable defines env name to store the value of clean stale pvc enable
+	NFSCleanUpStalePvcEnable menv.ENVKey = "OPENEBS_IO_NFS_SERVER_CLEANUP_STALE_PVC_ENABLE"


This flag essentially enables/disables garbage collection. Even though the garbage collection currently removes stale PVCs, maybe the flag should be instead called OPENEBS_IO_NFS_SERVER_GARBAGE_COLLECTION_ENABLED? Then in case the logic of what the garbage collector does is changed, this flag won't be misleading :)

Even though the garbage collection currently removes stale PVCs, maybe the flag should be instead called OPENEBS_IO_NFS_SERVER_GARBAGE_COLLECTION_ENABLED?

sounds good

Then in case the logic of what the garbage collector does is changed, this flag won't be misleading

Yes, when the logic of cleanup is optimized, we actually do not need to turn off recycling. Currently, only for data recovery scenarios, completely turning off can solve my problem.

Signed-off-by: Kasakaze <njuptlzf@163.com>

njuptlzf · 2023-11-20T15:58:46Z

Sorry, I took too long to respond to this PR due to other things.
I just checked my local code. It was me debugging make test that caused the problem. It has been solved. @niladrih :)

Signed-off-by: Kasakaze <njuptlzf@163.com>

njuptlzf · 2023-11-20T16:34:33Z

@niladrih ptal. :)

codecov-commenter · 2023-11-22T05:46:27Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (5f0b16d) 50.17% compared to head (65cb5c8) 50.01%.
Report is 8 commits behind head on develop.

Files	Patch %	Lines
provisioner/provisioner.go	0.00%	9 Missing ⚠️
provisioner/env.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #167      +/-   ##
===========================================
- Coverage    50.17%   50.01%   -0.17%     
===========================================
  Files           38       38              
  Lines         2792     2801       +9     
===========================================
  Hits          1401     1401              
- Misses        1285     1294       +9     
  Partials       106      106

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

niladrih · 2023-11-22T05:57:09Z

@njuptlzf -- This file https://github.com/openebs/dynamic-nfs-provisioner/blob/develop/deploy/kubectl/openebs-nfs-provisioner.yaml needs to have to ENV in a commented state.

        env:
        #   OPENEBS_IO_K8S_MASTER enables openebs provisioner to connect to K8s
        #   based on this address. This is ignored if empty.
        #   This is supported for openebs provisioner version 0.5.2 onwards
        # - name: OPENEBS_IO_K8S_MASTER
        #   value: "http://10.128.0.12:8080"
        #   OPENEBS_IO_KUBE_CONFIG enables openebs provisioner to connect to K8s
        #   based on this config. This is ignored if empty.
        #   This is supported for openebs provisioner version 0.5.2 onwards
        # - name: OPENEBS_IO_KUBE_CONFIG
        #   value: "/home/ubuntu/.kube/config"
        #   OPENEBS_IO_NFS_SERVER_NODE_AFFINITY defines the node affinity rules to place NFS Server
        #   instance. It accepts affinity rules in multiple ways:
        #   - If NFS Server needs to be placed on storage nodes as well as only in
        #     zone-1 & zone-2 then value can be:
        #     value:  "kubernetes.io/zone:[zone-1,zone-2],kubernetes.io/storage-node".
        #   - If NFS Server needs to be placed only on storage nodes & nfs nodes then
        #     value can be:
        #     value:  "kubernetes.io/storage-node,kubernetes.io/nfs-node"
        # - name: OPENEBS_IO_NFS_SERVER_NODE_AFFINITY
        #   value: "kubernetes.io/storage-node,kubernetes.io/nfs-node"
        #
        #   Provide a switch to turn off the function of clearing stale pvc to avoid
        #.  garbage collecting an NFS backend PVC if the NFS PVC is deleted.
        # - name: OPENEBS_IO_NFS_SERVER_GARBAGE_COLLECTION_ENABLED
        #.  value: false

You may want to add the change in the helm chart as well, but that needs to be in a separate PR. We merge all helm chart changes at release-time, and I'd be able to pull in your change if you raise a commit with the helm chart change separately.

Unique use-case! Thank you for the contribution.

njuptlzf · 2023-11-22T06:14:36Z

@njuptlzf -- This file https://github.com/openebs/dynamic-nfs-provisioner/blob/develop/deploy/kubectl/openebs-nfs-provisioner.yaml needs to have to ENV in a commented state.

        env:
        #   OPENEBS_IO_K8S_MASTER enables openebs provisioner to connect to K8s
        #   based on this address. This is ignored if empty.
        #   This is supported for openebs provisioner version 0.5.2 onwards
        # - name: OPENEBS_IO_K8S_MASTER
        #   value: "http://10.128.0.12:8080"
        #   OPENEBS_IO_KUBE_CONFIG enables openebs provisioner to connect to K8s
        #   based on this config. This is ignored if empty.
        #   This is supported for openebs provisioner version 0.5.2 onwards
        # - name: OPENEBS_IO_KUBE_CONFIG
        #   value: "/home/ubuntu/.kube/config"
        #   OPENEBS_IO_NFS_SERVER_NODE_AFFINITY defines the node affinity rules to place NFS Server
        #   instance. It accepts affinity rules in multiple ways:
        #   - If NFS Server needs to be placed on storage nodes as well as only in
        #     zone-1 & zone-2 then value can be:
        #     value:  "kubernetes.io/zone:[zone-1,zone-2],kubernetes.io/storage-node".
        #   - If NFS Server needs to be placed only on storage nodes & nfs nodes then
        #     value can be:
        #     value:  "kubernetes.io/storage-node,kubernetes.io/nfs-node"
        # - name: OPENEBS_IO_NFS_SERVER_NODE_AFFINITY
        #   value: "kubernetes.io/storage-node,kubernetes.io/nfs-node"
        #
        #   Provide a switch to turn off the function of clearing stale pvc to avoid
        #.  garbage collecting an NFS backend PVC if the NFS PVC is deleted.
        # - name: OPENEBS_IO_NFS_SERVER_GARBAGE_COLLECTION_ENABLED
        #.  value: false

You may want to add the change in the helm chart as well, but that needs to be in a separate PR. We merge all helm chart changes at release-time, and I'd be able to pull in your change if you raise a commit with the helm chart change separately.

Unique use-case! Thank you for the contribution.

@niladrih A must! I will take a moment to introduce this ENV through another PR after testing.
Also, are there any other files besides helm that need to be modified?

Signed-off-by: Kasakaze <njuptlzf@163.com>

niladrih · 2023-11-22T06:52:38Z

@njuptlzf -- No. The helm chart is all that is left functionally. But, I'd invite you add a test in tests directory (ginkgo test), and add a .md file in the docs/tutorials directory explaining how this feature works.

njuptlzf · 2023-12-04T03:07:54Z

@njuptlzf -- No. The helm chart is all that is left functionally. But, I'd invite you add a test in tests directory (ginkgo test), and add a .md file in the docs/tutorials directory explaining how this feature works.

@niladrih -- Sorry, I missed it again, I will finish the helm and e2e testing within this week.

…e#167 Signed-off-by: Kasakaze <njuptlzf@163.com>

…e#167 Signed-off-by: Kasakaze <njuptlzf@163.com> Signed-off-by: Niladri Halder <niladri.halder26@gmail.com>

Signed-off-by: Niladri Halder <niladri.halder26@gmail.com>

@njuptlzf

…action (#180) * chore: add @njuptlzf's change (PR #167) to the changelog Signed-off-by: Niladri Halder <niladri.halder26@gmail.com> * fix(ci): replace crazy-max/ghaction-docker meta with docker/metadata-action Signed-off-by: Niladri Halder <niladri.halder26@gmail.com> --------- Signed-off-by: Niladri Halder <niladri.halder26@gmail.com>

…e#167 Signed-off-by: Kasakaze <njuptlzf@163.com> Signed-off-by: Niladri Halder <niladri.halder26@gmail.com>

* fix(ci): update helm/chart-testing-action to v2.6.1 Signed-off-by: Niladri Halder <niladri.halder26@gmail.com> * feat(helm): add gcEnable configuration to chart for PR #167 Signed-off-by: Kasakaze <njuptlzf@163.com> Signed-off-by: Niladri Halder <niladri.halder26@gmail.com> * charts: change helm value enableGarbageCollection to a bool Signed-off-by: Niladri Halder <niladri.halder26@gmail.com> --------- Signed-off-by: Niladri Halder <niladri.halder26@gmail.com> Signed-off-by: Kasakaze <njuptlzf@163.com> Co-authored-by: Kasakaze <njuptlzf@163.com>

feat: support unclear stale pvc

e99bc67

Signed-off-by: Kasakaze <njuptlzf@163.com>

njuptlzf force-pushed the cleanup_stale_pvc_enable branch from 199a938 to e99bc67 Compare October 9, 2023 03:58

ryshoooo reviewed Nov 20, 2023

View reviewed changes

make test

02840d0

Signed-off-by: Kasakaze <njuptlzf@163.com>

njuptlzf force-pushed the cleanup_stale_pvc_enable branch from 26ae9f1 to 02840d0 Compare November 20, 2023 15:54

njuptlzf requested a review from ryshoooo November 20, 2023 16:17

update: change CLEANUP_STALE_PVC_ENABLE to GARBAGE_COLLECTION_ENABLED

65cb5c8

Signed-off-by: Kasakaze <njuptlzf@163.com>

ryshoooo approved these changes Nov 20, 2023

View reviewed changes

niladrih self-requested a review November 22, 2023 05:50

update: add env comments to kubectl deploy

43943fa

Signed-off-by: Kasakaze <njuptlzf@163.com>

niladrih approved these changes Nov 29, 2023

View reviewed changes

niladrih merged commit ec6a0f9 into openebs-archive:develop Nov 29, 2023
7 checks passed

njuptlzf added a commit to njuptlzf/dynamic-nfs-provisioner that referenced this pull request Dec 7, 2023

feat(helm): add gcEnable configuration to chart for PR openebs-archiv…

3869f6b

…e#167 Signed-off-by: Kasakaze <njuptlzf@163.com>

njuptlzf mentioned this pull request Dec 7, 2023

feat(helm): add gcEnable configuration to chart for PR #167 #177

Closed

niladrih mentioned this pull request Dec 8, 2023

feat(chart): release changes for v0.11.0 #178

Merged

niladrih added a commit to niladrih/dynamic-nfs-provisioner that referenced this pull request Dec 8, 2023

chore: add @njuptlzf's change (PR openebs-archive#167) to the changelog

fe78383

Signed-off-by: Niladri Halder <niladri.halder26@gmail.com>

niladrih added a commit to niladrih/dynamic-nfs-provisioner that referenced this pull request Dec 8, 2023

chore: add @njuptlzf's change (PR openebs-archive#167) to the changelog

98127a4

Signed-off-by: Niladri Halder <niladri.halder26@gmail.com>

niladrih added a commit to niladrih/dynamic-nfs-provisioner that referenced this pull request Dec 8, 2023

chore: add @njuptlzf's change (PR openebs-archive#167) to the changelog

b8eadbf

Signed-off-by: Niladri Halder <niladri.halder26@gmail.com>

njuptlzf deleted the cleanup_stale_pvc_enable branch December 16, 2023 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support unclear stale pvc #167

feat: support unclear stale pvc #167

njuptlzf commented Oct 9, 2023 •

edited

njuptlzf commented Oct 9, 2023 •

edited

njuptlzf commented Oct 9, 2023

niladrih commented Nov 8, 2023

lizhifengones commented Nov 12, 2023

ryshoooo Nov 20, 2023

njuptlzf Nov 20, 2023 •

edited

njuptlzf commented Nov 20, 2023 •

edited

njuptlzf commented Nov 20, 2023

codecov-commenter commented Nov 22, 2023

niladrih commented Nov 22, 2023

njuptlzf commented Nov 22, 2023 •

edited

niladrih commented Nov 22, 2023

njuptlzf commented Dec 4, 2023 •

edited

feat: support unclear stale pvc #167

feat: support unclear stale pvc #167

Conversation

njuptlzf commented Oct 9, 2023 • edited

Pull Request template

njuptlzf commented Oct 9, 2023 • edited

njuptlzf commented Oct 9, 2023

niladrih commented Nov 8, 2023

lizhifengones commented Nov 12, 2023

ryshoooo Nov 20, 2023

Choose a reason for hiding this comment

njuptlzf Nov 20, 2023 • edited

Choose a reason for hiding this comment

njuptlzf commented Nov 20, 2023 • edited

njuptlzf commented Nov 20, 2023

codecov-commenter commented Nov 22, 2023

Codecov Report

niladrih commented Nov 22, 2023

njuptlzf commented Nov 22, 2023 • edited

niladrih commented Nov 22, 2023

njuptlzf commented Dec 4, 2023 • edited

njuptlzf commented Oct 9, 2023 •

edited

njuptlzf commented Oct 9, 2023 •

edited

njuptlzf Nov 20, 2023 •

edited

njuptlzf commented Nov 20, 2023 •

edited

njuptlzf commented Nov 22, 2023 •

edited

njuptlzf commented Dec 4, 2023 •

edited