Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(garbage-collector): garbage-collector to remove stale NFS resouces #80

Merged
merged 7 commits into from
Aug 9, 2021

Conversation

mynktl
Copy link
Contributor

@mynktl mynktl commented Jul 28, 2021

Signed-off-by: mayank [email protected]

Why is this PR required? What issue does it fix?:
This PR fixes #79.

What this PR does?:

  • This PR adds garbage collector to remove stale nfs resources. Garbage collector runs at an interval of 5 minutes and checks the PVC(backend PVC) created by the nfs-provisioner in the nfs-server namespace. Garbage collector process the PVC from nfs-server namespace having a label "openebs.io/cas-type=nfs-kernel".
    To map the backend PVC to NFS PVC(created by a user to provision NFS PV), provisioner adds the following labels to the backend PVC.

    • nfs.openebs.io/nfs-pvc-name:
    • nfs.openebs.io/nfs-pvc-namespace:
    • nfs.openebs.io/nfs-pvc-uid:

    If backend PVC is missing any label from the above list, then the garbage collector will not process that backend PVC.

    If relevant NFS PVC(nfs-pvc-ns/nfs-pvc-name) exists in cluster and having UID value or, garbage collector couldn't find
    the relevant NFS PVC due to network/permission issues, then garbage collector will not perform clean-up for that backend PVC.

    If relevant NFS PVC doesn't exist for the backend PVC then the garbage collector will check for the relevant NFS PV resource to process the backend PVC.
    If NFS PV resource doesn't exist then the garbage collector will remove the resources created for NFS PV, i.e Service, Deployment, backend PVC.

    If NFS PV exists for the backend PVC then the garbage collector will not remove the resources for that PV.

  • This PR also modifies the NFS-Provisioner provisioning behavior. While provisioning, the provisioner will wait for 1 minute to bound backend PVC(created by provisioner for NFS PV). If backend PVC is not bound within 1 minute period then provisioner will return an error timed out waiting for PVC{ns/name} to bound.

Does this PR require any upgrade changes?: No

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:

mayank added 3 commits July 28, 2021 18:57
…ces.

Garbage collector runs at an interval of 5 minutes and checks the PVC(backend PVC) created by the nfs-provisioner in the nfs-server namespace.
Garbage collector process the PVC from nfs-server namespace having a label "openebs.io/cas-type=nfs-kernel".

To map the backend PVC to NFS PVC(created by a user to provision NFS PV), provisioner adds the following labels to the backend PVC.
- nfs.openebs.io/nfs-pvc-name: <nfs-pvc-name>
- nfs.openebs.io/nfs-pvc-namespace: <nfs-pvc-ns>
- nfs.openebs.io/nfs-pvc-uid: <nfs-pvc-uid>

If backend PVC is missing any label from the above list, then the garbage collector will not process that backend PVC.

If relevant NFS PVC(nfs-pvc-ns/nfs-pvc-name) exists in cluster and having UID value <nfs-pvc-uid> or, garbage collector couldn't find
the relevant NFS PVC due to network/permission issues, then garbage collector will not perform clean-up for that backend PVC.

If relevant NFS PVC doesn't exist for the backend PVC then the garbage collector will check for the relevant NFS PV resource to process the backend PVC.
If NFS PV resource doesn't exist then the garbage collector will remove the resources created for NFS PV, i.e Service, Deployment, backend PVC.

If NFS PV exists for the backend PVC then the garbage collector will not remove the resources for that PV.

--

This commit also modifies the NFS-Provisioner provisioning behavior. While provisioning, the provisioner will wait for 1 minute
to bound backend PVC(created by provisioner for NFS PV). If backend PVC is not bound within 1 minute period then provisioner will
return an error "timed out waiting for PVC{ns/name} to bound".

Signed-off-by: mayank <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2021

Codecov Report

Merging #80 (617b550) into develop (d65ac1f) will increase coverage by 1.79%.
The diff coverage is 77.69%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #80      +/-   ##
===========================================
+ Coverage    45.19%   46.99%   +1.79%     
===========================================
  Files           27       29       +2     
  Lines         2239     2349     +110     
===========================================
+ Hits          1012     1104      +92     
- Misses        1157     1167      +10     
- Partials        70       78       +8     
Impacted Files Coverage Δ
provisioner/provisioner.go 0.00% <0.00%> (ø)
provisioner/provisioner_kernel_nfs_server.go 0.00% <0.00%> (ø)
provisioner/garbage_collector.go 80.00% <80.00%> (ø)
provisioner/helper_kernel_nfs_server.go 80.07% <86.36%> (+3.96%) ⬆️
provisioner/pvc_tracker.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d65ac1f...617b550. Read the comment docs.

mayank added 3 commits July 29, 2021 14:05
- TEST NFS PROVISIONER WITH INVALID BACKEND SC

  Old behaviour:
      - Create NFS PVC with NFS SC having invalid Backend SC
      - Wait for NFS PVC to bound
      - Check nfs-server pod should be in pending state
      - Check backend-pvc in pending state

  New behaviour:
      - Create NFS PVC with NFS SC having invalid Backend SC
      - Check NFS PVC is in pending state

Signed-off-by: mayank <[email protected]>
@mynktl mynktl marked this pull request as ready for review July 29, 2021 14:21
@mynktl mynktl requested review from mittachaitu and kmova July 29, 2021 14:21
Copy link
Contributor

@mittachaitu mittachaitu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided few comments... rest of changes looks good to me

provisioner/garbage_collector.go Outdated Show resolved Hide resolved
provisioner/garbage_collector_test.go Outdated Show resolved Hide resolved
provisioner/garbage_collector_test.go Show resolved Hide resolved
provisioner/helper_kernel_nfs_server.go Outdated Show resolved Hide resolved
provisioner/helper_kernel_nfs_server.go Outdated Show resolved Hide resolved
Comment on lines -104 to -105
When("verifying nfs-server state", func() {
It("should have nfs-server in pending state", func() {
Copy link
Contributor

@mittachaitu mittachaitu Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Even with the changes nfs-server deployment will be created right? Why do we need to remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to a timing issue. The same behavior is added in TEST GARBAGE COLLECTION OF NFS RESOURCES.

tests/provisioner_with_invalid_backend_sc.go Show resolved Hide resolved
Copy link
Contributor

@mittachaitu mittachaitu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes are good

@kmova kmova merged commit 185189b into openebs-archive:develop Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nfs-volume deletion is not idempotent leading to stale nfs server resources
4 participants