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.

Sign up for GitHub

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

Jump to bottom

Add repository maintenance job #7451

Merged
merged 1 commit into from
Mar 28, 2024
Merged

Add repository maintenance job #7451

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.

@qiuming-best qiuming-best marked this pull request as draft February 21, 2024 07:23
@codecov Codecov
Copy link

codecov bot commented Feb 21, 2024 •
edited
Loading

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 13 times, most recently from 375d8ab to 6363355 Compare February 26, 2024 09:45
@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
Lyndon-Li
Lyndon-Li reviewed Mar 1, 2024
View reviewed changes
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
ywk253100
ywk253100 reviewed Mar 4, 2024
View reviewed changes
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
ywk253100
ywk253100 reviewed Mar 5, 2024
View reviewed changes
pkg/cmd/cli/repomantenance/maintenance.go Outdated Show resolved Hide resolved
pkg/cmd/cli/repomantenance/maintenance.go Outdated Show resolved Hide resolved
ywk253100
ywk253100 reviewed Mar 14, 2024
View reviewed changes
pkg/repository/maintenance.go
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
ywk253100
ywk253100 reviewed Mar 28, 2024
View reviewed changes
pkg/cmd/cli/repomantenance/maintenance.go Outdated
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

@qiuming-best
Add maintenance job
8d63c76 
Signed-off-by: Ming Qiu <mqiu@vmware.com>
Lyndon-Li
Lyndon-Li approved these changes Mar 28, 2024
View reviewed changes
pkg/cmd/cli/install/install.go Outdated
@@ -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
Reviewers

@ywk253100 ywk253100 ywk253100 approved these changes

@Lyndon-Li Lyndon-Li Lyndon-Li approved these changes

@reasonerjt reasonerjt Awaiting requested review from reasonerjt

@shubham-pampattiwar shubham-pampattiwar Awaiting requested review from shubham-pampattiwar

@sseago sseago Awaiting requested review from sseago

@anshulahuja98 anshulahuja98 Awaiting requested review from anshulahuja98

Assignees

@qiuming-best qiuming-best

Labels
has-changelog has-unit-tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qiuming-best @ywk253100 @Lyndon-Li