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

Don't replace PVCs when resources change #1705

Merged
merged 6 commits into from
Sep 10, 2021
Merged

Conversation

jaxxstorm
Copy link
Contributor

PVCs often contain critical data that users don't want to lose.

Having the resources field trigger a replace can mean that users can't take advantage of volume resizing.

This allows users to resize volumes and update the field without worrying about a triggering of replacements

Related issues (optional)

fixes #1665

@jaxxstorm jaxxstorm added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Sep 8, 2021
@github-actions
Copy link

github-actions bot commented Sep 8, 2021

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

1 similar comment
@github-actions
Copy link

github-actions bot commented Sep 8, 2021

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@github-actions
Copy link

github-actions bot commented Sep 8, 2021

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@roderik
Copy link

roderik commented Sep 8, 2021

On AWS, the default storage class does not allow volume expansion. That takes a complex installation of the CSI driver. (which i have so no issue for me personally)

Even so, this would be a breaking change for anyone depending on a replace on request change (which is needed with AWS gp2). The "best" solution would be to check the underlying storageclass to see if it has volume expansion enabled. Otherwise, i think this needs to be an extra options to enable this.

@jaxxstorm jaxxstorm removed the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Sep 8, 2021
@jaxxstorm
Copy link
Contributor Author

@roderik - unfortunately there isn't a way to examine the storageclass to determine if the PVC is replaceable.

This mechanism allows users to do a replace by opting in (using the replacement annotation)

My logic here is that destroying PVCs is inherently a bad practice as it will nearly always contain user data that is required.

@roderik
Copy link

roderik commented Sep 9, 2021

@roderik - unfortunately there isn't a way to examine the storageclass to determine if the PVC is replaceable.

This mechanism allows users to do a replace by opting in (using the replacement annotation)

My logic here is that destroying PVCs is inherently a bad practice as it will nearly always contain user data that is required.

I follow that logic and it will error out if someone depends on this dangerous behavior and will not destroy anything or any data. That is a big improvement over the current behavior.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

I'm ok with this change, but let's add a note to the changelog that users can retain the previous behavior by setting the replaceOnChanges option, possibly with an example.

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

1 similar comment
@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@viveklak viveklak merged commit ef82aac into master Sep 10, 2021
@pulumi-bot pulumi-bot deleted the jaxxstorm/pvc-replace branch September 10, 2021 21:00
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.

PVC size update in the spec causes the resource to be replaced instead of updating
4 participants