Bug 1732914: Operator upgrades fail when versions field is not set #973
Conversation
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@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. |
openshift-ci-robot
added
the
size/M
There was a problem hiding this comment.
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
dinhxuanvu
force-pushed
the
crd-versions-fix
branch
from
0879ae4
to
ac7a3e8
Compare
| if oldVersion.Name == newVersion.Name { | ||
| versionPresent = true | ||
| } | ||
| newCRDVersions := map[string]struct{}{} |
There was a problem hiding this comment.
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.
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.
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.
@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. |
dinhxuanvu
force-pushed
the
crd-versions-fix
branch
from
ac7a3e8
to
6bd1ca2
Compare
openshift-ci-robot
added
size/L
dinhxuanvu
force-pushed
the
crd-versions-fix
branch
from
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>
dinhxuanvu
force-pushed
the
crd-versions-fix
branch
from
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 |
openshift-ci-robot
added
the
approved
|
/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. |
versionfieldversionscenariosSigned-off-by: Vu Dinh vdinh@redhat.com