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

[velero] Default to empty volumeSnapshotLocation. #468

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

toothbrush
Copy link

@toothbrush toothbrush commented Jun 14, 2023

Special notes for your reviewer:

I ran into an issue where i needed to override volumeSnapshotLocation: [] in my values.yaml file, otherwise i received the following scary error:

Error: UPGRADE FAILED: post-upgrade hooks failed: unable to build kubernetes object for post-upgrade hook velero/templates/volumesnapshotlocation.yaml: error validating "": error validating data: ValidationError(VolumeSnapshotLocation.spec): missing required field "provider" in io.velero.v1.VolumeSnapshotLocation.spec

I believe that the default behaviour from v3 of the chart is preserved, because previously, leaving the value of volumeSnapshotLocation with empty values:

volumeSnapshotLocation:
# name is the name of the volume snapshot location where snapshots are being taken. Required.
name:
# provider is the name for the volume snapshot provider. If omitted
# `configuration.provider` will be used instead.
provider:
# Additional provider-specific configuration. See link above
# for details of required/optional fields for your provider.
config: {}

Would prevent volume snapshots from working. I think. If i understand correctly 😅

Checklist

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the values.yaml or README.md
  • Title of the PR starts with chart name (e.g. [velero])

@toothbrush toothbrush changed the title Default to empty volumeSnapshotLocation. [velero] Default to empty volumeSnapshotLocation. Jun 14, 2023
@toothbrush toothbrush force-pushed the paul/default-empty-volumeSnapshotLocation branch 2 times, most recently from b1b7165 to 705d163 Compare June 14, 2023 06:28
Without this default, i hit an error deploying Velero.

Signed-off-by: paul david <[email protected]>
@toothbrush toothbrush force-pushed the paul/default-empty-volumeSnapshotLocation branch from 705d163 to 8e43b4f Compare June 14, 2023 06:31
@toothbrush toothbrush marked this pull request as ready for review June 14, 2023 06:33
@jenting
Copy link
Collaborator

jenting commented Jun 15, 2023

@toothbrush Thanks for your contribution.

Perhaps you could change snapshotsEnabled: false in your values.yaml, without having to make code change.
Please let me know if it works for you or not.

@toothbrush
Copy link
Author

Perhaps you could change snapshotsEnabled: false in your values.yaml, without having to make code change.
Please let me know if it works for you or not.

That's actually what i ended up doing, after staring more at the Helm charts 😃

However i still believe it's a bug that in the default configuration, this Helm chart (appears to) result in broken config (i.e, an invalid entry in the volumeSnapshotLocation slice).

Copy link
Collaborator

@jenting jenting left a comment

Choose a reason for hiding this comment

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

Please leave the comment description as is.

Also, it's be good if you could update backupStorageLocation as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants