-
Notifications
You must be signed in to change notification settings - Fork 545
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
Bug 1732914: Operator upgrades fail when versions field is not set #973
Bug 1732914: Operator upgrades fail when versions field is not set #973
Conversation
@dinhxuanvu: This pull request references a valid Bugzilla bug. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this @dinhxuanvu. Looks like the only thing left is an e2e test verifying the fix.
Also, I don't think we need to fix this now, but the current logic doesn't leave room for API deprecation - We should probably think about how we want to handle that soon.
cc @ecordell
0879ae4
to
ac7a3e8
Compare
if oldVersion.Name == newVersion.Name { | ||
versionPresent = true | ||
} | ||
newCRDVersions := map[string]struct{}{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the behavior that we want here actually something like:
- There must be a nonempty intersection of oldCRDVersions and newCRDVersions
- For any version in both oldCRD and newCRD, ensure the validation is unchanged (or has only backwards-compatible changes)
I'm thinking that this used as-is will prevent anyone from ever deprecating an older api version. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Developers can deprecate/disable a version of CRD by simply changing Served
field into false
. The reason I think we shouldn't allow developers to remove a version straight out without at least disabling it first to avoid the confusion that a CSV may reference that version or an existing CR with that version but the new CRD has no record of that version. Also, if a CSV is still referencing to a version that is not present in new CRD, the InstallPlan
won't include a step to upgrade that CRD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to make sure that this is documented. Can we add some doc's in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shawn-hurley The doc PR is referenced in this PR. It is #984.
will this change be back ported to 4.2? |
@rthallisey This is 4.2 bug fix so it should be in 4.2. |
ac7a3e8
to
6bd1ca2
Compare
6bd1ca2
to
ca0392b
Compare
1. Fix the ensureCRDVersions to account for `version` field 2. Add test cases for `version` scenarios Signed-off-by: Vu Dinh <vdinh@redhat.com>
ca0392b
to
fcb5719
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dinhxuanvu, ecordell The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
5 similar comments
/retest |
/retest |
/retest |
/retest |
/retest |
/retest |
6 similar comments
/retest |
/retest |
/retest |
/retest |
/retest |
/retest |
@dinhxuanvu: All pull requests linked via external trackers have merged. Bugzilla bug 1732914 has been moved to the MODIFIED state. In response to this:
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/test-infra repository. |
version
fieldversion
scenariosSigned-off-by: Vu Dinh vdinh@redhat.com