This repository was archived by the owner on Mar 16, 2024. It is now read-only.
Only set Status.Defaults.Volumes if it hasn't been set #2297
Conversation
This file contains hidden or 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
tylerslaton
commented
Oct 26, 2023
| volumes: | ||
| foo: | ||
| class: test-volume-class | ||
| size: 4Gi |
There was a problem hiding this comment.
In this particular test instance, we expect this to remain 4Gi after going through a reconciliation.
g-linville
approved these changes
Oct 26, 2023
4101482 to
3d4a715
Compare
g-linville
approved these changes
Nov 2, 2023
thedadams
reviewed
Nov 2, 2023
This also includes a migration step away from Status.Defaults.VolumeSize. It removes the logic that originally set the VolumeSize field and instead hands that logic off to the `Status.Defaults.Volumes` map field. If VolumeSize was previously set, then we will set that in the Volumes map field as the default. This prevents any issues from occuring with existing volumes when the default VolumeClass is updated. Signed-off-by: tylerslaton <mtslaton1@gmail.com>
3d4a715 to
b85986f
Compare
thedadams
approved these changes
Nov 2, 2023
Signed-off-by: tylerslaton <mtslaton1@gmail.com>
b85986f to
3bb3a13
Compare
|
@thedadams @g-linville I was giving this a test before merging and noticed some behavior that was not quite right, mainly around the migration. I've gone ahead and updated this PR to fix that issue. Please let me know if I can answer any questions. Re-requesting review for the time being. The changes are in a new commit for simplicity. |
g-linville
approved these changes
Nov 2, 2023
g-linville
reviewed
Nov 2, 2023
4e940b8 to
a08975e
Compare
thedadams
reviewed
Nov 2, 2023
pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-same-gen/expected.golden
Show resolved
Hide resolved
…en set Signed-off-by: tylerslaton <mtslaton1@gmail.com>
a08975e to
cb3ec26
Compare
thedadams
approved these changes
Nov 2, 2023
Signed-off-by: tylerslaton <mtslaton1@gmail.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
If the Volume already has defaults, skip it. We don't want to overwrite defaults for volumes values as it can lead to unexpected behavior when volume classes are updated. One example is a volume class going down in size, which in turn will cause the volume to be sized down and likely go into an error state on the next app update.
This also includes a migration step away from Status.Defaults.VolumeSize. It removes the logic that originally set the VolumeSize field and instead hands that logic off to the
Status.Defaults.Volumesmap field. If VolumeSize was previously set, then we will set that in the Volumes map field as the default. This prevents any issues from occuring with existing volumes when the default VolumeClass is updated.Checklist
This is a title (#1216). Here's an example