Add design for backup repository configurations #7963

Lyndon-Li · 2024-07-02T06:16:54Z

Add design for backup repository configurations for issue #7620, #7301

Signed-off-by: Lyndon-Li <[email protected]>

codecov · 2024-07-02T06:26:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.82%. Comparing base (9c20b5c) to head (c01c679).
Report is 49 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7963      +/-   ##
==========================================
+ Coverage   58.79%   58.82%   +0.02%     
==========================================
  Files         345      346       +1     
  Lines       28766    28892     +126     
==========================================
+ Hits        16914    16995      +81     
- Misses      10423    10468      +45     
  Partials     1429     1429

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

mpryc

In general I do like the design.

design/backup-repo-config.md

shubham-pampattiwar · 2024-07-11T06:58:55Z

design/backup-repo-config.md

+The BackupRepository configMap is created by users. If the configMap is not there, nothing is specified to the backup repository CR, so the Unified Repo modules use the hard-coded values to configure the backup repository.  
+The BackupRepository configMap is backup repository type specific, that is, for each type of backup repository, there is one configMap. Therefore, a ```backup-repository-config``` label should be applied to the configMap with the value of the repository's type. During the backup repository creation, the configMap is searched by the label against the repository type, if no configMap is found, nothing is set to the backup repository CR.  
+
+### Configurations


Should we also document the list of parameters that are configurable and the ones that are hard coded for kopia repo ?

cache-limit-mb and enable-compression are the supported parameters.
There are no other parameters that should be exposed to users, so they are not relevant to this design. But we can find all the parameters for kopia from the code at https://github.com/vmware-tanzu/velero/blob/main/pkg/repository/udmrepo/kopialib/backend/common.go

design/backup-repo-config.md

Lyndon-Li · 2024-07-16T11:40:17Z

@shubham-pampattiwar Made some slight changes, please review again. Thanks.

shubham-pampattiwar

/lgtm

mpryc · 2024-07-18T18:34:51Z

design/backup-repo-config.md

