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

Change clone strategy to avoid PowerStore snapshot limits #3520

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

coulof
Copy link

@coulof coulof commented Nov 7, 2024

What this PR does / why we need it:
PowerStore has a limit of 32 to 64 writable snapshots which prevents to create more VMs from a base OS.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

* Dell PowerStore uses `cloneStrategy: copy` over `csi-clone` to avoid array snapshot limits

@kubevirt-bot kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 7, 2024
@kubevirt-bot
Copy link
Contributor

Hi @coulof. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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-sigs/prow repository.

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign akalenyu for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 7, 2024
@akalenyu
Copy link
Collaborator

akalenyu commented Nov 7, 2024

So CSI/snapshot clones normally create ephemeral snapshots, which should keep us within limits.
Could you please attach some PowerStore source? I am probably misunderstanding the issue.

I want to ensure I poke at this a bit, since doing host assisted clones all over is going to be a major hit for anyone who uses this solution.

@coulof coulof changed the title Change clone strategy to avoid PowerStore snapshot limits [WIP] Change clone strategy to avoid PowerStore snapshot limits Nov 7, 2024
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2024
@coulof coulof changed the title [WIP] Change clone strategy to avoid PowerStore snapshot limits Change clone strategy to avoid PowerStore snapshot limits Nov 27, 2024
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2024
@coulof coulof changed the title Change clone strategy to avoid PowerStore snapshot limits [WIP] Change clone strategy to avoid PowerStore snapshot limits Nov 27, 2024
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2024
@coulof
Copy link
Author

coulof commented Nov 28, 2024

So CSI/snapshot clones normally create ephemeral snapshots, which should keep us within limits. Could you please attach some PowerStore source? I am probably misunderstanding the issue.

I want to ensure I poke at this a bit, since doing host assisted clones all over is going to be a major hit for anyone who uses this solution.

In PowerStore, a PVC restore from a Snapshot is actually a writable Snapshot. There are limited number of writable snapshot per LUN therefore we have to switch to host copy.

@coulof coulof changed the title [WIP] Change clone strategy to avoid PowerStore snapshot limits Change clone strategy to avoid PowerStore snapshot limits Nov 28, 2024
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2024
@akalenyu
Copy link
Collaborator

So CSI/snapshot clones normally create ephemeral snapshots, which should keep us within limits. Could you please attach some PowerStore source? I am probably misunderstanding the issue.
I want to ensure I poke at this a bit, since doing host assisted clones all over is going to be a major hit for anyone who uses this solution.

In PowerStore, a PVC restore from a Snapshot is actually a writable Snapshot. There are limited number of writable snapshot per LUN therefore we have to switch to host copy.

In that case, you also want to not use snapshots as golden sources (many VMs will restore from them), right

- Ceph rbd scales better when golden sources are stored as snapshots (more info in [clone-from-volumesnapshot-source](./clone-from-volumesnapshot-source.md))
https://github.com/kubevirt/containerized-data-importer/blob/c6089cbcb01ab58e75b25be1770ce0f3f4625014/pkg/storagecapabilities/storagecapabilities.go#L109-L112

But my worries remain, host-assisted is a big hammer,
But other things could break too, for example, you're saying the number of VMSnapshot/VMRestores one can make is also limited?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants