Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Add require_volume_annotation for restic plugin #1132

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Add require_volume_annotation for restic plugin #1132

merged 1 commit into from
Nov 19, 2020

Conversation

ipochi
Copy link
Member

@ipochi ipochi commented Oct 28, 2020 •
edited
Loading

This PR is based on top of #1131 and would need to be merged first.

Adds a new field require_volume_annotation that was introduced in Velero v1.5.2.

If enabled the user will have to manually add annotations to the pod when taking backups.

Default is false, i.e volumes will be backed up without the need for annotation.

@ipochi ipochi force-pushed the imran/add-defaultvolumestorestic-in-velero branch from 1e9c9bc to 5cc4021 Compare October 28, 2020 07:20
invidian
invidian previously requested changes Oct 28, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Some nits

assets/terraform-modules/aws/flatcar-linux/kubernetes/versions.tf Outdated Show resolved Hide resolved
pkg/components/velero/component.go Outdated Show resolved Hide resolved
docs/configuration-reference/components/velero.md Outdated Show resolved Hide resolved
@invidian
Copy link
Member

I just noticed this PR includes commits from #1131, sorry that I put all review comments here. It's good to mention such things in PR description :)

@ipochi ipochi force-pushed the imran/add-defaultvolumestorestic-in-velero branch from 5cc4021 to 264861a Compare October 28, 2020 11:10
@ipochi ipochi requested a review from invidian October 28, 2020 11:14
@ipochi ipochi force-pushed the imran/add-defaultvolumestorestic-in-velero branch from 264861a to 93b9487 Compare October 28, 2020 11:17
@ipochi ipochi dismissed invidian’s stale review October 28, 2020 11:27

Requesting to re-review.

@ipochi ipochi changed the title Add default_volumes_to_restic for restic plugin Add require_volume_annotation for restic plugin Oct 28, 2020
invidian
invidian suggested changes Oct 29, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

There are conflicts which needs to be resolved.

@ipochi
Copy link
Member Author

ipochi commented Oct 29, 2020

@invidian yes, I found a bug in the existing code and resolving that.

@ipochi ipochi force-pushed the imran/add-defaultvolumestorestic-in-velero branch 2 times, most recently from d599311 to a3ff583 Compare November 3, 2020 05:47
invidian
invidian suggested changes Nov 3, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Just one nit, otherwise LGTM

docs/configuration-reference/components/velero.md Outdated Show resolved Hide resolved
@ipochi
Copy link
Member Author

ipochi commented Nov 5, 2020

@invidian There is an opt-out feature in Velero which should be added as annotation if volume is not be backed up by restic by default.

Should we flip the default value for require_volume_annotation to false i.e opt-out strategy where volume will not be considered for back up if the resource has the following annotation backup.velero.io/backup-volumes-excludes ?

@invidian
Copy link
Member

invidian commented Nov 5, 2020

Should we flip the default value for require_volume_annotation to false i.e opt-out strategy where volume will not be considered for back up if the resource has the following annotation backup.velero.io/backup-volumes-excludes ?

Sounds good 👍 If we have a place for it, we could mention it in our documentation for Velero component.

@ipochi ipochi force-pushed the imran/add-defaultvolumestorestic-in-velero branch from a3ff583 to 12176ee Compare November 18, 2020 12:13
@ipochi ipochi requested a review from invidian November 18, 2020 12:13
@ipochi
velero: add new field to restic plugin
b8d7197 
This commit adds a new field `require_volume_annotation`.

If enabled the user will not have to manually add annotations to the pod
when taking backups.

Signed-off-by: Imran Pochi <imran@kinvolk.io>
@ipochi ipochi force-pushed the imran/add-defaultvolumestorestic-in-velero branch from 12176ee to b8d7197 Compare November 18, 2020 12:17
invidian
invidian approved these changes Nov 18, 2020
View reviewed changes
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

LGTM

@ipochi ipochi requested a review from surajssd November 19, 2020 10:25
surajssd
surajssd approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

LGTM

@invidian invidian merged commit 26dd7c1 into master Nov 19, 2020
@invidian invidian deleted the imran/add-defaultvolumestorestic-in-velero branch November 19, 2020 10:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@surajssd surajssd surajssd approved these changes

@invidian invidian invidian approved these changes

@knrt10 knrt10 Awaiting requested review from knrt10

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ipochi @invidian @surajssd