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

Issue 7027: backup exposer -- don't assume first volume as the backup volume #7038

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

Lyndon-Li
Copy link
Contributor

Fix issue #7027, data mover backup exposer should not assume the first volume as the backup volume in backup pod

@Lyndon-Li Lyndon-Li marked this pull request as ready for review October 31, 2023 04:14
@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #7038 (8e44240) into main (03e582c) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #7038      +/-   ##
==========================================
+ Coverage   61.03%   61.12%   +0.08%     
==========================================
  Files         252      252              
  Lines       26900    26910      +10     
==========================================
+ Hits        16419    16449      +30     
+ Misses       9319     9297      -22     
- Partials     1162     1164       +2     
Files Coverage Δ
pkg/exposer/csi_snapshot.go 90.88% <100.00%> (+9.38%) ⬆️

... and 1 file with indirect coverage changes

return nil, errors.Errorf("backup pod %s doesn't have the expected backup volume", pod.Name)
}

curLog.WithField("pod", pod.Name).Infof("Backup volume is found in pod at index %v", i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be debug log ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is fine to be an info log, as same as other logs in the same function, because expose is not a frequently called operation and on the other hand, it is hard to troubleshoot, so we would rather add some info logs, so that they could give helps whenever we need to debug the expose process.

@@ -204,6 +204,7 @@ func (e *csiSnapshotExposer) GetExposed(ctx context.Context, ownerObject corev1.

backupPodName := ownerObject.Name
backupPVCName := ownerObject.Name
volumeName := string(ownerObject.UID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the volumeName is equal to ownerObject.UID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we name the volume as ownerObject.UID when creating the backup pod, so it is always true that the backup volume is with the name of ownerObject.UID in the backup pod.
See the createBackupPod function for how the backup pod is created.

@qiuming-best qiuming-best merged commit e17751f into vmware-tanzu:main Nov 1, 2023
27 checks passed
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.

4 participants