+  repoConfig: |
+    {
+      "cacheLimitMB": 2048,
+      "enableCompression": true


I also think it's worth considering allowing other repo configuration options to be configurable via this design. Namely thinking of different kopia repo options e.g.:
https://github.com/wawa0210/velero/blob/89c10ddcc04e4dc7354f284434c2ddb8c121780c/pkg/repository/udmrepo/repo_options.go#L63-L65

@weshayutin ^^

Let's do this incrementally in future based on the requirements.
For now, the 3 options you mentioned are too advanced(especially for the hash algorithm and chunking algorithm, without a good knowledge of the deduplication and the status of their own data, users cannot decide a better value than the default ones) and not general enough in terms of unified repository concepts(many backup repositories/storages are using private chunking/hashing algorithms and don't allow users to change, because those have heavy impacts on the QOS of the repo).

Lyndon-Li · 2024-07-22T10:51:54Z

@mpryc @shubham-pampattiwar @blackpiglet @reasonerjt
After some discussions, we made below changes to this design:

Move configurations for all repo types into one configMap. Previously one configMap for one repo type
Use name to find the configMap and require the name to be explicitly set to velero server parameters. Previously use label to find the configMap and don't require the label to be explicitly set to velero
Add a installation flag to set the velero server parameter

Please have further review. Thanks.

shubham-pampattiwar · 2024-07-22T16:12:33Z

@Lyndon-Li I am fine with either of the approaches. Would like to know why the change in approach though ?

Lyndon-Li · 2024-07-23T02:13:02Z

@Lyndon-Li I am fine with either of the approaches. Would like to why the change in approach though ?

After some discussion, we would like to unify the way to set configurations through configMap (including node-agent configs, backup repository configs and maintenance job configs) to velero:

The configMap should be always searched by name
The configMap name should be set to velero server/node-agent at start time, though users may be allowed to change the content of the configMap later. Otherwise, the configMap will be ignored

The would help the downstream users (which integrate velero) to better control velero's behavior in a consistent way. cc @reasonerjt

design/backup-repo-config.md

reasonerjt · 2024-07-23T07:59:20Z

design/backup-repo-config.md

+To create the configMap, users need to save something like the above sample to a file and then run below commands:  
+```
+kubectl create cm <config-name> -n velero --from-file=<json file name>
+kubectl label cm <config-name> -n velero backup-repository-config=<repository type>


Is the step of adding label still needed?

No, it is not required, I have removed it.

reasonerjt · 2024-07-23T08:01:23Z

design/backup-repo-config.md

+## Installation
+In order to set the ```--backup-repository-config``` parameter to the velero server, a new installation parameter ```--backup-repository-config``` will be added. An example of the installation as below:
+```
+velero install --backup-repository-config <configmap-name>


This could be optional b/c we can hard code a configmap name and pass it to the velero server, and the user will only need to create the configmap with the hard-coded name.

If we took this approach ^ we may not need to update the installation flow, bearing in mind that when velero is installed it's very possible the namespace of velero is not created.

We will remove this installation flag and keep the velero server flag.
By default installation using velero CLI, the server flag is not set; users need to modify the deployment to set the flag and wait velero server to restart.

reasonerjt · 2024-07-23T08:05:55Z

design/backup-repo-config.md

+
+Below are the struct for the configMap:  
+``` golang
+type RepoConfig struct {


I wanna clarify, it's the RepoConfig will be populated to the CR right? If that's the case, should the field in the backup repo CR be singular (repositoryConfig) or plural (repositoryConfigs)? Singular may be slightly better?

Changed the field in the CRD to repositoryConfig

Signed-off-by: Lyndon-Li <[email protected]>

kaovilai · 2024-07-23T13:59:20Z

design/backup-repo-config.md

+The BackupRepository configMap is created by users in velero installation namespace. The configMap name must be specified in the velero server parameter ```--backup-repository-config```, otherwise, it won't effect.  
+If the configMap name is specified but the configMap doesn't exist by the time of a backup repository is created, the configMap name is ignored.  
+For any reason, if the configMap doesn't effect, nothing is specified to the backup repository CR, so the Unified Repo modules use the hard-coded values to configure the backup repository.  


Will velero be watching for changes to CM? or will it only parse the value at startup? Or per backup?

I think its at startup and per backup, @Lyndon-Li lets clarify that

When a backupRepository CR is created, this CM is visited. So Velero doesn't watch the changes to the CM, but users are allowed to change the content during runtime, the changes will effect for the next created backupRepository. See below statement in the design:
When the backup repository CR is created by the BackupRepository controller, the configurations in the configMap are copied to the ```repositoryConfig``` field.

issue 7620: design for backup repo configurations

3fa8f6c

Signed-off-by: Lyndon-Li <[email protected]>

github-actions bot added the Area/Design Design Documents label Jul 2, 2024

Lyndon-Li force-pushed the issue-fix-7620-design branch from 6d62803 to 028e221 Compare July 2, 2024 06:19

github-actions bot added the has-changelog label Jul 2, 2024

Lyndon-Li marked this pull request as ready for review July 2, 2024 06:28

github-actions bot assigned Lyndon-Li Jul 2, 2024

github-actions bot requested review from shubham-pampattiwar and ywk253100 July 2, 2024 06:28

Lyndon-Li requested review from reasonerjt, blackpiglet and sseago July 2, 2024 06:29

Lyndon-Li force-pushed the issue-fix-7620-design branch from 028e221 to 21f34db Compare July 3, 2024 02:51

mpryc reviewed Jul 9, 2024

View reviewed changes

design/backup-repo-config.md Show resolved Hide resolved

design/backup-repo-config.md Outdated Show resolved Hide resolved

Lyndon-Li force-pushed the issue-fix-7620-design branch from 21f34db to f497a21 Compare July 11, 2024 02:36

shubham-pampattiwar reviewed Jul 11, 2024

View reviewed changes

mpryc reviewed Jul 11, 2024

View reviewed changes

design/backup-repo-config.md Outdated Show resolved Hide resolved

Lyndon-Li force-pushed the issue-fix-7620-design branch from f497a21 to 8b2e616 Compare July 11, 2024 08:06

shubham-pampattiwar previously approved these changes Jul 12, 2024

View reviewed changes

Lyndon-Li dismissed shubham-pampattiwar’s stale review via 399a2f9 July 16, 2024 11:13

Lyndon-Li force-pushed the issue-fix-7620-design branch 3 times, most recently from c81094d to 12f0c66 Compare July 16, 2024 11:39

Lyndon-Li requested a review from shubham-pampattiwar July 16, 2024 11:39

mpryc approved these changes Jul 16, 2024

View reviewed changes

shubham-pampattiwar previously approved these changes Jul 18, 2024

View reviewed changes

mpryc reviewed Jul 18, 2024

View reviewed changes

blackpiglet previously approved these changes Jul 19, 2024

View reviewed changes

Lyndon-Li dismissed blackpiglet’s stale review via f208b0d July 22, 2024 10:43

Lyndon-Li dismissed shubham-pampattiwar’s stale review via f208b0d July 22, 2024 10:43

Lyndon-Li force-pushed the issue-fix-7620-design branch 3 times, most recently from 52d0425 to dd29985 Compare July 22, 2024 10:47

blackpiglet previously approved these changes Jul 22, 2024

View reviewed changes

reasonerjt reviewed Jul 23, 2024

View reviewed changes

issue 7620: add design for backup repository configurations

c01c679

Signed-off-by: Lyndon-Li <[email protected]>

Lyndon-Li dismissed blackpiglet’s stale review via c01c679 July 23, 2024 10:27

Lyndon-Li force-pushed the issue-fix-7620-design branch from dd29985 to c01c679 Compare July 23, 2024 10:27

Lyndon-Li requested a review from reasonerjt July 23, 2024 10:34

kaovilai reviewed Jul 23, 2024

View reviewed changes

shubham-pampattiwar approved these changes Jul 24, 2024

View reviewed changes

blackpiglet approved these changes Jul 25, 2024

View reviewed changes

reasonerjt merged commit d9ca147 into vmware-tanzu:main Jul 26, 2024
66 checks passed

Lyndon-Li mentioned this pull request Aug 1, 2024

Does not support containers running with random uid #8067

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add design for backup repository configurations #7963

Add design for backup repository configurations #7963

Lyndon-Li commented Jul 2, 2024

codecov bot commented Jul 2, 2024 •

edited

Loading

mpryc left a comment

shubham-pampattiwar Jul 11, 2024

Lyndon-Li Jul 11, 2024

Lyndon-Li commented Jul 16, 2024

shubham-pampattiwar left a comment

mpryc Jul 18, 2024

mpryc Jul 18, 2024

Lyndon-Li Jul 19, 2024

Lyndon-Li commented Jul 22, 2024 •

edited

Loading

shubham-pampattiwar commented Jul 22, 2024 •

edited

Loading

Lyndon-Li commented Jul 23, 2024

reasonerjt Jul 23, 2024

Lyndon-Li Jul 23, 2024

reasonerjt Jul 23, 2024

Lyndon-Li Jul 23, 2024

reasonerjt Jul 23, 2024 •

edited

Loading

Lyndon-Li Jul 23, 2024

kaovilai Jul 23, 2024

shubham-pampattiwar Jul 23, 2024

Lyndon-Li Jul 24, 2024

Add design for backup repository configurations #7963

Add design for backup repository configurations #7963

Conversation

Lyndon-Li commented Jul 2, 2024

codecov bot commented Jul 2, 2024 • edited Loading

Codecov Report

mpryc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lyndon-Li commented Jul 16, 2024

shubham-pampattiwar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lyndon-Li commented Jul 22, 2024 • edited Loading

shubham-pampattiwar commented Jul 22, 2024 • edited Loading

Lyndon-Li commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reasonerjt Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 2, 2024 •

edited

Loading

Lyndon-Li commented Jul 22, 2024 •

edited

Loading

shubham-pampattiwar commented Jul 22, 2024 •

edited

Loading

reasonerjt Jul 23, 2024 •

edited

Loading