📖 Add CRD Upgrade Safety documentation #1090
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
trgeiger
force-pushed
the
crd-update-docs
branch
from
81f0bf7 to
9aa7e54
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1090 +/- ##
==========================================
- Coverage 75.31% 75.23% -0.09%
==========================================
Files 31 35 +4
Lines 1904 1914 +10
==========================================
+ Hits 1434 1440 +6
- Misses 327 331 +4
Partials 143 143
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
trgeiger
force-pushed
the
crd-update-docs
branch
from
9aa7e54 to
b05f4fc
Compare
|
|
||
| The following changes to an existing CRD are safe for backwards compatibility and will | ||
| not cause the CRD Upgrade Safety preflight check to halt the upgrade: | ||
| - Adding new enum values to the list of allowed enum values in a field |
There was a problem hiding this comment.
I think it is fine to leave in for now because it is not considered a breaking change in our validations, but something we identified is that Kubernetes API conventions state that adding new enum values is actually considered a breaking change.
| - An existing required field is changed to optional in an existing version | ||
| - The minimum value of an existing field is decreased in an existing version | ||
| - The maximum value of an existing field is increased in an existing version | ||
| - A new version of the CRD is added with no modifications to existing versions |
There was a problem hiding this comment.
No change needed in this PR, but something that I think we may have overlooked when originally developing the preflight checks is that new versions of the CRD must be backwards compatible with the served versions OR provide conversion logic to prevent data loss and invalidation of resources stored at an older version.
@joelanford I think we may need to revisit the CRD Upgrade Safety preflight checks to ensure we capture behavior to check for breaking changes between all stored and served versions of the CRD when there is no conversion strategy specified.
There was a problem hiding this comment.
given the long discussion this birthed in Slack, do we want to hold off on these docs until that work is done or would we just update this once that's complete? Not sure on what kind of timeline you were imagining all this.
There was a problem hiding this comment.
We can let the doc through and update it as necessary
| versions: | ||
| - name: v1alpha1 | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Should we include what the error message looks like when a change like this is caught?
There was a problem hiding this comment.
I like putting example outputs in collapsible blocks.
trgeiger
force-pushed
the
crd-update-docs
branch
from
b05f4fc to
810503d
Compare
| - The scope changes from Cluster to Namespace or from Namespace to Cluster | ||
| - An existing stored version of the CRD is removed | ||
| - A new required field is added to an existing version of the CRD | ||
| - An existing field is removed from an existing version of the CRD | ||
| - An existing field type is changed in an existing version of the CRD | ||
| - A new default value is added to a field that did not previously have a default value | ||
| - The default value of a field is changed | ||
| - An existing default value of a field is removed | ||
| - New enum restrictions are added to an existing field which did not previously have enum restrictions | ||
| - Existing enum values from an existing field are removed | ||
| - The minimum value of an existing field is increased in an existing version | ||
| - The maximum value of an existing field is decreased in an existing version | ||
| - Minimum or maximum field constraints are added to a field that did not previously have constraints |
There was a problem hiding this comment.
I wonder if in a future enhancement, it would be worth refactoring this content into a table?
There was a problem hiding this comment.
That could be nice. I spent a few minutes trying to think of how exactly that would be structured and couldn't really come up with a good idea. Maybe columns for safe/unsafe changes or something?
There was a problem hiding this comment.
I have played around with it too, but I haven't been able to beat your list. I also haven't been able to figure out a good information architecture.
I do feel like like it would be nice to have this be a bit more scan-able as a reference. Maybe we will come up with something when we downstream this doc?
| versions: | ||
| - name: v1alpha1 | ||
| ``` | ||
|
|
| versions: | ||
| - name: v1alpha1 | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I like putting example outputs in collapsible blocks.
trgeiger
force-pushed
the
crd-update-docs
branch
3 times, most recently
from
8416d49 to
d732bdd
Compare
trgeiger
force-pushed
the
crd-update-docs
branch
2 times, most recently
from
4983271 to
0c07a19
Compare
|
The example error output doesn't wrap, should I just manually place some line breaks so it's not such a long horizontal scroll? |
| - The scope changes from Cluster to Namespace or from Namespace to Cluster | ||
| - An existing stored version of the CRD is removed | ||
| - A new required field is added to an existing version of the CRD | ||
| - An existing field is removed from an existing version of the CRD | ||
| - An existing field type is changed in an existing version of the CRD | ||
| - A new default value is added to a field that did not previously have a default value | ||
| - The default value of a field is changed | ||
| - An existing default value of a field is removed | ||
| - New enum restrictions are added to an existing field which did not previously have enum restrictions | ||
| - Existing enum values from an existing field are removed | ||
| - The minimum value of an existing field is increased in an existing version | ||
| - The maximum value of an existing field is decreased in an existing version | ||
| - Minimum or maximum field constraints are added to a field that did not previously have constraints |
There was a problem hiding this comment.
I have played around with it too, but I haven't been able to beat your list. I also haven't been able to figure out a good information architecture.
I do feel like like it would be nice to have this be a bit more scan-able as a reference. Maybe we will come up with something when we downstream this doc?
|
|
||
| ??? failure "Error output" | ||
| ``` | ||
| validating upgrade for CRD "test.example.com" failed: CustomResourceDefinition test.example.com failed upgrade safety validation. "NoScopeChange" validation failed: scope changed from "Namespaced" to "Cluster" |
There was a problem hiding this comment.
I agree that the long line is a little hard to read, but I have always been told to keep example outputs as they are printed in the terminal.
docs/refs/crd-upgrade-safety.md
Outdated
|
|
||
| ### Removing a previous version | ||
|
|
||
| In this example, the only previous version, `v1alpha1`, has been replaced with the new version: |
There was a problem hiding this comment.
| In this example, the only previous version, `v1alpha1`, has been replaced with the new version: | |
| In this example, the stored version `v1alpha1`, has been removed before it is no longer a stored version: |
There was a problem hiding this comment.
The second clause of your suggestion is a little confusing to me. Is the distinction here that it's okay to remove a stored version if it's not in use? And not okay to move if it's being used? In which case would something like "has been removed while still in use?"
There was a problem hiding this comment.
Or maybe just simply "has been removed?"
There was a problem hiding this comment.
Yeah, I think that wording sounds good. You can't remove a stored version while there are still instances of that version stored in etcd.
Signed-off-by: Tayler Geiger <[email protected]>
trgeiger
force-pushed
the
crd-update-docs
branch
from
0c07a19 to
bb019fd
Compare
5f9b27c
Signed-off-by: Tayler Geiger <[email protected]>
Signed-off-by: Tayler Geiger <[email protected]>
Description
Closes #746
Reviewer Checklist