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

Add support for backup PVC configuration #8109

Merged

Conversation

shubham-pampattiwar
Copy link
Collaborator

Thank you for contributing to Velero!

Please add a summary of your change

Implementation for Design #7982

Does your change fix a particular issue?

Fixes #7747 and #7700

Functional testing pending

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 58.82353% with 7 lines in your changes missing coverage. Please review.

Project coverage is 59.01%. Comparing base (07c03a8) to head (8eac360).
Report is 4 commits behind head on main.

Files Patch % Lines
pkg/cmd/cli/nodeagent/server.go 0.00% 4 Missing ⚠️
pkg/exposer/csi_snapshot.go 72.72% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8109      +/-   ##
==========================================
- Coverage   59.03%   59.01%   -0.03%     
==========================================
  Files         358      358              
  Lines       30141    30154      +13     
==========================================
+ Hits        17795    17796       +1     
- Misses      10907    10917      +10     
- Partials     1439     1441       +2     

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

Signed-off-by: Shubham Pampattiwar <[email protected]>

add changelog file

Signed-off-by: Shubham Pampattiwar <[email protected]>

make update

Signed-off-by: Shubham Pampattiwar <[email protected]>

pass backupPVCConfig to exposer as part of csi params

Signed-off-by: Shubham Pampattiwar <[email protected]>
@Lyndon-Li Lyndon-Li merged commit 4e781d4 into vmware-tanzu:main Aug 15, 2024
43 of 45 checks passed
@Lyndon-Li
Copy link
Contributor

@shubham-pampattiwar
I just realized one problem after this PR is merged:

	backupPVCStorageClass := csiExposeParam.StorageClass
	backupPVCReadOnly := false

	if value, exists := csiExposeParam.BackupPVCConfig[csiExposeParam.StorageClass]; exists {
		backupPVCStorageClass = value.StorageClass
		backupPVCReadOnly = value.ReadOnly
	}

Should be modified to:

	backupPVCStorageClass := csiExposeParam.StorageClass
	backupPVCReadOnly := false

	if value, exists := csiExposeParam.BackupPVCConfig[csiExposeParam.StorageClass]; exists {
		if value.StorageClass != "" {
			backupPVCStorageClass = value.StorageClass
		}

		backupPVCReadOnly = value.ReadOnly
	}

value.StorageClass may be empty from the configuration (it is not a must-have field), in this case, we should use the value from csiExposeParam.StorageClass, otherwise, the provision will fail.

I add this change to PR #8115. Just for your information.

@shubham-pampattiwar
Copy link
Collaborator Author

Thank you @Lyndon-Li !

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.

Allow setting the /spec/accessModes of a PVC created for the CSI snapshot data movement
3 participants