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 repository maintenance job #7451

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

qiuming-best
Copy link
Contributor

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)
#7375

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 Feb 21, 2024

Codecov Report

Attention: Patch coverage is 58.78378% with 122 lines in your changes are missing coverage. Please review.

Project coverage is 61.74%. Comparing base (2518824) to head (8d63c76).
Report is 8 commits behind head on main.

Files Patch % Lines
pkg/repository/maintenance.go 67.90% 35 Missing and 17 partials ⚠️
pkg/repository/manager.go 2.27% 43 Missing ⚠️
pkg/controller/backup_repository_controller.go 26.31% 13 Missing and 1 partial ⚠️
pkg/cmd/server/server.go 57.14% 9 Missing ⚠️
pkg/util/velero/velero.go 92.85% 2 Missing ⚠️
pkg/datapath/file_system.go 0.00% 1 Missing ⚠️
pkg/repository/provider/unified_repo.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7451      +/-   ##
==========================================
+ Coverage   61.71%   61.74%   +0.02%     
==========================================
  Files         263      265       +2     
  Lines       28869    29257     +388     
==========================================
+ Hits        17816    18064     +248     
- Misses       9793     9910     +117     
- Partials     1260     1283      +23     

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

@qiuming-best qiuming-best force-pushed the maintenance-job branch 3 times, most recently from c8a8349 to f6bb743 Compare February 29, 2024 09:10
@qiuming-best qiuming-best force-pushed the maintenance-job branch 3 times, most recently from 5e4a396 to cea75bc Compare March 1, 2024 06:23
pkg/util/velero/velero.go Outdated Show resolved Hide resolved
@qiuming-best qiuming-best force-pushed the maintenance-job branch 2 times, most recently from ae9dbcf to bb571f8 Compare March 1, 2024 07:50
@qiuming-best qiuming-best marked this pull request as ready for review March 1, 2024 08:04
pkg/cmd/cli/install/install.go Show resolved Hide resolved
pkg/cmd/cli/repomantenance/maintenance.go Outdated Show resolved Hide resolved
pkg/cmd/cli/repomantenance/maintenance.go Outdated Show resolved Hide resolved
pkg/repository/manager.go Show resolved Hide resolved
pkg/cmd/cli/repomantenance/maintenance.go Outdated Show resolved Hide resolved
pkg/cmd/cli/repomantenance/maintenance.go Outdated Show resolved Hide resolved
return jobName
}

func buildMaintenanceJob(m MaintenanceConfig, param provider.RepoParam, cli client.Client, namespace string) (*batchv1.Job, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we copy the Velero pod directly and only remove or replace the specific needed fields (e.g. cmd)?
I raise this because the Velero pod may be configured in different ways to support kinds of scenarios, e.g. a label is added to it to support auth with Azure Workload Identity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed the label settings for the job, I added them now.
As some properties are also used by other features later such as data mover microservice, so I want these properties to be a series of common functions.

@qiuming-best qiuming-best force-pushed the maintenance-job branch 6 times, most recently from f227be4 to 6751207 Compare March 28, 2024 02:44
RepoName string
BackupStorageLocation string
RepoType string
KeepLatestMaitenanceJobs int
Copy link
Contributor

Choose a reason for hiding this comment

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

Is KeepLatestMaitenanceJobs used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! I should remove it

Signed-off-by: Ming Qiu <mqiu@vmware.com>
@@ -128,6 +135,13 @@ func (o *Options) BindFlags(flags *pflag.FlagSet) {
flags.BoolVar(&o.DefaultSnapshotMoveData, "default-snapshot-move-data", o.DefaultSnapshotMoveData, "Bool flag to configure Velero server to move data by default for all snapshots supporting data movement. Optional.")
flags.BoolVar(&o.DisableInformerCache, "disable-informer-cache", o.DisableInformerCache, "Disable informer cache for Get calls on restore. With this enabled, it will speed up restore in cases where there are backup resources which already exist in the cluster, but for very large clusters this will increase velero memory usage. Default is false (don't disable). Optional.")
flags.BoolVar(&o.ScheduleSkipImmediately, "schedule-skip-immediately", o.ScheduleSkipImmediately, "Skip the first scheduled backup immediately after creating a schedule. Default is false (don't skip).")
flags.Var(o.FormatFlag, "log-format", fmt.Sprintf("The format for log output. Valid values are %s.", strings.Join(o.FormatFlag.AllowedValues(), ", ")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to expose log-format and log-level from installation? Previously, we don't have them since they are not required only in troubleshooting cases.

@qiuming-best qiuming-best merged commit e80bdcf into vmware-tanzu:main Mar 28, 2024
65 of 66 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.

3 participants