-
Notifications
You must be signed in to change notification settings - Fork 366
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] Chart ux improvements #138
Open
kav
wants to merge
23
commits into
vmware-tanzu:main
Choose a base branch
from
pelotech:chart-ux-improvements
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
f9fcf95
add schedule labels
cesarokuti d648f35
Remove Prometheus operator CRDs
106c462
Fixes #90 - schedule.template
kav 4af5769
Fixes #89 - InitContainers/Provider
kav a2a9791
Fixes #88 - ExistingSecret
kav b8d769e
Reving version and adding docs
kav ddd47dc
Removing config table
kav ac3d3b2
Fixing version
kav 3e431d8
Adding plugins for all providers
kav cb95888
renaming secret file to align with velero install
kav 64d4fc4
Adding plugins and tests for initContainers
kav 39fb672
Unifying with scheduleLabels and fixing helm2
kav e6d94f9
Inferring useSecret
kav cb2f065
Adding credentials tests
kav 54692e1
Switching deployRestic to restic.enabled
kav e4921e1
Adding snapshots before refactoring configuration out
kav 379b4a1
Removing configuration key
kav 6b5e7d1
Adding backcompat
kav 4d2343d
WIP: Adding backcompat testing
kav 26d1a8a
Back compat for schedules.
kav 5abd8a8
Merge remote-tracking branch 'upstream/main' into chart-ux-improvements
kav d8d1ee1
linting
kav 9dc8a22
Updating tests and snapshots post main merge
kav File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
installCRDs: true | ||
|
||
# Set provider name and backup storage location bucket name | ||
configuration: | ||
provider: aws | ||
backupStorageLocation: | ||
bucket: velero | ||
config: | ||
region: us-west-1 | ||
profile: test | ||
volumeSnapshotLocation: | ||
provider: aws | ||
config: | ||
bucket: velero | ||
region: us-west-1 | ||
|
||
# Set a service account so that the CRD clean up job has proper permissions to delete CRDs | ||
serviceAccount: | ||
server: | ||
name: velero | ||
|
||
schedules: | ||
mybackup: | ||
labels: | ||
myenv: foo | ||
schedule: "0 0 * * *" | ||
template: | ||
ttl: "240h" | ||
includedNamespaces: | ||
- foo | ||
|
||
# Whether or not to clean up CustomResourceDefintions when deleting a release. | ||
# Cleaning up CRDs will delete the BackupStorageLocation and VolumeSnapshotLocation instances, which would have to be reconfigured. | ||
# Backup data in object storage will _not_ be deleted, however Backup instances in the Kubernetes API will. | ||
# Always clean up CRDs in CI. | ||
cleanUpCRDs: true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea that the user does not need to specify the plugin images anymore and I think the below code is a bit redundant from my usage.
However, we have a use case that we'll mount two plugin images, one for AWS and the other one for CSI, so our command is:
With this PR, I think our use case can't be fulfilled. But I'd rather see if it's possible to remove the redundant volumeMounts[0] and the charts would help us generate automatically, which becomes:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add additional initContainers as before, the one for the provider will be added automatically. At present you'd add (assuming aws is the provider):
Are you using both because the other provider is in a backup storage location or snapshot location? Can we just add initContainers for all the various providers across the values file and leave initContainers as a standard extension point for fallback.
I'd recommend against automounting plugins at
/target
for all initContainers as that is a general extension point. If you really want something similar perhaps it'sworth adding a list of plugin images to add over and above the provider one? Something likewhich then creates an initContainer entry with the volume mount.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we're using S3-compatible (minion) which requires velero-plugin-for-aws and also deploy ceph-csi to take a volume snapshot by Kubernetes volume snapshot CRDs with velero-plugin-for-csi.
Sounds good. Personally I'd like this approach then we could remove the
initContainers
in values.yaml.Any feedback on this? cc @carlisia @nrb @ashish-amarnath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added initContainers for each provider configured at the various levels in the values so if you have different providers for configuration, backupStorageLocation, and snapshotLocation it will add all the plugins.
It looks like only the configuration.provider has a secret injected into the primary container. Is that a concern? Should we be refactoring the credentials to allow for multiple providers here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth mentioning I removed the "data" in helpers and put it into values, it dumbs the logic down nicely and moves the images to standard patterns. This is nicer as it makes the images Flux HelmOperator friendly and I'm sure has other similar benefits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Velero installs plugins as initContainers with the expectation that they will be copied to the correct mountpoint for invocation; all plugins should end up in the same directory for them to work properly at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be a problem. They are all copied to
/target
in theplugins
volume as expected. As you mention re the secret though we'll only have one set of credentials right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kav sorry for the late reply.
I'd prefer to change the installation way from
to
then, the installation command is aligned to
velero install
.WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great to me! I wrote a reply discussing the merits of that vs a yaml list and then realized it's a single split call and erased it and implemented support for both.
Updated, we now support
--set plugins velero/velero-plugin-for-aws:v1.1.0,velero/velero-plugin-for-csi:v0.1.1
and
and even
Also added unit tests for all of that using https://github.com/quintush/helm-unittest