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

Delete temp pvc #1388

Merged
merged 5 commits into from
Sep 16, 2024
Merged

Conversation

tesshuflower
Copy link
Contributor

Describe what this PR does

  • New spec field cleanupTempDestinationPVC for ReplicationDestinations to allow deleting the temp/dynamically provisioned destination PVC at the end of a sync. Available for all movers (except syncthing which has no ReplicationDestination).
  • New spec field (for restic only) cleanupCachePVC for ReplicationDestinations to allow deleting the cache PVC at the end of a sync.

Is there anything that requires special attention?

  • Even though there's more code duplication, I did not put this field in the VolumePopulator so it's explicit when we call EnsureNewPVC() whether or not it should be marked as temporary or not.
  • Struggling again with good names for these fields, particularly cleanupTempDestinationPVC....I left "Temp" in the name to try to indicate that if you set destinationPVC then this would not be cleaned up. Considered cleanupDynamicDestinationPVC but seems long.
  • The wording of temp also gets a bit muddy, so I started referring it to a dynamically provisioned PVC in some places. Since normally we'd want to leave it around, but technically it is still "temporary" since we will clean it up when the ReplicationDestination is removed.

Related issues:
Fixes: #1158

Will cleanup dynamically provisioned dest pvc
at the end of a replicationdestination sync cycle

Also
- restic only - spec update to delete cache pvc
at the end of a replicationdestintation sync cycle

Signed-off-by: Tesshu Flower <tflower@redhat.com>
Signed-off-by: Tesshu Flower <tflower@redhat.com>
- also fix name for restic enableFileDeletion
parameter in docs

Signed-off-by: Tesshu Flower <tflower@redhat.com>
- allow for pvc termination to take a bit

Signed-off-by: Tesshu Flower <tflower@redhat.com>
@tesshuflower
Copy link
Contributor Author

/retest

@tesshuflower
Copy link
Contributor Author

/cc @JohnStrunk

Signed-off-by: Tesshu Flower <tflower@redhat.com>
@openshift-ci openshift-ci bot added size/XL and removed size/XXL labels Sep 16, 2024
Copy link

sonarcloud bot commented Sep 16, 2024

Copy link
Member

@JohnStrunk JohnStrunk left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

openshift-ci bot commented Sep 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JohnStrunk, tesshuflower

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:
  • OWNERS [JohnStrunk,tesshuflower]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.5%. Comparing base (e693289) to head (affccc3).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
controllers/mover/restic/mover.go 80.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1388     +/-   ##
=======================================
+ Coverage   67.3%   67.5%   +0.1%     
=======================================
  Files         57      57             
  Lines       5834    5841      +7     
=======================================
+ Hits        3931    3944     +13     
+ Misses      1615    1612      -3     
+ Partials     288     285      -3     
Files with missing lines Coverage Δ
controllers/mover/rclone/builder.go 90.1% <100.0%> (+0.1%) ⬆️
controllers/mover/rclone/mover.go 74.3% <100.0%> (ø)
controllers/mover/restic/builder.go 91.2% <100.0%> (+0.1%) ⬆️
controllers/mover/rsync/builder.go 91.6% <100.0%> (+<0.1%) ⬆️
controllers/mover/rsync/mover.go 77.9% <100.0%> (ø)
controllers/mover/rsynctls/builder.go 91.1% <100.0%> (+<0.1%) ⬆️
controllers/mover/rsynctls/mover.go 74.2% <100.0%> (ø)
controllers/mover/syncthing/mover.go 83.5% <100.0%> (ø)
controllers/volumehandler/volumehandler.go 56.3% <100.0%> (+0.2%) ⬆️
controllers/mover/restic/mover.go 82.6% <80.0%> (ø)

... and 1 file with indirect coverage changes

@openshift-merge-bot openshift-merge-bot bot merged commit e99212b into backube:main Sep 16, 2024
30 checks passed
@tesshuflower tesshuflower deleted the delete_temp_pvc branch September 16, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide option to cleanup temporary objects (PVCs) made by Restore Process
2 participants