Backup repo config #8093
Backup repo config #8093
Conversation
Signed-off-by: Lyndon-Li <[email protected]>
Lyndon-Li
force-pushed
the
backkup-repo-config
branch
from
c06739b
to
f859373
Compare
Lyndon-Li
force-pushed
the
backkup-repo-config
branch
from
f859373
to
ef4851a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8093 +/- ##
==========================================
- Coverage 58.93% 58.83% -0.10%
==========================================
Files 353 358 +5
Lines 29643 30123 +480
==========================================
+ Hits 17470 17723 +253
- Misses 10733 10951 +218
- Partials 1440 1449 +9 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Lyndon-Li <[email protected]>
Lyndon-Li
force-pushed
the
backkup-repo-config
branch
from
ef4851a
to
82d9fe4
Compare
| @@ -253,6 +254,8 @@ func NewCommand(f client.Factory) *cobra.Command { | |||
| command.Flags().StringVar(&config.maintenanceCfg.CPULimit, "maintenance-job-cpu-limit", config.maintenanceCfg.CPULimit, "CPU limit for maintenance job. Default is no limit.") | |||
| command.Flags().StringVar(&config.maintenanceCfg.MemLimit, "maintenance-job-mem-limit", config.maintenanceCfg.MemLimit, "Memory limit for maintenance job. Default is no limit.") | |||
|
|
|||
| command.Flags().StringVar(&config.backukpRepoConfig, "backup-repository-config", config.backukpRepoConfig, "The name of configMap containing backup repository configurations.") | |||
There was a problem hiding this comment.
Should we also specify the repo configurations currently supported and the default values if not set ?
There was a problem hiding this comment.
As you can see in the sample, the configuration is a structured data so it is self-explained. And also we will have a doc for backup repo config where all the supported values will be listed.
This backup repo config is not a enumeration/boolean value, we may have many and even complex-type values in future, so it is hard to explain all the supported values in this CLI helper.
| @@ -33,8 +33,7 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| maxDataCacheMB = 2000 | |||
| maxMetadataCacheMB = 2000 | |||
| defaultCacheLimitMB = 5000 | |||
There was a problem hiding this comment.
Curious about how we ended up with this default value ?
There was a problem hiding this comment.
No special reason, it is impossible to find a universal value which works for most of the cases, so this value doesn't have practical meanings.
| result[udmrepo.StoreOptionCacheLimit] = v | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Shouldn't we check for enableCompression config as well ?
There was a problem hiding this comment.
As our previous discussion, compression is not supported by Kopia repo in 1.15.
From the design whether/when/how a configuration is picked is decided by the specific repo, so here it is fine for the repo not touching the configuration if it doesn't support it.
We just want to support compression as a user case in the design.
However, for implementation, we may need some more values, e.g., what is the escape size in the case that it is a waste of time/resource to do compression for small files.
There was a problem hiding this comment.
Maybe it makes sense to enable/disable compression at a backup level rather than at the backup repository level, but yeah I get your point.
There was a problem hiding this comment.
Generally speaking, compression could be at backup level, but most often, it is at repository level.
Compression is eventually done by the repository, either client part or server part. Therefore, even if we have it on backup level, we need to pass the configuration all the way down to the repository. Otherwise, server side dedup & compression will be disabled.
While for Unified Repo concepts, repository with server side dedup & compression is supported, so we will probably have it at repository level only.
Anyway, this is out of the scope of the current PR, but a consideration point when we implement compression. Let see if we have other ideas then.
shubham-pampattiwar
merged commit b62b38f
into
vmware-tanzu:main
Fix issue #7620, add backup repository configuration implementation and support cacheLimit configuration for Kopia repo