stable/velero: Update Velero to v1.2.0 #18492

carlisia · 2019-10-31T18:46:58Z

Signed-off-by: Carlisia [email protected]

Is this a new chart

NOTE: We're experiencing a high volume of PRs to this repo and reviews will be delayed. Please host your own chart repository and submit your repository to the Helm Hub instead of this repo to make them discoverable to the community. Here is how to submit new chart repositories to the Helm Hub.

What this PR does / why we need it:

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

fixes #

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md
Title of the PR starts with chart name (e.g. [stable/mychartname])

k8s-ci-robot · 2019-10-31T18:47:12Z

Hi @carlisia. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

prydonius

This is great, thanks @carlisia. If we can remove the Velero version change, we can just do a patch bump on the chart here

stable/velero/values.yaml

prydonius · 2019-10-31T19:03:07Z

Ah the plugin docs is relevant to 1.2.0, so we should hold this and use this PR to release 1.2.0. #17585 can then rebase once this is merged.

/hold

skriss

Looks great! couple minor things

stable/velero/README.md

nrb · 2019-11-07T19:58:06Z

stable/velero/README.md


-### Configuration


Is this information added elsewhere? There's the CLI command example, but it lacks the description/required fields from this table, which I find useful.

We had talked about this in the past in the context of the velero install command. What we had decided was to have the CLI itself be the source of documentation, instead of trying to keep such documentation up to date in multiple places. At least this is my recollection, and also doesn't mean we can't change it.

Yeah, that's true of velero install, though the Helm chart is a bit different. For example, all of these options can (an normally, should) be in a values.yaml file versus just being on the command line. Also, when they're on the command line, they're not their own flags, but rather key/value pairs given to set.

There's also options like credentials.existingSecret that aren't in velero install that have some interactions with other values. That's not immediately clear from just the command line instructions here.

Finally, there's also Helm standards that mention documenting what's possible in a values.yaml file.

I think I'd propose having both the CLI example that was added as well as the table here.

Gotcha. But these values are all documented in values.yaml. Do they need to be duplicated in the readme too?

No, I don't think they need to duplicated - in the values.yaml is fine. I'll mark this as resolved.

I think the convention is to actually have them documented. Personally I find dot notation way better searchable than YAML notation. Example:

configuration.backupStorageLocation.config.storageAccount
vs

configuration: # a lot of documentation # a lot of documentation # a lot of documentation # a lot of documentation # a lot of documentation backupStorageLocation: # a lot of documentation # a lot of documentation # a lot of documentation # a lot of documentation # a lot of documentation config: # a lot of documentation # a lot of documentation # a lot of documentation # a lot of documentation # a lot of documentation storageAccount

That's a good point. While it's well taken, I think in order to get Helm users onto v1.2, we should defer this discussion to a separate PR and focus this one on any changes necessary to get 1.2 installable at this stage.

billimek · 2019-11-07T21:40:57Z

Should we include a facility within velero to allow for the helm chart to install the necessary plugin(s) which now require the --plugins argument to be passed to the velero container?

See v1.2.0 release notes

prydonius · 2019-11-07T22:06:07Z

/hold cancel

prydonius · 2019-11-07T22:07:35Z

@billimek the docs have been updated to reflect that in this PR: https://github.com/helm/charts/pull/18492/files#diff-600a674d7d9abb8a92992fae1f8bc6d5R54. I do think we can make this UX a little nicer by making it simpler to specify plugin images, but maybe something we can address in a follow up PR?

thabach · 2019-11-09T16:57:59Z

I commented on Google groups in regards to the (old, the 1.1.0) Helm chart yesterday, see here https://groups.google.com/forum/#!topic/projectvelero/pUm3CyrIusA

A fix for the first issue should be straightforward and I would love to supply a pull but where-to ? The second issue I am not sure if you guys feel like that should be steered by the Helm chart, as customisation can be done via a ConfigMap according to https://velero.io/docs/master/restic/ , it would certainly make things simpler.

Heya

great work and congrats to the new 1.2 release !

I happen to run into two issues with the Helm chart https://github.com/helm/charts/tree/master/stable/velero for velero v1.1.0 (the latest available).

the restic.podVolumePath configuration is not taken into account at all, but the default gets always applied

while the image.repository setting is taken into account in assembling the velero image path, but not in addressing the velero-restic-restore-helper init-container (which prevents me from doing a restore in the first place within our corporate image caching setup)

Maybe that could be fixed for the soon to be released v1.2.0 helm chart, I would certainly highly appreciate.

