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

Make Azure disk snapshot SKU configurable (#96) #96

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

Make Azure disk snapshot SKU configurable (#96) #96

wants to merge 4 commits into from

Conversation

pavb74
Copy link

@pavb74 pavb74 commented May 2, 2021

Add a new supported key to the config ('sku') which allows
to specify the SKU used for the Azure disk snapshot
The values supported depend of the Azure zone, it can be:
Premium_LRS, Standard_LRS or Standard_ZRS

Signed-off-by: Pierre-Andre Vieillard-Baron [email protected]

pavb74 pushed a commit to pavb74/velero-plugin-for-microsoft-azure that referenced this pull request May 2, 2021
Make Azure disk snapshot SKU configurable (vmware-tanzu#96)
391f245 
Add a new supported key to the config ('sku') which allows
to specify the SKU used for the Azure disk snapshot
The values supported depend of the Azure zone, it can be:
Premium_LRS, Standard_LRS or Standard_ZRS

Signed-off-by: Pierre-Andre Vieillard-Baron <[email protected]>
@pavb74 pavb74 changed the title Make Azure disk snapshot SKU configurable (#3721) Make Azure disk snapshot SKU configurable (#96) May 2, 2021
pavb74 pushed a commit to pavb74/velero-plugin-for-microsoft-azure that referenced this pull request May 11, 2021
Make Azure disk snapshot SKU configurable (vmware-tanzu#96)
9310a7a 
Add a new supported key to the config ('sku') which allows
to specify the SKU used for the Azure disk snapshot
The values supported depend of the Azure zone, it can be:
Premium_LRS, Standard_LRS or Standard_ZRS

Signed-off-by: Vieillard-Baron Pierre-André <[email protected]>
Make Azure disk snapshot SKU configurable (vmware-tanzu#96)
9c3fb62 
Add a new supported key to the config ('sku') which allows
to specify the SKU used for the Azure disk snapshot
The values supported depend of the Azure zone, it can be:
Premium_LRS, Standard_LRS or Standard_ZRS

Signed-off-by: Vieillard-Baron Pierre-André <[email protected]>
@fabiocastagnino
Copy link

Hi all, what we can to do to have this PR merged?
It could be very usefull

Thank you

@jenting jenting requested review from sseago and removed request for carlisia, nrb and ashish-amarnath July 19, 2021 10:47
jenting
jenting suggested changes Jul 19, 2021
View reviewed changes
changelogs/unreleased/pr-pavb74 Outdated Show resolved Hide resolved
velero-plugin-for-microsoft-azure/volume_snapshotter.go Outdated Show resolved Hide resolved
velero-plugin-for-microsoft-azure/volume_snapshotter.go Outdated Show resolved Hide resolved
sseago
sseago requested changes Jul 20, 2021
View reviewed changes
Copy link
Collaborator

@sseago sseago left a comment

Choose a reason for hiding this comment

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

To add to the review from @jenting -- the requested changes seem to be missing a line, referenced in my comment.

velero-plugin-for-microsoft-azure/volume_snapshotter.go Outdated Show resolved Hide resolved
@jenting
Copy link
Contributor

jenting commented Jul 23, 2021 •
edited
Loading

@pavb74
Would you mind amending the last 3 commits and adding the signed-off-by?
Then I think it's good to be merged.

@jenting
Copy link
Contributor

jenting commented Jul 29, 2021

@pavb74
Would you mind amending the last 3 commits and adding the signed-off-by?
Then I think it's good to be merged.

ping @pavb74

pavb74 and others added 3 commits July 29, 2021 11:54
@pavb74
Update changelogs/unreleased/pr-pavb74
c49e0b5 
Co-authored-by: JenTing Hsiao <[email protected]>
Signed-off-by: Vieillard-Baron Pierre-André <[email protected]>
@pavb74
Update velero-plugin-for-microsoft-azure/volume_snapshotter.go
5481cb9 
Co-authored-by: JenTing Hsiao <[email protected]>
Signed-off-by: Vieillard-Baron Pierre-André <[email protected]>
@pavb74
Update velero-plugin-for-microsoft-azure/volume_snapshotter.go
1fa7ff0 
Co-authored-by: JenTing Hsiao <[email protected]>
Signed-off-by: Vieillard-Baron Pierre-André <[email protected]>
@pavb74
Copy link
Author

pavb74 commented Jul 29, 2021

@jenting Done, sorry for the delay

zubron
zubron suggested changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

Thanks, @pab74, and sorry for keeping you waiting for a review. I have a questions about the behaviour of the SKU if different storage types are being used, primarily because this is a setting that will apply to all volumes types that are being snapshotted by the plugin, and is not set per backup.

Also, could you please update the Volume Snapshot Location documentation to include this new config field and the valid values. Thanks!

velero-plugin-for-microsoft-azure/volume_snapshotter.go
if val := config[snapsSkuConfigKey]; val != "" {
var snapshotsSkuName disk.SnapshotStorageAccountTypes
var found bool
for _, name := range disk.PossibleSnapshotStorageAccountTypesValues() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the possible values and their descriptions, it seems like different SKU types apply to different storage types, e.g. Premium_LRS is for Premium SSDs, Standard_LRS is for standard HDDs. If a user sets the SKU but has a mix of storage types, what is the behaviour? Does it fall back to an appropriate SKU for that storage type?

@yyvess
Copy link
Contributor

yyvess commented Mar 9, 2022 •
edited
Loading

Update of azure-sdk-for-go version must fix SKU issue without needed to add any new config.

I create a PR with this version update =>
#131

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sseago sseago sseago requested changes

@zubron zubron zubron requested changes

@jenting jenting jenting approved these changes

@dsu-igeek dsu-igeek Awaiting requested review from dsu-igeek

Requested changes must be addressed to merge this pull request.

Assignees

@pavb74 pavb74

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pavb74 @fabiocastagnino @jenting @yyvess @sseago @zubron