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

pb-4093 Deleting the volumesnapshotcontent in the case of failed native CSI backup. #1460

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

siva-portworx
Copy link
Contributor

@siva-portworx siva-portworx commented Jul 25, 2023

What type of PR is this?

Uncomment only one and also add the corresponding label in the PR:
bug

What this PR does / why we need it:

     pb-4093 Deleting the volumesnapshotcontent in the case of failed native
    CSI backup

            - Adding the vsc name in the map in CleanupBackupResources with
              out any status check ( success/failure/in-progress)
            - In SnapshotStatus() also removed the check for ready state to
              add the volumesnapshotcontent name in the snapshotInfo. If the status
              volumesnapshotcontent is present, we will add the VSC name in the snapshotInfo.

Does this PR change a user-facing CRD or CLI?:
no

Is a release note needed?:

Issue: The volumesnapshotcontent are not getting deleted when the native CSI backup fails with the timeout error.
User Impact: The volumesnapshotcontent will get accumulated in the case of native CSI backup failures
Resolution: Taking care of deleting the volumesnapshotcontent in the case of failure as well.

Does this change need to be cherry-picked to a release branch?:
Yes.
23.7 branch and 23.7.1 branch.

Testing:
Vaidated the fix on IBM setup with timeout error.

@cnbu-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

CSI backup

	- Adding the vsc name in the map in CleanupBackupResources with
	  out any status check ( success/failure/in-progress)
	- In SnapshotStatus() also removed the check for ready state to
	  add the volumesnapshotcontent name in the snapshotInfo. If the status
	  volumesnapshotcontent is present, we will add the VSC name in the snapshotInfo.
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.07% ⚠️

Comparison is base (a2a5e83) 66.68% compared to head (9eb63e6) 66.61%.

❗ Current head 9eb63e6 differs from pull request most recent head 975f981. Consider uploading reports for the commit 975f981 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1460      +/-   ##
==========================================
- Coverage   66.68%   66.61%   -0.07%     
==========================================
  Files          43       43              
  Lines        4991     4991              
==========================================
- Hits         3328     3325       -3     
- Misses       1339     1341       +2     
- Partials      324      325       +1     

see 1 file with indirect coverage changes

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

Copy link
Contributor

@diptiranjanpx diptiranjanpx left a comment

Choose a reason for hiding this comment

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

Is similar change required in px-backup delete path also?

@siva-portworx
Copy link
Contributor Author

Is similar change required in px-backup delete path also?

In the case of delete path, if the snapshot itself fails, we will not have snapshot.json. Then we will not apply any VS or VSC.
In the case of success, it work as it is.

@diptiranjanpx
Copy link
Contributor

Is similar change required in px-backup delete path also?

In the case of delete path, if the snapshot itself fails, we will not have snapshot.json. Then we will not apply any VS or VSC. In the case of success, it work as it is.

In case the backup was successful, but during delete the volumesnapshotcontent did not become ready, are we going to delete the applied VS and VSC CRs even though they are in failed condition? If yes, then it looks fine.

@siva-portworx
Copy link
Contributor Author

Merge the PR as Functional test is blocking. Will track, it if we see any failure.

@siva-portworx siva-portworx merged commit fd94ef7 into master Jul 25, 2023
2 checks passed
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.

4 participants