Kind regards, Christian.

nrb · 2019-11-12T21:35:56Z

@thabach Thanks for reporting those problems. Could you file those as separate issues in this repo and assign them to me, so we can get them addressed? I don't know that we'll include them in this PR, but we may do a follow-up point release.

thabach · 2019-11-13T15:05:24Z

@thabach Thanks for reporting those problems. Could you file those as separate issues in this repo and assign them to me, so we can get them addressed? I don't know that we'll include them in this PR, but we may do a follow-up point release.

@nrb just realised that I was sitting on an old restic-daemonset.yaml-template and the first bullet was already resolved some time ago - sorry for the false alarm. For the second problem I will report a separate issue (done here #18844).

prydonius · 2019-11-15T00:47:45Z

@carlisia are you able to rebase this and fix conflicts?

carlisia · 2019-12-02T17:59:41Z

Getting this rebased now..

carlisia · 2019-12-02T20:09:01Z

@prydonius / @nrb PTAL.

davidkarlsen · 2019-12-03T02:04:44Z

/ok-to-test

skriss · 2019-12-03T16:34:13Z

stable/velero/README.md

+helm upgrade <RELEASE NAME> --set initContainers.backupStorageLocation.name=aws,initContainers.volumeSnapshotLocation.name=aws stable/velero
+```
+
+## Upgrading


Let's add an "Upgrading to v1.2.0" subsection here. I think we can also remove the "Upgrading to v0.11.0" section, and I'd be fine dropping the "Upgrading to v1.0.0" section as well.

skriss · 2019-12-03T16:36:15Z

otherwise, LGTM!

prydonius · 2019-12-05T02:36:44Z

@carlisia looks like this is good to go once the conflicts are resolved!

JoaoBraveCoding · 2019-12-05T13:36:49Z

stable/velero/Chart.yaml

 description: A Helm chart for velero
 name: velero
-version: 2.5.0
+version: 2.6.0


Suggested change

version: 2.6.0

version: 2.7.0

JoaoBraveCoding · 2019-12-05T14:18:45Z

stable/velero/values.yaml

-  #     - name: plugins
-  #       mountPath: /target
+  #     - mountPath: /target
+  #       name: plugin


Suggested change

# name: plugin

# name: plugins

JoaoBraveCoding · 2019-12-05T14:18:58Z

stable/velero/values.yaml

-  # - name:
-  #   image:
+  # - name: velero-plugin-for-aws
+  #   image: velero/velero-plugin-for-aws:v1.0.0-beta.1


Suggested change

# image: velero/velero-plugin-for-aws:v1.0.0-beta.1

# image: velero/velero-plugin-for-aws:v1.0.0

where do I need to specify the plugins inside of the helm chart. Is there a reference to the values file? Actualy im having an issue upgrading.
#19427

You have to specify it in the initContainers field in the values file like is exemplified here. Hope this was the answer you were looking for.

I could solve the problem by using init containers:

initContainers: - name: velero-plugin-for-microsoft-azure image: velero/velero-plugin-for-microsoft-azure:v1.0.0 volumeMounts: - name: plugins mountPath: /target

@JoaoBraveCoding yes, thats it. I think I didn´t see your message on Friday, but today. Thank you!

ZeroVEVO · 2019-12-06T13:08:46Z

stable/velero/values.yaml

-priorityClassName: {}
-
-# Init containers to add to the Velero deployment's pod spec. Optional.
+# Init containers to add to the Velero deployment's pod spec. At least one plugin provider image is required.


this would be very helpful, otherwise i wont know where to add the plugins to the chart

skriss · 2019-12-06T15:51:57Z

@carlisia let's try to get this rebased and merged soon so we get updated and don't continue to hit merge conflicts!

Signed-off-by: Carlisia <[email protected]>

nrb · 2019-12-09T20:15:42Z

/ok-to-test

nrb · 2019-12-09T20:20:10Z

/lgtm
/approved

k8s-ci-robot · 2019-12-09T20:20:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlisia, nrb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~stable/velero/OWNERS~~ [nrb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Update Velero to v1.2.0 Signed-off-by: Carlisia <[email protected]> * Bump release version Signed-off-by: Carlisia <[email protected]> * Fix docs Signed-off-by: Carlisia <[email protected]> * Code reviews Signed-off-by: Carlisia <[email protected]>

* Update Velero to v1.2.0 Signed-off-by: Carlisia <[email protected]> * Bump release version Signed-off-by: Carlisia <[email protected]> * Fix docs Signed-off-by: Carlisia <[email protected]> * Code reviews Signed-off-by: Carlisia <[email protected]> Signed-off-by: Artur <[email protected]>

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2019

k8s-ci-robot requested a review from nrb October 31, 2019 18:46

helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Oct 31, 2019

k8s-ci-robot requested a review from prydonius October 31, 2019 18:46

helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 31, 2019

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 31, 2019

carlisia changed the title ~~Update Velero to v1.2.0~~ stable/velero: Update Velero to v1.2.0 Oct 31, 2019

prydonius approved these changes Oct 31, 2019

View reviewed changes

stable/velero/values.yaml Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 31, 2019

prydonius mentioned this pull request Oct 31, 2019

[stable/velero] add support for cross-subscription backups/restore #17585

Closed

4 tasks

carlisia force-pushed the c-velero-1.2.0-beta branch from 26db60a to 8b376c5 Compare October 31, 2019 19:14

carlisia mentioned this pull request Oct 31, 2019

Helm chart: better document plugin installation, credentials secret creation vmware-tanzu/velero#1819

Closed

carlisia marked this pull request as ready for review November 7, 2019 19:13

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2019

skriss suggested changes Nov 7, 2019

View reviewed changes

stable/velero/README.md Outdated Show resolved Hide resolved

stable/velero/README.md Outdated Show resolved Hide resolved

nrb reviewed Nov 7, 2019

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2019

carlisia force-pushed the c-velero-1.2.0-beta branch from 81be461 to 614db71 Compare December 2, 2019 20:02

skriss reviewed Dec 3, 2019

View reviewed changes

This was referenced Dec 5, 2019

[stable/velero] Update Velero to v1.2.0 #19353

Merged

velero v1.2.0 --dry-run -o yaml inserts "nullable" in generated kube manifest files vmware-tanzu/velero#2077

Closed

JoaoBraveCoding reviewed Dec 5, 2019

View reviewed changes

ZeroVEVO mentioned this pull request Dec 6, 2019

[stable/velero] Plugins for Velero missing after Upgrade to 1.2.0 #19427

Closed

ZeroVEVO reviewed Dec 6, 2019

View reviewed changes

ZeroVEVO approved these changes Dec 6, 2019

View reviewed changes

Carlisia added 4 commits December 9, 2019 07:53

Update Velero to v1.2.0

51e9573

Signed-off-by: Carlisia <[email protected]>

Bump release version

3be0ed9

Signed-off-by: Carlisia <[email protected]>

Fix docs

6e79d77

Signed-off-by: Carlisia <[email protected]>

Code reviews

2446b15

Signed-off-by: Carlisia <[email protected]>

carlisia force-pushed the c-velero-1.2.0-beta branch from 6957095 to 2446b15 Compare December 9, 2019 16:04

k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 9, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2019

k8s-ci-robot merged commit 06a9968 into helm:master Dec 9, 2019

carlisia deleted the c-velero-1.2.0-beta branch December 9, 2019 20:27

robinkb mentioned this pull request Dec 17, 2019

Migrate Velero Helm chart away from github.com/helm/charts vmware-tanzu/helm-charts#3

Closed

	# image: velero/velero-plugin-for-aws:v1.0.0-beta.1
	# image: velero/velero-plugin-for-aws:v1.0.0


		### Configuration

stable/velero: Update Velero to v1.2.0 #18492

stable/velero: Update Velero to v1.2.0 #18492

Conversation

carlisia commented Oct 31, 2019 • edited Loading

Is this a new chart

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

k8s-ci-robot commented Oct 31, 2019

prydonius left a comment

Choose a reason for hiding this comment

prydonius commented Oct 31, 2019

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billimek commented Nov 7, 2019

prydonius commented Nov 7, 2019

prydonius commented Nov 7, 2019

thabach commented Nov 9, 2019 • edited Loading

nrb commented Nov 12, 2019

thabach commented Nov 13, 2019 • edited Loading

prydonius commented Nov 15, 2019

carlisia commented Dec 2, 2019

carlisia commented Dec 2, 2019

davidkarlsen commented Dec 3, 2019

Choose a reason for hiding this comment

skriss commented Dec 3, 2019

prydonius commented Dec 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Dec 6, 2019

nrb commented Dec 9, 2019

nrb commented Dec 9, 2019

k8s-ci-robot commented Dec 9, 2019

carlisia commented Oct 31, 2019 •

edited

Loading

thabach commented Nov 9, 2019 •

edited

Loading

thabach commented Nov 13, 2019 •

edited

Loading